From 1ba3f525648f8b2d51f2acd17945d91d9d588315 Mon Sep 17 00:00:00 2001 From: Marley Powell Date: Tue, 8 Aug 2023 14:10:19 +0100 Subject: [PATCH 1/9] fix: :bug: implemented a circular dependency check in the `ServiceRegistry` Signed-off-by: Marley Powell --- .../src/wiring/ServiceRegistry.test.ts | 95 +++++++++++++++++++ .../src/wiring/ServiceRegistry.ts | 39 ++++++++ 2 files changed, 134 insertions(+) diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts index e698ade348..0e1d8614f9 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts @@ -330,6 +330,101 @@ describe('ServiceRegistry', () => { ); }); + 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, + }); + + const registry = new ServiceRegistry([factoryA(), factoryB()]); + + await expect(registry.get(refA, 'catalog')).rejects.toThrow( + "Failed to instantiate service 'a' for 'catalog' because of the following circular dependency: 'a' -> 'b' -> 'a'", + ); + }); + + 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, + }); + + 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: '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', + }); + + 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 decorate error messages thrown by the top-level factory function', async () => { const myFactory = createServiceFactory({ service: ref1, diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.ts index b5b55f67cf..de26619816 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts @@ -146,6 +146,43 @@ export class ServiceRegistry implements EnumerableServiceHolder { } } + #checkForCircularDeps(factory: InternalServiceFactory, pluginId: string) { + const head = factory; + + const nodes = Object.values(factory.deps); + + const depChain: Array> = [ + head.service, + ]; + + let node = nodes.shift(); + + if (node) { + depChain.push(node); + } + + while (!!node && node.id !== head.service.id) { + const nodeFactory = this.#providedFactories.get(node.id); + const nodeDeps = nodeFactory?.deps; + if (nodeDeps) { + nodes.unshift(...Object.values(nodeDeps)); + } + node = nodes.shift(); + if (node) { + depChain.push(node); + } + } + + const isCircular = node?.id === head.service.id; + + if (isCircular) { + const circularDepChain = depChain.map(r => `'${r.id}'`).join(' -> '); + throw new Error( + `Failed to instantiate service '${factory.service.id}' for '${pluginId}' because of the following circular dependency: ${circularDepChain}`, + ); + } + } + getServiceRefs(): ServiceRef[] { return Array.from(this.#providedFactories.values()).map(f => f.service); } @@ -156,6 +193,7 @@ 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)) { @@ -179,6 +217,7 @@ 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)) { From b219d097b3f41b9fe33f55661c8440d2e47dfd44 Mon Sep 17 00:00:00 2001 From: Marley Powell Date: Tue, 8 Aug 2023 14:12:48 +0100 Subject: [PATCH 2/9] chore: added changeset Signed-off-by: Marley Powell --- .changeset/tricky-melons-fold.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tricky-melons-fold.md diff --git a/.changeset/tricky-melons-fold.md b/.changeset/tricky-melons-fold.md new file mode 100644 index 0000000000..1a155379bb --- /dev/null +++ b/.changeset/tricky-melons-fold.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': patch +--- + +fix: :bug: implemented a circular dependency check in the `ServiceRegistry` From 1d12a7fa7dac3c3dec660540ced0999c4c05acda Mon Sep 17 00:00:00 2001 From: Marley Powell Date: Thu, 17 Aug 2023 11:59:01 +0100 Subject: [PATCH 3/9] fix: :bug: updated `detectCircularDependency` in `DependencyGraph` to return circular dependencies starting from the first node Signed-off-by: Marley Powell --- .../src/lib/DependencyGraph.test.ts | 13 ++++++++++++- .../backend-app-api/src/lib/DependencyGraph.ts | 14 +++++++------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/packages/backend-app-api/src/lib/DependencyGraph.test.ts b/packages/backend-app-api/src/lib/DependencyGraph.test.ts index 65efac0252..0ac993484b 100644 --- a/packages/backend-app-api/src/lib/DependencyGraph.test.ts +++ b/packages/backend-app-api/src/lib/DependencyGraph.test.ts @@ -66,6 +66,17 @@ describe('DependencyGraph', () => { ).toEqual(['1', '2', '1']); }); + it('should detect a circular dep starting from the first node', async () => { + 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 a larger distant circular dep', async () => { expect( DependencyGraph.fromMap({ @@ -74,7 +85,7 @@ 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']); }); }); diff --git a/packages/backend-app-api/src/lib/DependencyGraph.ts b/packages/backend-app-api/src/lib/DependencyGraph.ts index 8fd679f555..3e7618a096 100644 --- a/packages/backend-app-api/src/lib/DependencyGraph.ts +++ b/packages/backend-app-api/src/lib/DependencyGraph.ts @@ -108,16 +108,16 @@ 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) { + for (const provider of providerNodes) { + if (provider === startNode) { return [...path, startNode.value]; } - if (!visited.has(consumer)) { - stack.push([consumer, [...path, consumer.value]]); + if (!visited.has(provider)) { + stack.push([provider, [...path, provider.value]]); } } } From 51d21f22c903fddc569aed98fa18741ebd5621b8 Mon Sep 17 00:00:00 2001 From: Marley Powell Date: Thu, 17 Aug 2023 12:12:01 +0100 Subject: [PATCH 4/9] refactor: :recycle: updated `ServiceRegistry` to use `DependencyGraph` for circular dependency checks Signed-off-by: Marley Powell --- .../src/wiring/ServiceRegistry.test.ts | 60 +++++++++++++++++++ .../src/wiring/ServiceRegistry.ts | 44 +++++--------- 2 files changed, 75 insertions(+), 29 deletions(-) diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts index d2f36339fa..25fcef3885 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts @@ -425,6 +425,66 @@ describe('ServiceRegistry', () => { ); }); + 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 () => { const myFactory = createServiceFactory({ service: ref1, diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.ts index d9d7120dbd..cdc6f0b711 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts @@ -25,6 +25,7 @@ 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 @@ -147,36 +148,21 @@ export class ServiceRegistry implements EnumerableServiceHolder { } #checkForCircularDeps(factory: InternalServiceFactory, pluginId: string) { - const head = factory; + const tree = DependencyGraph.fromIterable( + Array.from(this.#providedFactories).map( + ([serviceId, serviceFactory]) => ({ + value: { serviceId, serviceFactory }, + provides: [serviceId], + consumes: serviceFactory ? Object.keys(serviceFactory.deps) : [], + }), + ), + ); + const circular = tree.detectCircularDependency(); - const nodes = Object.values(factory.deps); - - const depChain: Array> = [ - head.service, - ]; - - let node = nodes.shift(); - - if (node) { - depChain.push(node); - } - - while (!!node && node.id !== head.service.id) { - const nodeFactory = this.#providedFactories.get(node.id); - const nodeDeps = nodeFactory?.deps; - if (nodeDeps) { - nodes.unshift(...Object.values(nodeDeps)); - } - node = nodes.shift(); - if (node) { - depChain.push(node); - } - } - - const isCircular = node?.id === head.service.id; - - if (isCircular) { - const circularDepChain = depChain.map(r => `'${r.id}'`).join(' -> '); + 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}`, ); From 9238fa3144decf8ae111c1323528e1651ac487d0 Mon Sep 17 00:00:00 2001 From: Marley Powell Date: Thu, 17 Aug 2023 12:16:17 +0100 Subject: [PATCH 5/9] chore: updated changeset Signed-off-by: Marley Powell --- .changeset/tricky-melons-fold.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.changeset/tricky-melons-fold.md b/.changeset/tricky-melons-fold.md index 1a155379bb..69e60d48f0 100644 --- a/.changeset/tricky-melons-fold.md +++ b/.changeset/tricky-melons-fold.md @@ -3,3 +3,4 @@ --- fix: :bug: implemented a circular dependency check in the `ServiceRegistry` +fix: :bug: updated `detectCircularDependency` in `DependencyGraph` to return circular dependencies starting from the first node From bef7098987330785bf453067080c3fafced163a7 Mon Sep 17 00:00:00 2001 From: Marley Powell Date: Fri, 18 Aug 2023 10:30:48 +0100 Subject: [PATCH 6/9] feat: :sparkles: updated `DependencyGraph` to deduplicate the second occurence of a circular cycle Signed-off-by: Marley Powell --- .../src/lib/DependencyGraph.test.ts | 288 +++++++++++++++++- .../src/lib/DependencyGraph.ts | 57 +++- 2 files changed, 328 insertions(+), 17 deletions(-) diff --git a/packages/backend-app-api/src/lib/DependencyGraph.test.ts b/packages/backend-app-api/src/lib/DependencyGraph.test.ts index 0ac993484b..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,18 @@ describe('DependencyGraph', () => { ).toEqual(['1', '2', '1']); }); - it('should detect a circular dep starting from the first node', async () => { + it('should detect a larger distance circular dependency', () => { + expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'] }, + 2: { provides: ['b'], consumes: ['a', 'e'] }, + 3: { provides: ['c'], consumes: ['b'] }, + 4: { provides: ['d', 'e'], consumes: ['c', 'a'] }, + }).detectCircularDependency(), + ).toEqual(['2', '4', '3', '2']); + }); + + it('should detect a circular dependency starting from the first node', () => { expect( DependencyGraph.fromMap({ 1: { provides: ['a'], consumes: ['b'] }, @@ -77,15 +88,266 @@ describe('DependencyGraph', () => { ).toEqual(['1', '2', '3', '4', '1']); }); - it('should detect a larger distant circular dep', async () => { + it('should detect all independent circular dependency cycles', () => { expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'] }, - 2: { provides: ['b'], consumes: ['a', 'e'] }, - 3: { provides: ['c'], consumes: ['b'] }, - 4: { provides: ['d', 'e'], consumes: ['c', 'a'] }, - }).detectCircularDependency(), - ).toEqual(['2', '4', '3', '2']); + 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 3e7618a096..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[]]>([ @@ -114,7 +159,11 @@ export class DependencyGraph { ); for (const provider of providerNodes) { if (provider === startNode) { - return [...path, startNode.value]; + if (cycleKeys.tryAdd(path)) { + yield [...path, startNode.value]; + } + + break; } if (!visited.has(provider)) { stack.push([provider, [...path, provider.value]]); From 8ba316e2870448129f27403ef876f3b91f3e3826 Mon Sep 17 00:00:00 2001 From: Marley Powell Date: Fri, 18 Aug 2023 11:38:22 +0100 Subject: [PATCH 7/9] 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; } /** From 5c29b98d30c0ddf9f8f45320e977f0ad6f6f21e0 Mon Sep 17 00:00:00 2001 From: Marley Powell Date: Thu, 24 Aug 2023 14:31:36 +0100 Subject: [PATCH 8/9] refactor: implemented code review suggestions Signed-off-by: Marley Powell --- .changeset/tricky-melons-fold.md | 7 +- .../src/wiring/BackendInitializer.test.ts | 21 ---- .../src/wiring/BackendInitializer.ts | 4 +- .../src/wiring/ServiceRegistry.test.ts | 104 ++++++++---------- .../src/wiring/ServiceRegistry.ts | 8 +- packages/backend-app-api/src/wiring/types.ts | 1 - 6 files changed, 57 insertions(+), 88 deletions(-) diff --git a/.changeset/tricky-melons-fold.md b/.changeset/tricky-melons-fold.md index 8dbcb9ab5e..bc9d45d539 100644 --- a/.changeset/tricky-melons-fold.md +++ b/.changeset/tricky-melons-fold.md @@ -1,6 +1,7 @@ --- -'@backstage/backend-app-api': patch +'@backstage/backend-app-api': minor --- -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 +refactor!: updated `ServiceRegistry` to have a static create method and private constructor. +fix: updated `ServiceRegistry` to perform circular dependency check on creation. +fix: 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 2aa24fddd3..791b9b1c3a 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -324,25 +324,4 @@ 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 63a55bb0bd..8f652c2c08 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -167,13 +167,11 @@ export class BackendInitializer { } async #doStart(): Promise { - this.#serviceHolder = new ServiceRegistry([ + this.#serviceHolder = ServiceRegistry.create([ ...this.#defaultApiFactories, ...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 fca5933ce5..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,7 +323,7 @@ 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'", @@ -347,9 +347,7 @@ describe('ServiceRegistry', () => { factory: async ({ a }) => a, }); - const registry = new ServiceRegistry([factoryA(), factoryB()]); - - expect(() => registry.checkForCircularDeps()).toThrow( + expect(() => ServiceRegistry.create([factoryA(), factoryB()])).toThrow( `Circular dependencies detected: 'a' -> 'b' -> 'a'`, ); @@ -385,14 +383,14 @@ describe('ServiceRegistry', () => { factory: async ({ c }) => c, }); - const registry = new ServiceRegistry([ - factoryA(), - factoryB(), - factoryC(), - factoryD(), - ]); - - expect(() => registry.checkForCircularDeps()).toThrow( + expect(() => + ServiceRegistry.create([ + factoryA(), + factoryB(), + factoryC(), + factoryD(), + ]), + ).toThrow( `Circular dependencies detected: 'a' -> 'b' -> 'a' 'c' -> 'd' -> 'c'`, @@ -422,13 +420,9 @@ describe('ServiceRegistry', () => { factory: async ({ a }) => a, }); - const registry = new ServiceRegistry([ - factoryA(), - factoryB(), - factoryC(), - ]); - - expect(() => registry.checkForCircularDeps()).toThrow( + expect(() => + ServiceRegistry.create([factoryA(), factoryB(), factoryC()]), + ).toThrow( `Circular dependencies detected: 'a' -> 'b' -> 'c' -> 'a'`, ); @@ -464,14 +458,14 @@ describe('ServiceRegistry', () => { factory: async () => 'd', }); - const registry = new ServiceRegistry([ - factoryA(), - factoryB(), - factoryC(), - factoryD(), - ]); - - expect(() => registry.checkForCircularDeps()).toThrow( + expect(() => + ServiceRegistry.create([ + factoryA(), + factoryB(), + factoryC(), + factoryD(), + ]), + ).toThrow( `Circular dependencies detected: 'a' -> 'b' -> 'c' -> 'a'`, ); @@ -500,13 +494,9 @@ describe('ServiceRegistry', () => { factory: async ({ a }) => a, }); - const registry = new ServiceRegistry([ - factoryA(), - factoryB(), - factoryC(), - ]); - - expect(() => registry.checkForCircularDeps()).toThrow( + expect(() => + ServiceRegistry.create([factoryA(), factoryB(), factoryC()]), + ).toThrow( `Circular dependencies detected: 'a' -> 'c' -> 'a'`, ); @@ -535,13 +525,9 @@ describe('ServiceRegistry', () => { factory: async ({ b }) => b, }); - const registry = new ServiceRegistry([ - factoryA(), - factoryB(), - factoryC(), - ]); - - expect(() => registry.checkForCircularDeps()).toThrow( + expect(() => + ServiceRegistry.create([factoryA(), factoryB(), factoryC()]), + ).toThrow( `Circular dependencies detected: 'b' -> 'c' -> 'b'`, ); @@ -560,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", @@ -576,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", @@ -591,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 2c850175c9..c89916974b 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts @@ -58,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, @@ -76,7 +82,7 @@ export class ServiceRegistry implements EnumerableServiceHolder { >(); readonly #dependencyGraph: DependencyGraph; - constructor(factories: Array) { + private constructor(factories: Array) { this.#providedFactories = new Map( factories.map(sf => [sf.service.id, toInternalServiceFactory(sf)]), ); diff --git a/packages/backend-app-api/src/wiring/types.ts b/packages/backend-app-api/src/wiring/types.ts index 07974c182e..552a69a792 100644 --- a/packages/backend-app-api/src/wiring/types.ts +++ b/packages/backend-app-api/src/wiring/types.ts @@ -39,7 +39,6 @@ export interface CreateSpecializedBackendOptions { export interface ServiceHolder { get(api: ServiceRef, pluginId: string): Promise | undefined; - checkForCircularDeps(): void; } /** From e9292f564baf9dcfc740ed72c9a401d218b933b0 Mon Sep 17 00:00:00 2001 From: Marley <55280588+marleypowell@users.noreply.github.com> Date: Wed, 30 Aug 2023 11:47:26 +0100 Subject: [PATCH 9/9] Update .changeset/tricky-melons-fold.md Co-authored-by: Patrik Oldsberg Signed-off-by: Marley <55280588+marleypowell@users.noreply.github.com> --- .changeset/tricky-melons-fold.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.changeset/tricky-melons-fold.md b/.changeset/tricky-melons-fold.md index bc9d45d539..036413ea8e 100644 --- a/.changeset/tricky-melons-fold.md +++ b/.changeset/tricky-melons-fold.md @@ -1,7 +1,5 @@ --- -'@backstage/backend-app-api': minor +'@backstage/backend-app-api': patch --- -refactor!: updated `ServiceRegistry` to have a static create method and private constructor. -fix: updated `ServiceRegistry` to perform circular dependency check on creation. -fix: updated `detectCircularDependency` in `DependencyGraph` to return circular dependencies starting from the first node +Backend startup will now fail if any circular service dependencies are detected.