From 8ba316e2870448129f27403ef876f3b91f3e3826 Mon Sep 17 00:00:00 2001 From: Marley Powell Date: Fri, 18 Aug 2023 11:38:22 +0100 Subject: [PATCH] fix: :bug: implemented up front circular dependency check in the `BackendInitializer` Signed-off-by: Marley Powell --- .changeset/tricky-melons-fold.md | 2 +- .../src/wiring/BackendInitializer.test.ts | 21 ++ .../src/wiring/BackendInitializer.ts | 2 + .../src/wiring/ServiceRegistry.test.ts | 333 +++++++++++------- .../src/wiring/ServiceRegistry.ts | 40 +-- packages/backend-app-api/src/wiring/types.ts | 1 + 6 files changed, 243 insertions(+), 156 deletions(-) diff --git a/.changeset/tricky-melons-fold.md b/.changeset/tricky-melons-fold.md index 69e60d48f0..8dbcb9ab5e 100644 --- a/.changeset/tricky-melons-fold.md +++ b/.changeset/tricky-melons-fold.md @@ -2,5 +2,5 @@ '@backstage/backend-app-api': patch --- -fix: :bug: implemented a circular dependency check in the `ServiceRegistry` +fix: :bug: implemented up front circular dependency check in the `BackendInitializer` fix: :bug: updated `detectCircularDependency` in `DependencyGraph` to return circular dependencies starting from the first node diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index 791b9b1c3a..2aa24fddd3 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -324,4 +324,25 @@ describe('BackendInitializer', () => { "Extension point registered for plugin 'testA' may not be used by module for plugin 'testB'", ); }); + + it('should throw if circular dependency cycles are detected', async () => { + const refA = createServiceRef({ id: 'a' }); + const refB = createServiceRef({ id: 'b' }); + const init = new BackendInitializer([ + createServiceFactory({ + service: refA, + deps: { b: refB }, + factory: async ({ b }) => b, + })(), + createServiceFactory({ + service: refB, + deps: { a: refA }, + factory: async ({ a }) => a, + })(), + ]); + await expect(init.start()).rejects.toThrow( + `Circular dependencies detected: + 'a' -> 'b' -> 'a'`, + ); + }); }); diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 93d2f6942a..63a55bb0bd 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -172,6 +172,8 @@ export class BackendInitializer { ...this.#providedServiceFactories, ]); + this.#serviceHolder.checkForCircularDeps(); + const featureDiscovery = await this.#serviceHolder.get( featureDiscoveryServiceRef, 'root', diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts index 25fcef3885..fca5933ce5 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts @@ -330,159 +330,222 @@ describe('ServiceRegistry', () => { ); }); - it('should throw if there are shallow circular dependencies', async () => { - const refA = createServiceRef({ id: 'a' }); - const refB = createServiceRef({ id: 'b' }); + describe('checkForCircularDeps', () => { + it('should throw if there are shallow circular dependencies', async () => { + const refA = createServiceRef({ id: 'a' }); + const refB = createServiceRef({ id: 'b' }); - const factoryA = createServiceFactory({ - service: refA, - deps: { b: refB }, - factory: async ({ b }) => b, + const factoryA = createServiceFactory({ + service: refA, + deps: { b: refB }, + factory: async ({ b }) => b, + }); + + const factoryB = createServiceFactory({ + service: refB, + deps: { a: refA }, + factory: async ({ a }) => a, + }); + + const registry = new ServiceRegistry([factoryA(), factoryB()]); + + expect(() => registry.checkForCircularDeps()).toThrow( + `Circular dependencies detected: + 'a' -> 'b' -> 'a'`, + ); }); - const factoryB = createServiceFactory({ - service: refB, - deps: { a: refA }, - factory: async ({ a }) => a, + it('should throw if there are multiple circular dependency cycles', async () => { + const refA = createServiceRef({ id: 'a' }); + const refB = createServiceRef({ id: 'b' }); + const refC = createServiceRef({ id: 'c' }); + const refD = createServiceRef({ id: 'd' }); + + const factoryA = createServiceFactory({ + service: refA, + deps: { b: refB }, + factory: async ({ b }) => b, + }); + + const factoryB = createServiceFactory({ + service: refB, + deps: { a: refA }, + factory: async ({ a }) => a, + }); + + const factoryC = createServiceFactory({ + service: refC, + deps: { d: refD }, + factory: async ({ d }) => d, + }); + + const factoryD = createServiceFactory({ + service: refD, + deps: { c: refC }, + factory: async ({ c }) => c, + }); + + const registry = new ServiceRegistry([ + factoryA(), + factoryB(), + factoryC(), + factoryD(), + ]); + + expect(() => registry.checkForCircularDeps()).toThrow( + `Circular dependencies detected: + 'a' -> 'b' -> 'a' + 'c' -> 'd' -> 'c'`, + ); }); - const registry = new ServiceRegistry([factoryA(), factoryB()]); + it('should throw if there are deep circular dependencies', async () => { + const refA = createServiceRef({ id: 'a' }); + const refB = createServiceRef({ id: 'b' }); + const refC = createServiceRef({ id: 'c' }); - await expect(registry.get(refA, 'catalog')).rejects.toThrow( - "Failed to instantiate service 'a' for 'catalog' because of the following circular dependency: 'a' -> 'b' -> 'a'", - ); - }); + const factoryA = createServiceFactory({ + service: refA, + deps: { b: refB }, + factory: async ({ b }) => b, + }); - it('should throw if there are deep circular dependencies', async () => { - const refA = createServiceRef({ id: 'a' }); - const refB = createServiceRef({ id: 'b' }); - const refC = createServiceRef({ id: 'c' }); + const factoryB = createServiceFactory({ + service: refB, + deps: { c: refC }, + factory: async ({ c }) => c, + }); - const factoryA = createServiceFactory({ - service: refA, - deps: { b: refB }, - factory: async ({ b }) => b, + const factoryC = createServiceFactory({ + service: refC, + deps: { a: refA }, + factory: async ({ a }) => a, + }); + + const registry = new ServiceRegistry([ + factoryA(), + factoryB(), + factoryC(), + ]); + + expect(() => registry.checkForCircularDeps()).toThrow( + `Circular dependencies detected: + 'a' -> 'b' -> 'c' -> 'a'`, + ); }); - const factoryB = createServiceFactory({ - service: refB, - deps: { c: refC }, - factory: async ({ c }) => c, + it('should throw if there are deep circular dependencies 2', async () => { + const refA = createServiceRef({ id: 'a' }); + const refB = createServiceRef({ id: 'b' }); + const refC = createServiceRef({ id: 'c' }); + const refD = createServiceRef({ id: 'd' }); + + const factoryA = createServiceFactory({ + service: refA, + deps: { b: refB }, + factory: async ({ b }) => b, + }); + + const factoryB = createServiceFactory({ + service: refB, + deps: { c: refC, d: refD }, + factory: async ({ c, d }) => c + d, + }); + + const factoryC = createServiceFactory({ + service: refC, + deps: { a: refA }, + factory: async ({ a }) => a, + }); + + const factoryD = createServiceFactory({ + service: refD, + deps: {}, + factory: async () => 'd', + }); + + const registry = new ServiceRegistry([ + factoryA(), + factoryB(), + factoryC(), + factoryD(), + ]); + + expect(() => registry.checkForCircularDeps()).toThrow( + `Circular dependencies detected: + 'a' -> 'b' -> 'c' -> 'a'`, + ); }); - const factoryC = createServiceFactory({ - service: refC, - deps: { a: refA }, - factory: async ({ a }) => a, + it('should throw if there are circular dependencies', async () => { + const refA = createServiceRef({ id: 'a' }); + const refB = createServiceRef({ id: 'b' }); + const refC = createServiceRef({ id: 'c' }); + + const factoryA = createServiceFactory({ + service: refA, + deps: { b: refB, c: refC }, + factory: async ({ b, c }) => b + c, + }); + + const factoryB = createServiceFactory({ + service: refB, + deps: {}, + factory: async () => 'b', + }); + + const factoryC = createServiceFactory({ + service: refC, + deps: { a: refA }, + factory: async ({ a }) => a, + }); + + const registry = new ServiceRegistry([ + factoryA(), + factoryB(), + factoryC(), + ]); + + expect(() => registry.checkForCircularDeps()).toThrow( + `Circular dependencies detected: + 'a' -> 'c' -> 'a'`, + ); }); - const registry = new ServiceRegistry([factoryA(), factoryB(), factoryC()]); + it('should not infinitely loop if there are circular dependencies where not all nodes are in the cycle', async () => { + const refA = createServiceRef({ id: 'a' }); + const refB = createServiceRef({ id: 'b' }); + const refC = createServiceRef({ id: 'c' }); - await expect(registry.get(refA, 'catalog')).rejects.toThrow( - "Failed to instantiate service 'a' for 'catalog' because of the following circular dependency: 'a' -> 'b' -> 'c' -> 'a'", - ); - }); + const factoryA = createServiceFactory({ + service: refA, + deps: { b: refB }, + factory: async ({ b }) => b, + }); - it('should throw if there are deep circular dependencies 2', async () => { - const refA = createServiceRef({ id: 'a' }); - const refB = createServiceRef({ id: 'b' }); - const refC = createServiceRef({ id: 'c' }); - const refD = createServiceRef({ id: 'd' }); + const factoryB = createServiceFactory({ + service: refB, + deps: { c: refC }, + factory: async ({ c }) => c, + }); - const factoryA = createServiceFactory({ - service: refA, - deps: { b: refB }, - factory: async ({ b }) => b, + const factoryC = createServiceFactory({ + service: refC, + deps: { b: refB }, + factory: async ({ b }) => b, + }); + + const registry = new ServiceRegistry([ + factoryA(), + factoryB(), + factoryC(), + ]); + + expect(() => registry.checkForCircularDeps()).toThrow( + `Circular dependencies detected: + 'b' -> 'c' -> 'b'`, + ); }); - - const factoryB = createServiceFactory({ - service: refB, - deps: { c: refC, d: refD }, - factory: async ({ c, d }) => c + d, - }); - - const factoryC = createServiceFactory({ - service: refC, - deps: { a: refA }, - factory: async ({ a }) => a, - }); - - const factoryD = createServiceFactory({ - service: refD, - deps: {}, - factory: async () => 'd', - }); - - const registry = new ServiceRegistry([ - factoryA(), - factoryB(), - factoryC(), - factoryD(), - ]); - - await expect(registry.get(refA, 'catalog')).rejects.toThrow( - "Failed to instantiate service 'a' for 'catalog' because of the following circular dependency: 'a' -> 'b' -> 'c' -> 'a'", - ); - }); - - it('should throw if there are circular dependencies', async () => { - const refA = createServiceRef({ id: 'a' }); - const refB = createServiceRef({ id: 'b' }); - const refC = createServiceRef({ id: 'c' }); - - const factoryA = createServiceFactory({ - service: refA, - deps: { b: refB, c: refC }, - factory: async ({ b, c }) => b + c, - }); - - const factoryB = createServiceFactory({ - service: refB, - deps: {}, - factory: async () => 'b', - }); - - const factoryC = createServiceFactory({ - service: refC, - deps: { a: refA }, - factory: async ({ a }) => a, - }); - - const registry = new ServiceRegistry([factoryA(), factoryB(), factoryC()]); - - await expect(registry.get(refC, 'catalog')).rejects.toThrow( - "Failed to instantiate service 'c' for 'catalog' because of the following circular dependency: 'a' -> 'c' -> 'a'", - ); - }); - - it('should not infinitely loop if there are circular dependencies where not all nodes are in the cycle', async () => { - const refA = createServiceRef({ id: 'a' }); - const refB = createServiceRef({ id: 'b' }); - const refC = createServiceRef({ id: 'c' }); - - const factoryA = createServiceFactory({ - service: refA, - deps: { b: refB }, - factory: async ({ b }) => b, - }); - - const factoryB = createServiceFactory({ - service: refB, - deps: { c: refC }, - factory: async ({ c }) => c, - }); - - const factoryC = createServiceFactory({ - service: refC, - deps: { b: refB }, - factory: async ({ b }) => b, - }); - - const registry = new ServiceRegistry([factoryA(), factoryB(), factoryC()]); - - await expect(registry.get(refA, 'catalog')).rejects.toThrow( - "Failed to instantiate service 'a' for 'catalog' because of the following circular dependency: 'b' -> 'c' -> 'b'", - ); }); it('should decorate error messages thrown by the top-level factory function', async () => { diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.ts index cdc6f0b711..2c850175c9 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts @@ -20,7 +20,7 @@ import { coreServices, createServiceFactory, } from '@backstage/backend-plugin-api'; -import { stringifyError } from '@backstage/errors'; +import { ConflictError, stringifyError } from '@backstage/errors'; import { EnumerableServiceHolder } from './types'; // Direct internal import to avoid duplication // eslint-disable-next-line @backstage/no-forbidden-package-imports @@ -74,6 +74,7 @@ export class ServiceRegistry implements EnumerableServiceHolder { InternalServiceFactory, Promise >(); + readonly #dependencyGraph: DependencyGraph; constructor(factories: Array) { this.#providedFactories = new Map( @@ -81,6 +82,15 @@ export class ServiceRegistry implements EnumerableServiceHolder { ); this.#loadedDefaultFactories = new Map(); this.#implementations = new Map(); + this.#dependencyGraph = DependencyGraph.fromIterable( + Array.from(this.#providedFactories).map( + ([serviceId, serviceFactory]) => ({ + value: serviceId, + provides: [serviceId], + consumes: Object.values(serviceFactory.deps).map(d => d.id), + }), + ), + ); } #resolveFactory( @@ -147,25 +157,17 @@ export class ServiceRegistry implements EnumerableServiceHolder { } } - #checkForCircularDeps(factory: InternalServiceFactory, pluginId: string) { - const tree = DependencyGraph.fromIterable( - Array.from(this.#providedFactories).map( - ([serviceId, serviceFactory]) => ({ - value: { serviceId, serviceFactory }, - provides: [serviceId], - consumes: serviceFactory ? Object.keys(serviceFactory.deps) : [], - }), - ), + checkForCircularDeps(): void { + const circularDependencies = Array.from( + this.#dependencyGraph.detectCircularDependencies(), ); - const circular = tree.detectCircularDependency(); - if (circular) { - const circularDepChain = circular - .map(({ serviceId }) => `'${serviceId}'`) - .join(' -> '); - throw new Error( - `Failed to instantiate service '${factory.service.id}' for '${pluginId}' because of the following circular dependency: ${circularDepChain}`, - ); + if (circularDependencies.length) { + const cycles = circularDependencies + .map(c => c.map(id => `'${id}'`).join(' -> ')) + .join('\n '); + + throw new ConflictError(`Circular dependencies detected:\n ${cycles}`); } } @@ -179,7 +181,6 @@ export class ServiceRegistry implements EnumerableServiceHolder { let existing = this.#rootServiceImplementations.get(factory); if (!existing) { this.#checkForMissingDeps(factory, pluginId); - this.#checkForCircularDeps(factory, pluginId); const rootDeps = new Array>(); for (const [name, serviceRef] of Object.entries(factory.deps)) { @@ -203,7 +204,6 @@ export class ServiceRegistry implements EnumerableServiceHolder { let implementation = this.#implementations.get(factory); if (!implementation) { this.#checkForMissingDeps(factory, pluginId); - this.#checkForCircularDeps(factory, pluginId); const rootDeps = new Array>(); for (const [name, serviceRef] of Object.entries(factory.deps)) { diff --git a/packages/backend-app-api/src/wiring/types.ts b/packages/backend-app-api/src/wiring/types.ts index 552a69a792..07974c182e 100644 --- a/packages/backend-app-api/src/wiring/types.ts +++ b/packages/backend-app-api/src/wiring/types.ts @@ -39,6 +39,7 @@ export interface CreateSpecializedBackendOptions { export interface ServiceHolder { get(api: ServiceRef, pluginId: string): Promise | undefined; + checkForCircularDeps(): void; } /**