diff --git a/.changeset/tricky-melons-fold.md b/.changeset/tricky-melons-fold.md new file mode 100644 index 0000000000..036413ea8e --- /dev/null +++ b/.changeset/tricky-melons-fold.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': patch +--- + +Backend startup will now fail if any circular service dependencies are detected. diff --git a/packages/backend-app-api/src/lib/DependencyGraph.test.ts b/packages/backend-app-api/src/lib/DependencyGraph.test.ts index 65efac0252..394359e486 100644 --- a/packages/backend-app-api/src/lib/DependencyGraph.test.ts +++ b/packages/backend-app-api/src/lib/DependencyGraph.test.ts @@ -27,7 +27,7 @@ describe('DependencyGraph', () => { }); describe('detectCircularDependency', () => { - it('should return undefined with not deps', () => { + it('should return undefined with no deps', () => { expect( DependencyGraph.fromMap({ 1: {}, @@ -38,7 +38,7 @@ describe('DependencyGraph', () => { ).toBeUndefined(); }); - it('should return undefined with no circular deps', async () => { + it('should return undefined with no circular deps', () => { expect( DependencyGraph.fromMap({ 1: { provides: ['a'] }, @@ -49,7 +49,7 @@ describe('DependencyGraph', () => { ).toBeUndefined(); }); - it('should detect an immediate circular dep', async () => { + it('should detect an immediate circular dependency', () => { expect( DependencyGraph.fromMap({ 1: { provides: ['a'], consumes: ['a'] }, @@ -57,7 +57,7 @@ describe('DependencyGraph', () => { ).toEqual(['1', '1']); }); - it('should detect a small circular dep', async () => { + it('should detect a small circular dependency', () => { expect( DependencyGraph.fromMap({ 1: { provides: ['a'], consumes: ['b'] }, @@ -66,7 +66,7 @@ describe('DependencyGraph', () => { ).toEqual(['1', '2', '1']); }); - it('should detect a larger distant circular dep', async () => { + it('should detect a larger distance circular dependency', () => { expect( DependencyGraph.fromMap({ 1: { provides: ['a'] }, @@ -74,7 +74,280 @@ describe('DependencyGraph', () => { 3: { provides: ['c'], consumes: ['b'] }, 4: { provides: ['d', 'e'], consumes: ['c', 'a'] }, }).detectCircularDependency(), - ).toEqual(['2', '3', '4', '2']); + ).toEqual(['2', '4', '3', '2']); + }); + + it('should detect a circular dependency starting from the first node', () => { + expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'], consumes: ['b'] }, + 2: { provides: ['b'], consumes: ['c'] }, + 3: { provides: ['c'], consumes: ['d'] }, + 4: { provides: ['d'], consumes: ['a'] }, + }).detectCircularDependency(), + ).toEqual(['1', '2', '3', '4', '1']); + }); + + it('should detect all independent circular dependency cycles', () => { + expect( + Array.from( + DependencyGraph.fromMap({ + // Cycle 1 + 1: { provides: ['a'], consumes: ['b'] }, + 2: { provides: ['b'], consumes: ['c'] }, + 3: { provides: ['c'], consumes: ['a'] }, + + // Cycle 2 + 4: { provides: ['d'], consumes: ['e'] }, + 5: { provides: ['e'], consumes: ['f'] }, + 6: { provides: ['f'], consumes: ['d'] }, + + // Cycle 3 + 7: { provides: ['g'], consumes: ['h'] }, + 8: { provides: ['h'], consumes: ['i'] }, + 9: { provides: ['i'], consumes: ['g'] }, + }).detectCircularDependencies(), + ), + ).toEqual([ + ['1', '2', '3', '1'], + ['4', '5', '6', '4'], + ['7', '8', '9', '7'], + ]); + }); + + it('should only detect unique circular dependency cycles', () => { + expect( + Array.from( + DependencyGraph.fromMap({ + 1: { provides: ['a'], consumes: ['b'] }, + 2: { provides: ['b'], consumes: ['c', 'a'] }, + 3: { provides: ['c'], consumes: ['d', 'a'] }, + 4: { provides: ['d'], consumes: ['e', 'a'] }, + 5: { provides: ['e'], consumes: ['f', 'a'] }, + 6: { provides: ['f'], consumes: ['h', 'a'] }, + 7: { provides: ['h'], consumes: ['a'] }, + }).detectCircularDependencies(), + ), + ).toEqual([ + ['1', '2', '1'], + ['1', '2', '3', '1'], + ['1', '2', '3', '4', '1'], + ['1', '2', '3', '4', '5', '1'], + ['1', '2', '3', '4', '5', '6', '1'], + ['1', '2', '3', '4', '5', '6', '7', '1'], + ]); + }); + + it('should detect circular dependency cycles in order when fromIterable', () => { + expect( + Array.from( + DependencyGraph.fromIterable([ + { value: 'a', provides: ['a'], consumes: ['b'] }, + { value: 'b', provides: ['b'], consumes: ['c', 'a'] }, + { value: 'c', provides: ['c'], consumes: ['d', 'a'] }, + { value: '4', provides: ['d'], consumes: ['e', 'a'] }, + { value: '5', provides: ['e'], consumes: ['f', 'a'] }, + { value: '6', provides: ['f'], consumes: ['h', 'a'] }, + ]).detectCircularDependencies(), + ), + ).toEqual([ + ['a', 'b', 'a'], + ['a', 'b', 'c', 'a'], + ['a', 'b', 'c', '4', 'a'], + ['a', 'b', 'c', '4', '5', 'a'], + ['a', 'b', 'c', '4', '5', '6', 'a'], + ]); + }); + + it('should detect circular dependency cycles in order by key when fromMap', () => { + expect( + Array.from( + DependencyGraph.fromMap({ + 1: { provides: ['a'], consumes: ['b'] }, + 2: { provides: ['b'], consumes: ['c', 'a'] }, + 3: { provides: ['c'], consumes: ['d', 'a'] }, + 4: { provides: ['d'], consumes: ['e', 'a'] }, + 5: { provides: ['e'], consumes: ['f', 'a'] }, + 6: { provides: ['f'], consumes: ['h', 'a'] }, + }).detectCircularDependencies(), + ), + ).toEqual([ + ['1', '2', '1'], + ['1', '2', '3', '1'], + ['1', '2', '3', '4', '1'], + ['1', '2', '3', '4', '5', '1'], + ['1', '2', '3', '4', '5', '6', '1'], + ]); + }); + + it('should detect circular dependency cycles in order by key when fromMap 2', () => { + expect( + Array.from( + DependencyGraph.fromMap({ + a: { provides: ['a'], consumes: ['b'] }, + b: { provides: ['b'], consumes: ['c', 'a'] }, + c: { provides: ['c'], consumes: ['d', 'a'] }, + 4: { provides: ['d'], consumes: ['e', 'a'] }, + 5: { provides: ['e'], consumes: ['f', 'a'] }, + 6: { provides: ['f'], consumes: ['h', 'a'] }, + }).detectCircularDependencies(), + ), + ).toEqual([ + ['4', 'a', 'b', 'c', '4'], + ['5', 'a', 'b', 'c', '4', '5'], + ['6', 'a', 'b', 'c', '4', '5', '6'], + ['a', 'b', 'a'], + ['a', 'b', 'c', 'a'], + ]); + }); + + it('should detect circular dependency cycles in order by key when fromMap 3', () => { + expect( + Array.from( + DependencyGraph.fromMap({ + a: { provides: ['a'], consumes: ['b'] }, + b: { provides: ['b'], consumes: ['c', 'a'] }, + c: { provides: ['c'], consumes: ['d', 'a'] }, + d: { provides: ['d'], consumes: ['e', 'a'] }, + e: { provides: ['e'], consumes: ['f', 'a'] }, + f: { provides: ['f'], consumes: ['h', 'a'] }, + }).detectCircularDependencies(), + ), + ).toEqual([ + ['a', 'b', 'a'], + ['a', 'b', 'c', 'a'], + ['a', 'b', 'c', 'd', 'a'], + ['a', 'b', 'c', 'd', 'e', 'a'], + ['a', 'b', 'c', 'd', 'e', 'f', 'a'], + ]); + }); + + it('should detect circular dependency cycles with duplicate keys when fromIterable', () => { + expect( + Array.from( + DependencyGraph.fromIterable([ + { value: 'a', provides: ['a'], consumes: ['b'] }, + { value: 'a', provides: ['b'], consumes: ['c', 'a'] }, + { value: 'a', provides: ['c'], consumes: ['d', 'a'] }, + { value: 'a', provides: ['d'], consumes: ['e', 'a'] }, + { value: 'a', provides: ['e'], consumes: ['f', 'a'] }, + { value: 'a', provides: ['f'], consumes: ['h', 'a'] }, + ]).detectCircularDependencies(), + ), + ).toEqual([ + ['a', 'a', 'a'], + ['a', 'a', 'a', 'a'], + ['a', 'a', 'a', 'a', 'a'], + ['a', 'a', 'a', 'a', 'a', 'a'], + ['a', 'a', 'a', 'a', 'a', 'a', 'a'], + ]); + }); + + it('should detect circular dependency cycles with object values when fromIterable', () => { + expect( + Array.from( + DependencyGraph.fromIterable([ + { value: { key: 1 }, provides: ['a'], consumes: ['b'] }, + { value: { key: 2 }, provides: ['b'], consumes: ['c', 'a'] }, + { value: { key: 3 }, provides: ['c'], consumes: ['d', 'a'] }, + { value: { key: 4 }, provides: ['d'], consumes: ['e', 'a'] }, + { value: { key: 5 }, provides: ['e'], consumes: ['f', 'a'] }, + { value: { key: 6 }, provides: ['f'], consumes: ['h', 'a'] }, + ]).detectCircularDependencies(), + ), + ).toEqual([ + [{ key: 1 }, { key: 2 }, { key: 1 }], + [{ key: 1 }, { key: 2 }, { key: 3 }, { key: 1 }], + [{ key: 1 }, { key: 2 }, { key: 3 }, { key: 4 }, { key: 1 }], + [ + { key: 1 }, + { key: 2 }, + { key: 3 }, + { key: 4 }, + { key: 5 }, + { key: 1 }, + ], + [ + { key: 1 }, + { key: 2 }, + { key: 3 }, + { key: 4 }, + { key: 5 }, + { key: 6 }, + { key: 1 }, + ], + ]); + }); + + it('should detect circular dependency cycles with array values when fromIterable', () => { + expect( + Array.from( + DependencyGraph.fromIterable([ + { value: [1], provides: ['a'], consumes: ['b'] }, + { value: [2], provides: ['b'], consumes: ['c', 'a'] }, + { value: [3], provides: ['c'], consumes: ['d', 'a'] }, + { value: [4], provides: ['d'], consumes: ['e', 'a'] }, + { value: [5], provides: ['e'], consumes: ['f', 'a'] }, + { value: [6], provides: ['f'], consumes: ['h', 'a'] }, + ]).detectCircularDependencies(), + ), + ).toEqual([ + [[1], [2], [1]], + [[1], [2], [3], [1]], + [[1], [2], [3], [4], [1]], + [[1], [2], [3], [4], [5], [1]], + [[1], [2], [3], [4], [5], [6], [1]], + ]); + }); + + it('should detect circular dependency cycles by reference with symbol values when fromIterable', () => { + const symbol1 = Symbol(1); + const symbol2 = Symbol(2); + const symbol3 = Symbol(3); + const symbol4 = Symbol(4); + const symbol5 = Symbol(5); + const symbol6 = Symbol(6); + + expect( + Array.from( + DependencyGraph.fromIterable([ + { value: symbol1, provides: ['a'], consumes: ['b'] }, + { value: symbol2, provides: ['b'], consumes: ['c', 'a'] }, + { value: symbol3, provides: ['c'], consumes: ['d', 'a'] }, + { value: symbol4, provides: ['d'], consumes: ['e', 'a'] }, + { value: symbol5, provides: ['e'], consumes: ['f', 'a'] }, + { value: symbol6, provides: ['f'], consumes: ['h', 'a'] }, + ]).detectCircularDependencies(), + ), + ).toEqual([ + [symbol1, symbol2, symbol1], + [symbol1, symbol2, symbol3, symbol1], + [symbol1, symbol2, symbol3, symbol4, symbol1], + [symbol1, symbol2, symbol3, symbol4, symbol5, symbol1], + [symbol1, symbol2, symbol3, symbol4, symbol5, symbol6, symbol1], + ]); + }); + + it('should ignore circular dependency cycles by reference with symbol values when fromMap', () => { + const symbol1 = Symbol('1'); + const symbol2 = Symbol('2'); + const symbol3 = Symbol('3'); + const symbol4 = Symbol('4'); + const symbol5 = Symbol('5'); + const symbol6 = Symbol('6'); + + expect( + Array.from( + DependencyGraph.fromMap({ + [symbol1]: { provides: ['a'], consumes: ['b'] }, + [symbol2]: { provides: ['b'], consumes: ['c', 'a'] }, + [symbol3]: { provides: ['c'], consumes: ['d', 'a'] }, + [symbol4]: { provides: ['d'], consumes: ['e', 'a'] }, + [symbol5]: { provides: ['e'], consumes: ['f', 'a'] }, + [symbol6]: { provides: ['f'], consumes: ['h', 'a'] }, + }).detectCircularDependencies(), + ), + ).toEqual([]); }); }); diff --git a/packages/backend-app-api/src/lib/DependencyGraph.ts b/packages/backend-app-api/src/lib/DependencyGraph.ts index 8fd679f555..86fad3fbf2 100644 --- a/packages/backend-app-api/src/lib/DependencyGraph.ts +++ b/packages/backend-app-api/src/lib/DependencyGraph.ts @@ -37,6 +37,37 @@ class Node { ) {} } +/** @internal */ +class CycleKeySet { + static from(nodes: Array>) { + return new CycleKeySet(nodes); + } + + #nodeIds: Map; + #cycleKeys: Set; + + private constructor(nodes: Array>) { + this.#nodeIds = new Map(nodes.map((n, i) => [n.value, i])); + this.#cycleKeys = new Set(); + } + + tryAdd(path: T[]): boolean { + const cycleKey = this.#getCycleKey(path); + if (this.#cycleKeys.has(cycleKey)) { + return false; + } + this.#cycleKeys.add(cycleKey); + return true; + } + + #getCycleKey(path: T[]): string { + return path + .map(n => this.#nodeIds.get(n)!) + .sort() + .join(','); + } +} + /** * Internal helper to help validate and traverse a dependency graph. * @internal @@ -78,7 +109,9 @@ export class DependencyGraph { } } - // Find all nodes that consume dependencies that are not provided by any other node + /** + * Find all nodes that consume dependencies that are not provided by any other node. + */ findUnsatisfiedDeps(): Array<{ value: T; unsatisfied: string[] }> { const unsatisfiedDependencies = []; for (const node of this.#nodes.values()) { @@ -92,9 +125,21 @@ export class DependencyGraph { return unsatisfiedDependencies; } - // Detect circular dependencies within the graph, returning the path of nodes that - // form a cycle, with the same node as the first and last element of the array. + /** + * Detect the first circular dependency within the graph, returning the path of nodes that + * form a cycle, with the same node as the first and last element of the array. + */ detectCircularDependency(): T[] | undefined { + return this.detectCircularDependencies().next().value; + } + + /** + * Detect circular dependencies within the graph, returning the path of nodes that + * form a cycle, with the same node as the first and last element of the array. + */ + *detectCircularDependencies(): Generator { + const cycleKeys = CycleKeySet.from(this.#nodes); + for (const startNode of this.#nodes) { const visited = new Set>(); const stack = new Array<[node: Node, path: T[]]>([ @@ -108,16 +153,20 @@ export class DependencyGraph { continue; } visited.add(node); - for (const produced of node.provides) { - const consumerNodes = this.#nodes.filter(other => - other.consumes.has(produced), + for (const consumed of node.consumes) { + const providerNodes = this.#nodes.filter(other => + other.provides.has(consumed), ); - for (const consumer of consumerNodes) { - if (consumer === startNode) { - return [...path, startNode.value]; + for (const provider of providerNodes) { + if (provider === startNode) { + if (cycleKeys.tryAdd(path)) { + yield [...path, startNode.value]; + } + + break; } - if (!visited.has(consumer)) { - stack.push([consumer, [...path, consumer.value]]); + if (!visited.has(provider)) { + stack.push([provider, [...path, provider.value]]); } } } diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 39c4bca265..dbc807b9c3 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -164,7 +164,7 @@ export class BackendInitializer { } async #doStart(): Promise { - this.#serviceHolder = new ServiceRegistry([ + this.#serviceHolder = ServiceRegistry.create([ ...this.#defaultApiFactories, ...this.#providedServiceFactories, ]); diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts index b60a24c8d8..76bcd071cd 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts @@ -90,12 +90,12 @@ const refDefault2b = createServiceRef<{ x: number }>({ describe('ServiceRegistry', () => { it('should return undefined if there is no factory defined', async () => { - const registry = new ServiceRegistry([]); + const registry = ServiceRegistry.create([]); expect(registry.get(ref1, 'catalog')).toBe(undefined); }); it('should return an implementation for a registered ref', async () => { - const registry = new ServiceRegistry([sf1()]); + const registry = ServiceRegistry.create([sf1()]); await expect(registry.get(ref1, 'catalog')).resolves.toEqual({ x: 1 }); await expect(registry.get(ref1, 'scaffolder')).resolves.toEqual({ x: 1 }); expect(await registry.get(ref1, 'catalog')).toBe( @@ -110,7 +110,7 @@ describe('ServiceRegistry', () => { }); it('should handle multiple factories with different serviceRefs', async () => { - const registry = new ServiceRegistry([sf1(), sf2()]); + const registry = ServiceRegistry.create([sf1(), sf2()]); await expect(registry.get(ref1, 'catalog')).resolves.toEqual({ x: 1, @@ -131,7 +131,7 @@ describe('ServiceRegistry', () => { return { x: 2 }; }, }); - const registry = new ServiceRegistry([factory(), sf1()]); + const registry = ServiceRegistry.create([factory(), sf1()]); await expect(registry.get(ref2, 'catalog')).rejects.toThrow( "Failed to instantiate 'root' scoped service '2' because it depends on 'plugin' scoped service '1'.", ); @@ -145,7 +145,7 @@ describe('ServiceRegistry', () => { return { x: rootDep.x }; }, }); - const registry = new ServiceRegistry([factory(), sf2()]); + const registry = ServiceRegistry.create([factory(), sf2()]); await expect(registry.get(ref1, 'catalog')).resolves.toEqual({ x: 2, }); @@ -160,7 +160,7 @@ describe('ServiceRegistry', () => { return { x: rootDep.x }; }, }); - const registry = new ServiceRegistry([factory(), sf2()]); + const registry = ServiceRegistry.create([factory(), sf2()]); await expect(registry.get(ref, 'catalog')).resolves.toEqual({ x: 2, }); @@ -175,35 +175,35 @@ describe('ServiceRegistry', () => { return { pluginId: meta.getId() }; }, }); - const registry = new ServiceRegistry([factory()]); + const registry = ServiceRegistry.create([factory()]); await expect(registry.get(ref, 'catalog')).resolves.toEqual({ pluginId: 'catalog', }); }); it('should use the last factory for each ref', async () => { - const registry = new ServiceRegistry([sf2(), sf2b()]); + const registry = ServiceRegistry.create([sf2(), sf2b()]); await expect(registry.get(ref2, 'catalog')).resolves.toEqual({ x: 22, }); }); it('should use the defaultFactory from the ref if not provided to the registry', async () => { - const registry = new ServiceRegistry([]); + const registry = ServiceRegistry.create([]); await expect(registry.get(refDefault1, 'catalog')).resolves.toEqual({ x: 10, }); }); it('should not use the defaultFactory from the ref if provided to the registry', async () => { - const registry = new ServiceRegistry([sf1()]); + const registry = ServiceRegistry.create([sf1()]); await expect(registry.get(refDefault1, 'catalog')).resolves.toEqual({ x: 1, }); }); it('should handle duplicate defaultFactories by duplicating the implementations', async () => { - const registry = new ServiceRegistry([]); + const registry = ServiceRegistry.create([]); await expect(registry.get(refDefault2a, 'catalog')).resolves.toEqual({ x: 20, }); @@ -234,7 +234,7 @@ describe('ServiceRegistry', () => { defaultFactory: factoryLoader, }); - const registry = new ServiceRegistry([]); + const registry = ServiceRegistry.create([]); await Promise.all([ expect(registry.get(ref, 'catalog')).resolves.toBeUndefined(), expect(registry.get(ref, 'catalog')).resolves.toBeUndefined(), @@ -252,7 +252,7 @@ describe('ServiceRegistry', () => { factory, }); - const registry = new ServiceRegistry([myFactory()]); + const registry = ServiceRegistry.create([myFactory()]); await Promise.all([ registry.get(ref1, 'catalog')!, @@ -274,7 +274,7 @@ describe('ServiceRegistry', () => { factory, }); - const registry = new ServiceRegistry([myFactory()]); + const registry = ServiceRegistry.create([myFactory()]); await Promise.all([ registry.get(ref1, 'catalog')!, @@ -296,7 +296,7 @@ describe('ServiceRegistry', () => { }, }); - const registry = new ServiceRegistry([myFactory()]); + const registry = ServiceRegistry.create([myFactory()]); await expect(registry.get(ref1, 'catalog')).rejects.toThrow( "Failed to instantiate service '1' for 'catalog' because the following dependent services are missing: '2'", @@ -323,13 +323,217 @@ describe('ServiceRegistry', () => { }, }); - const registry = new ServiceRegistry([factoryA(), factoryB()]); + const registry = ServiceRegistry.create([factoryA(), factoryB()]); await expect(registry.get(refA, 'catalog')).rejects.toThrow( "Failed to instantiate service 'a' for 'catalog' because the factory function threw an error, Error: Failed to instantiate service 'b' for 'catalog' because the following dependent services are missing: 'c', 'd'", ); }); + 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 factoryB = createServiceFactory({ + service: refB, + deps: { a: refA }, + factory: async ({ a }) => a, + }); + + expect(() => ServiceRegistry.create([factoryA(), factoryB()])).toThrow( + `Circular dependencies detected: + 'a' -> 'b' -> '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, + }); + + expect(() => + ServiceRegistry.create([ + factoryA(), + factoryB(), + factoryC(), + factoryD(), + ]), + ).toThrow( + `Circular dependencies detected: + 'a' -> 'b' -> 'a' + 'c' -> 'd' -> 'c'`, + ); + }); + + 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 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: { a: refA }, + factory: async ({ a }) => a, + }); + + expect(() => + ServiceRegistry.create([factoryA(), factoryB(), factoryC()]), + ).toThrow( + `Circular dependencies detected: + 'a' -> 'b' -> 'c' -> 'a'`, + ); + }); + + 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', + }); + + expect(() => + ServiceRegistry.create([ + factoryA(), + factoryB(), + factoryC(), + factoryD(), + ]), + ).toThrow( + `Circular dependencies detected: + '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, + }); + + expect(() => + ServiceRegistry.create([factoryA(), factoryB(), factoryC()]), + ).toThrow( + `Circular dependencies detected: + '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, + }); + + expect(() => + ServiceRegistry.create([factoryA(), factoryB(), factoryC()]), + ).toThrow( + `Circular dependencies detected: + 'b' -> 'c' -> 'b'`, + ); + }); + }); + it('should decorate error messages thrown by the top-level factory function', async () => { const myFactory = createServiceFactory({ service: ref1, @@ -342,7 +546,7 @@ describe('ServiceRegistry', () => { }, }); - const registry = new ServiceRegistry([myFactory()]); + const registry = ServiceRegistry.create([myFactory()]); await expect(registry.get(ref1, 'catalog')).rejects.toThrow( "Failed to instantiate service '1' because createRootContext threw an error, Error: top-level error", @@ -358,7 +562,7 @@ describe('ServiceRegistry', () => { }, }); - const registry = new ServiceRegistry([myFactory()]); + const registry = ServiceRegistry.create([myFactory()]); await expect(registry.get(ref1, 'catalog')).rejects.toThrow( "Failed to instantiate service '1' for 'catalog' because the factory function threw an error, Error: error in plugin", @@ -373,7 +577,7 @@ describe('ServiceRegistry', () => { }, }); - const registry = new ServiceRegistry([]); + const registry = ServiceRegistry.create([]); await expect(registry.get(ref, 'catalog')).rejects.toThrow( "Failed to instantiate service '1' because the default factory loader threw an error, Error: default factory error", diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.ts index b6de05b361..c89916974b 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts @@ -20,11 +20,12 @@ 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 import { InternalServiceFactory } from '@backstage/backend-plugin-api/src/services/system/types'; +import { DependencyGraph } from '../lib/DependencyGraph'; /** * Keep in sync with `@backstage/backend-plugin-api/src/services/system/types.ts` * @internal @@ -57,6 +58,12 @@ const pluginMetadataServiceFactory = createServiceFactory( ); export class ServiceRegistry implements EnumerableServiceHolder { + static create(factories: Array): EnumerableServiceHolder { + const registry = new ServiceRegistry(factories); + registry.checkForCircularDeps(); + return registry; + } + readonly #providedFactories: Map; readonly #loadedDefaultFactories: Map< Function, @@ -73,13 +80,23 @@ export class ServiceRegistry implements EnumerableServiceHolder { InternalServiceFactory, Promise >(); + readonly #dependencyGraph: DependencyGraph; - constructor(factories: Array) { + private constructor(factories: Array) { this.#providedFactories = new Map( factories.map(sf => [sf.service.id, toInternalServiceFactory(sf)]), ); 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( @@ -146,6 +163,20 @@ export class ServiceRegistry implements EnumerableServiceHolder { } } + checkForCircularDeps(): void { + const circularDependencies = Array.from( + this.#dependencyGraph.detectCircularDependencies(), + ); + + if (circularDependencies.length) { + const cycles = circularDependencies + .map(c => c.map(id => `'${id}'`).join(' -> ')) + .join('\n '); + + throw new ConflictError(`Circular dependencies detected:\n ${cycles}`); + } + } + getServiceRefs(): ServiceRef[] { return Array.from(this.#providedFactories.values()).map(f => f.service); }