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 opensearch-project#3901 Signed-off-by: Anan Zhuang <[email protected]>
ananzh · Apr 21, 2023 · c52ddfd · c52ddfd
1 parent 86d42bc
commit c52ddfd
Show file tree

Hide file tree

Showing 7 changed files with 138 additions and 333 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -238,6 +238,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - [Multi DataSource] UX enhancement on Data source management stack ([#2521](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2521))
 - [Multi DataSource] UX enhancement on Update stored password modal for Data source management stack ([#2532](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2532))
 - [Monaco editor] Add json worker support ([#3424](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3424))
+- Replace re2 with RegExp in timeline and add unit tests ([#3908](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3908))
 
 ### 🐛 Bug Fixes
 

diff --git a/package.json b/package.json
@@ -199,7 +199,6 @@
     "pegjs": "0.10.0",
     "proxy-from-env": "1.0.0",
     "query-string": "^6.13.2",
-    "re2": "1.17.4",
     "react": "^16.14.0",
     "react-dom": "^16.12.0",
     "react-input-range": "^1.3.0",

diff --git a/src/dev/build/tasks/patch_native_modules_task.test.ts b/src/dev/build/tasks/patch_native_modules_task.test.ts
@@ -9,8 +9,8 @@ import {
   createAnyInstanceSerializer,
   createAbsolutePathSerializer,
 } from '@osd/dev-utils';
-import { Build, Config } from '../lib';
-import { PatchNativeModules } from './patch_native_modules_task';
+import { Build, Config, read, download, untar, gunzip } from '../lib';
+import { createPatchNativeModulesTask } from './patch_native_modules_task';
 
 const log = new ToolingLog();
 const testWriter = new ToolingLogCollectingWriter();
@@ -19,16 +19,16 @@ expect.addSnapshotSerializer(createAnyInstanceSerializer(Config));
 expect.addSnapshotSerializer(createAnyInstanceSerializer(ToolingLog));
 expect.addSnapshotSerializer(createAbsolutePathSerializer());
 
-jest.mock('../lib/download');
-jest.mock('../lib/fs', () => ({
-  ...jest.requireActual('../lib/fs'),
-  untar: jest.fn(),
-  gunzip: jest.fn(),
-}));
-
-const { untar } = jest.requireMock('../lib/fs');
-const { gunzip } = jest.requireMock('../lib/fs');
-const { download } = jest.requireMock('../lib/download');
+jest.mock('../lib', () => {
+  const originalModule = jest.requireActual('../lib');
+  return {
+    ...originalModule,
+    download: jest.fn(),
+    gunzip: jest.fn(),
+    untar: jest.fn(),
+    read: jest.fn(),
+  };
+});
 
 async function setup() {
   const config = await Config.create({
@@ -38,14 +38,15 @@ async function setup() {
       linux: false,
       linuxArm: false,
       darwin: false,
+      windows: false,
     },
   });
 
   const build = new Build(config);
 
-  download.mockImplementation(() => {});
-  untar.mockImplementation(() => {});
-  gunzip.mockImplementation(() => {});
+  (read as jest.MockedFunction<typeof read>).mockImplementation(async () => {
+    return JSON.stringify({ version: mockPackage.version });
+  });
 
   return { config, build };
 }
@@ -55,38 +56,77 @@ beforeEach(() => {
   jest.clearAllMocks();
 });
 
-it('patch native modules task downloads the correct platform package', async () => {
-  const { config, build } = await setup();
-  config.targetPlatforms.linuxArm = true;
-  await PatchNativeModules.run(config, log, build);
-  expect(download.mock.calls.length).toBe(1);
-  expect(download.mock.calls).toMatchInlineSnapshot(`
+const mockPackage = {
+  name: 'mock-native-module',
+  version: '1.0.0',
+  destinationPath: 'path/to/destination',
+  extractMethod: 'untar',
+  archives: {
+    'linux-arm64': {
+      url: 'https://example.com/mock-native-module/linux-arm64.tar.gz',
+      sha256: 'mock-sha256',
+    },
+    'linux-x64': {
+      url: 'https://example.com/mock-native-module/linux-x64.gz',
+      sha256: 'mock-sha256',
+    },
+  },
+};
+
+describe('patch native modules task', () => {
+  it('patch native modules task downloads the correct platform package', async () => {
+    const { config, build } = await setup();
+    config.targetPlatforms.linuxArm = true;
+    const PatchNativeModulesWithMock = createPatchNativeModulesTask([mockPackage]);
+    await PatchNativeModulesWithMock.run(config, log, build);
+    expect((download as jest.MockedFunction<typeof download>).mock.calls.length).toBe(1);
+    expect((download as jest.MockedFunction<typeof download>).mock.calls).toMatchInlineSnapshot(`
     Array [
       Array [
         Object {
-          "destination": <absolute path>/.native_modules/re2/linux-arm64-83.tar.gz,
+          "destination": <absolute path>/.native_modules/mock-native-module/linux-arm64.tar.gz,
           "log": <ToolingLog>,
           "retries": 3,
-          "sha256": "d86ced75b794fbf518b90908847b3c09a50f3ff5a2815aa30f53080f926a2873",
-          "url": "https://d1v1sj258etie.cloudfront.net/node-re2/releases/download/1.17.4/linux-arm64-83.tar.gz",
+          "sha256": "mock-sha256",
+          "url": "https://example.com/mock-native-module/linux-arm64.tar.gz",
         },
       ],
     ]
   `);
-});
+  });
 
-it('for .tar.gz artifact, patch native modules task unzip it via untar', async () => {
-  const { config, build } = await setup();
-  config.targetPlatforms.linuxArm = true;
-  await PatchNativeModules.run(config, log, build);
-  expect(untar.mock.calls.length).toBe(1);
-  expect(gunzip.mock.calls.length).toBe(0);
-});
+  it('for .tar.gz artifact, patch native modules task unzip it via untar', async () => {
+    const { config, build } = await setup();
+    config.targetPlatforms.linuxArm = true;
+    const PatchNativeModulesWithMock = createPatchNativeModulesTask([mockPackage]);
+    await PatchNativeModulesWithMock.run(config, log, build);
+    expect(untar).toHaveBeenCalled();
+    expect(gunzip).not.toHaveBeenCalled();
+  });
 
-it('for .gz artifact, patch native modules task unzip it via gunzip', async () => {
-  const { config, build } = await setup();
-  config.targetPlatforms.linux = true;
-  await PatchNativeModules.run(config, log, build);
-  expect(untar.mock.calls.length).toBe(0);
-  expect(gunzip.mock.calls.length).toBe(1);
+  it('for .gz artifact, patch native modules task unzip it via gunzip', async () => {
+    const mockPackageGZ = {
+      ...mockPackage,
+      extractMethod: 'gunzip',
+    };
+    const { config, build } = await setup();
+    config.targetPlatforms.linux = true;
+    const PatchNativeModulesWithMock = createPatchNativeModulesTask([mockPackageGZ]);
+    await PatchNativeModulesWithMock.run(config, log, build);
+    expect(gunzip).toHaveBeenCalled();
+    expect(untar).not.toHaveBeenCalled();
+  });
+
+  it('throws error for unsupported extract methods', async () => {
+    const mockPackageUnsupported = {
+      ...mockPackage,
+      extractMethod: 'unsupported',
+    };
+    const { config, build } = await setup();
+    config.targetPlatforms.linux = true;
+    const PatchNativeModulesWithMock = createPatchNativeModulesTask([mockPackageUnsupported]);
+    await expect(PatchNativeModulesWithMock.run(config, log, build)).rejects.toThrow(
+      'Extract method of unsupported is not supported'
+    );
+  });
 });
diff --git a/src/dev/build/tasks/patch_native_modules_task.ts b/src/dev/build/tasks/patch_native_modules_task.ts
@@ -52,45 +52,7 @@ interface Package {
   >;
 }
 
-/* Process for updating URLs and checksums after bumping the version of `re2` or NodeJS:
- *   1. Match the `version` with the version in the yarn.lock file.
- *   2. Match the module version, the digits at the end of the filename, with the output of
- *      `node -p process.versions.modules`.
- *   3. Confirm that the URLs exist for each platform-architecture combo on
- *      https://github.com/uhop/node-re2/releases/tag/[VERSION]; reach out to maintainers for ARM
- *      releases of `re2` as they currently don't have an official ARM release.
- *   4. Generate new checksums for each artifact by downloading each one and calling
- *      `shasum -a 256` or `sha256sum` on the downloaded file.
- */
-const packages: Package[] = [
-  {
-    name: 're2',
-    version: '1.17.4',
-    destinationPath: 'node_modules/re2/build/Release/re2.node',
-    extractMethod: 'gunzip',
-    archives: {
-      'darwin-x64': {
-        url: 'https://github.com/uhop/node-re2/releases/download/1.17.4/darwin-x64-83.gz',
-        sha256: '9112ed93c1544ecc6397f7ff20bd2b28f3b04c7fbb54024e10f9a376a132a87d',
-      },
-      'linux-x64': {
-        url: 'https://github.com/uhop/node-re2/releases/download/1.17.4/linux-x64-83.gz',
-        sha256: '86e03540783a18c41f81df0aec320b1f64aca6cbd3a87fc1b7a9b4109c5f5986',
-      },
-      'linux-arm64': {
-        url:
-          'https://d1v1sj258etie.cloudfront.net/node-re2/releases/download/1.17.4/linux-arm64-83.tar.gz',
-        sha256: 'd86ced75b794fbf518b90908847b3c09a50f3ff5a2815aa30f53080f926a2873',
-        overriddenExtractMethod: 'untar',
-        overriddenDestinationPath: 'node_modules/re2/build/Release',
-      },
-      'win32-x64': {
-        url: 'https://github.com/uhop/node-re2/releases/download/1.17.4/win32-x64-83.gz',
-        sha256: '2f842d9757288afd4bd5dec0e7b370a4c3e89ac98050598b17abb9e8e00e3294',
-      },
-    },
-  },
-];
+export const packages: Package[] = [];
 
 async function getInstalledVersion(config: Config, packageName: string) {
   const packageJSONPath = config.resolveFromRepo(
@@ -145,15 +107,20 @@ async function patchModule(
   }
 }
 
-export const PatchNativeModules: Task = {
-  description: 'Patching platform-specific native modules',
-  async run(config, log, build) {
-    for (const pkg of packages) {
-      await Promise.all(
-        config.getTargetPlatforms().map(async (platform) => {
-          await patchModule(config, log, build, platform, pkg);
-        })
-      );
-    }
-  },
-};
+export function createPatchNativeModulesTask(customPackages?: Package[]): Task {
+  return {
+    description: 'Patching platform-specific native modules',
+    async run(config, log, build) {
+      const targetPackages = customPackages || packages;
+      for (const pkg of targetPackages) {
+        await Promise.all(
+          config.getTargetPlatforms().map(async (platform) => {
+            await patchModule(config, log, build, platform, pkg);
+          })
+        );
+      }
+    },
+  };
+}
+
+export const PatchNativeModules = createPatchNativeModulesTask();
diff --git a/src/plugins/vis_type_timeline/server/series_functions/label.js b/src/plugins/vis_type_timeline/server/series_functions/label.js
@@ -62,10 +62,8 @@ export default new Chainable('label', {
     const config = args.byName;
     return alter(args, function (eachSeries) {
       if (config.regex) {
-        // not using a standard `import` so that if there's an issue with the re2 native module
-        // that it doesn't prevent OpenSearch Dashboards from starting up and we only have an issue using Timeline labels
-        const RE2 = require('re2');
-        eachSeries.label = eachSeries.label.replace(new RE2(config.regex), config.label);
+        const regex = new RegExp(config.regex);
+        eachSeries.label = eachSeries.label.replace(regex, config.label);
       } else {
         eachSeries.label = config.label;
       }

diff --git a/src/plugins/vis_type_timeline/server/series_functions/label.test.js b/src/plugins/vis_type_timeline/server/series_functions/label.test.js
@@ -51,4 +51,24 @@ describe('label.js', () => {
       expect(r.output.list[0].label).to.equal('beerative');
     });
   });
+
+  it('can use a regex to capture groups to modify series label', () => {
+    return invoke(fn, [seriesList, 'beer$2', '(N)(egative)']).then((r) => {
+      expect(r.output.list[0].label).to.equal('beeregative');
+    });
+  });
+
+  it('can handle different regex patterns', () => {
+    const seriesListCopy1 = JSON.parse(JSON.stringify(seriesList));
+    const seriesListCopy2 = JSON.parse(JSON.stringify(seriesList));
+
+    return Promise.all([
+      invoke(fn, [seriesListCopy1, 'beer$1 - $2', '(N)(egative)']).then((r) => {
+        expect(r.output.list[0].label).to.equal('beerN - egative');
+      }),
+      invoke(fn, [seriesListCopy2, 'beer$1_$2', '(N)(eg.*)']).then((r) => {
+        expect(r.output.list[0].label).to.equal('beerN_egative');
+      }),
+    ]);
+  });
 });