fix(plugins): Make plugin loading work end to end

docs/maintainers/todo.md

@@ -38,20 +38,22 @@ versions, as they are listed in source code / generated documentation.
 
 Plugins:
 
-- [ ] Test that the Java connector resolves plugins from the project-local
+- [x] Test that the Java connector resolves plugins from the project-local
       `node_modules`. The connector bundle is extracted to a temp directory and
       run from there, so bare-specifier `import()` may resolve from the wrong
       place - needs an end-to-end test loading a plugin by name from a Java
-      project.
-- [ ] Expose `loadPlugins` in the TS DSL (currently only the Java DSL exposes
+      project. (Fixed by resolving plugin names against the working directory
+      in `BoundaryPluginLoader`; tested by `PluginLoadTest` in dsl-java.)
+- [x] Expose `loadPlugins` in the TS DSL (currently only the Java DSL exposes
       it)
-- [ ] Unwrap the module namespace when loading plugins in
+- [x] Unwrap the module namespace when loading plugins in
       `BoundaryPluginLoader` - a dynamic `import()` namespace never exposes
       `description` etc for plugins built with `export default` or
       `module.exports = plugin` (only individual named exports work), so the
       documented "default export one `ContractCasePlugin` object" shape needs
       `.default` unwrapping, plus an end-to-end test loading a third-party
-      plugin by name.
+      plugin by name. (Fixed with `mustResolvePlugin` in `case-plugin-base`;
+      tested by `BoundaryPluginLoader.spec.ts`.)
 - [ ] Complete the DSL generator so plugin authors can run it - hook it into
       the CLI and remove the hardcoded paths in
       `case-definition-generator/src/index.ts`.

packages/case-connector/eslint.config.mjs

@@ -3,8 +3,8 @@ import { globalIgnores } from 'eslint/config';
 
 export default [
   globalIgnores(
-    ['cjs.js', 'rename-inner-webpack-vars-loader.cjs'],
-    'Ignore CJS node entry point and webpack loader',
+    ['cjs.js', 'rename-inner-webpack-vars-loader.cjs', 'test-fixtures/'],
+    'Ignore CJS node entry point, webpack loader, and test fixture packages',
   ),
   ...lintConfig,
   {

packages/case-connector/package.json

@@ -51,6 +51,7 @@
   "devDependencies": {
     "@arethetypeswrong/cli": "^0.18.2",
     "@contract-case/case-maintainer-config": "0.30.1",
+    "@contract-case/test-plugin-fixture": "file:test-fixtures/contract-case-test-plugin",
     "@contract-case/eslint-config-case-maintainer": "0.30.1",
     "@knighted/duel": "^2.1.6",
     "@types/google-protobuf": "^3.15.12",

packages/case-connector/src/connectors/case-boundary/internals/BoundaryPluginLoader.spec.ts

@@ -0,0 +1,133 @@
+import { describe, expect, it } from 'vitest';
+
+import { BoundaryPluginLoader } from './BoundaryPluginLoader.js';
+import {
+  BoundaryFailure,
+  BoundaryResult,
+  BoundaryResultTypeConstants,
+  BoundarySuccess,
+  ContractCaseBoundaryConfig,
+  ILogPrinter,
+  IResultPrinter,
+} from './boundary/index.js';
+
+const logPrinter: ILogPrinter = {
+  log: () => Promise.resolve(new BoundarySuccess()),
+};
+
+const resultPrinter: IResultPrinter = {
+  printMatchError: () => Promise.resolve(new BoundarySuccess()),
+  printMessageError: () => Promise.resolve(new BoundarySuccess()),
+  printTestTitle: () => Promise.resolve(new BoundarySuccess()),
+};
+
+const config: ContractCaseBoundaryConfig = {
+  testRunId: 'PLUGIN-LOADER-SPEC',
+  internals: {},
+};
+
+const makeLoader = () =>
+  new BoundaryPluginLoader(config, logPrinter, resultPrinter, [
+    'plugin-loader-spec-version',
+  ]);
+
+const expectSuccess = (result: BoundaryResult) => {
+  expect(
+    result.resultType,
+    result instanceof BoundaryFailure ? result.message : undefined,
+  ).toBe(BoundaryResultTypeConstants.RESULT_SUCCESS);
+};
+
+const expectFailure = (result: BoundaryResult): BoundaryFailure => {
+  expect(result.resultType).toBe(BoundaryResultTypeConstants.RESULT_FAILURE);
+  return result as BoundaryFailure;
+};
+
+describe('BoundaryPluginLoader', () => {
+  // The fixture package provides the same minimal plugin behind one entry
+  // point per packaging style a real plugin might be built with. The
+  // documented shape is "the plugin object is the module's default export" -
+  // all of these must load.
+  describe.each([
+    ['TypeScript-compiled default export', '@contract-case/test-plugin-fixture'],
+    [
+      'CommonJS module.exports',
+      '@contract-case/test-plugin-fixture/module-exports',
+    ],
+    ['native ESM default export', '@contract-case/test-plugin-fixture/esm-default'],
+    [
+      'CommonJS named exports',
+      '@contract-case/test-plugin-fixture/named-exports',
+    ],
+  ])('with a plugin packaged as %s', (_style, moduleName) => {
+    it('loads successfully', async () => {
+      expectSuccess(await makeLoader().loadPlugins([moduleName]));
+    });
+
+    it('is idempotent when loaded again', async () => {
+      expectSuccess(await makeLoader().loadPlugins([moduleName]));
+    });
+  });
+
+  describe('with a module that loads but is not a plugin', () => {
+    it('fails with a message explaining the expected shape', async () => {
+      const failure = expectFailure(
+        await makeLoader().loadPlugins([
+          '@contract-case/test-plugin-fixture/not-a-plugin',
+        ]),
+      );
+      expect(failure.message).toContain(
+        "doesn't contain a ContractCase plugin",
+      );
+      expect(failure.message).toContain(
+        '@contract-case/test-plugin-fixture/not-a-plugin',
+      );
+      expect(failure.contractCaseErrorCode).toBe('INVALID_PLUGIN_MODULE');
+    });
+  });
+
+  describe('with a package that is not installed', () => {
+    it('fails and suggests installing the package', async () => {
+      const failure = expectFailure(
+        await makeLoader().loadPlugins([
+          '@contract-case/definitely-not-a-real-plugin',
+        ]),
+      );
+      expect(failure.message).toContain(
+        "Unable to load plugin '@contract-case/definitely-not-a-real-plugin'",
+      );
+      expect(failure.message).toContain('npm install');
+    });
+  });
+
+  describe.each([
+    ['a remote URI', 'https://example.com/evil.js'],
+    ['an inline URI', 'data:text/javascript,export default {}'],
+    ['a relative path', './some/local/path'],
+    ['a path traversal', '../../../etc/passwd'],
+    ['an absolute path', '/etc/passwd'],
+    ['a non-package string', 'not a package name'],
+  ])('with an unsafe module specifier (%s)', (_kind, moduleName) => {
+    it('fails without attempting to load it', async () => {
+      const failure = expectFailure(
+        await makeLoader().loadPlugins([moduleName]),
+      );
+      expect(failure.message).toContain('Unsafe plugin module specifier');
+      expect(failure.contractCaseErrorCode).toBe('INVALID_PLUGIN_NAME');
+    });
+  });
+
+  describe('with a mix of loadable and unloadable plugins', () => {
+    it('fails, reporting the plugin that could not be loaded', async () => {
+      const failure = expectFailure(
+        await makeLoader().loadPlugins([
+          '@contract-case/test-plugin-fixture/module-exports',
+          '@contract-case/definitely-not-a-real-plugin',
+        ]),
+      );
+      expect(failure.message).toContain(
+        '@contract-case/definitely-not-a-real-plugin',
+      );
+    });
+  });
+});

packages/case-connector/src/connectors/case-boundary/internals/BoundaryPluginLoader.ts

@@ -1,22 +1,108 @@
+import { createRequire } from 'node:module';
+import { join } from 'node:path';
+import { pathToFileURL } from 'node:url';
+
 import {
   CaseConfigurationError,
   CaseCoreError,
   PluginLoader,
+  mustResolvePlugin,
 } from '@contract-case/case-core';
 
 import {
   ContractCaseBoundaryConfig,
   ILogPrinter,
   IResultPrinter,
   BoundaryResult,
+  BoundarySuccess,
 } from './boundary/index.js';
 import { versionString } from '../../../entities/versionString.js';
 import {
   convertConfig,
   jsErrorToFailure,
   wrapLogPrinter,
 } from './mappers/index.js';
-import { BoundarySuccess } from './index.js';
+
+/**
+ * Resolves a plugin module name against the user's project (ie, the current
+ * working directory), falling back to normal resolution relative to this
+ * file.
+ *
+ * The cwd resolution step exists because this code doesn't always run from
+ * the user's project: when ContractCase is called from a host language like
+ * Java, the connector runs from a temporary directory, so resolving relative
+ * to this file would never find plugins the user installed in their project's
+ * node_modules.
+ *
+ * @param moduleName - a previously validated bare module name
+ * @returns the resolved path to the module, or null if it couldn't be
+ * resolved from the working directory (in which case the caller should fall
+ * back to normal resolution, so that this continues to work if the module is
+ * resolvable from this file but not from the working directory).
+ */
+const resolveFromWorkingDirectory = (moduleName: string): string | null => {
+  try {
+    return createRequire(join(process.cwd(), 'noop.js')).resolve(moduleName);
+  } catch {
+    return null;
+  }
+};
+
+const loadOne = (
+  moduleName: string,
+): Promise<ReturnType<typeof mustResolvePlugin>> =>
+  Promise.resolve()
+    .then(() => {
+      if (
+        // Forbid URIs
+        moduleName.includes(':') ||
+        // Forbid strings that start with '.', as they may be hidden files or path traversal
+        moduleName.startsWith('.') ||
+        // Forbid strings that start with / , as only local packages are supported
+        moduleName.startsWith('/') ||
+        // Forbid strings that don't look like node module names
+        !/^(?:@[a-z0-9][a-z0-9._-]*\/)?[a-z0-9][a-z0-9._-]*(?:\/[a-z0-9][a-z0-9._-]*)*$/i.test(
+          moduleName,
+        )
+      ) {
+        throw new CaseConfigurationError(
+          `Unsafe plugin module specifier. Plugin names must be a valid local nodejs package name, and may not be remote or inline URIs. Provided name was: '${moduleName}'`,
+          'DONT_ADD_LOCATION',
+          'INVALID_PLUGIN_NAME',
+        );
+      }
+      const resolvedPath = resolveFromWorkingDirectory(moduleName);
+      // webpack ignore is needed here so that the final bundle for host
+      // languages is able to import arbitrary libs, without webpack failing
+      return import(
+        /* webpackIgnore: true */ resolvedPath != null
+          ? pathToFileURL(resolvedPath).href
+          : moduleName
+      )
+        .catch((e) => {
+          // Some test environments can't do a dynamic import() at all (eg
+          // Jest without --experimental-vm-modules). In those environments,
+          // fall back to loading the plugin with require() instead.
+          if (
+            e != null &&
+            (e.code === 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING' ||
+              `${e.message}`.includes('--experimental-vm-modules'))
+          ) {
+            return createRequire(join(process.cwd(), 'noop.js'))(
+              resolvedPath != null ? resolvedPath : moduleName,
+            );
+          }
+          throw e;
+        })
+        .catch((e) => {
+          throw new CaseConfigurationError(
+            `Unable to load plugin '${moduleName}': ${e.message}\n\nPlease check that the plugin's package is installed (for example, with \`npm install --save-dev ${moduleName}\`) in the project the tests are running from.`,
+            'DONT_ADD_LOCATION',
+            'UNDOCUMENTED',
+          );
+        });
+    })
+    .then((moduleContents) => mustResolvePlugin(moduleContents, moduleName));
 
 /**
  * A BoundaryPluginLoader allows loading plugins into the core
@@ -77,48 +163,15 @@ export class BoundaryPluginLoader {
 
   async loadPlugins(moduleNames: string[]): Promise<BoundaryResult> {
     this.initialiseLoader();
-    return Promise.all(
-      moduleNames.map((moduleName) => {
-        if (
-          // Forbid URIs
-          moduleName.includes(':') ||
-          // Forbid strings that start with '.', as they may be hidden files or path traversal
-          moduleName.startsWith('.') ||
-          // Forbid strings that start with / , as only local packages are supported
-          moduleName.startsWith('/') ||
-          // Forbid strings that don't look like node module names
-          !/^(?:@[a-z0-9][a-z0-9._-]*\/)?[a-z0-9][a-z0-9._-]*(?:\/[a-z0-9][a-z0-9._-]*)*$/i.test(
-            moduleName,
-          )
-        ) {
-          throw new CaseConfigurationError(
-            `Unsafe plugin module specifier. Plugin names must be a valid local nodejs package name, and may not be remote or inline URIs. Provided name was: '${moduleName}'`,
-            'DONT_ADD_LOCATION',
-            'INVALID_PLUGIN_NAME',
+    return Promise.all(moduleNames.map((moduleName) => loadOne(moduleName)))
+      .then((plugins) => {
+        if (this.loader == null) {
+          throw new CaseCoreError(
+            'this.loader was not initialised during loadPlugins. This should never happen, as initialiseLoader() is meant to be called first',
           );
         }
-        // webpack ignore is needed here so that the final bundle for host
-        // languages is able to import arbitrary libs, without webpack failing
-        return import(/* webpackIgnore: true */ moduleName);
-      }),
-    )
-      .then(
-        (plugins) => {
-          if (this.loader == null) {
-            throw new CaseCoreError(
-              'this.loader was not initialised during loadPlugins. This should never happen, as initialiseLoader() is meant to be called first',
-            );
-          }
-          this.loader.loadPlugins(plugins);
-        },
-        (e) => {
-          throw new CaseConfigurationError(
-            `Unable to load plugin: ${e.message}`,
-            'DONT_ADD_LOCATION',
-            'UNDOCUMENTED',
-          );
-        },
-      )
+        this.loader.loadPlugins(plugins);
+      })
       .then(() => new BoundarySuccess())
       .catch(jsErrorToFailure);
   }