Skip to content

Commit c52ddfd

Browse files
committed
Replace re2 with RegExp in timeline and add unit tests
Remove re2 usage and replace it with JavaScript built-in RegExp object. Also add more unit tests to make sure that using RegExp has same expressions as using re2 library. Issue Resolve #3901 Signed-off-by: Anan Zhuang <ananzh@amazon.com>
1 parent 86d42bc commit c52ddfd

File tree

7 files changed

+138
-333
lines changed

7 files changed

+138
-333
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
238238
- [Multi DataSource] UX enhancement on Data source management stack ([#2521](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2521))
239239
- [Multi DataSource] UX enhancement on Update stored password modal for Data source management stack ([#2532](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2532))
240240
- [Monaco editor] Add json worker support ([#3424](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3424))
241+
- Replace re2 with RegExp in timeline and add unit tests ([#3908](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3908))
241242

242243
### 🐛 Bug Fixes
243244

package.json

-1
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,6 @@
199199
"pegjs": "0.10.0",
200200
"proxy-from-env": "1.0.0",
201201
"query-string": "^6.13.2",
202-
"re2": "1.17.4",
203202
"react": "^16.14.0",
204203
"react-dom": "^16.12.0",
205204
"react-input-range": "^1.3.0",

src/dev/build/tasks/patch_native_modules_task.test.ts

+78-38
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ import {
99
createAnyInstanceSerializer,
1010
createAbsolutePathSerializer,
1111
} from '@osd/dev-utils';
12-
import { Build, Config } from '../lib';
13-
import { PatchNativeModules } from './patch_native_modules_task';
12+
import { Build, Config, read, download, untar, gunzip } from '../lib';
13+
import { createPatchNativeModulesTask } from './patch_native_modules_task';
1414

1515
const log = new ToolingLog();
1616
const testWriter = new ToolingLogCollectingWriter();
@@ -19,16 +19,16 @@ expect.addSnapshotSerializer(createAnyInstanceSerializer(Config));
1919
expect.addSnapshotSerializer(createAnyInstanceSerializer(ToolingLog));
2020
expect.addSnapshotSerializer(createAbsolutePathSerializer());
2121

22-
jest.mock('../lib/download');
23-
jest.mock('../lib/fs', () => ({
24-
...jest.requireActual('../lib/fs'),
25-
untar: jest.fn(),
26-
gunzip: jest.fn(),
27-
}));
28-
29-
const { untar } = jest.requireMock('../lib/fs');
30-
const { gunzip } = jest.requireMock('../lib/fs');
31-
const { download } = jest.requireMock('../lib/download');
22+
jest.mock('../lib', () => {
23+
const originalModule = jest.requireActual('../lib');
24+
return {
25+
...originalModule,
26+
download: jest.fn(),
27+
gunzip: jest.fn(),
28+
untar: jest.fn(),
29+
read: jest.fn(),
30+
};
31+
});
3232

3333
async function setup() {
3434
const config = await Config.create({
@@ -38,14 +38,15 @@ async function setup() {
3838
linux: false,
3939
linuxArm: false,
4040
darwin: false,
41+
windows: false,
4142
},
4243
});
4344

4445
const build = new Build(config);
4546

46-
download.mockImplementation(() => {});
47-
untar.mockImplementation(() => {});
48-
gunzip.mockImplementation(() => {});
47+
(read as jest.MockedFunction<typeof read>).mockImplementation(async () => {
48+
return JSON.stringify({ version: mockPackage.version });
49+
});
4950

5051
return { config, build };
5152
}
@@ -55,38 +56,77 @@ beforeEach(() => {
5556
jest.clearAllMocks();
5657
});
5758

58-
it('patch native modules task downloads the correct platform package', async () => {
59-
const { config, build } = await setup();
60-
config.targetPlatforms.linuxArm = true;
61-
await PatchNativeModules.run(config, log, build);
62-
expect(download.mock.calls.length).toBe(1);
63-
expect(download.mock.calls).toMatchInlineSnapshot(`
59+
const mockPackage = {
60+
name: 'mock-native-module',
61+
version: '1.0.0',
62+
destinationPath: 'path/to/destination',
63+
extractMethod: 'untar',
64+
archives: {
65+
'linux-arm64': {
66+
url: 'https://example.com/mock-native-module/linux-arm64.tar.gz',
67+
sha256: 'mock-sha256',
68+
},
69+
'linux-x64': {
70+
url: 'https://example.com/mock-native-module/linux-x64.gz',
71+
sha256: 'mock-sha256',
72+
},
73+
},
74+
};
75+
76+
describe('patch native modules task', () => {
77+
it('patch native modules task downloads the correct platform package', async () => {
78+
const { config, build } = await setup();
79+
config.targetPlatforms.linuxArm = true;
80+
const PatchNativeModulesWithMock = createPatchNativeModulesTask([mockPackage]);
81+
await PatchNativeModulesWithMock.run(config, log, build);
82+
expect((download as jest.MockedFunction<typeof download>).mock.calls.length).toBe(1);
83+
expect((download as jest.MockedFunction<typeof download>).mock.calls).toMatchInlineSnapshot(`
6484
Array [
6585
Array [
6686
Object {
67-
"destination": <absolute path>/.native_modules/re2/linux-arm64-83.tar.gz,
87+
"destination": <absolute path>/.native_modules/mock-native-module/linux-arm64.tar.gz,
6888
"log": <ToolingLog>,
6989
"retries": 3,
70-
"sha256": "d86ced75b794fbf518b90908847b3c09a50f3ff5a2815aa30f53080f926a2873",
71-
"url": "https://d1v1sj258etie.cloudfront.net/node-re2/releases/download/1.17.4/linux-arm64-83.tar.gz",
90+
"sha256": "mock-sha256",
91+
"url": "https://example.com/mock-native-module/linux-arm64.tar.gz",
7292
},
7393
],
7494
]
7595
`);
76-
});
96+
});
7797

78-
it('for .tar.gz artifact, patch native modules task unzip it via untar', async () => {
79-
const { config, build } = await setup();
80-
config.targetPlatforms.linuxArm = true;
81-
await PatchNativeModules.run(config, log, build);
82-
expect(untar.mock.calls.length).toBe(1);
83-
expect(gunzip.mock.calls.length).toBe(0);
84-
});
98+
it('for .tar.gz artifact, patch native modules task unzip it via untar', async () => {
99+
const { config, build } = await setup();
100+
config.targetPlatforms.linuxArm = true;
101+
const PatchNativeModulesWithMock = createPatchNativeModulesTask([mockPackage]);
102+
await PatchNativeModulesWithMock.run(config, log, build);
103+
expect(untar).toHaveBeenCalled();
104+
expect(gunzip).not.toHaveBeenCalled();
105+
});
85106

86-
it('for .gz artifact, patch native modules task unzip it via gunzip', async () => {
87-
const { config, build } = await setup();
88-
config.targetPlatforms.linux = true;
89-
await PatchNativeModules.run(config, log, build);
90-
expect(untar.mock.calls.length).toBe(0);
91-
expect(gunzip.mock.calls.length).toBe(1);
107+
it('for .gz artifact, patch native modules task unzip it via gunzip', async () => {
108+
const mockPackageGZ = {
109+
...mockPackage,
110+
extractMethod: 'gunzip',
111+
};
112+
const { config, build } = await setup();
113+
config.targetPlatforms.linux = true;
114+
const PatchNativeModulesWithMock = createPatchNativeModulesTask([mockPackageGZ]);
115+
await PatchNativeModulesWithMock.run(config, log, build);
116+
expect(gunzip).toHaveBeenCalled();
117+
expect(untar).not.toHaveBeenCalled();
118+
});
119+
120+
it('throws error for unsupported extract methods', async () => {
121+
const mockPackageUnsupported = {
122+
...mockPackage,
123+
extractMethod: 'unsupported',
124+
};
125+
const { config, build } = await setup();
126+
config.targetPlatforms.linux = true;
127+
const PatchNativeModulesWithMock = createPatchNativeModulesTask([mockPackageUnsupported]);
128+
await expect(PatchNativeModulesWithMock.run(config, log, build)).rejects.toThrow(
129+
'Extract method of unsupported is not supported'
130+
);
131+
});
92132
});

src/dev/build/tasks/patch_native_modules_task.ts

+18-51
Original file line numberDiff line numberDiff line change
@@ -52,45 +52,7 @@ interface Package {
5252
>;
5353
}
5454

55-
/* Process for updating URLs and checksums after bumping the version of `re2` or NodeJS:
56-
* 1. Match the `version` with the version in the yarn.lock file.
57-
* 2. Match the module version, the digits at the end of the filename, with the output of
58-
* `node -p process.versions.modules`.
59-
* 3. Confirm that the URLs exist for each platform-architecture combo on
60-
* https://github.com/uhop/node-re2/releases/tag/[VERSION]; reach out to maintainers for ARM
61-
* releases of `re2` as they currently don't have an official ARM release.
62-
* 4. Generate new checksums for each artifact by downloading each one and calling
63-
* `shasum -a 256` or `sha256sum` on the downloaded file.
64-
*/
65-
const packages: Package[] = [
66-
{
67-
name: 're2',
68-
version: '1.17.4',
69-
destinationPath: 'node_modules/re2/build/Release/re2.node',
70-
extractMethod: 'gunzip',
71-
archives: {
72-
'darwin-x64': {
73-
url: 'https://github.com/uhop/node-re2/releases/download/1.17.4/darwin-x64-83.gz',
74-
sha256: '9112ed93c1544ecc6397f7ff20bd2b28f3b04c7fbb54024e10f9a376a132a87d',
75-
},
76-
'linux-x64': {
77-
url: 'https://github.com/uhop/node-re2/releases/download/1.17.4/linux-x64-83.gz',
78-
sha256: '86e03540783a18c41f81df0aec320b1f64aca6cbd3a87fc1b7a9b4109c5f5986',
79-
},
80-
'linux-arm64': {
81-
url:
82-
'https://d1v1sj258etie.cloudfront.net/node-re2/releases/download/1.17.4/linux-arm64-83.tar.gz',
83-
sha256: 'd86ced75b794fbf518b90908847b3c09a50f3ff5a2815aa30f53080f926a2873',
84-
overriddenExtractMethod: 'untar',
85-
overriddenDestinationPath: 'node_modules/re2/build/Release',
86-
},
87-
'win32-x64': {
88-
url: 'https://github.com/uhop/node-re2/releases/download/1.17.4/win32-x64-83.gz',
89-
sha256: '2f842d9757288afd4bd5dec0e7b370a4c3e89ac98050598b17abb9e8e00e3294',
90-
},
91-
},
92-
},
93-
];
55+
export const packages: Package[] = [];
9456

9557
async function getInstalledVersion(config: Config, packageName: string) {
9658
const packageJSONPath = config.resolveFromRepo(
@@ -145,15 +107,20 @@ async function patchModule(
145107
}
146108
}
147109

148-
export const PatchNativeModules: Task = {
149-
description: 'Patching platform-specific native modules',
150-
async run(config, log, build) {
151-
for (const pkg of packages) {
152-
await Promise.all(
153-
config.getTargetPlatforms().map(async (platform) => {
154-
await patchModule(config, log, build, platform, pkg);
155-
})
156-
);
157-
}
158-
},
159-
};
110+
export function createPatchNativeModulesTask(customPackages?: Package[]): Task {
111+
return {
112+
description: 'Patching platform-specific native modules',
113+
async run(config, log, build) {
114+
const targetPackages = customPackages || packages;
115+
for (const pkg of targetPackages) {
116+
await Promise.all(
117+
config.getTargetPlatforms().map(async (platform) => {
118+
await patchModule(config, log, build, platform, pkg);
119+
})
120+
);
121+
}
122+
},
123+
};
124+
}
125+
126+
export const PatchNativeModules = createPatchNativeModulesTask();

src/plugins/vis_type_timeline/server/series_functions/label.js

+2-4
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,8 @@ export default new Chainable('label', {
6262
const config = args.byName;
6363
return alter(args, function (eachSeries) {
6464
if (config.regex) {
65-
// not using a standard `import` so that if there's an issue with the re2 native module
66-
// that it doesn't prevent OpenSearch Dashboards from starting up and we only have an issue using Timeline labels
67-
const RE2 = require('re2');
68-
eachSeries.label = eachSeries.label.replace(new RE2(config.regex), config.label);
65+
const regex = new RegExp(config.regex);
66+
eachSeries.label = eachSeries.label.replace(regex, config.label);
6967
} else {
7068
eachSeries.label = config.label;
7169
}

src/plugins/vis_type_timeline/server/series_functions/label.test.js

+20
Original file line numberDiff line numberDiff line change
@@ -51,4 +51,24 @@ describe('label.js', () => {
5151
expect(r.output.list[0].label).to.equal('beerative');
5252
});
5353
});
54+
55+
it('can use a regex to capture groups to modify series label', () => {
56+
return invoke(fn, [seriesList, 'beer$2', '(N)(egative)']).then((r) => {
57+
expect(r.output.list[0].label).to.equal('beeregative');
58+
});
59+
});
60+
61+
it('can handle different regex patterns', () => {
62+
const seriesListCopy1 = JSON.parse(JSON.stringify(seriesList));
63+
const seriesListCopy2 = JSON.parse(JSON.stringify(seriesList));
64+
65+
return Promise.all([
66+
invoke(fn, [seriesListCopy1, 'beer$1 - $2', '(N)(egative)']).then((r) => {
67+
expect(r.output.list[0].label).to.equal('beerN - egative');
68+
}),
69+
invoke(fn, [seriesListCopy2, 'beer$1_$2', '(N)(eg.*)']).then((r) => {
70+
expect(r.output.list[0].label).to.equal('beerN_egative');
71+
}),
72+
]);
73+
});
5474
});

0 commit comments

Comments
 (0)