Skip to content

Commit 8aaf168

Browse files
afharokibanamachine
afharo
and
kibanamachine
authored
[7.x] [Plugins Discovery] Enforce camelCase plugin IDs (#90752) (#91140)
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
1 parent 5249766 commit 8aaf168

42 files changed

Lines changed: 86 additions & 100 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎src/core/server/plugins/discovery/plugin_manifest_parser.test.ts‎

Lines changed: 31 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,10 @@
99
import { mockReadFile } from './plugin_manifest_parser.test.mocks';
1010

1111
import { PluginDiscoveryErrorType } from './plugin_discovery_error';
12-
import { loggingSystemMock } from '../../logging/logging_system.mock';
1312

1413
import { resolve } from 'path';
1514
import { parseManifest } from './plugin_manifest_parser';
1615

17-
const logger = loggingSystemMock.createLogger();
1816
const pluginPath = resolve('path', 'existent-dir');
1917
const pluginManifestPath = resolve(pluginPath, 'kibana.json');
2018
const packageInfo = {
@@ -34,7 +32,7 @@ test('return error when manifest is empty', async () => {
3432
cb(null, Buffer.from(''));
3533
});
3634

37-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
35+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
3836
message: `Unexpected end of JSON input (invalid-manifest, ${pluginManifestPath})`,
3937
type: PluginDiscoveryErrorType.InvalidManifest,
4038
path: pluginManifestPath,
@@ -46,7 +44,7 @@ test('return error when manifest content is null', async () => {
4644
cb(null, Buffer.from('null'));
4745
});
4846

49-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
47+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
5048
message: `Plugin manifest must contain a JSON encoded object. (invalid-manifest, ${pluginManifestPath})`,
5149
type: PluginDiscoveryErrorType.InvalidManifest,
5250
path: pluginManifestPath,
@@ -58,7 +56,7 @@ test('return error when manifest content is not a valid JSON', async () => {
5856
cb(null, Buffer.from('not-json'));
5957
});
6058

61-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
59+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
6260
message: `Unexpected token o in JSON at position 1 (invalid-manifest, ${pluginManifestPath})`,
6361
type: PluginDiscoveryErrorType.InvalidManifest,
6462
path: pluginManifestPath,
@@ -70,7 +68,7 @@ test('return error when plugin id is missing', async () => {
7068
cb(null, Buffer.from(JSON.stringify({ version: 'some-version' })));
7169
});
7270

73-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
71+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
7472
message: `Plugin manifest must contain an "id" property. (invalid-manifest, ${pluginManifestPath})`,
7573
type: PluginDiscoveryErrorType.InvalidManifest,
7674
path: pluginManifestPath,
@@ -82,45 +80,32 @@ test('return error when plugin id includes `.` characters', async () => {
8280
cb(null, Buffer.from(JSON.stringify({ id: 'some.name', version: 'some-version' })));
8381
});
8482

85-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
83+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
8684
message: `Plugin "id" must not include \`.\` characters. (invalid-manifest, ${pluginManifestPath})`,
8785
type: PluginDiscoveryErrorType.InvalidManifest,
8886
path: pluginManifestPath,
8987
});
9088
});
9189

92-
test('logs warning if pluginId is not in camelCase format', async () => {
90+
test('return error when pluginId is not in camelCase format', async () => {
91+
expect.assertions(1);
9392
mockReadFile.mockImplementation((path, cb) => {
9493
cb(null, Buffer.from(JSON.stringify({ id: 'some_name', version: 'kibana', server: true })));
9594
});
9695

97-
expect(loggingSystemMock.collect(logger).warn).toHaveLength(0);
98-
await parseManifest(pluginPath, packageInfo, logger);
99-
expect(loggingSystemMock.collect(logger).warn).toMatchInlineSnapshot(`
100-
Array [
101-
Array [
102-
"Expect plugin \\"id\\" in camelCase, but found: some_name",
103-
],
104-
]
105-
`);
106-
});
107-
108-
test('does not log pluginId format warning in dist mode', async () => {
109-
mockReadFile.mockImplementation((path, cb) => {
110-
cb(null, Buffer.from(JSON.stringify({ id: 'some_name', version: 'kibana', server: true })));
96+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
97+
message: `Plugin "id" must be camelCase, but found: some_name. (invalid-manifest, ${pluginManifestPath})`,
98+
type: PluginDiscoveryErrorType.InvalidManifest,
99+
path: pluginManifestPath,
111100
});
112-
113-
expect(loggingSystemMock.collect(logger).warn).toHaveLength(0);
114-
await parseManifest(pluginPath, { ...packageInfo, dist: true }, logger);
115-
expect(loggingSystemMock.collect(logger).warn.length).toBe(0);
116101
});
117102

118103
test('return error when plugin version is missing', async () => {
119104
mockReadFile.mockImplementation((path, cb) => {
120105
cb(null, Buffer.from(JSON.stringify({ id: 'someId' })));
121106
});
122107

123-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
108+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
124109
message: `Plugin manifest for "someId" must contain a "version" property. (invalid-manifest, ${pluginManifestPath})`,
125110
type: PluginDiscoveryErrorType.InvalidManifest,
126111
path: pluginManifestPath,
@@ -132,7 +117,7 @@ test('return error when plugin expected Kibana version is lower than actual vers
132117
cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '6.4.2' })));
133118
});
134119

135-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
120+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
136121
message: `Plugin "someId" is only compatible with Kibana version "6.4.2", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${pluginManifestPath})`,
137122
type: PluginDiscoveryErrorType.IncompatibleVersion,
138123
path: pluginManifestPath,
@@ -147,7 +132,7 @@ test('return error when plugin expected Kibana version cannot be interpreted as
147132
);
148133
});
149134

150-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
135+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
151136
message: `Plugin "someId" is only compatible with Kibana version "non-sem-ver", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${pluginManifestPath})`,
152137
type: PluginDiscoveryErrorType.IncompatibleVersion,
153138
path: pluginManifestPath,
@@ -159,7 +144,7 @@ test('return error when plugin config path is not a string', async () => {
159144
cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0', configPath: 2 })));
160145
});
161146

162-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
147+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
163148
message: `The "configPath" in plugin manifest for "someId" should either be a string or an array of strings. (invalid-manifest, ${pluginManifestPath})`,
164149
type: PluginDiscoveryErrorType.InvalidManifest,
165150
path: pluginManifestPath,
@@ -174,7 +159,7 @@ test('return error when plugin config path is an array that contains non-string
174159
);
175160
});
176161

177-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
162+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
178163
message: `The "configPath" in plugin manifest for "someId" should either be a string or an array of strings. (invalid-manifest, ${pluginManifestPath})`,
179164
type: PluginDiscoveryErrorType.InvalidManifest,
180165
path: pluginManifestPath,
@@ -186,7 +171,7 @@ test('return error when plugin expected Kibana version is higher than actual ver
186171
cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.1' })));
187172
});
188173

189-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
174+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
190175
message: `Plugin "someId" is only compatible with Kibana version "7.0.1", but used Kibana version is "7.0.0-alpha1". (incompatible-version, ${pluginManifestPath})`,
191176
type: PluginDiscoveryErrorType.IncompatibleVersion,
192177
path: pluginManifestPath,
@@ -198,7 +183,7 @@ test('return error when both `server` and `ui` are set to `false` or missing', a
198183
cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0' })));
199184
});
200185

201-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
186+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
202187
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})`,
203188
type: PluginDiscoveryErrorType.InvalidManifest,
204189
path: pluginManifestPath,
@@ -211,7 +196,7 @@ test('return error when both `server` and `ui` are set to `false` or missing', a
211196
);
212197
});
213198

214-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
199+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
215200
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})`,
216201
type: PluginDiscoveryErrorType.InvalidManifest,
217202
path: pluginManifestPath,
@@ -234,7 +219,7 @@ test('return error when manifest contains unrecognized properties', async () =>
234219
);
235220
});
236221

237-
await expect(parseManifest(pluginPath, packageInfo, logger)).rejects.toMatchObject({
222+
await expect(parseManifest(pluginPath, packageInfo)).rejects.toMatchObject({
238223
message: `Manifest for plugin "someId" contains the following unrecognized properties: unknownOne,unknownTwo. (invalid-manifest, ${pluginManifestPath})`,
239224
type: PluginDiscoveryErrorType.InvalidManifest,
240225
path: pluginManifestPath,
@@ -247,20 +232,20 @@ describe('configPath', () => {
247232
cb(null, Buffer.from(JSON.stringify({ id: 'plugin', version: '7.0.0', server: true })));
248233
});
249234

250-
const manifest = await parseManifest(pluginPath, packageInfo, logger);
235+
const manifest = await parseManifest(pluginPath, packageInfo);
251236
expect(manifest.configPath).toBe(manifest.id);
252237
});
253238

254239
test('falls back to plugin id in snakeCase format', async () => {
255240
mockReadFile.mockImplementation((path, cb) => {
256-
cb(null, Buffer.from(JSON.stringify({ id: 'SomeId', version: '7.0.0', server: true })));
241+
cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0', server: true })));
257242
});
258243

259-
const manifest = await parseManifest(pluginPath, packageInfo, logger);
244+
const manifest = await parseManifest(pluginPath, packageInfo);
260245
expect(manifest.configPath).toBe('some_id');
261246
});
262247

263-
test('not formated to snakeCase if defined explicitly as string', async () => {
248+
test('not formatted to snakeCase if defined explicitly as string', async () => {
264249
mockReadFile.mockImplementation((path, cb) => {
265250
cb(
266251
null,
@@ -270,11 +255,11 @@ describe('configPath', () => {
270255
);
271256
});
272257

273-
const manifest = await parseManifest(pluginPath, packageInfo, logger);
258+
const manifest = await parseManifest(pluginPath, packageInfo);
274259
expect(manifest.configPath).toBe('somePath');
275260
});
276261

277-
test('not formated to snakeCase if defined explicitly as an array of strings', async () => {
262+
test('not formatted to snakeCase if defined explicitly as an array of strings', async () => {
278263
mockReadFile.mockImplementation((path, cb) => {
279264
cb(
280265
null,
@@ -284,7 +269,7 @@ describe('configPath', () => {
284269
);
285270
});
286271

287-
const manifest = await parseManifest(pluginPath, packageInfo, logger);
272+
const manifest = await parseManifest(pluginPath, packageInfo);
288273
expect(manifest.configPath).toEqual(['somePath']);
289274
});
290275
});
@@ -294,7 +279,7 @@ test('set defaults for all missing optional fields', async () => {
294279
cb(null, Buffer.from(JSON.stringify({ id: 'someId', version: '7.0.0', server: true })));
295280
});
296281

297-
await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({
282+
await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
298283
id: 'someId',
299284
configPath: 'some_id',
300285
version: '7.0.0',
@@ -325,7 +310,7 @@ test('return all set optional fields as they are in manifest', async () => {
325310
);
326311
});
327312

328-
await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({
313+
await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
329314
id: 'someId',
330315
configPath: ['some', 'path'],
331316
version: 'some-version',
@@ -355,7 +340,7 @@ test('return manifest when plugin expected Kibana version matches actual version
355340
);
356341
});
357342

358-
await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({
343+
await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
359344
id: 'someId',
360345
configPath: 'some-path',
361346
version: 'some-version',
@@ -385,7 +370,7 @@ test('return manifest when plugin expected Kibana version is `kibana`', async ()
385370
);
386371
});
387372

388-
await expect(parseManifest(pluginPath, packageInfo, logger)).resolves.toEqual({
373+
await expect(parseManifest(pluginPath, packageInfo)).resolves.toEqual({
389374
id: 'someId',
390375
configPath: 'some_id',
391376
version: 'some-version',

‎src/core/server/plugins/discovery/plugin_manifest_parser.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import { coerce } from 'semver';
1212
import { promisify } from 'util';
1313
import { snakeCase } from 'lodash';
1414
import { isConfigPath, PackageInfo } from '../../config';
15-
import { Logger } from '../../logging';
1615
import { PluginManifest } from '../types';
1716
import { PluginDiscoveryError } from './plugin_discovery_error';
1817
import { isCamelCase } from './is_camel_case';
@@ -63,8 +62,7 @@ const KNOWN_MANIFEST_FIELDS = (() => {
6362
*/
6463
export async function parseManifest(
6564
pluginPath: string,
66-
packageInfo: PackageInfo,
67-
log: Logger
65+
packageInfo: PackageInfo
6866
): Promise<PluginManifest> {
6967
const manifestPath = resolve(pluginPath, MANIFEST_FILE_NAME);
7068

@@ -105,8 +103,11 @@ export async function parseManifest(
105103
);
106104
}
107105

108-
if (!packageInfo.dist && !isCamelCase(manifest.id)) {
109-
log.warn(`Expect plugin "id" in camelCase, but found: ${manifest.id}`);
106+
if (!isCamelCase(manifest.id)) {
107+
throw PluginDiscoveryError.invalidManifest(
108+
manifestPath,
109+
new Error(`Plugin "id" must be camelCase, but found: ${manifest.id}.`)
110+
);
110111
}
111112

112113
if (!manifest.version || typeof manifest.version !== 'string') {

‎src/core/server/plugins/discovery/plugins_discovery.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ function createPlugin$(
179179
coreContext: CoreContext,
180180
instanceInfo: InstanceInfo
181181
) {
182-
return from(parseManifest(path, coreContext.env.packageInfo, log)).pipe(
182+
return from(parseManifest(path, coreContext.env.packageInfo)).pipe(
183183
map((manifest) => {
184184
log.debug(`Successfully discovered plugin "${manifest.id}" at "${path}"`);
185185
const opaqueId = Symbol(manifest.id);

‎test/common/fixtures/plugins/newsfeed/kibana.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"id": "newsfeed-fixtures",
2+
"id": "newsfeedFixtures",
33
"version": "kibana",
44
"server": true,
55
"ui": false

‎test/interpreter_functional/plugins/kbn_tp_run_pipeline/kibana.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"id": "kbn_tp_run_pipeline",
2+
"id": "kbnTpRunPipeline",
33
"version": "0.0.1",
44
"kibanaVersion": "kibana",
55
"requiredPlugins": [

‎test/plugin_functional/plugins/app_link_test/kibana.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"id": "app_link_test",
2+
"id": "appLinkTest",
33
"version": "0.0.1",
44
"kibanaVersion": "kibana",
55
"server": false,

‎test/plugin_functional/plugins/core_app_status/kibana.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"id": "core_app_status",
2+
"id": "coreAppStatus",
33
"version": "0.0.1",
44
"kibanaVersion": "kibana",
55
"configPath": ["core_app_status"],

‎test/plugin_functional/plugins/core_plugin_a/kibana.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"id": "core_plugin_a",
2+
"id": "corePluginA",
33
"version": "0.0.1",
44
"kibanaVersion": "kibana",
55
"configPath": ["core_plugin_a"],

‎test/plugin_functional/plugins/core_plugin_appleave/kibana.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"id": "core_plugin_appleave",
2+
"id": "corePluginAppleave",
33
"version": "0.0.1",
44
"kibanaVersion": "kibana",
55
"configPath": ["core_plugin_appleave"],

‎test/plugin_functional/plugins/core_plugin_b/kibana.json‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
{
2-
"id": "core_plugin_b",
2+
"id": "corePluginB",
33
"version": "0.0.1",
44
"kibanaVersion": "kibana",
55
"configPath": ["core_plugin_b"],
66
"server": true,
77
"ui": true,
8-
"requiredPlugins": ["core_plugin_a"],
9-
"optionalPlugins": ["core_plugin_c"]
8+
"requiredPlugins": ["corePluginA"],
9+
"optionalPlugins": ["corePluginC"]
1010
}

0 commit comments

Comments
 (0)