[Plugins Discovery] Enforce camelCase plugin IDs

elastic · Feb 9, 2021 · fcac06d · fcac06d
1 parent 82d1672
commit fcac06d
Show file tree

Hide file tree

Showing 38 changed files with 82 additions and 96 deletions.
diff --git a/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts
@@ -9,12 +9,10 @@
 import { mockReadFile } from './plugin_manifest_parser.test.mocks';
 
 import { PluginDiscoveryErrorType } from './plugin_discovery_error';
-import { loggingSystemMock } from '../../logging/logging_system.mock';
 
 import { resolve } from 'path';
 import { parseManifest } from './plugin_manifest_parser';
 
-const logger = loggingSystemMock.createLogger();
 const pluginPath = resolve('path', 'existent-dir');
 const pluginManifestPath = resolve(pluginPath, 'kibana.json');
 const packageInfo = {
@@ -34,7 +32,7 @@ test('return error when manifest is empty', async () => {
     cb(null, Buffer.from(''));
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Unexpected end of JSON input (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
@@ -46,7 +44,7 @@ test('return error when manifest content is null', async () => {
     cb(null, Buffer.from('null'));
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Plugin manifest must contain a JSON encoded object. (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
@@ -58,7 +56,7 @@ test('return error when manifest content is not a valid JSON', async () => {
     cb(null, Buffer.from('not-json'));
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Unexpected token o in JSON at position 1 (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
@@ -70,7 +68,7 @@ test('return error when plugin id is missing', async () => {
     cb(null, Buffer.from(JSON.stringify({ version: 'some-version' })));
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Plugin manifest must contain an "id" property. (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
@@ -82,45 +80,32 @@ test('return error when plugin id includes `.` characters', async () => {
     cb(null, Buffer.from(JSON.stringify({ id: 'some.name', version: 'some-version' })));
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Plugin "id" must not include \`.\` characters. (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
   });
 });
 
-test('logs warning if pluginId is not in camelCase format', async () => {
+test('return error when pluginId is not in camelCase format', async () => {
+  expect.assertions(1);
   mockReadFile.mockImplementation((path, cb) => {
     cb(null, Buffer.from(JSON.stringify({ id: 'some_name', version: 'kibana', server: true })));
   });
 
-  expect(loggingSystemMock.collect(logger).warn).toHaveLength(0);
-  await parseManifest(pluginPath, packageInfo, logger);
-  expect(loggingSystemMock.collect(logger).warn).toMatchInlineSnapshot(`
-    Array [
-      Array [
-        "Expect plugin \\"id\\" in camelCase, but found: some_name",
-      ],
-    ]
-  `);
-});
-
-test('does not log pluginId format warning in dist mode', async () => {
-  mockReadFile.mockImplementation((path, cb) => {
-    cb(null, Buffer.from(JSON.stringify({ id: 'some_name', version: 'kibana', server: true })));
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
+    message: `Plugin "id" must be camelCase, but found: some_name. (invalid-manifest, ${pluginManifestPath})`,
+    type: PluginDiscoveryErrorType.InvalidManifest,
+    path: pluginManifestPath,
   });
-
-  expect(loggingSystemMock.collect(logger).warn).toHaveLength(0);
-  await parseManifest(pluginPath, { ...packageInfo, dist: true }, logger);
-  expect(loggingSystemMock.collect(logger).warn.length).toBe(0);
 });
 
 test('return error when plugin version is missing', async () => {
   mockReadFile.mockImplementation((path, cb) => {
     cb(null, Buffer.from(JSON.stringify({ id: 'someId' })));
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Plugin manifest for "someId" must contain a "version" property. (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
@@ -132,7 +117,7 @@ test('return error when plugin expected Kibana version is lower than actual vers
     cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '6.4.2' })));
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Plugin "someId" is only compatible with Kibana version "6.4.2", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.IncompatibleVersion,
     path: pluginManifestPath,
@@ -147,7 +132,7 @@ test('return error when plugin expected Kibana version cannot be interpreted as
     );
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Plugin "someId" is only compatible with Kibana version "non-sem-ver", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.IncompatibleVersion,
     path: pluginManifestPath,
@@ -159,7 +144,7 @@ test('return error when plugin config path is not a string', async () => {
     cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0', configPath: 2 })));
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `The "configPath" in plugin manifest for "someId" should either be a string or an array of strings. (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
@@ -174,7 +159,7 @@ test('return error when plugin config path is an array that contains non-string
     );
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `The "configPath" in plugin manifest for "someId" should either be a string or an array of strings. (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
@@ -186,7 +171,7 @@ test('return error when plugin expected Kibana version is higher than actual ver
     cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.1' })));
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Plugin "someId" is only compatible with Kibana version "7.0.1", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.IncompatibleVersion,
     path: pluginManifestPath,
@@ -198,7 +183,7 @@ test('return error when both `server` and `ui` are set to `false` or missing', a
     cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0' })));
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Both "server" and "ui" are missing or set to "false" in plugin manifest for "someId", but at least one of these must be set to "true". (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
@@ -211,7 +196,7 @@ test('return error when both `server` and `ui` are set to `false` or missing', a
     );
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Both "server" and "ui" are missing or set to "false" in plugin manifest for "someId", but at least one of these must be set to "true". (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
@@ -234,7 +219,7 @@ test('return error when manifest contains unrecognized properties', async () =>
     );
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
+  await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
     message: `Manifest for plugin "someId" contains the following unrecognized properties: unknownOne,unknownTwo. (invalid-manifest, ${pluginManifestPath})`,
     type: PluginDiscoveryErrorType.InvalidManifest,
     path: pluginManifestPath,
@@ -247,20 +232,20 @@ describe('configPath', () => {
       cb(null, Buffer.from(JSON.stringify({ id: 'plugin', version: '7.0.0', server: true })));
     });
 
-    const manifest = await parseManifest(pluginPath, packageInfo, logger);
+    const manifest = await parseManifest(pluginPath, packageInfo);
     expect(manifest.configPath).toBe(manifest.id);
   });
 
   test('falls back to plugin id in snakeCase format', async () => {
     mockReadFile.mockImplementation((path, cb) => {
-      cb(null, Buffer.from(JSON.stringify({ id: 'SomeId', version: '7.0.0', server: true })));
+      cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0', server: true })));
     });
 
-    const manifest = await parseManifest(pluginPath, packageInfo, logger);
+    const manifest = await parseManifest(pluginPath, packageInfo);
     expect(manifest.configPath).toBe('some_id');
   });
 
-  test('not formated to snakeCase if defined explicitly as string', async () => {
+  test('not formatted to snakeCase if defined explicitly as string', async () => {
     mockReadFile.mockImplementation((path, cb) => {
       cb(
         null,
@@ -270,11 +255,11 @@ describe('configPath', () => {
       );
     });
 
-    const manifest = await parseManifest(pluginPath, packageInfo, logger);
+    const manifest = await parseManifest(pluginPath, packageInfo);
     expect(manifest.configPath).toBe('somePath');
   });
 
-  test('not formated to snakeCase if defined explicitly as an array of strings', async () => {
+  test('not formatted to snakeCase if defined explicitly as an array of strings', async () => {
     mockReadFile.mockImplementation((path, cb) => {
       cb(
         null,
@@ -284,7 +269,7 @@ describe('configPath', () => {
       );
     });
 
-    const manifest = await parseManifest(pluginPath, packageInfo, logger);
+    const manifest = await parseManifest(pluginPath, packageInfo);
     expect(manifest.configPath).toEqual(['somePath']);
   });
 });
@@ -294,7 +279,7 @@ test('set defaults for all missing optional fields', async () => {
     cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0', server: true })));
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({
+  await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
     id: 'someId',
     configPath: 'some_id',
     version: '7.0.0',
@@ -325,7 +310,7 @@ test('return all set optional fields as they are in manifest', async () => {
     );
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({
+  await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
     id: 'someId',
     configPath: ['some', 'path'],
     version: 'some-version',
@@ -355,7 +340,7 @@ test('return manifest when plugin expected Kibana version matches actual version
     );
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({
+  await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
     id: 'someId',
     configPath: 'some-path',
     version: 'some-version',
@@ -385,7 +370,7 @@ test('return manifest when plugin expected Kibana version is `kibana`', async ()
     );
   });
 
-  await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({
+  await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
     id: 'someId',
     configPath: 'some_id',
     version: 'some-version',

diff --git a/src/core/server/plugins/discovery/plugin_manifest_parser.ts b/src/core/server/plugins/discovery/plugin_manifest_parser.ts
@@ -12,7 +12,6 @@ import { coerce } from 'semver';
 import { promisify } from 'util';
 import { snakeCase } from 'lodash';
 import { isConfigPath, PackageInfo } from '../../config';
-import { Logger } from '../../logging';
 import { PluginManifest } from '../types';
 import { PluginDiscoveryError } from './plugin_discovery_error';
 import { isCamelCase } from './is_camel_case';
@@ -63,8 +62,7 @@ const KNOWN_MANIFEST_FIELDS = (() => {
  */
 export async function parseManifest(
   pluginPath: string,
-  packageInfo: PackageInfo,
-  log: Logger
+  packageInfo: PackageInfo
 ): Promise<PluginManifest> {
   const manifestPath = resolve(pluginPath, MANIFEST_FILE_NAME);
 
@@ -105,8 +103,11 @@ export async function parseManifest(
     );
   }
 
-  if (!packageInfo.dist && !isCamelCase(manifest.id)) {
-    log.warn(`Expect plugin "id" in camelCase, but found: ${manifest.id}`);
+  if (!isCamelCase(manifest.id)) {
+    throw PluginDiscoveryError.invalidManifest(
+      manifestPath,
+      new Error(`Plugin "id" must be camelCase, but found: ${manifest.id}.`)
+    );
   }
 
   if (!manifest.version || typeof manifest.version !== 'string') {

diff --git a/src/core/server/plugins/discovery/plugins_discovery.ts b/src/core/server/plugins/discovery/plugins_discovery.ts
@@ -179,7 +179,7 @@ function createPlugin$(
   coreContext: CoreContext,
   instanceInfo: InstanceInfo
 ) {
-  return from(parseManifest(path, coreContext.env.packageInfo, log)).pipe(
+  return from(parseManifest(path, coreContext.env.packageInfo)).pipe(
     map((manifest) => {
       log.debug(`Successfully discovered plugin "${manifest.id}" at "${path}"`);
       const opaqueId = Symbol(manifest.id);

diff --git a/test/common/fixtures/plugins/newsfeed/kibana.json b/test/common/fixtures/plugins/newsfeed/kibana.json
@@ -1,5 +1,5 @@
 {
-  "id": "newsfeed-fixtures",
+  "id": "newsfeedFixtures",
   "version": "kibana",
   "server": true,
   "ui": false

diff --git a/test/plugin_functional/plugins/app_link_test/kibana.json b/test/plugin_functional/plugins/app_link_test/kibana.json
@@ -1,5 +1,5 @@
 {
-  "id": "app_link_test",
+  "id": "appLinkTest",
   "version": "0.0.1",
   "kibanaVersion": "kibana",
   "server": false,

diff --git a/test/plugin_functional/plugins/core_app_status/kibana.json b/test/plugin_functional/plugins/core_app_status/kibana.json
@@ -1,5 +1,5 @@
 {
-  "id": "core_app_status",
+  "id": "coreAppStatus",
   "version": "0.0.1",
   "kibanaVersion": "kibana",
   "configPath": ["core_app_status"],

diff --git a/test/plugin_functional/plugins/core_plugin_a/kibana.json b/test/plugin_functional/plugins/core_plugin_a/kibana.json
@@ -1,5 +1,5 @@
 {
-  "id": "core_plugin_a",
+  "id": "corePluginA",
   "version": "0.0.1",
   "kibanaVersion": "kibana",
   "configPath": ["core_plugin_a"],

diff --git a/test/plugin_functional/plugins/core_plugin_appleave/kibana.json b/test/plugin_functional/plugins/core_plugin_appleave/kibana.json
@@ -1,5 +1,5 @@
 {
-  "id": "core_plugin_appleave",
+  "id": "corePluginAppleave",
   "version": "0.0.1",
   "kibanaVersion": "kibana",
   "configPath": ["core_plugin_appleave"],

diff --git a/test/plugin_functional/plugins/core_plugin_b/kibana.json b/test/plugin_functional/plugins/core_plugin_b/kibana.json
@@ -1,10 +1,10 @@
 {
-  "id": "core_plugin_b",
+  "id": "corePluginB",
   "version": "0.0.1",
   "kibanaVersion": "kibana",
   "configPath": ["core_plugin_b"],
   "server": true,
   "ui": true,
-  "requiredPlugins": ["core_plugin_a"],
-  "optionalPlugins": ["core_plugin_c"]
+  "requiredPlugins": ["corePluginA"],
+  "optionalPlugins": ["corePluginC"]
 }
diff --git a/test/plugin_functional/plugins/core_plugin_b/public/plugin.tsx b/test/plugin_functional/plugins/core_plugin_b/public/plugin.tsx
@@ -16,7 +16,7 @@ declare global {
 }
 
 export interface CorePluginBDeps {
-  core_plugin_a: CorePluginAPluginSetup;
+  corePluginA: CorePluginAPluginSetup;
 }
 
 export class CorePluginBPlugin
@@ -37,7 +37,7 @@ export class CorePluginBPlugin
 
     return {
       sayHi() {
-        return `Plugin A said: ${deps.core_plugin_a.getGreeting()}`;
+        return `Plugin A said: ${deps.corePluginA.getGreeting()}`;
       },
     };
   }

diff --git a/test/plugin_functional/plugins/core_plugin_chromeless/kibana.json b/test/plugin_functional/plugins/core_plugin_chromeless/kibana.json
@@ -1,5 +1,5 @@
 {
-  "id": "core_plugin_chromeless",
+  "id": "corePluginChromeless",
   "version": "0.0.1",
   "kibanaVersion": "kibana",
   "configPath": ["core_plugin_chromeless"],

diff --git a/test/plugin_functional/plugins/core_plugin_helpmenu/kibana.json b/test/plugin_functional/plugins/core_plugin_helpmenu/kibana.json
@@ -1,5 +1,5 @@
 {
-  "id": "core_plugin_helpmenu",
+  "id": "corePluginHelpmenu",
   "version": "0.0.1",
   "kibanaVersion": "kibana",
   "configPath": ["core_plugin_helpmenu"],

diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/kibana.json b/test/plugin_functional/plugins/core_plugin_route_timeouts/kibana.json
@@ -1,5 +1,5 @@
 {
-  "id": "core_plugin_route_timeouts",
+  "id": "corePluginRouteTimeouts",
   "version": "0.0.1",
   "kibanaVersion": "kibana",
   "configPath": ["core_plugin_route_timeouts"],

diff --git a/test/plugin_functional/plugins/core_provider_plugin/kibana.json b/test/plugin_functional/plugins/core_provider_plugin/kibana.json
@@ -1,8 +1,8 @@
 {
-  "id": "core_provider_plugin",
+  "id": "coreProviderPlugin",
   "version": "0.0.1",
   "kibanaVersion": "kibana",
-  "optionalPlugins": ["core_plugin_a", "core_plugin_b", "licensing", "globalSearchTest"],
+  "optionalPlugins": ["corePluginA", "corePluginB", "licensing", "globalSearchTest"],
   "server": false,
   "ui": true
 }