From 666eda060b9e4afd11e2fe12ad0d0b0056d56413 Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Sat, 18 Jan 2020 15:44:32 +0100 Subject: [PATCH] Enforce camelCase format for a plugin id (#53759) (#55270) * add isCamelCase function * add a warning if id is not in camelCase * document pluginId expected in camelCase * regen docs * add a test for logging * update tests. warn can be called several times for different reasons * pluginPath falls back to plugin id in snake_case * update tests * update docs * add example with id & configPath different formats Co-authored-by: Elastic Machine Co-authored-by: Elastic Machine --- ...ugin-server.discoveredplugin.configpath.md | 2 +- .../kibana-plugin-server.discoveredplugin.md | 2 +- ...plugin-server.pluginmanifest.configpath.md | 7 +- .../kibana-plugin-server.pluginmanifest.id.md | 2 +- .../kibana-plugin-server.pluginmanifest.md | 4 +- src/core/CONVENTIONS.md | 1 + .../config_deprecation.test.ts | 25 +-- .../plugins/discovery/is_camel_case.test.ts | 42 +++++ .../server/plugins/discovery/is_camel_case.ts | 22 +++ .../discovery/plugin_manifest_parser.test.ts | 158 +++++++++++++----- .../discovery/plugin_manifest_parser.ts | 11 +- .../plugins/discovery/plugins_discovery.ts | 2 +- src/core/server/plugins/types.ts | 11 +- src/core/server/server.api.md | 8 +- 14 files changed, 224 insertions(+), 73 deletions(-) create mode 100644 src/core/server/plugins/discovery/is_camel_case.test.ts create mode 100644 src/core/server/plugins/discovery/is_camel_case.ts diff --git a/docs/development/core/server/kibana-plugin-server.discoveredplugin.configpath.md b/docs/development/core/server/kibana-plugin-server.discoveredplugin.configpath.md index b0830b8d72238b..4de20b9c6cccfe 100644 --- a/docs/development/core/server/kibana-plugin-server.discoveredplugin.configpath.md +++ b/docs/development/core/server/kibana-plugin-server.discoveredplugin.configpath.md @@ -4,7 +4,7 @@ ## DiscoveredPlugin.configPath property -Root configuration path used by the plugin, defaults to "id". +Root configuration path used by the plugin, defaults to "id" in snake\_case format. Signature: diff --git a/docs/development/core/server/kibana-plugin-server.discoveredplugin.md b/docs/development/core/server/kibana-plugin-server.discoveredplugin.md index aadf9763b16049..ea13422458c7fc 100644 --- a/docs/development/core/server/kibana-plugin-server.discoveredplugin.md +++ b/docs/development/core/server/kibana-plugin-server.discoveredplugin.md @@ -16,7 +16,7 @@ export interface DiscoveredPlugin | Property | Type | Description | | --- | --- | --- | -| [configPath](./kibana-plugin-server.discoveredplugin.configpath.md) | ConfigPath | Root configuration path used by the plugin, defaults to "id". | +| [configPath](./kibana-plugin-server.discoveredplugin.configpath.md) | ConfigPath | Root configuration path used by the plugin, defaults to "id" in snake\_case format. | | [id](./kibana-plugin-server.discoveredplugin.id.md) | PluginName | Identifier of the plugin. | | [optionalPlugins](./kibana-plugin-server.discoveredplugin.optionalplugins.md) | readonly PluginName[] | An optional list of the other plugins that if installed and enabled \*\*may be\*\* leveraged by this plugin for some additional functionality but otherwise are not required for this plugin to work properly. | | [requiredPlugins](./kibana-plugin-server.discoveredplugin.requiredplugins.md) | readonly PluginName[] | An optional list of the other plugins that \*\*must be\*\* installed and enabled for this plugin to function properly. | diff --git a/docs/development/core/server/kibana-plugin-server.pluginmanifest.configpath.md b/docs/development/core/server/kibana-plugin-server.pluginmanifest.configpath.md index 39c1eeda47e0ec..6ffe396aa2ed1d 100644 --- a/docs/development/core/server/kibana-plugin-server.pluginmanifest.configpath.md +++ b/docs/development/core/server/kibana-plugin-server.pluginmanifest.configpath.md @@ -4,10 +4,15 @@ ## PluginManifest.configPath property -Root [configuration path](./kibana-plugin-server.configpath.md) used by the plugin, defaults to "id". +Root [configuration path](./kibana-plugin-server.configpath.md) used by the plugin, defaults to "id" in snake\_case format. Signature: ```typescript readonly configPath: ConfigPath; ``` + +## Example + +id: myPlugin configPath: my\_plugin + diff --git a/docs/development/core/server/kibana-plugin-server.pluginmanifest.id.md b/docs/development/core/server/kibana-plugin-server.pluginmanifest.id.md index 44e61f11fa215b..104046f3ce7d0c 100644 --- a/docs/development/core/server/kibana-plugin-server.pluginmanifest.id.md +++ b/docs/development/core/server/kibana-plugin-server.pluginmanifest.id.md @@ -4,7 +4,7 @@ ## PluginManifest.id property -Identifier of the plugin. +Identifier of the plugin. Must be a string in camelCase. Part of a plugin public contract. Other plugins leverage it to access plugin API, navigate to the plugin, etc. Signature: diff --git a/docs/development/core/server/kibana-plugin-server.pluginmanifest.md b/docs/development/core/server/kibana-plugin-server.pluginmanifest.md index 9bb208a809b22d..c39a702389fb3f 100644 --- a/docs/development/core/server/kibana-plugin-server.pluginmanifest.md +++ b/docs/development/core/server/kibana-plugin-server.pluginmanifest.md @@ -20,8 +20,8 @@ Should never be used in code outside of Core but is exported for documentation p | Property | Type | Description | | --- | --- | --- | -| [configPath](./kibana-plugin-server.pluginmanifest.configpath.md) | ConfigPath | Root [configuration path](./kibana-plugin-server.configpath.md) used by the plugin, defaults to "id". | -| [id](./kibana-plugin-server.pluginmanifest.id.md) | PluginName | Identifier of the plugin. | +| [configPath](./kibana-plugin-server.pluginmanifest.configpath.md) | ConfigPath | Root [configuration path](./kibana-plugin-server.configpath.md) used by the plugin, defaults to "id" in snake\_case format. | +| [id](./kibana-plugin-server.pluginmanifest.id.md) | PluginName | Identifier of the plugin. Must be a string in camelCase. Part of a plugin public contract. Other plugins leverage it to access plugin API, navigate to the plugin, etc. | | [kibanaVersion](./kibana-plugin-server.pluginmanifest.kibanaversion.md) | string | The version of Kibana the plugin is compatible with, defaults to "version". | | [optionalPlugins](./kibana-plugin-server.pluginmanifest.optionalplugins.md) | readonly PluginName[] | An optional list of the other plugins that if installed and enabled \*\*may be\*\* leveraged by this plugin for some additional functionality but otherwise are not required for this plugin to work properly. | | [requiredPlugins](./kibana-plugin-server.pluginmanifest.requiredplugins.md) | readonly PluginName[] | An optional list of the other plugins that \*\*must be\*\* installed and enabled for this plugin to function properly. | diff --git a/src/core/CONVENTIONS.md b/src/core/CONVENTIONS.md index 18f82766bdbc16..1245ffb92059fb 100644 --- a/src/core/CONVENTIONS.md +++ b/src/core/CONVENTIONS.md @@ -32,6 +32,7 @@ my_plugin/    ├── index.ts    └── plugin.ts ``` +- [Manifest file](/docs/development/core/server/kibana-plugin-server.pluginmanifest.md) should be defined on top level. - Both `server` and `public` should have an `index.ts` and a `plugin.ts` file: - `index.ts` should only contain: - The `plugin` export diff --git a/src/core/server/config/integration_tests/config_deprecation.test.ts b/src/core/server/config/integration_tests/config_deprecation.test.ts index e85f8567bfc683..5bc3887f05f931 100644 --- a/src/core/server/config/integration_tests/config_deprecation.test.ts +++ b/src/core/server/config/integration_tests/config_deprecation.test.ts @@ -36,7 +36,13 @@ describe('configuration deprecations', () => { await root.setup(); const logs = loggingServiceMock.collect(mockLoggingService); - expect(logs.warn).toMatchInlineSnapshot(`Array []`); + const warnings = logs.warn.flatMap(i => i); + expect(warnings).not.toContain( + '"optimize.lazy" is deprecated and has been replaced by "optimize.watch"' + ); + expect(warnings).not.toContain( + '"optimize.lazyPort" is deprecated and has been replaced by "optimize.watchPort"' + ); }); it('should log deprecation warnings for core deprecations', async () => { @@ -50,15 +56,12 @@ describe('configuration deprecations', () => { await root.setup(); const logs = loggingServiceMock.collect(mockLoggingService); - expect(logs.warn).toMatchInlineSnapshot(` - Array [ - Array [ - "\\"optimize.lazy\\" is deprecated and has been replaced by \\"optimize.watch\\"", - ], - Array [ - "\\"optimize.lazyPort\\" is deprecated and has been replaced by \\"optimize.watchPort\\"", - ], - ] - `); + const warnings = logs.warn.flatMap(i => i); + expect(warnings).toContain( + '"optimize.lazy" is deprecated and has been replaced by "optimize.watch"' + ); + expect(warnings).toContain( + '"optimize.lazyPort" is deprecated and has been replaced by "optimize.watchPort"' + ); }); }); diff --git a/src/core/server/plugins/discovery/is_camel_case.test.ts b/src/core/server/plugins/discovery/is_camel_case.test.ts new file mode 100644 index 00000000000000..eb8cb8f170dac1 --- /dev/null +++ b/src/core/server/plugins/discovery/is_camel_case.test.ts @@ -0,0 +1,42 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { isCamelCase } from './is_camel_case'; + +describe('isCamelCase', () => { + it('matches a string in camelCase', () => { + expect(isCamelCase('foo')).toBe(true); + expect(isCamelCase('foo1')).toBe(true); + expect(isCamelCase('fooBar')).toBe(true); + expect(isCamelCase('fooBarBaz')).toBe(true); + expect(isCamelCase('fooBAR')).toBe(true); + }); + + it('does not match strings in other cases', () => { + expect(isCamelCase('AAA')).toBe(false); + expect(isCamelCase('FooBar')).toBe(false); + expect(isCamelCase('3Foo')).toBe(false); + expect(isCamelCase('o_O')).toBe(false); + expect(isCamelCase('foo_bar')).toBe(false); + expect(isCamelCase('foo_')).toBe(false); + expect(isCamelCase('_fooBar')).toBe(false); + expect(isCamelCase('fooBar_')).toBe(false); + expect(isCamelCase('_fooBar_')).toBe(false); + }); +}); diff --git a/src/core/server/plugins/discovery/is_camel_case.ts b/src/core/server/plugins/discovery/is_camel_case.ts new file mode 100644 index 00000000000000..12069ec473f8de --- /dev/null +++ b/src/core/server/plugins/discovery/is_camel_case.ts @@ -0,0 +1,22 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +const camelCaseRegExp = /^[a-z]{1}([a-zA-Z0-9]{1,})$/; +export function isCamelCase(candidate: string) { + return camelCaseRegExp.test(candidate); +} diff --git a/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts index 17b1ac7b860452..979accb1f769ef 100644 --- a/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts +++ b/src/core/server/plugins/discovery/plugin_manifest_parser.test.ts @@ -20,10 +20,12 @@ import { PluginDiscoveryErrorType } from './plugin_discovery_error'; import { mockReadFile } from './plugin_manifest_parser.test.mocks'; +import { loggingServiceMock } from '../../logging/logging_service.mock'; import { resolve } from 'path'; import { parseManifest } from './plugin_manifest_parser'; +const logger = loggingServiceMock.createLogger(); const pluginPath = resolve('path', 'existent-dir'); const pluginManifestPath = resolve(pluginPath, 'kibana.json'); const packageInfo = { @@ -43,7 +45,7 @@ test('return error when manifest is empty', async () => { cb(null, Buffer.from('')); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ + await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({ message: `Unexpected end of JSON input (invalid-manifest, ${pluginManifestPath})`, type: PluginDiscoveryErrorType.InvalidManifest, path: pluginManifestPath, @@ -55,7 +57,7 @@ test('return error when manifest content is null', async () => { cb(null, Buffer.from('null')); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ + await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({ message: `Plugin manifest must contain a JSON encoded object. (invalid-manifest, ${pluginManifestPath})`, type: PluginDiscoveryErrorType.InvalidManifest, path: pluginManifestPath, @@ -67,7 +69,7 @@ test('return error when manifest content is not a valid JSON', async () => { cb(null, Buffer.from('not-json')); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ + await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({ message: `Unexpected token o in JSON at position 1 (invalid-manifest, ${pluginManifestPath})`, type: PluginDiscoveryErrorType.InvalidManifest, path: pluginManifestPath, @@ -79,7 +81,7 @@ test('return error when plugin id is missing', async () => { cb(null, Buffer.from(JSON.stringify({ version: 'some-version' }))); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ + await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({ message: `Plugin manifest must contain an "id" property. (invalid-manifest, ${pluginManifestPath})`, type: PluginDiscoveryErrorType.InvalidManifest, path: pluginManifestPath, @@ -91,20 +93,36 @@ 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)).rejects.toMatchObject({ + await expect(parseManifest(pluginPath, packageInfo, logger)).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 () => { + mockReadFile.mockImplementation((path, cb) => { + cb(null, Buffer.from(JSON.stringify({ id: 'some_name', version: 'kibana', server: true }))); + }); + + expect(loggingServiceMock.collect(logger).warn).toHaveLength(0); + await parseManifest(pluginPath, packageInfo, logger); + expect(loggingServiceMock.collect(logger).warn).toMatchInlineSnapshot(` + Array [ + Array [ + "Expect plugin \\"id\\" in camelCase, but found: some_name", + ], + ] + `); +}); + test('return error when plugin version is missing', async () => { mockReadFile.mockImplementation((path, cb) => { - cb(null, Buffer.from(JSON.stringify({ id: 'some-id' }))); + cb(null, Buffer.from(JSON.stringify({ id: 'someId' }))); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ - message: `Plugin manifest for "some-id" must contain a "version" property. (invalid-manifest, ${pluginManifestPath})`, + await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({ + message: `Plugin manifest for "someId" must contain a "version" property. (invalid-manifest, ${pluginManifestPath})`, type: PluginDiscoveryErrorType.InvalidManifest, path: pluginManifestPath, }); @@ -112,11 +130,11 @@ test('return error when plugin version is missing', async () => { test('return error when plugin expected Kibana version is lower than actual version', async () => { mockReadFile.mockImplementation((path, cb) => { - cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '6.4.2' }))); + cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '6.4.2' }))); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ - message: `Plugin "some-id" is only compatible with Kibana version "6.4.2", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${pluginManifestPath})`, + await expect(parseManifest(pluginPath, packageInfo, logger)).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, }); @@ -126,12 +144,12 @@ test('return error when plugin expected Kibana version cannot be interpreted as mockReadFile.mockImplementation((path, cb) => { cb( null, - Buffer.from(JSON.stringify({ id: 'some-id', version: '1.0.0', kibanaVersion: 'non-sem-ver' })) + Buffer.from(JSON.stringify({ id: 'someId', version: '1.0.0', kibanaVersion: 'non-sem-ver' })) ); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ - message: `Plugin "some-id" is only compatible with Kibana version "non-sem-ver", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${pluginManifestPath})`, + await expect(parseManifest(pluginPath, packageInfo, logger)).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, }); @@ -139,11 +157,11 @@ test('return error when plugin expected Kibana version cannot be interpreted as test('return error when plugin config path is not a string', async () => { mockReadFile.mockImplementation((path, cb) => { - cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.0', configPath: 2 }))); + cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0', configPath: 2 }))); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ - message: `The "configPath" in plugin manifest for "some-id" should either be a string or an array of strings. (invalid-manifest, ${pluginManifestPath})`, + await expect(parseManifest(pluginPath, packageInfo, logger)).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, }); @@ -153,12 +171,12 @@ test('return error when plugin config path is an array that contains non-string mockReadFile.mockImplementation((path, cb) => { cb( null, - Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.0', configPath: ['config', 2] })) + Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0', configPath: ['config', 2] })) ); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ - message: `The "configPath" in plugin manifest for "some-id" should either be a string or an array of strings. (invalid-manifest, ${pluginManifestPath})`, + await expect(parseManifest(pluginPath, packageInfo, logger)).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, }); @@ -166,11 +184,11 @@ test('return error when plugin config path is an array that contains non-string test('return error when plugin expected Kibana version is higher than actual version', async () => { mockReadFile.mockImplementation((path, cb) => { - cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.1' }))); + cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.1' }))); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ - message: `Plugin "some-id" is only compatible with Kibana version "7.0.1", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${pluginManifestPath})`, + await expect(parseManifest(pluginPath, packageInfo, logger)).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, }); @@ -178,11 +196,11 @@ test('return error when plugin expected Kibana version is higher than actual ver test('return error when both `server` and `ui` are set to `false` or missing', async () => { mockReadFile.mockImplementation((path, cb) => { - cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.0' }))); + cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0' }))); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ - message: `Both "server" and "ui" are missing or set to "false" in plugin manifest for "some-id", but at least one of these must be set to "true". (invalid-manifest, ${pluginManifestPath})`, + await expect(parseManifest(pluginPath, packageInfo, logger)).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, }); @@ -190,12 +208,12 @@ test('return error when both `server` and `ui` are set to `false` or missing', a mockReadFile.mockImplementation((path, cb) => { cb( null, - Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.0', server: false, ui: false })) + Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0', server: false, ui: false })) ); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ - message: `Both "server" and "ui" are missing or set to "false" in plugin manifest for "some-id", but at least one of these must be set to "true". (invalid-manifest, ${pluginManifestPath})`, + await expect(parseManifest(pluginPath, packageInfo, logger)).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, }); @@ -207,7 +225,7 @@ test('return error when manifest contains unrecognized properties', async () => null, Buffer.from( JSON.stringify({ - id: 'some-id', + id: 'someId', version: '7.0.0', server: true, unknownOne: 'one', @@ -217,21 +235,69 @@ test('return error when manifest contains unrecognized properties', async () => ); }); - await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({ - message: `Manifest for plugin "some-id" contains the following unrecognized properties: unknownOne,unknownTwo. (invalid-manifest, ${pluginManifestPath})`, + await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({ + message: `Manifest for plugin "someId" contains the following unrecognized properties: unknownOne,unknownTwo. (invalid-manifest, ${pluginManifestPath})`, type: PluginDiscoveryErrorType.InvalidManifest, path: pluginManifestPath, }); }); +describe('configPath', () => { + test('falls back to plugin id if not specified', async () => { + mockReadFile.mockImplementation((path, cb) => { + cb(null, Buffer.from(JSON.stringify({ id: 'plugin', version: '7.0.0', server: true }))); + }); + + const manifest = await parseManifest(pluginPath, packageInfo, logger); + 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 }))); + }); + + const manifest = await parseManifest(pluginPath, packageInfo, logger); + expect(manifest.configPath).toBe('some_id'); + }); + + test('not formated to snakeCase if defined explicitly as string', async () => { + mockReadFile.mockImplementation((path, cb) => { + cb( + null, + Buffer.from( + JSON.stringify({ id: 'someId', configPath: 'somePath', version: '7.0.0', server: true }) + ) + ); + }); + + const manifest = await parseManifest(pluginPath, packageInfo, logger); + expect(manifest.configPath).toBe('somePath'); + }); + + test('not formated to snakeCase if defined explicitly as an array of strings', async () => { + mockReadFile.mockImplementation((path, cb) => { + cb( + null, + Buffer.from( + JSON.stringify({ id: 'someId', configPath: ['somePath'], version: '7.0.0', server: true }) + ) + ); + }); + + const manifest = await parseManifest(pluginPath, packageInfo, logger); + expect(manifest.configPath).toEqual(['somePath']); + }); +}); + test('set defaults for all missing optional fields', async () => { mockReadFile.mockImplementation((path, cb) => { - cb(null, Buffer.from(JSON.stringify({ id: 'some-id', version: '7.0.0', server: true }))); + cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0', server: true }))); }); - await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({ - id: 'some-id', - configPath: 'some-id', + await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({ + id: 'someId', + configPath: 'some_id', version: '7.0.0', kibanaVersion: '7.0.0', optionalPlugins: [], @@ -247,7 +313,7 @@ test('return all set optional fields as they are in manifest', async () => { null, Buffer.from( JSON.stringify({ - id: 'some-id', + id: 'someId', configPath: ['some', 'path'], version: 'some-version', kibanaVersion: '7.0.0', @@ -259,8 +325,8 @@ test('return all set optional fields as they are in manifest', async () => { ); }); - await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({ - id: 'some-id', + await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({ + id: 'someId', configPath: ['some', 'path'], version: 'some-version', kibanaVersion: '7.0.0', @@ -277,7 +343,7 @@ test('return manifest when plugin expected Kibana version matches actual version null, Buffer.from( JSON.stringify({ - id: 'some-id', + id: 'someId', configPath: 'some-path', version: 'some-version', kibanaVersion: '7.0.0-alpha2', @@ -288,8 +354,8 @@ test('return manifest when plugin expected Kibana version matches actual version ); }); - await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({ - id: 'some-id', + await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({ + id: 'someId', configPath: 'some-path', version: 'some-version', kibanaVersion: '7.0.0-alpha2', @@ -306,7 +372,7 @@ test('return manifest when plugin expected Kibana version is `kibana`', async () null, Buffer.from( JSON.stringify({ - id: 'some-id', + id: 'someId', version: 'some-version', kibanaVersion: 'kibana', requiredPlugins: ['some-required-plugin'], @@ -317,9 +383,9 @@ test('return manifest when plugin expected Kibana version is `kibana`', async () ); }); - await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({ - id: 'some-id', - configPath: 'some-id', + await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({ + id: 'someId', + configPath: 'some_id', version: 'some-version', kibanaVersion: 'kibana', optionalPlugins: [], diff --git a/src/core/server/plugins/discovery/plugin_manifest_parser.ts b/src/core/server/plugins/discovery/plugin_manifest_parser.ts index 93c993a0fa3738..573109c9db35a0 100644 --- a/src/core/server/plugins/discovery/plugin_manifest_parser.ts +++ b/src/core/server/plugins/discovery/plugin_manifest_parser.ts @@ -21,9 +21,12 @@ import { readFile, stat } from 'fs'; import { resolve } from 'path'; 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'; const fsReadFileAsync = promisify(readFile); const fsStatAsync = promisify(stat); @@ -67,7 +70,7 @@ const KNOWN_MANIFEST_FIELDS = (() => { * @param packageInfo Kibana package info. * @internal */ -export async function parseManifest(pluginPath: string, packageInfo: PackageInfo) { +export async function parseManifest(pluginPath: string, packageInfo: PackageInfo, log: Logger) { const manifestPath = resolve(pluginPath, MANIFEST_FILE_NAME); let manifestContent; @@ -107,6 +110,10 @@ export async function parseManifest(pluginPath: string, packageInfo: PackageInfo ); } + if (!isCamelCase(manifest.id)) { + log.warn(`Expect plugin "id" in camelCase, but found: ${manifest.id}`); + } + if (!manifest.version || typeof manifest.version !== 'string') { throw PluginDiscoveryError.invalidManifest( manifestPath, @@ -161,7 +168,7 @@ export async function parseManifest(pluginPath: string, packageInfo: PackageInfo id: manifest.id, version: manifest.version, kibanaVersion: expectedKibanaVersion, - configPath: manifest.configPath || manifest.id, + configPath: manifest.configPath || snakeCase(manifest.id), requiredPlugins: Array.isArray(manifest.requiredPlugins) ? manifest.requiredPlugins : [], optionalPlugins: Array.isArray(manifest.optionalPlugins) ? manifest.optionalPlugins : [], ui: includesUiPlugin, diff --git a/src/core/server/plugins/discovery/plugins_discovery.ts b/src/core/server/plugins/discovery/plugins_discovery.ts index 79238afdf5c81b..e7f82c9dc15ada 100644 --- a/src/core/server/plugins/discovery/plugins_discovery.ts +++ b/src/core/server/plugins/discovery/plugins_discovery.ts @@ -112,7 +112,7 @@ function processPluginSearchPaths$(pluginDirs: readonly string[], log: Logger) { * @param coreContext Kibana core context. */ function createPlugin$(path: string, log: Logger, coreContext: CoreContext) { - return from(parseManifest(path, coreContext.env.packageInfo)).pipe( + return from(parseManifest(path, coreContext.env.packageInfo, log)).pipe( map(manifest => { log.debug(`Successfully discovered plugin "${manifest.id}" at "${path}"`); const opaqueId = Symbol(manifest.id); diff --git a/src/core/server/plugins/types.ts b/src/core/server/plugins/types.ts index e717871912f46f..a89e2f8c684e40 100644 --- a/src/core/server/plugins/types.ts +++ b/src/core/server/plugins/types.ts @@ -105,7 +105,8 @@ export type PluginOpaqueId = symbol; */ export interface PluginManifest { /** - * Identifier of the plugin. + * Identifier of the plugin. Must be a string in camelCase. Part of a plugin public contract. + * Other plugins leverage it to access plugin API, navigate to the plugin, etc. */ readonly id: PluginName; @@ -121,7 +122,11 @@ export interface PluginManifest { /** * Root {@link ConfigPath | configuration path} used by the plugin, defaults - * to "id". + * to "id" in snake_case format. + * + * @example + * id: myPlugin + * configPath: my_plugin */ readonly configPath: ConfigPath; @@ -162,7 +167,7 @@ export interface DiscoveredPlugin { readonly id: PluginName; /** - * Root configuration path used by the plugin, defaults to "id". + * Root configuration path used by the plugin, defaults to "id" in snake_case format. */ readonly configPath: ConfigPath; diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 6a58666716f420..7e3fc2eb660011 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -2005,9 +2005,9 @@ export const validBodyOutput: readonly ["data", "stream"]; // src/core/server/legacy/types.ts:161:3 - (ae-forgotten-export) The symbol "LegacyAppSpec" needs to be exported by the entry point index.d.ts // src/core/server/legacy/types.ts:162:16 - (ae-forgotten-export) The symbol "LegacyPluginSpec" needs to be exported by the entry point index.d.ts // src/core/server/plugins/plugins_service.ts:43:5 - (ae-forgotten-export) The symbol "InternalPluginInfo" needs to be exported by the entry point index.d.ts -// src/core/server/plugins/types.ts:221:3 - (ae-forgotten-export) The symbol "KibanaConfigType" needs to be exported by the entry point index.d.ts -// src/core/server/plugins/types.ts:221:3 - (ae-forgotten-export) The symbol "SharedGlobalConfigKeys" needs to be exported by the entry point index.d.ts -// src/core/server/plugins/types.ts:222:3 - (ae-forgotten-export) The symbol "ElasticsearchConfigType" needs to be exported by the entry point index.d.ts -// src/core/server/plugins/types.ts:223:3 - (ae-forgotten-export) The symbol "PathConfigType" needs to be exported by the entry point index.d.ts +// src/core/server/plugins/types.ts:226:3 - (ae-forgotten-export) The symbol "KibanaConfigType" needs to be exported by the entry point index.d.ts +// src/core/server/plugins/types.ts:226:3 - (ae-forgotten-export) The symbol "SharedGlobalConfigKeys" needs to be exported by the entry point index.d.ts +// src/core/server/plugins/types.ts:227:3 - (ae-forgotten-export) The symbol "ElasticsearchConfigType" needs to be exported by the entry point index.d.ts +// src/core/server/plugins/types.ts:228:3 - (ae-forgotten-export) The symbol "PathConfigType" needs to be exported by the entry point index.d.ts ```