refactor: ♻️ updated ServiceRegistry to use DependencyGraph for circular dependency checks

Signed-off-by: Marley Powell <marley.powell@exclaimer.com>

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<string>({ id: 'a' });
+    const refB = createServiceRef<string>({ id: 'b' });
+    const refC = createServiceRef<string>({ 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<string>({ id: 'a' });
+    const refB = createServiceRef<string>({ id: 'b' });
+    const refC = createServiceRef<string>({ 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,

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<ServiceRef<unknown, 'plugin' | 'root'>> = [
-      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}`,
       );