diff --git a/.changeset/busy-hairs-attend.md b/.changeset/busy-hairs-attend.md new file mode 100644 index 0000000000..523396a5d4 --- /dev/null +++ b/.changeset/busy-hairs-attend.md @@ -0,0 +1,5 @@ +--- +'@backstage/frontend-plugin-api': patch +--- + +Updated documentation for `createApiRef` to clarify the role of the ID in specifying the owning plugin of an API. diff --git a/.changeset/quiet-coins-protect.md b/.changeset/quiet-coins-protect.md new file mode 100644 index 0000000000..758050232a --- /dev/null +++ b/.changeset/quiet-coins-protect.md @@ -0,0 +1,5 @@ +--- +'@backstage/frontend-app-api': minor +--- + +BREAKING: The ability for plugins to override APIs has been restricted to only allow overrides of APIs within the same plugin. For example, a plugin can no longer override any of the core APIs provided by the `app` plugin, this must be done with an `app` module instead. diff --git a/docs/frontend-system/building-plugins/01-index.md b/docs/frontend-system/building-plugins/01-index.md index fd88478b37..d9acf76e15 100644 --- a/docs/frontend-system/building-plugins/01-index.md +++ b/docs/frontend-system/building-plugins/01-index.md @@ -124,7 +124,7 @@ export interface ExampleApi { } export const exampleApiRef = createApiRef({ - id: 'plugin.example', + id: 'plugin.example.api', }); export class DefaultExampleApi implements ExampleApi { diff --git a/docs/frontend-system/utility-apis/02-creating.md b/docs/frontend-system/utility-apis/02-creating.md index c632d9a909..2f6959e0ce 100644 --- a/docs/frontend-system/utility-apis/02-creating.md +++ b/docs/frontend-system/utility-apis/02-creating.md @@ -36,6 +36,9 @@ export const workApiRef = createApiRef({ Both of these are properly exported publicly from the package, so that consumers can reach them. +The frontend system infers the owning plugin for an API from the `ApiRef` id, so +use the pattern `plugin..*` to make ownership explicit. This ensures that other plugins can't mistakenly override your API. + ## Providing an extension through your plugin The plugin itself now wants to provide this API and its default implementation, in the form of an API extension. Doing so means that when users install the Example plugin, an instance of the Work utility API will also be automatically available in their apps - both to the Example plugin itself, and to others. We do this in the main plugin package, not the `-react` package. diff --git a/docs/frontend-system/utility-apis/04-configuring.md b/docs/frontend-system/utility-apis/04-configuring.md index 1f21d9b1ba..53527301ef 100644 --- a/docs/frontend-system/utility-apis/04-configuring.md +++ b/docs/frontend-system/utility-apis/04-configuring.md @@ -36,6 +36,8 @@ Well written input-enabled extension often have extension creator functions that Like with other extension types, you replace Utility APIs with your own custom implementation using [extension overrides](../architecture/25-extension-overrides.md). +Note that it is only possible to override a Utility API using a module for the plugin that originally provided the API. Attempting to override an API using a different plugin or module for a different plugin will result in a conflict error. + ```tsx title="in your app" /* highlight-add-start */ import { createFrontendModule } from '@backstage/frontend-plugin-api'; diff --git a/packages/frontend-app-api/report.api.md b/packages/frontend-app-api/report.api.md index bd0709813b..06b5e55278 100644 --- a/packages/frontend-app-api/report.api.md +++ b/packages/frontend-app-api/report.api.md @@ -109,6 +109,14 @@ export type AppErrorTypes = { node: AppNode; }; }; + API_FACTORY_CONFLICT: { + context: { + node: AppNode; + apiRefId: string; + pluginId: string; + existingPluginId: string; + }; + }; ROUTE_DUPLICATE: { context: { routeId: string; diff --git a/packages/frontend-app-api/src/wiring/createErrorCollector.ts b/packages/frontend-app-api/src/wiring/createErrorCollector.ts index c20e92d0f0..dab69ba19e 100644 --- a/packages/frontend-app-api/src/wiring/createErrorCollector.ts +++ b/packages/frontend-app-api/src/wiring/createErrorCollector.ts @@ -66,6 +66,14 @@ export type AppErrorTypes = { API_EXTENSION_INVALID: { context: { node: AppNode }; }; + API_FACTORY_CONFLICT: { + context: { + node: AppNode; + apiRefId: string; + pluginId: string; + existingPluginId: string; + }; + }; // routing ROUTE_DUPLICATE: { context: { routeId: string }; diff --git a/packages/frontend-app-api/src/wiring/createSpecializedApp.test.tsx b/packages/frontend-app-api/src/wiring/createSpecializedApp.test.tsx index 5622af0448..6c79002ad9 100644 --- a/packages/frontend-app-api/src/wiring/createSpecializedApp.test.tsx +++ b/packages/frontend-app-api/src/wiring/createSpecializedApp.test.tsx @@ -20,7 +20,9 @@ import { coreExtensionData, createExtension, createFrontendPlugin, + createFrontendModule, ApiBlueprint, + createApiRef, createRouteRef, createExternalRouteRef, createExtensionInput, @@ -36,21 +38,22 @@ import { MemoryRouter } from 'react-router-dom'; import { ApiProvider, ConfigReader } from '@backstage/core-app-api'; import { Fragment } from 'react'; +function makeAppPlugin(label: string = 'Test') { + return createFrontendPlugin({ + pluginId: 'app', + extensions: [ + createExtension({ + attachTo: { id: 'root', input: 'app' }, + output: [coreExtensionData.reactElement], + factory: () => [coreExtensionData.reactElement(
{label}
)], + }), + ], + }); +} describe('createSpecializedApp', () => { it('should render the root app', () => { const app = createSpecializedApp({ - features: [ - createFrontendPlugin({ - pluginId: 'test', - extensions: [ - createExtension({ - attachTo: { id: 'root', input: 'app' }, - output: [coreExtensionData.reactElement], - factory: () => [coreExtensionData.reactElement(
Test
)], - }), - ], - }), - ], + features: [makeAppPlugin()], }); render(app.tree.root.instance!.getData(coreExtensionData.reactElement)); @@ -60,32 +63,7 @@ describe('createSpecializedApp', () => { it('should deduplicate features keeping the last received one', () => { const app = createSpecializedApp({ - features: [ - createFrontendPlugin({ - pluginId: 'test', - extensions: [ - createExtension({ - attachTo: { id: 'root', input: 'app' }, - output: [coreExtensionData.reactElement], - factory: () => [ - coreExtensionData.reactElement(
Test 1
), - ], - }), - ], - }), - createFrontendPlugin({ - pluginId: 'test', - extensions: [ - createExtension({ - attachTo: { id: 'root', input: 'app' }, - output: [coreExtensionData.reactElement], - factory: () => [ - coreExtensionData.reactElement(
Test 2
), - ], - }), - ], - }), - ], + features: [makeAppPlugin('Test 1'), makeAppPlugin('Test 2')], }); render(app.tree.root.instance!.getData(coreExtensionData.reactElement)); @@ -249,8 +227,9 @@ describe('createSpecializedApp', () => { const app = createSpecializedApp({ features: [ - createFrontendPlugin({ - pluginId: 'first', + makeAppPlugin(), + createFrontendModule({ + pluginId: 'app', extensions: [ ApiBlueprint.make({ params: defineParams => @@ -264,9 +243,8 @@ describe('createSpecializedApp', () => { }), ], }), - createFrontendPlugin({ - pluginId: 'test', - featureFlags: [{ name: 'a' }, { name: 'b' }], + createFrontendModule({ + pluginId: 'app', extensions: [ createExtension({ attachTo: { id: 'root', input: 'app' }, @@ -311,6 +289,107 @@ describe('createSpecializedApp', () => { expect(mockAnalyticsApi).toHaveBeenCalled(); }); + it('should select the API factory from the owning plugin on conflict', () => { + const testApiRef = createApiRef<{ value: string }>({ id: 'test.api' }); + + const app = createSpecializedApp({ + features: [ + makeAppPlugin(), + createFrontendPlugin({ + pluginId: 'other-before', + extensions: [ + ApiBlueprint.make({ + params: defineParams => + defineParams({ + api: testApiRef, + deps: {}, + factory: () => ({ value: 'other' }), + }), + }), + ], + }), + createFrontendPlugin({ + pluginId: 'test', + extensions: [ + ApiBlueprint.make({ + params: defineParams => + defineParams({ + api: testApiRef, + deps: {}, + factory: () => ({ value: 'owner' }), + }), + }), + ], + }), + createFrontendPlugin({ + pluginId: 'other-after', + extensions: [ + ApiBlueprint.make({ + params: defineParams => + defineParams({ + api: testApiRef, + deps: {}, + factory: () => ({ value: 'other' }), + }), + }), + ], + }), + ], + }); + + expect(app.errors).toEqual([ + expect.objectContaining({ + code: 'API_FACTORY_CONFLICT', + message: expect.stringContaining("API 'test.api'"), + }), + expect.objectContaining({ + code: 'API_FACTORY_CONFLICT', + message: expect.stringContaining("API 'test.api'"), + }), + ]); + + expect(app.apis.get(testApiRef)).toEqual({ value: 'owner' }); + }); + + it('should allow API overrides within the same plugin', () => { + const testApiRef = createApiRef<{ value: string }>({ id: 'test.api' }); + + const app = createSpecializedApp({ + features: [ + makeAppPlugin(), + createFrontendPlugin({ + pluginId: 'test', + extensions: [ + ApiBlueprint.make({ + params: defineParams => + defineParams({ + api: testApiRef, + deps: {}, + factory: () => ({ value: 'plugin' }), + }), + }), + ], + }), + createFrontendModule({ + pluginId: 'test', + extensions: [ + ApiBlueprint.make({ + params: defineParams => + defineParams({ + api: testApiRef, + deps: {}, + factory: () => ({ value: 'module' }), + }), + }), + ], + }), + ], + }); + + expect(app.errors).toBeUndefined(); + expect(app.apis.get(testApiRef)).toEqual({ value: 'module' }); + }); + it('should use provided apis', async () => { const app = createSpecializedApp({ advanced: { diff --git a/packages/frontend-app-api/src/wiring/createSpecializedApp.tsx b/packages/frontend-app-api/src/wiring/createSpecializedApp.tsx index 9e75d19454..f902500e9f 100644 --- a/packages/frontend-app-api/src/wiring/createSpecializedApp.tsx +++ b/packages/frontend-app-api/src/wiring/createSpecializedApp.tsx @@ -388,7 +388,10 @@ function createApiFactories(options: { collector: ErrorCollector; }): AnyApiFactory[] { const emptyApiHolder = ApiRegistry.from([]); - const factories = new Array(); + const factoriesById = new Map< + string, + { pluginId: string; factory: AnyApiFactory } + >(); for (const apiNode of options.tree.root.edges.attachments.get('apis') ?? []) { if (!instantiateAppNodeTree(apiNode, emptyApiHolder, options.collector)) { @@ -396,7 +399,45 @@ function createApiFactories(options: { } const apiFactory = apiNode.instance?.getData(ApiBlueprint.dataRefs.factory); if (apiFactory) { - factories.push(apiFactory); + const apiRefId = apiFactory.api.id; + const ownerId = getApiOwnerId(apiRefId); + const pluginId = apiNode.spec.plugin.id ?? 'app'; + const existingFactory = factoriesById.get(apiRefId); + + // This allows modules to override factories provided by the plugin, but + // it rejects API overrides from other plugins. In the event of a + // conflict, the owning plugin is attempted to be inferred from the API + // reference ID. + if (existingFactory && existingFactory.pluginId !== pluginId) { + const shouldReplace = + ownerId === pluginId && existingFactory.pluginId !== ownerId; + const acceptedPluginId = shouldReplace + ? pluginId + : existingFactory.pluginId; + const rejectedPluginId = shouldReplace + ? existingFactory.pluginId + : pluginId; + + options.collector.report({ + code: 'API_FACTORY_CONFLICT', + message: `API '${apiRefId}' is already provided by plugin '${acceptedPluginId}', cannot also be provided by '${rejectedPluginId}'.`, + context: { + node: apiNode, + apiRefId, + pluginId: rejectedPluginId, + existingPluginId: acceptedPluginId, + }, + }); + if (shouldReplace) { + factoriesById.set(apiRefId, { + pluginId, + factory: apiFactory, + }); + } + continue; + } + + factoriesById.set(apiRefId, { pluginId, factory: apiFactory }); } else { options.collector.report({ code: 'API_EXTENSION_INVALID', @@ -408,7 +449,23 @@ function createApiFactories(options: { } } - return factories; + return Array.from(factoriesById.values(), entry => entry.factory); +} + +// TODO(Rugvip): It would be good if this was more explicit, but I think that +// might need to wait for some future update for API factories. +function getApiOwnerId(apiRefId: string): string { + const [prefix, ...rest] = apiRefId.split('.'); + if (!prefix) { + return apiRefId; + } + if (prefix === 'core') { + return 'app'; + } + if (prefix === 'plugin' && rest[0]) { + return rest[0]; + } + return prefix; } function createApiHolder(options: { diff --git a/packages/frontend-plugin-api/src/apis/system/ApiRef.ts b/packages/frontend-plugin-api/src/apis/system/ApiRef.ts index adedff9f73..0db3d89bd5 100644 --- a/packages/frontend-plugin-api/src/apis/system/ApiRef.ts +++ b/packages/frontend-plugin-api/src/apis/system/ApiRef.ts @@ -53,7 +53,15 @@ class ApiRefImpl implements ApiRef { } /** - * Creates a reference to an API. + * Creates a reference to an API. The provided `id` is a stable identifier for + * the API implementation. + * + * @remarks + * + * The frontend system infers the owning plugin for an API from the `id`. The + * recommended pattern is `plugin..*` (for example, + * `plugin.catalog.entity-presentation`). This ensures that other plugins can't + * mistakenly override your API implementation. * * @param config - The descriptor of the API to reference. * @returns An API reference.