From ebf083d9024de3aca1f038260d69fe1e134ed3d1 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Sat, 7 Dec 2024 20:12:23 +0100 Subject: [PATCH 1/3] backend-app-api: enable service overrides over feature loaders Signed-off-by: Patrik Oldsberg --- .changeset/heavy-tomatoes-laugh.md | 5 + .../architecture/07-feature-loaders.md | 23 ++++ .../src/wiring/BackendInitializer.test.ts | 116 ++++++++++++++++++ .../src/wiring/BackendInitializer.ts | 33 ++++- .../src/wiring/ServiceRegistry.ts | 10 ++ 5 files changed, 185 insertions(+), 2 deletions(-) create mode 100644 .changeset/heavy-tomatoes-laugh.md diff --git a/.changeset/heavy-tomatoes-laugh.md b/.changeset/heavy-tomatoes-laugh.md new file mode 100644 index 0000000000..9a4740e209 --- /dev/null +++ b/.changeset/heavy-tomatoes-laugh.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': minor +--- + +Service factories added by feature loaders now have lower priority and will be ignored if a factory for the same service is added directly by `backend.add(serviceFactory)`. diff --git a/docs/backend-system/architecture/07-feature-loaders.md b/docs/backend-system/architecture/07-feature-loaders.md index 43954ec443..3edabfabbd 100644 --- a/docs/backend-system/architecture/07-feature-loaders.md +++ b/docs/backend-system/architecture/07-feature-loaders.md @@ -73,6 +73,29 @@ export default createBackendFeatureLoader({ }); ``` +### Overriding service factories + +Service factories registered by feature loaders have lower priority by ones added directly via `backend.add`. This allows you to use a feature loader for a larger number of service implementations, but still override individual services. + +The ordering in which different feature loaders or service factories are added does not matter. There is also no priority between feature loaders, if two different feature loaders add a factory for the same service, the backend will fail to start. + +```ts +const backend = createBackend(); + +backend.add( + createBackendFeatureLoader({ + async *loader() { + yield import('./commonDiscoveryService'); // discovery service + yield import('./commonRootLoggerService'); // root logger service + }, + }), +); + +backend.add(import('./myDiscoveryService')); // discovery service +``` + +The result of the above example is that the backend starts up with `./myDiscoveryService` as the discovery service implementation, while `./commonDiscoveryService` is ignored. The `./commonRootLoggerService` will still be used. + ### Dynamic logic A feature loader can also be asynchronous, and for example fetch data from an external source to determine which features to load: diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index f6d65e883e..5e98a71d46 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -25,6 +25,7 @@ import { createBackendModule, createExtensionPoint, createBackendFeatureLoader, + ServiceRef, } from '@backstage/backend-plugin-api'; import { BackendInitializer } from './BackendInitializer'; import { instanceMetadataServiceRef } from '@backstage/backend-plugin-api/alpha'; @@ -50,6 +51,18 @@ const baseFactories = [ loggerServiceFactory, ]; +function mkNoopFactory(ref: ServiceRef<{}, 'plugin'>) { + const fn = jest.fn().mockReturnValue({}); + return Object.assign( + fn, + createServiceFactory({ + service: ref, + deps: {}, + factory: fn, + }), + ); +} + const testPlugin = createBackendPlugin({ pluginId: 'test', register(reg) { @@ -167,6 +180,109 @@ describe('BackendInitializer', () => { expect(moduleInit).toHaveBeenCalled(); }); + it('should ignore services provided by feature loaders that have already been explicitly added', async () => { + const ref = createServiceRef<{}>({ id: '1' }); + const factory1 = mkNoopFactory(ref); + const factory2 = mkNoopFactory(ref); + const factory3 = mkNoopFactory(ref); + + const init = new BackendInitializer([...baseFactories, factory1]); + init.add(factory2); + init.add( + createBackendFeatureLoader({ + deps: {}, + *loader() { + yield factory3; + }, + }), + ); + init.add( + createBackendPlugin({ + pluginId: 'tester', + register(reg) { + reg.registerInit({ + deps: { ref }, + async init() {}, + }); + }, + }), + ); + + await init.start(); + + expect(factory1).not.toHaveBeenCalled(); + expect(factory2).toHaveBeenCalled(); + expect(factory3).not.toHaveBeenCalled(); + }); + + // Note: this is an important escape hatch in case to loaders conflict and you need to select the winning service factory + it('should allow duplicate service from feature loaders if overridden', async () => { + const ref = createServiceRef<{}>({ id: '1' }); + const factory1 = mkNoopFactory(ref); + const factory2 = mkNoopFactory(ref); + const factory3 = mkNoopFactory(ref); + const factory4 = mkNoopFactory(ref); + + const init = new BackendInitializer([...baseFactories, factory1]); + init.add(factory2); + init.add( + createBackendFeatureLoader({ + deps: {}, + *loader() { + yield factory3; + yield factory4; + }, + }), + ); + init.add( + createBackendPlugin({ + pluginId: 'tester', + register(reg) { + reg.registerInit({ + deps: { ref }, + async init() {}, + }); + }, + }), + ); + + await init.start(); + + expect(factory1).not.toHaveBeenCalled(); + expect(factory2).toHaveBeenCalled(); + expect(factory3).not.toHaveBeenCalled(); + expect(factory4).not.toHaveBeenCalled(); + }); + + it('should reject duplicate service factories from feature loader without an explicit override', async () => { + const ref = createServiceRef<{}>({ id: '1' }); + const factory1 = mkNoopFactory(ref); + const factory2 = mkNoopFactory(ref); + const factory3 = mkNoopFactory(ref); + + const init = new BackendInitializer([...baseFactories, factory1]); + init.add( + createBackendFeatureLoader({ + deps: {}, + *loader() { + yield factory2; + }, + }), + ); + init.add( + createBackendFeatureLoader({ + deps: {}, + *loader() { + yield factory3; + }, + }), + ); + + await expect(init.start()).rejects.toThrow( + 'Duplicate service implementations provided for 1 by both feature loader created at', + ); + }); + it('should refuse to override already initialized services through loaded features', async () => { const ref1 = createServiceRef<{ x: number }>({ id: '1', diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 3e7b46fc9e..89e07ac405 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -519,6 +519,11 @@ export class BackendInitializer { } async #applyBackendFeatureLoaders(loaders: InternalBackendFeatureLoader[]) { + const servicesAddedByLoaders = new Map< + string, + InternalBackendFeatureLoader + >(); + for (const loader of loaders) { const deps = new Map(); const missingRefs = new Set(); @@ -564,8 +569,32 @@ export class BackendInitializer { if (isBackendFeatureLoader(feature)) { newLoaders.push(feature); } else { - didAddServiceFactory ||= isServiceFactory(feature); - this.#addFeature(feature); + // This block makes sure that feature loaders do not provide duplicate + // implementations for the same service, but at the same time allows + // service factories provided by feature loaders to be overridden by + // ones that are explicitly installed with backend.add(serviceFactory). + // + // If a factory has already been explicitly installed, the service + // factory provided by the loader will simply be ignored. + if (isServiceFactory(feature)) { + const conflictingLoader = servicesAddedByLoaders.get( + feature.service.id, + ); + if (conflictingLoader) { + throw new Error( + `Duplicate service implementations provided for ${feature.service.id} by both feature loader ${loader.description} and feature loader ${conflictingLoader.description}`, + ); + } + + // Check that this service wasn't already explicitly added by backend.add(serviceFactory) + if (!this.#serviceRegistry.hasBeenAdded(feature.service)) { + didAddServiceFactory = true; + servicesAddedByLoaders.set(feature.service.id, loader); + this.#addFeature(feature); + } + } else { + this.#addFeature(feature); + } } } diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.ts index 0abd5605ae..162c175615 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts @@ -191,6 +191,16 @@ export class ServiceRegistry { } } + hasBeenAdded(ref: ServiceRef) { + if (ref.id === coreServices.pluginMetadata.id) { + return true; + } + if (ref.multiton) { + return false; + } + return this.#addedFactoryIds.has(ref.id); + } + add(factory: ServiceFactory) { const factoryId = factory.service.id; if (factoryId === coreServices.pluginMetadata.id) { From 7cb6d145db6f50d1a5c7fa5e3363aa7e00186116 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 9 Dec 2024 09:44:57 +0100 Subject: [PATCH 2/3] backend-app-api: fix handling of multiton factories from feature loaders Signed-off-by: Patrik Oldsberg --- .../src/wiring/BackendInitializer.test.ts | 42 +++++++++++++++++++ .../src/wiring/BackendInitializer.ts | 2 +- .../src/wiring/ServiceRegistry.ts | 3 -- 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index 5e98a71d46..86b0b51bd9 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -215,6 +215,48 @@ describe('BackendInitializer', () => { expect(factory3).not.toHaveBeenCalled(); }); + it('should include all multiton service factories', async () => { + expect.assertions(5); + + const ref = createServiceRef({ id: '1', multiton: true }); + const factory1 = mkNoopFactory(ref).mockResolvedValue(1); + const factory2 = mkNoopFactory(ref).mockResolvedValue(2); + const factory3 = mkNoopFactory(ref).mockResolvedValue(3); + const factory4 = mkNoopFactory(ref).mockResolvedValue(4); + + const init = new BackendInitializer([...baseFactories, factory1]); + init.add(factory2); + init.add( + createBackendFeatureLoader({ + deps: {}, + *loader() { + yield factory3; + yield factory4; + }, + }), + ); + init.add( + createBackendPlugin({ + pluginId: 'tester', + register(reg) { + reg.registerInit({ + deps: { ns: ref }, + async init({ ns }) { + expect(ns).toEqual([1, 2, 3, 4]); + }, + }); + }, + }), + ); + + await init.start(); + + expect(factory1).toHaveBeenCalled(); + expect(factory2).toHaveBeenCalled(); + expect(factory3).toHaveBeenCalled(); + expect(factory4).toHaveBeenCalled(); + }); + // Note: this is an important escape hatch in case to loaders conflict and you need to select the winning service factory it('should allow duplicate service from feature loaders if overridden', async () => { const ref = createServiceRef<{}>({ id: '1' }); diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 89e07ac405..440a334d9b 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -576,7 +576,7 @@ export class BackendInitializer { // // If a factory has already been explicitly installed, the service // factory provided by the loader will simply be ignored. - if (isServiceFactory(feature)) { + if (isServiceFactory(feature) && !feature.service.multiton) { const conflictingLoader = servicesAddedByLoaders.get( feature.service.id, ); diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.ts index 162c175615..90b108206e 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts @@ -195,9 +195,6 @@ export class ServiceRegistry { if (ref.id === coreServices.pluginMetadata.id) { return true; } - if (ref.multiton) { - return false; - } return this.#addedFactoryIds.has(ref.id); } From f740164569ba9b6af9c9ee28c3a33db8c716b059 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Tue, 10 Dec 2024 11:27:10 +0100 Subject: [PATCH 3/3] Update docs/backend-system/architecture/07-feature-loaders.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Signed-off-by: Patrik Oldsberg --- docs/backend-system/architecture/07-feature-loaders.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/backend-system/architecture/07-feature-loaders.md b/docs/backend-system/architecture/07-feature-loaders.md index 3edabfabbd..b2cc574bf9 100644 --- a/docs/backend-system/architecture/07-feature-loaders.md +++ b/docs/backend-system/architecture/07-feature-loaders.md @@ -75,7 +75,7 @@ export default createBackendFeatureLoader({ ### Overriding service factories -Service factories registered by feature loaders have lower priority by ones added directly via `backend.add`. This allows you to use a feature loader for a larger number of service implementations, but still override individual services. +Service factories registered by feature loaders have lower priority than ones added directly via `backend.add`. This allows you to use a feature loader for a larger number of service implementations, but still override individual services. The ordering in which different feature loaders or service factories are added does not matter. There is also no priority between feature loaders, if two different feature loaders add a factory for the same service, the backend will fail to start.