From 3d3c661d57041bd08764314dfa5ee847240b0824 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Tue, 19 Sep 2023 15:29:41 +0200 Subject: [PATCH 01/12] feat(frontent-app-api): prevent root extension replacement Signed-off-by: Camila Belo --- packages/frontend-app-api/src/wiring/parameters.ts | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/frontend-app-api/src/wiring/parameters.ts b/packages/frontend-app-api/src/wiring/parameters.ts index 1b2831aaca..acde9d9ec0 100644 --- a/packages/frontend-app-api/src/wiring/parameters.ts +++ b/packages/frontend-app-api/src/wiring/parameters.ts @@ -188,6 +188,14 @@ export interface ExtensionInstanceParameters { config?: unknown; } +function preventRootExtensionOverride(id: string) { + if (id === 'root') { + throw new Error( + 'There is a root extension in the app config file and root extensions are not configurable', + ); + } +} + /** @internal */ export function mergeExtensionParameters(options: { sources: BackstagePlugin[]; @@ -220,8 +228,10 @@ export function mergeExtensionParameters(options: { ]; for (const overrideParam of parameters) { + const extensionId = overrideParam.id; + preventRootExtensionOverride(extensionId); const existingIndex = overrides.findIndex( - e => e.extension.id === overrideParam.id, + e => e.extension.id === extensionId, ); if (existingIndex !== -1) { const existing = overrides[existingIndex]; @@ -243,7 +253,7 @@ export function mergeExtensionParameters(options: { } } } else { - throw new Error(`Extension ${overrideParam.id} does not exist`); + throw new Error(`Extension ${extensionId} does not exist`); } } From 77485641b02a318c7cef3416e2b9f90adebf208a Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Tue, 19 Sep 2023 15:32:36 +0200 Subject: [PATCH 02/12] feat(frontent-app-api): prevent cyclical dependency Signed-off-by: Camila Belo --- .../frontend-app-api/src/wiring/createApp.tsx | 59 ++++++++++++++++--- 1 file changed, 51 insertions(+), 8 deletions(-) diff --git a/packages/frontend-app-api/src/wiring/createApp.tsx b/packages/frontend-app-api/src/wiring/createApp.tsx index 35d2af6f6d..bc16fd9559 100644 --- a/packages/frontend-app-api/src/wiring/createApp.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.tsx @@ -161,6 +161,38 @@ export function createExtensionTree(options: { }; } +function hasCyclicalExtensionDependency( + extensionId: string, + extensionAncestorIds: string[] = [], +) { + return extensionAncestorIds.includes(extensionId); +} + +function stringifyExtensionDependencyGraph( + extensionId: string, + extensionAncestorIds: string[] = [], +) { + const [rootAncestorId, ...rest] = extensionAncestorIds.concat(extensionId); + return rest.reduce((graphString, ancestorId) => { + return `${graphString} → ${ancestorId}`; + }, rootAncestorId); +} + +function preventCyclicalExtensionDependency( + extensionId: string, + extensionAncestorIds: string[], +) { + if (hasCyclicalExtensionDependency(extensionId, extensionAncestorIds)) { + const extensionDependencyGraph = stringifyExtensionDependencyGraph( + extensionId, + extensionAncestorIds, + ); + throw new Error( + `There is a cyclical dependency with the extension "${extensionId}": ${extensionDependencyGraph}`, + ); + } +} + /** * @internal */ @@ -208,19 +240,30 @@ export function createInstances(options: { function createInstance( instanceParams: ExtensionInstanceParameters, + extensionAncestorIds: string[] = [], ): ExtensionInstance { - const existingInstance = instances.get(instanceParams.extension.id); + const extensionId = instanceParams.extension.id; + const existingInstance = instances.get(extensionId); if (existingInstance) { return existingInstance; } + preventCyclicalExtensionDependency(extensionId, extensionAncestorIds); + const attachments = new Map( - Array.from( - attachmentMap.get(instanceParams.extension.id)?.entries() ?? [], - ).map(([inputName, attachmentConfigs]) => [ - inputName, - attachmentConfigs.map(createInstance), - ]), + Array.from(attachmentMap.get(extensionId)?.entries() ?? []).map( + ([inputName, attachmentConfigs]) => { + return [ + inputName, + attachmentConfigs.map(attachmentConfig => + createInstance( + attachmentConfig, + extensionAncestorIds.concat(extensionId), + ), + ), + ]; + }, + ), ); const newInstance = createExtensionInstance({ @@ -230,7 +273,7 @@ export function createInstances(options: { attachments, }); - instances.set(instanceParams.extension.id, newInstance); + instances.set(extensionId, newInstance); return newInstance; } From 24fa3afb5f6da93521ca15e8edfec79b500f2ca9 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Wed, 20 Sep 2023 09:26:03 +0200 Subject: [PATCH 03/12] test(frontend-app-api): root override and cyclical extensions Signed-off-by: Camila Belo --- packages/frontend-app-api/package.json | 1 + .../src/wiring/createApp.test.tsx | 66 +++++++++++++++++++ yarn.lock | 1 + 3 files changed, 68 insertions(+) create mode 100644 packages/frontend-app-api/src/wiring/createApp.test.tsx diff --git a/packages/frontend-app-api/package.json b/packages/frontend-app-api/package.json index 772dd62a36..7cd4557ea1 100644 --- a/packages/frontend-app-api/package.json +++ b/packages/frontend-app-api/package.json @@ -24,6 +24,7 @@ }, "devDependencies": { "@backstage/cli": "workspace:^", + "@backstage/test-utils": "workspace:^", "@testing-library/jest-dom": "^5.10.1" }, "configSchema": "config.d.ts", diff --git a/packages/frontend-app-api/src/wiring/createApp.test.tsx b/packages/frontend-app-api/src/wiring/createApp.test.tsx new file mode 100644 index 0000000000..a2f42257f0 --- /dev/null +++ b/packages/frontend-app-api/src/wiring/createApp.test.tsx @@ -0,0 +1,66 @@ +/* + * Copyright 2023 The Backstage Authors + * + * Licensed 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 { createExtension, createPlugin } from '@backstage/frontend-plugin-api'; +import { createInstances } from './createApp'; + +import { MockConfigApi } from '@backstage/test-utils'; + +describe('createInstances', () => { + it('throws an error when a root extension is overridden', () => { + const config = new MockConfigApi({ + app: { + extensions: [ + { + root: { + at: '', + }, + }, + ], + }, + }); + const plugins = [ + createPlugin({ + id: 'plugin', + extensions: [], + }), + ]; + expect(() => createInstances({ config, plugins })).toThrow( + 'There is a root extension in the app config file and root extensions are not configurable', + ); + }); + + it('throws an error when cyclical dependencies are found', () => { + const config = new MockConfigApi({}); + const plugins = [ + createPlugin({ + id: 'plugin', + extensions: [ + createExtension({ + id: 'root', + at: 'core.routes/route', + inputs: {}, + output: {}, + factory() {}, + }), + ], + }), + ]; + expect(() => createInstances({ config, plugins })).toThrow( + 'There is a cyclical dependency with the extension "core.layout": core.layout → core.routes → root → core.layout', + ); + }); +}); diff --git a/yarn.lock b/yarn.lock index 0d7896d082..9cb61d15f6 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4213,6 +4213,7 @@ __metadata: "@backstage/core-plugin-api": "workspace:^" "@backstage/frontend-plugin-api": "workspace:^" "@backstage/plugin-graphiql": "workspace:^" + "@backstage/test-utils": "workspace:^" "@backstage/types": "workspace:^" "@material-ui/core": ^4.12.4 "@testing-library/jest-dom": ^5.10.1 From 66d51a4827a3dd8cbb23a64989245237f2fda0a9 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Wed, 20 Sep 2023 09:31:39 +0200 Subject: [PATCH 04/12] chore(frontend-app-api): add changeset file Signed-off-by: Camila Belo --- .changeset/wild-jobs-greet.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/wild-jobs-greet.md diff --git a/.changeset/wild-jobs-greet.md b/.changeset/wild-jobs-greet.md new file mode 100644 index 0000000000..be0ddb98be --- /dev/null +++ b/.changeset/wild-jobs-greet.md @@ -0,0 +1,5 @@ +--- +'@backstage/frontend-app-api': patch +--- + +Prevents root extension configuration and cyclic dependency between extensions when creating extension instances. From f86bd410e658f03b644fb7c3b3fee4383cac2935 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Wed, 20 Sep 2023 11:10:09 +0200 Subject: [PATCH 05/12] refactor(frontend-app-api): to inline validation Signed-off-by: Camila Belo --- .../frontend-app-api/src/wiring/createApp.tsx | 42 ++++--------------- .../frontend-app-api/src/wiring/parameters.ts | 17 ++++---- 2 files changed, 17 insertions(+), 42 deletions(-) diff --git a/packages/frontend-app-api/src/wiring/createApp.tsx b/packages/frontend-app-api/src/wiring/createApp.tsx index bc16fd9559..abd135cc5f 100644 --- a/packages/frontend-app-api/src/wiring/createApp.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.tsx @@ -161,38 +161,6 @@ export function createExtensionTree(options: { }; } -function hasCyclicalExtensionDependency( - extensionId: string, - extensionAncestorIds: string[] = [], -) { - return extensionAncestorIds.includes(extensionId); -} - -function stringifyExtensionDependencyGraph( - extensionId: string, - extensionAncestorIds: string[] = [], -) { - const [rootAncestorId, ...rest] = extensionAncestorIds.concat(extensionId); - return rest.reduce((graphString, ancestorId) => { - return `${graphString} → ${ancestorId}`; - }, rootAncestorId); -} - -function preventCyclicalExtensionDependency( - extensionId: string, - extensionAncestorIds: string[], -) { - if (hasCyclicalExtensionDependency(extensionId, extensionAncestorIds)) { - const extensionDependencyGraph = stringifyExtensionDependencyGraph( - extensionId, - extensionAncestorIds, - ); - throw new Error( - `There is a cyclical dependency with the extension "${extensionId}": ${extensionDependencyGraph}`, - ); - } -} - /** * @internal */ @@ -248,7 +216,15 @@ export function createInstances(options: { return existingInstance; } - preventCyclicalExtensionDependency(extensionId, extensionAncestorIds); + // Prevent cyclical dependencies + if (extensionAncestorIds.includes(extensionId)) { + const extensionDependencyGraph = extensionAncestorIds + .concat(extensionId) + .join(' → '); + throw new Error( + `There is a cyclical dependency with the extension "${extensionId}": ${extensionDependencyGraph}`, + ); + } const attachments = new Map( Array.from(attachmentMap.get(extensionId)?.entries() ?? []).map( diff --git a/packages/frontend-app-api/src/wiring/parameters.ts b/packages/frontend-app-api/src/wiring/parameters.ts index acde9d9ec0..b18853ab5c 100644 --- a/packages/frontend-app-api/src/wiring/parameters.ts +++ b/packages/frontend-app-api/src/wiring/parameters.ts @@ -188,14 +188,6 @@ export interface ExtensionInstanceParameters { config?: unknown; } -function preventRootExtensionOverride(id: string) { - if (id === 'root') { - throw new Error( - 'There is a root extension in the app config file and root extensions are not configurable', - ); - } -} - /** @internal */ export function mergeExtensionParameters(options: { sources: BackstagePlugin[]; @@ -229,7 +221,14 @@ export function mergeExtensionParameters(options: { for (const overrideParam of parameters) { const extensionId = overrideParam.id; - preventRootExtensionOverride(extensionId); + + // Prevent root parametrization + if (extensionId === 'root') { + throw new Error( + 'There is a root extension in the app config file and root extensions are not configurable', + ); + } + const existingIndex = overrides.findIndex( e => e.extension.id === extensionId, ); From cc94fbd3b3ebab0049a5df7148440e3d51ec2b07 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Wed, 20 Sep 2023 11:46:15 +0200 Subject: [PATCH 06/12] feat(frontend-app-api): prevent plugins overring root extensions Signed-off-by: Camila Belo --- .../src/wiring/createApp.test.tsx | 8 ++--- .../frontend-app-api/src/wiring/parameters.ts | 36 ++++++++++++------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/packages/frontend-app-api/src/wiring/createApp.test.tsx b/packages/frontend-app-api/src/wiring/createApp.test.tsx index a2f42257f0..339948eec1 100644 --- a/packages/frontend-app-api/src/wiring/createApp.test.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.test.tsx @@ -20,7 +20,7 @@ import { createInstances } from './createApp'; import { MockConfigApi } from '@backstage/test-utils'; describe('createInstances', () => { - it('throws an error when a root extension is overridden', () => { + it('throws an error when a root extension is parametrized', () => { const config = new MockConfigApi({ app: { extensions: [ @@ -39,11 +39,11 @@ describe('createInstances', () => { }), ]; expect(() => createInstances({ config, plugins })).toThrow( - 'There is a root extension in the app config file and root extensions are not configurable', + 'A "root" extension was detected on the config file and root extensions are not configurable', ); }); - it('throws an error when cyclical dependencies are found', () => { + it('throws an error when a root extension is overridden', () => { const config = new MockConfigApi({}); const plugins = [ createPlugin({ @@ -60,7 +60,7 @@ describe('createInstances', () => { }), ]; expect(() => createInstances({ config, plugins })).toThrow( - 'There is a cyclical dependency with the extension "core.layout": core.layout → core.routes → root → core.layout', + 'The following plugins are overriding root extensions and root extensions cannot be overridden: plugin', ); }); }); diff --git a/packages/frontend-app-api/src/wiring/parameters.ts b/packages/frontend-app-api/src/wiring/parameters.ts index b18853ab5c..1f4fda12e9 100644 --- a/packages/frontend-app-api/src/wiring/parameters.ts +++ b/packages/frontend-app-api/src/wiring/parameters.ts @@ -196,18 +196,30 @@ export function mergeExtensionParameters(options: { }): ExtensionInstanceParameters[] { const { sources, builtinExtensions, parameters } = options; + const pluginExtensions = sources.flatMap(source => + source.extensions.map(extension => ({ ...extension, source })), + ); + + // Prevent root override + if (pluginExtensions.some(({ id }) => id === 'root')) { + const rootPluginIds = pluginExtensions + .filter(({ id }) => id === 'root') + .map(({ source }) => source.id); + throw new Error( + `The following plugins are overriding root extensions and root extensions cannot be overridden: ${rootPluginIds}`, + ); + } + const overrides = [ - ...sources.flatMap(plugin => - plugin.extensions.map(extension => ({ - extension, - params: { - source: plugin, - at: extension.at, - disabled: extension.disabled, - config: undefined as unknown, - }, - })), - ), + ...pluginExtensions.map(({ source, ...extension }) => ({ + extension, + params: { + source, + at: extension.at, + disabled: extension.disabled, + config: undefined as unknown, + }, + })), ...builtinExtensions.map(extension => ({ extension, params: { @@ -225,7 +237,7 @@ export function mergeExtensionParameters(options: { // Prevent root parametrization if (extensionId === 'root') { throw new Error( - 'There is a root extension in the app config file and root extensions are not configurable', + 'A "root" extension was detected on the config file and root extensions are not configurable', ); } From cfd94f29edc9aeb0178e1c23c3e3df03616b2386 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Wed, 20 Sep 2023 15:11:05 +0200 Subject: [PATCH 07/12] test(frontend-app-api): cover more levels of cyclical dependencies Signed-off-by: Camila Belo --- .../src/wiring/createApp.test.tsx | 176 +++++++++++++++++- 1 file changed, 175 insertions(+), 1 deletion(-) diff --git a/packages/frontend-app-api/src/wiring/createApp.test.tsx b/packages/frontend-app-api/src/wiring/createApp.test.tsx index 339948eec1..dcbd996f15 100644 --- a/packages/frontend-app-api/src/wiring/createApp.test.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.test.tsx @@ -14,10 +14,18 @@ * limitations under the License. */ -import { createExtension, createPlugin } from '@backstage/frontend-plugin-api'; +import { + createExtension, + createExtensionDataRef, + createExtensionInput, + createPageExtension, + createPlugin, +} from '@backstage/frontend-plugin-api'; import { createInstances } from './createApp'; import { MockConfigApi } from '@backstage/test-utils'; +import React from 'react'; +import { createRouteRef } from '@backstage/core-plugin-api'; describe('createInstances', () => { it('throws an error when a root extension is parametrized', () => { @@ -63,4 +71,170 @@ describe('createInstances', () => { 'The following plugins are overriding root extensions and root extensions cannot be overridden: plugin', ); }); + + describe('throws an error when immediate cyclical dependencies are found', () => { + it('in an immediate level (e.g., A(plugin.page) -> A(plugin.page))', () => { + const config = new MockConfigApi({}); + + const addonExtensionData = + createExtensionDataRef('plugin.page.addon'); + + const addon = createExtension({ + id: 'plugin.page', // cyclical + at: 'plugin.page/addons', + inputs: {}, + output: { + element: addonExtensionData, + }, + factory({ bind }) { + bind({ + element:
Addon
, + }); + }, + }); + + const page = createPageExtension({ + id: 'plugin.page', + defaultPath: '/', + routeRef: createRouteRef({ id: 'plugins.page.addon' }), + inputs: { + addons: createExtensionInput({ + element: addonExtensionData, + }), + }, + loader: async ({ inputs }) => ( +
Page {inputs.addons.map(({ element }) => element)}
+ ), + }); + + const plugins = [ + createPlugin({ + id: 'plugin', + extensions: [page, addon], + }), + ]; + + expect(() => createInstances({ config, plugins })).toThrow( + 'There is a cyclical dependency with the extension "plugin.page": core.layout → core.routes → plugin.page → plugin.page', + ); + }); + + it('in an intermediate level (e.g., A(core.routes) -> B(plugin.page) -> A(core.routes))', () => { + const config = new MockConfigApi({}); + + const addonExtensionData = + createExtensionDataRef('plugin.page.addon'); + + const addon = createExtension({ + id: 'core.routes', // cyclical + at: 'plugin.page/addons', + inputs: {}, + output: { + element: addonExtensionData, + }, + factory({ bind }) { + bind({ + element:
Addon
, + }); + }, + }); + + const page = createPageExtension({ + id: 'plugin.page', + defaultPath: '/', + routeRef: createRouteRef({ id: 'plugins.page.addon' }), + inputs: { + addons: createExtensionInput({ + element: addonExtensionData, + }), + }, + loader: async ({ inputs }) => ( +
Page {inputs.addons.map(({ element }) => element)}
+ ), + }); + + const plugins = [ + createPlugin({ + id: 'plugin', + extensions: [page, addon], + }), + ]; + + expect(() => createInstances({ config, plugins })).toThrow( + 'There is a cyclical dependency with the extension "core.routes": core.layout → core.routes → plugin.page → core.routes', + ); + }); + + it('in an deep level (e.g., A(core.layout) -> B(core.routes) -> C(plugin.page) -> D(plugin.page.addon) -> B(core.routes))', () => { + const config = new MockConfigApi({}); + + const addonRendererInput = createExtensionDataRef<() => JSX.Element>( + 'plugin.page.addon.renderer', + ); + + const renderer = createExtension({ + id: 'core.routes', // cyclical + at: 'plugin.page.addon/renderer', + inputs: {}, + output: { + renderer: addonRendererInput, + }, + factory({ bind }) { + bind({ + renderer: () =>
Addon
, + }); + }, + }); + + const addonElementInput = createExtensionDataRef( + 'plugin.page.addon.element', + ); + + const addon = createExtension({ + id: 'plugin.page.addon', + at: 'plugin.page/addons', + inputs: { + renderer: createExtensionInput( + { + element: addonRendererInput, + }, + { singleton: true, required: false }, + ), + }, + output: { + element: addonElementInput, + }, + factory({ bind, inputs }) { + bind({ + element: inputs.renderer?.element() ??
Addon
, + }); + }, + }); + + const page = createPageExtension({ + id: 'plugin.page', + defaultPath: '/', + routeRef: createRouteRef({ id: 'plugins.page.addon' }), + inputs: { + addons: createExtensionInput({ + element: addonElementInput, + }), + }, + loader: async ({ inputs }) => ( +
Page {inputs.addons.map(({ element }) => element)}
+ ), + }); + + const plugins = [ + createPlugin({ + id: 'plugin', + extensions: [page, addon, renderer], + }), + ]; + + expect(() => createInstances({ config, plugins })).toThrow( + 'There is a cyclical dependency with the extension "core.routes": core.layout → core.routes → plugin.page → plugin.page.addon → core.routes', + ); + }); + }); }); From 8e0907c37dbc9fe3aabd94f1666c3542c574d911 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Wed, 20 Sep 2023 16:35:06 +0200 Subject: [PATCH 08/12] fix(frontend-app-api): root override error message Co-authored-by: Patrik Oldsberg Signed-off-by: Camila Belo --- packages/frontend-app-api/src/wiring/createApp.test.tsx | 2 +- packages/frontend-app-api/src/wiring/parameters.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/frontend-app-api/src/wiring/createApp.test.tsx b/packages/frontend-app-api/src/wiring/createApp.test.tsx index dcbd996f15..e649931403 100644 --- a/packages/frontend-app-api/src/wiring/createApp.test.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.test.tsx @@ -47,7 +47,7 @@ describe('createInstances', () => { }), ]; expect(() => createInstances({ config, plugins })).toThrow( - 'A "root" extension was detected on the config file and root extensions are not configurable', + "A 'root' extension configuration was detected, but the root extension is not configurable", ); }); diff --git a/packages/frontend-app-api/src/wiring/parameters.ts b/packages/frontend-app-api/src/wiring/parameters.ts index 1f4fda12e9..0711f90665 100644 --- a/packages/frontend-app-api/src/wiring/parameters.ts +++ b/packages/frontend-app-api/src/wiring/parameters.ts @@ -237,7 +237,7 @@ export function mergeExtensionParameters(options: { // Prevent root parametrization if (extensionId === 'root') { throw new Error( - 'A "root" extension was detected on the config file and root extensions are not configurable', + "A 'root' extension configuration was detected, but the root extension is not configurable", ); } From 4a5bba64d95f9f0cee9b13698fee7c3c8f1a2340 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Thu, 21 Sep 2023 08:18:22 +0200 Subject: [PATCH 09/12] test(frontend-app-api): don not dependend on core extensions Signed-off-by: Camila Belo --- .../src/wiring/createApp.test.tsx | 146 ++++++++++++------ 1 file changed, 101 insertions(+), 45 deletions(-) diff --git a/packages/frontend-app-api/src/wiring/createApp.test.tsx b/packages/frontend-app-api/src/wiring/createApp.test.tsx index e649931403..c06afeccc6 100644 --- a/packages/frontend-app-api/src/wiring/createApp.test.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.test.tsx @@ -73,126 +73,182 @@ describe('createInstances', () => { }); describe('throws an error when immediate cyclical dependencies are found', () => { - it('in an immediate level (e.g., A(plugin.page) -> A(plugin.page))', () => { + it('in an immediate level (e.g., A -> A)', () => { const config = new MockConfigApi({}); const addonExtensionData = createExtensionDataRef('plugin.page.addon'); - const addon = createExtension({ - id: 'plugin.page', // cyclical - at: 'plugin.page/addons', + const CyclicalA = createExtension({ + id: 'A', // Same id as ancestor A + at: 'A/addons', inputs: {}, output: { element: addonExtensionData, }, factory({ bind }) { bind({ - element:
Addon
, + element:
Cyclical A
, }); }, }); - const page = createPageExtension({ - id: 'plugin.page', + const A = createPageExtension({ + id: 'A', defaultPath: '/', - routeRef: createRouteRef({ id: 'plugins.page.addon' }), + routeRef: createRouteRef({ id: 'A.route' }), inputs: { addons: createExtensionInput({ element: addonExtensionData, }), }, loader: async ({ inputs }) => ( -
Page {inputs.addons.map(({ element }) => element)}
+
A {inputs.addons.map(({ element }) => element)}
), }); const plugins = [ createPlugin({ id: 'plugin', - extensions: [page, addon], + extensions: [A, CyclicalA], }), ]; expect(() => createInstances({ config, plugins })).toThrow( - 'There is a cyclical dependency with the extension "plugin.page": core.layout → core.routes → plugin.page → plugin.page', + /There is a cyclical dependency with the extension "A": (.*) → A → A/, ); }); - it('in an intermediate level (e.g., A(core.routes) -> B(plugin.page) -> A(core.routes))', () => { + it('in an intermediate level (e.g., A -> B -> A)', () => { const config = new MockConfigApi({}); const addonExtensionData = createExtensionDataRef('plugin.page.addon'); - const addon = createExtension({ - id: 'core.routes', // cyclical - at: 'plugin.page/addons', - inputs: {}, + const CyclicalA = createExtension({ + id: 'A', // Same id as ancestor A + at: 'B/addons', + inputs: { + addons: createExtensionInput({ + element: addonExtensionData, + }), + }, output: { element: addonExtensionData, }, factory({ bind }) { bind({ - element:
Addon
, + element:
Cyclical A
, }); }, }); - const page = createPageExtension({ - id: 'plugin.page', + const B = createExtension({ + id: 'B', + at: 'A/addons', + inputs: { + addons: createExtensionInput({ + element: addonExtensionData, + }), + }, + output: { + element: addonExtensionData, + }, + factory({ bind }) { + bind({ + element:
B
, + }); + }, + }); + + const A = createPageExtension({ + id: 'A', defaultPath: '/', - routeRef: createRouteRef({ id: 'plugins.page.addon' }), + routeRef: createRouteRef({ id: 'A.route' }), inputs: { addons: createExtensionInput({ element: addonExtensionData, }), }, loader: async ({ inputs }) => ( -
Page {inputs.addons.map(({ element }) => element)}
+
A {inputs.addons.map(({ element }) => element)}
), }); const plugins = [ createPlugin({ id: 'plugin', - extensions: [page, addon], + extensions: [A, B, CyclicalA], }), ]; expect(() => createInstances({ config, plugins })).toThrow( - 'There is a cyclical dependency with the extension "core.routes": core.layout → core.routes → plugin.page → core.routes', + /There is a cyclical dependency with the extension "A": (.*) → A → B → A/, ); }); - it('in an deep level (e.g., A(core.layout) -> B(core.routes) -> C(plugin.page) -> D(plugin.page.addon) -> B(core.routes))', () => { + it('in an deep level (e.g., A -> B -> C -> D -> B)', () => { const config = new MockConfigApi({}); - const addonRendererInput = createExtensionDataRef<() => JSX.Element>( - 'plugin.page.addon.renderer', - ); + const addonRendererInput = + createExtensionDataRef<() => JSX.Element>('A.addon.renderer'); - const renderer = createExtension({ - id: 'core.routes', // cyclical - at: 'plugin.page.addon/renderer', + const addonElementInput = + createExtensionDataRef('A.addon.element'); + + const CyclicalB = createExtension({ + id: 'B', // Same id as ancestor B + at: 'D/addons', inputs: {}, + output: { + element: addonElementInput, + }, + factory({ bind }) { + bind({ + element:
Cyclical B
, + }); + }, + }); + + const D = createExtension({ + id: 'D', + at: 'C/addons', + inputs: { + addons: createExtensionInput({ + element: addonElementInput, + }), + }, + output: { + element: addonElementInput, + }, + factory({ bind }) { + bind({ + element:
D
, + }); + }, + }); + + const C = createExtension({ + id: 'C', + at: 'B/renderer', + inputs: { + addons: createExtensionInput({ + element: addonElementInput, + }), + }, output: { renderer: addonRendererInput, }, factory({ bind }) { bind({ - renderer: () =>
Addon
, + renderer: () =>
C
, }); }, }); - const addonElementInput = createExtensionDataRef( - 'plugin.page.addon.element', - ); - - const addon = createExtension({ - id: 'plugin.page.addon', - at: 'plugin.page/addons', + const B = createExtension({ + id: 'B', + at: 'A/addons', inputs: { renderer: createExtensionInput( { @@ -206,34 +262,34 @@ describe('createInstances', () => { }, factory({ bind, inputs }) { bind({ - element: inputs.renderer?.element() ??
Addon
, + element: inputs.renderer?.element() ??
B
, }); }, }); - const page = createPageExtension({ - id: 'plugin.page', + const A = createPageExtension({ + id: 'A', defaultPath: '/', - routeRef: createRouteRef({ id: 'plugins.page.addon' }), + routeRef: createRouteRef({ id: 'A.route' }), inputs: { addons: createExtensionInput({ element: addonElementInput, }), }, loader: async ({ inputs }) => ( -
Page {inputs.addons.map(({ element }) => element)}
+
A {inputs.addons.map(({ element }) => element)}
), }); const plugins = [ createPlugin({ id: 'plugin', - extensions: [page, addon, renderer], + extensions: [A, B, C, D, CyclicalB], }), ]; expect(() => createInstances({ config, plugins })).toThrow( - 'There is a cyclical dependency with the extension "core.routes": core.layout → core.routes → plugin.page → plugin.page.addon → core.routes', + /There is a cyclical dependency with the extension "B": (.*) → A → B → C → D → B/, ); }); }); From 7899201594b60571580188dc38037ebe90da2768 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Thu, 21 Sep 2023 10:04:34 +0200 Subject: [PATCH 10/12] refactor(frontend-app-api): rephrase root overriding error message Signed-off-by: Camila Belo --- packages/frontend-app-api/src/wiring/createApp.test.tsx | 2 +- packages/frontend-app-api/src/wiring/parameters.ts | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/frontend-app-api/src/wiring/createApp.test.tsx b/packages/frontend-app-api/src/wiring/createApp.test.tsx index c06afeccc6..5702077c89 100644 --- a/packages/frontend-app-api/src/wiring/createApp.test.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.test.tsx @@ -68,7 +68,7 @@ describe('createInstances', () => { }), ]; expect(() => createInstances({ config, plugins })).toThrow( - 'The following plugins are overriding root extensions and root extensions cannot be overridden: plugin', + "The following plugin(s) are overriding the 'root' extension which is forbidden: plugin", ); }); diff --git a/packages/frontend-app-api/src/wiring/parameters.ts b/packages/frontend-app-api/src/wiring/parameters.ts index 0711f90665..0605d34001 100644 --- a/packages/frontend-app-api/src/wiring/parameters.ts +++ b/packages/frontend-app-api/src/wiring/parameters.ts @@ -206,7 +206,9 @@ export function mergeExtensionParameters(options: { .filter(({ id }) => id === 'root') .map(({ source }) => source.id); throw new Error( - `The following plugins are overriding root extensions and root extensions cannot be overridden: ${rootPluginIds}`, + `The following plugin(s) are overriding the 'root' extension which is forbidden: ${rootPluginIds.join( + ',', + )}`, ); } From 7041414b06ae580b07abda392e10568734516a53 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Thu, 21 Sep 2023 15:24:10 +0200 Subject: [PATCH 11/12] refactor(frontend-app-api): remove duplicated plugin extensions Signed-off-by: Camila Belo --- .changeset/wild-jobs-greet.md | 2 +- .../src/wiring/createApp.test.tsx | 19 ++++++++++------- .../frontend-app-api/src/wiring/createApp.tsx | 21 +------------------ .../frontend-app-api/src/wiring/parameters.ts | 17 ++++++++++++--- 4 files changed, 27 insertions(+), 32 deletions(-) diff --git a/.changeset/wild-jobs-greet.md b/.changeset/wild-jobs-greet.md index be0ddb98be..fdbe6d56ac 100644 --- a/.changeset/wild-jobs-greet.md +++ b/.changeset/wild-jobs-greet.md @@ -2,4 +2,4 @@ '@backstage/frontend-app-api': patch --- -Prevents root extension configuration and cyclic dependency between extensions when creating extension instances. +Prevents root extension override and duplicated plugin extensions. diff --git a/packages/frontend-app-api/src/wiring/createApp.test.tsx b/packages/frontend-app-api/src/wiring/createApp.test.tsx index 5702077c89..6fc294131c 100644 --- a/packages/frontend-app-api/src/wiring/createApp.test.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.test.tsx @@ -72,8 +72,8 @@ describe('createInstances', () => { ); }); - describe('throws an error when immediate cyclical dependencies are found', () => { - it('in an immediate level (e.g., A -> A)', () => { + describe('should deduplicated plugins', () => { + it('in an immediate dependency level (e.g., A -> A)', () => { const config = new MockConfigApi({}); const addonExtensionData = @@ -114,8 +114,9 @@ describe('createInstances', () => { }), ]; - expect(() => createInstances({ config, plugins })).toThrow( - /There is a cyclical dependency with the extension "A": (.*) → A → A/, + // It should not create an infinite loop + expect(() => createInstances({ config, plugins })).not.toThrow( + 'Maximum call stack size exceeded', ); }); @@ -182,8 +183,9 @@ describe('createInstances', () => { }), ]; - expect(() => createInstances({ config, plugins })).toThrow( - /There is a cyclical dependency with the extension "A": (.*) → A → B → A/, + // It should not create an infinite loop + expect(() => createInstances({ config, plugins })).not.toThrow( + 'Maximum call stack size exceeded', ); }); @@ -288,8 +290,9 @@ describe('createInstances', () => { }), ]; - expect(() => createInstances({ config, plugins })).toThrow( - /There is a cyclical dependency with the extension "B": (.*) → A → B → C → D → B/, + // It should not create an infinite loop + expect(() => createInstances({ config, plugins })).not.toThrow( + 'Maximum call stack size exceeded', ); }); }); diff --git a/packages/frontend-app-api/src/wiring/createApp.tsx b/packages/frontend-app-api/src/wiring/createApp.tsx index abd135cc5f..462bfcdccc 100644 --- a/packages/frontend-app-api/src/wiring/createApp.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.tsx @@ -208,7 +208,6 @@ export function createInstances(options: { function createInstance( instanceParams: ExtensionInstanceParameters, - extensionAncestorIds: string[] = [], ): ExtensionInstance { const extensionId = instanceParams.extension.id; const existingInstance = instances.get(extensionId); @@ -216,28 +215,10 @@ export function createInstances(options: { return existingInstance; } - // Prevent cyclical dependencies - if (extensionAncestorIds.includes(extensionId)) { - const extensionDependencyGraph = extensionAncestorIds - .concat(extensionId) - .join(' → '); - throw new Error( - `There is a cyclical dependency with the extension "${extensionId}": ${extensionDependencyGraph}`, - ); - } - const attachments = new Map( Array.from(attachmentMap.get(extensionId)?.entries() ?? []).map( ([inputName, attachmentConfigs]) => { - return [ - inputName, - attachmentConfigs.map(attachmentConfig => - createInstance( - attachmentConfig, - extensionAncestorIds.concat(extensionId), - ), - ), - ]; + return [inputName, attachmentConfigs.map(createInstance)]; }, ), ); diff --git a/packages/frontend-app-api/src/wiring/parameters.ts b/packages/frontend-app-api/src/wiring/parameters.ts index 0605d34001..f2fba520c3 100644 --- a/packages/frontend-app-api/src/wiring/parameters.ts +++ b/packages/frontend-app-api/src/wiring/parameters.ts @@ -196,9 +196,20 @@ export function mergeExtensionParameters(options: { }): ExtensionInstanceParameters[] { const { sources, builtinExtensions, parameters } = options; - const pluginExtensions = sources.flatMap(source => - source.extensions.map(extension => ({ ...extension, source })), - ); + const pluginExtensionIds = new Set(); + const pluginExtensions = sources.flatMap(source => { + return source.extensions + .filter(extension => { + // Filter out duplicated extensions + const id = extension.id; + if (!pluginExtensionIds.has(id)) { + pluginExtensionIds.add(id); + return true; + } + return false; + }) + .map(extension => ({ ...extension, source })); + }); // Prevent root override if (pluginExtensions.some(({ id }) => id === 'root')) { From 6142e35c96e8e2446128112b1d8bbac7b9940078 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Fri, 22 Sep 2023 10:08:41 +0200 Subject: [PATCH 12/12] refactor(frontend-app-api): throw error when there are duplicated extensions Signed-off-by: Camila Belo --- .../src/wiring/createApp.test.tsx | 245 ++---------------- .../frontend-app-api/src/wiring/parameters.ts | 48 +++- 2 files changed, 62 insertions(+), 231 deletions(-) diff --git a/packages/frontend-app-api/src/wiring/createApp.test.tsx b/packages/frontend-app-api/src/wiring/createApp.test.tsx index 6fc294131c..903b044ed3 100644 --- a/packages/frontend-app-api/src/wiring/createApp.test.tsx +++ b/packages/frontend-app-api/src/wiring/createApp.test.tsx @@ -16,8 +16,6 @@ import { createExtension, - createExtensionDataRef, - createExtensionInput, createPageExtension, createPlugin, } from '@backstage/frontend-plugin-api'; @@ -72,228 +70,37 @@ describe('createInstances', () => { ); }); - describe('should deduplicated plugins', () => { - it('in an immediate dependency level (e.g., A -> A)', () => { - const config = new MockConfigApi({}); + it('throws an error when duplicated extensions are detected', () => { + const config = new MockConfigApi({}); - const addonExtensionData = - createExtensionDataRef('plugin.page.addon'); - - const CyclicalA = createExtension({ - id: 'A', // Same id as ancestor A - at: 'A/addons', - inputs: {}, - output: { - element: addonExtensionData, - }, - factory({ bind }) { - bind({ - element:
Cyclical A
, - }); - }, - }); - - const A = createPageExtension({ - id: 'A', - defaultPath: '/', - routeRef: createRouteRef({ id: 'A.route' }), - inputs: { - addons: createExtensionInput({ - element: addonExtensionData, - }), - }, - loader: async ({ inputs }) => ( -
A {inputs.addons.map(({ element }) => element)}
- ), - }); - - const plugins = [ - createPlugin({ - id: 'plugin', - extensions: [A, CyclicalA], - }), - ]; - - // It should not create an infinite loop - expect(() => createInstances({ config, plugins })).not.toThrow( - 'Maximum call stack size exceeded', - ); + const ExtensionA = createPageExtension({ + id: 'A', + defaultPath: '/', + routeRef: createRouteRef({ id: 'A.route' }), + loader: async () =>
Extension A
, }); - it('in an intermediate level (e.g., A -> B -> A)', () => { - const config = new MockConfigApi({}); - - const addonExtensionData = - createExtensionDataRef('plugin.page.addon'); - - const CyclicalA = createExtension({ - id: 'A', // Same id as ancestor A - at: 'B/addons', - inputs: { - addons: createExtensionInput({ - element: addonExtensionData, - }), - }, - output: { - element: addonExtensionData, - }, - factory({ bind }) { - bind({ - element:
Cyclical A
, - }); - }, - }); - - const B = createExtension({ - id: 'B', - at: 'A/addons', - inputs: { - addons: createExtensionInput({ - element: addonExtensionData, - }), - }, - output: { - element: addonExtensionData, - }, - factory({ bind }) { - bind({ - element:
B
, - }); - }, - }); - - const A = createPageExtension({ - id: 'A', - defaultPath: '/', - routeRef: createRouteRef({ id: 'A.route' }), - inputs: { - addons: createExtensionInput({ - element: addonExtensionData, - }), - }, - loader: async ({ inputs }) => ( -
A {inputs.addons.map(({ element }) => element)}
- ), - }); - - const plugins = [ - createPlugin({ - id: 'plugin', - extensions: [A, B, CyclicalA], - }), - ]; - - // It should not create an infinite loop - expect(() => createInstances({ config, plugins })).not.toThrow( - 'Maximum call stack size exceeded', - ); + const ExtensionB = createPageExtension({ + id: 'B', + defaultPath: '/', + routeRef: createRouteRef({ id: 'B.route' }), + loader: async () =>
Extension B
, }); - it('in an deep level (e.g., A -> B -> C -> D -> B)', () => { - const config = new MockConfigApi({}); - - const addonRendererInput = - createExtensionDataRef<() => JSX.Element>('A.addon.renderer'); - - const addonElementInput = - createExtensionDataRef('A.addon.element'); - - const CyclicalB = createExtension({ - id: 'B', // Same id as ancestor B - at: 'D/addons', - inputs: {}, - output: { - element: addonElementInput, - }, - factory({ bind }) { - bind({ - element:
Cyclical B
, - }); - }, - }); - - const D = createExtension({ - id: 'D', - at: 'C/addons', - inputs: { - addons: createExtensionInput({ - element: addonElementInput, - }), - }, - output: { - element: addonElementInput, - }, - factory({ bind }) { - bind({ - element:
D
, - }); - }, - }); - - const C = createExtension({ - id: 'C', - at: 'B/renderer', - inputs: { - addons: createExtensionInput({ - element: addonElementInput, - }), - }, - output: { - renderer: addonRendererInput, - }, - factory({ bind }) { - bind({ - renderer: () =>
C
, - }); - }, - }); - - const B = createExtension({ - id: 'B', - at: 'A/addons', - inputs: { - renderer: createExtensionInput( - { - element: addonRendererInput, - }, - { singleton: true, required: false }, - ), - }, - output: { - element: addonElementInput, - }, - factory({ bind, inputs }) { - bind({ - element: inputs.renderer?.element() ??
B
, - }); - }, - }); - - const A = createPageExtension({ - id: 'A', - defaultPath: '/', - routeRef: createRouteRef({ id: 'A.route' }), - inputs: { - addons: createExtensionInput({ - element: addonElementInput, - }), - }, - loader: async ({ inputs }) => ( -
A {inputs.addons.map(({ element }) => element)}
- ), - }); - - const plugins = [ - createPlugin({ - id: 'plugin', - extensions: [A, B, C, D, CyclicalB], - }), - ]; - - // It should not create an infinite loop - expect(() => createInstances({ config, plugins })).not.toThrow( - 'Maximum call stack size exceeded', - ); + const PluginA = createPlugin({ + id: 'A', + extensions: [ExtensionA, ExtensionA], }); + + const PluginB = createPlugin({ + id: 'B', + extensions: [ExtensionA, ExtensionB, ExtensionB], + }); + + const plugins = [PluginA, PluginB]; + + expect(() => createInstances({ config, plugins })).toThrow( + "The following extensions are duplicated: The extension 'A' was provided 2 time(s) by the plugin 'A' and 1 time(s) by the plugin 'B', The extension 'B' was provided 2 time(s) by the plugin 'B'", + ); }); }); diff --git a/packages/frontend-app-api/src/wiring/parameters.ts b/packages/frontend-app-api/src/wiring/parameters.ts index f2fba520c3..efe0fbf678 100644 --- a/packages/frontend-app-api/src/wiring/parameters.ts +++ b/packages/frontend-app-api/src/wiring/parameters.ts @@ -196,19 +196,8 @@ export function mergeExtensionParameters(options: { }): ExtensionInstanceParameters[] { const { sources, builtinExtensions, parameters } = options; - const pluginExtensionIds = new Set(); const pluginExtensions = sources.flatMap(source => { - return source.extensions - .filter(extension => { - // Filter out duplicated extensions - const id = extension.id; - if (!pluginExtensionIds.has(id)) { - pluginExtensionIds.add(id); - return true; - } - return false; - }) - .map(extension => ({ ...extension, source })); + return source.extensions.map(extension => ({ ...extension, source })); }); // Prevent root override @@ -244,6 +233,41 @@ export function mergeExtensionParameters(options: { })), ]; + const duplicatedExtensionIds = new Set(); + const duplicatedExtensionData = overrides.reduce< + Record> + >((data, { extension, params }) => { + const extensionId = extension.id; + const extensionData = data?.[extensionId]; + if (extensionData) duplicatedExtensionIds.add(extensionId); + const pluginId = params.source?.id ?? 'internal'; + const pluginCount = extensionData?.[pluginId] ?? 0; + return { + ...data, + [extensionId]: { ...extensionData, [pluginId]: pluginCount + 1 }, + }; + }, {}); + + if (duplicatedExtensionIds.size > 0) { + throw new Error( + `The following extensions are duplicated: ${Array.from( + duplicatedExtensionIds, + ) + .map( + extensionId => + `The extension '${extensionId}' was provided ${Object.keys( + duplicatedExtensionData[extensionId], + ) + .map( + pluginId => + `${duplicatedExtensionData[extensionId][pluginId]} time(s) by the plugin '${pluginId}'`, + ) + .join(' and ')}`, + ) + .join(', ')}`, + ); + } + for (const overrideParam of parameters) { const extensionId = overrideParam.id;