PR review feedback

* Improve error message
* Add source field to PluginWrapper

Author: jportner

src/core/server/plugins/plugin.test.ts

@@ -39,6 +39,11 @@ jest.doMock(join('plugin-with-wrong-initializer-path', 'server'), () => ({ plugi
   virtual: true,
 });
 
+const OSS_PLUGIN_PATH_POSIX = '/kibana/src/plugins/ossPlugin';
+const OSS_PLUGIN_PATH_WINDOWS = 'C:\\kibana\\src\\plugins\\ossPlugin';
+const XPACK_PLUGIN_PATH_POSIX = '/kibana/x-pack/plugins/xPackPlugin';
+const XPACK_PLUGIN_PATH_WINDOWS = 'C:\\kibana\\x-pack\\plugins\\xPackPlugin';
+
 function createPluginManifest(manifestProps: Partial<PluginManifest> = {}): PluginManifest {
   return {
     id: 'some-plugin-id',
@@ -97,10 +102,49 @@ test('`constructor` correctly initializes plugin instance', () => {
   expect(plugin.name).toBe('some-plugin-id');
   expect(plugin.configPath).toBe('path');
   expect(plugin.path).toBe('some-plugin-path');
+  expect(plugin.source).toBe('external'); // see below for test cases for non-external sources (OSS and X-Pack)
   expect(plugin.requiredPlugins).toEqual(['some-required-dep']);
   expect(plugin.optionalPlugins).toEqual(['some-optional-dep']);
 });
 
+describe('`constructor` correctly sets non-external source', () => {
+  function createPlugin(path: string) {
+    const manifest = createPluginManifest();
+    const opaqueId = Symbol();
+    return new PluginWrapper({
+      path,
+      manifest,
+      opaqueId,
+      initializerContext: createPluginInitializerContext(
+        coreContext,
+        opaqueId,
+        manifest,
+        instanceInfo
+      ),
+    });
+  }
+
+  test('for OSS plugin in POSIX', () => {
+    const plugin = createPlugin(OSS_PLUGIN_PATH_POSIX);
+    expect(plugin.source).toBe('oss');
+  });
+
+  test('for OSS plugin in Windows', () => {
+    const plugin = createPlugin(OSS_PLUGIN_PATH_WINDOWS);
+    expect(plugin.source).toBe('oss');
+  });
+
+  test('for X-Pack plugin in POSIX', () => {
+    const plugin = createPlugin(XPACK_PLUGIN_PATH_POSIX);
+    expect(plugin.source).toBe('x-pack');
+  });
+
+  test('for X-Pack plugin in Windows', () => {
+    const plugin = createPlugin(XPACK_PLUGIN_PATH_WINDOWS);
+    expect(plugin.source).toBe('x-pack');
+  });
+});
+
 test('`setup` fails if `plugin` initializer is not exported', () => {
   const manifest = createPluginManifest();
   const opaqueId = Symbol();

src/core/server/plugins/plugin.ts

@@ -27,6 +27,9 @@ import {
 } from './types';
 import { CorePreboot, CoreSetup, CoreStart } from '..';
 
+const OSS_PATH_REGEX = /[\/|\\]src[\/|\\]plugins[\/|\\]/; // Matches src/plugins directory on POSIX and Windows
+const XPACK_PATH_REGEX = /[\/|\\]x-pack[\/|\\]plugins[\/|\\]/; // Matches x-pack/plugins directory on POSIX and Windows
+
 /**
  * Lightweight wrapper around discovered plugin that is responsible for instantiating
  * plugin and dispatching proper context and dependencies into plugin's lifecycle hooks.
@@ -40,6 +43,7 @@ export class PluginWrapper<
   TPluginsStart extends object = object
 > {
   public readonly path: string;
+  public readonly source: 'oss' | 'x-pack' | 'external';
   public readonly manifest: PluginManifest;
   public readonly opaqueId: PluginOpaqueId;
   public readonly name: PluginManifest['id'];
@@ -70,6 +74,7 @@ export class PluginWrapper<
     }
   ) {
     this.path = params.path;
+    this.source = getPluginSource(params.path);
     this.manifest = params.manifest;
     this.opaqueId = params.opaqueId;
     this.initializerContext = params.initializerContext;
@@ -204,3 +209,12 @@ export class PluginWrapper<
     return this.manifest.type === PluginType.preboot;
   }
 }
+
+function getPluginSource(path: string): 'oss' | 'x-pack' | 'external' {
+  if (OSS_PATH_REGEX.test(path)) {
+    return 'oss';
+  } else if (XPACK_PATH_REGEX.test(path)) {
+    return 'x-pack';
+  }
+  return 'external';
+}

src/core/server/plugins/plugins_service.test.ts

@@ -52,16 +52,9 @@ expect.addSnapshotSerializer(createAbsolutePathSerializer());
   });
 });
 
-const OSS_PLUGIN_PATH_POSIX = '/kibana/src/plugins/ossPlugin';
-const OSS_PLUGIN_PATH_WINDOWS = 'C:\\kibana\\src\\plugins\\ossPlugin';
-const XPACK_PLUGIN_PATH_POSIX = '/kibana/x-pack/plugins/xPackPlugin';
-const XPACK_PLUGIN_PATH_WINDOWS = 'C:\\kibana\\x-pack\\plugins\\xPackPlugin';
-[
-  OSS_PLUGIN_PATH_POSIX,
-  OSS_PLUGIN_PATH_WINDOWS,
-  XPACK_PLUGIN_PATH_POSIX,
-  XPACK_PLUGIN_PATH_WINDOWS,
-].forEach((path) => {
+const OSS_PLUGIN_PATH = '/kibana/src/plugins/ossPlugin';
+const XPACK_PLUGIN_PATH = '/kibana/x-pack/plugins/xPackPlugin';
+[OSS_PLUGIN_PATH, XPACK_PLUGIN_PATH].forEach((path) => {
   jest.doMock(join(path, 'server'), () => ({}), {
     virtual: true,
   });
@@ -272,6 +265,7 @@ describe('PluginsService', () => {
         xPackPath: string;
         dependency: 'requiredPlugin' | 'requiredBundle' | 'optionalPlugin';
       }) {
+        // Each plugin's source is derived from its path; the PluginWrapper test suite contains more detailed test cases around the paths (for both POSIX and Windows)
         mockDiscover.mockReturnValue({
           error$: from([]),
           plugin$: from([
@@ -296,7 +290,7 @@ describe('PluginsService', () => {
 
       async function expectError() {
         await expect(pluginsService.discover({ environment: environmentPreboot })).rejects.toThrow(
-          `X-Pack plugin or bundle with id "xPackPlugin" is required by OSS plugin "ossPlugin", which is prohibited.`
+          `X-Pack plugin or bundle with id "xPackPlugin" is required by OSS plugin "ossPlugin", which is prohibited. Consider making this an optional dependency instead.`
         );
         expect(standardMockPluginSystem.addPlugin).not.toHaveBeenCalled();
       }
@@ -307,64 +301,31 @@ describe('PluginsService', () => {
         expect(standardMockPluginSystem.addPlugin).toHaveBeenCalledTimes(2);
       }
 
-      describe('throws if an OSS plugin requires an X-Pack plugin', () => {
-        it('in POSIX', async () => {
-          mockDiscoveryResults({
-            ossPath: OSS_PLUGIN_PATH_POSIX,
-            xPackPath: XPACK_PLUGIN_PATH_POSIX,
-            dependency: 'requiredPlugin',
-          });
-          await expectError();
-        });
-
-        it('in Windows', async () => {
-          mockDiscoveryResults({
-            ossPath: OSS_PLUGIN_PATH_WINDOWS,
-            xPackPath: XPACK_PLUGIN_PATH_WINDOWS,
-            dependency: 'requiredPlugin',
-          });
-          await expectError();
+      it('throws if an OSS plugin requires an X-Pack plugin', async () => {
+        mockDiscoveryResults({
+          ossPath: OSS_PLUGIN_PATH,
+          xPackPath: XPACK_PLUGIN_PATH,
+          dependency: 'requiredPlugin',
         });
+        await expectError();
       });
 
-      describe('throws if an OSS plugin requires an X-Pack bundle', () => {
-        it('in POSIX', async () => {
-          mockDiscoveryResults({
-            ossPath: OSS_PLUGIN_PATH_POSIX,
-            xPackPath: XPACK_PLUGIN_PATH_POSIX,
-            dependency: 'requiredBundle',
-          });
-          await expectError();
-        });
-
-        it('in Windows', async () => {
-          mockDiscoveryResults({
-            ossPath: OSS_PLUGIN_PATH_WINDOWS,
-            xPackPath: XPACK_PLUGIN_PATH_WINDOWS,
-            dependency: 'requiredBundle',
-          });
-          await expectError();
+      it('throws if an OSS plugin requires an X-Pack bundle', async () => {
+        mockDiscoveryResults({
+          ossPath: OSS_PLUGIN_PATH,
+          xPackPath: XPACK_PLUGIN_PATH,
+          dependency: 'requiredBundle',
         });
+        await expectError();
       });
 
-      describe('does not throw if an OSS plugin has an optional dependency on an X-Pack plugin', () => {
-        it('in POSIX', async () => {
-          mockDiscoveryResults({
-            ossPath: OSS_PLUGIN_PATH_POSIX,
-            xPackPath: XPACK_PLUGIN_PATH_POSIX,
-            dependency: 'optionalPlugin',
-          });
-          await expectSuccess();
-        });
-
-        it('in Windows', async () => {
-          mockDiscoveryResults({
-            ossPath: OSS_PLUGIN_PATH_WINDOWS,
-            xPackPath: XPACK_PLUGIN_PATH_WINDOWS,
-            dependency: 'optionalPlugin',
-          });
-          await expectSuccess();
+      it('does not throw if an OSS plugin has an optional dependency on an X-Pack plugin', async () => {
+        mockDiscoveryResults({
+          ossPath: OSS_PLUGIN_PATH,
+          xPackPath: XPACK_PLUGIN_PATH,
+          dependency: 'optionalPlugin',
         });
+        await expectSuccess();
       });
     });
 

src/core/server/plugins/plugins_service.ts

@@ -86,9 +86,6 @@ export interface PluginsServiceDiscoverDeps {
   environment: InternalEnvironmentServicePreboot;
 }
 
-const OSS_PATH_REGEX = /[\/|\\]src[\/|\\]plugins[\/|\\]/; // Matches src/plugins directory on POSIX and Windows
-const XPACK_PATH_REGEX = /[\/|\\]x-pack[\/|\\]plugins[\/|\\]/; // Matches x-pack/plugins directory on POSIX and Windows
-
 /** @internal */
 export class PluginsService implements CoreService<PluginsServiceSetup, PluginsServiceStart> {
   private readonly log: Logger;
@@ -340,12 +337,12 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
       }
 
       // validate that OSS plugins do not have required dependencies on X-Pack plugins
-      if (OSS_PATH_REGEX.test(plugin.path)) {
+      if (plugin.source === 'oss') {
         for (const id of [...plugin.requiredPlugins, ...plugin.requiredBundles]) {
           const requiredPlugin = pluginEnableStatuses.get(id);
-          if (requiredPlugin && XPACK_PATH_REGEX.test(requiredPlugin.plugin.path)) {
+          if (requiredPlugin && requiredPlugin.plugin.source === 'x-pack') {
             throw new Error(
-              `X-Pack plugin or bundle with id "${id}" is required by OSS plugin "${pluginName}", which is prohibited.`
+              `X-Pack plugin or bundle with id "${id}" is required by OSS plugin "${pluginName}", which is prohibited. Consider making this an optional dependency instead.`
             );
           }
         }