From c7aa4ff1793cd529d9da954f251eac6df38307ca Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 10 Aug 2023 18:01:45 +0200 Subject: [PATCH 01/16] backend-plugin-api: allow modules to register extension points Signed-off-by: Patrik Oldsberg --- .changeset/gorgeous-dragons-peel.md | 6 ++++++ .../src/wiring/BackendInitializer.ts | 2 +- packages/backend-plugin-api/api-report.md | 5 +++++ .../src/wiring/factories.test.ts | 1 + .../backend-plugin-api/src/wiring/factories.ts | 16 ++++++++++++++-- packages/backend-plugin-api/src/wiring/types.ts | 5 +++++ 6 files changed, 32 insertions(+), 3 deletions(-) create mode 100644 .changeset/gorgeous-dragons-peel.md diff --git a/.changeset/gorgeous-dragons-peel.md b/.changeset/gorgeous-dragons-peel.md new file mode 100644 index 0000000000..d067ca0ae7 --- /dev/null +++ b/.changeset/gorgeous-dragons-peel.md @@ -0,0 +1,6 @@ +--- +'@backstage/backend-plugin-api': patch +'@backstage/backend-app-api': patch +--- + +Allow modules to register extension points. diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 6bc4e995e2..3f1c73928b 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -161,7 +161,7 @@ export class BackendInitializer { for (const r of feature.getRegistrations()) { const provides = new Set>(); - if (r.type === 'plugin') { + if (r.type === 'plugin' || r.type === 'module') { for (const [extRef, extImpl] of r.extensionPoints) { if (this.#extensionPoints.has(extRef)) { throw new Error( diff --git a/packages/backend-plugin-api/api-report.md b/packages/backend-plugin-api/api-report.md index a1ae6284c6..ed2eb819d7 100644 --- a/packages/backend-plugin-api/api-report.md +++ b/packages/backend-plugin-api/api-report.md @@ -31,6 +31,11 @@ export interface BackendModuleConfig { // @public export interface BackendModuleRegistrationPoints { + // (undocumented) + registerExtensionPoint( + ref: ExtensionPoint, + impl: TExtensionPoint, + ): void; // (undocumented) registerInit< Deps extends { diff --git a/packages/backend-plugin-api/src/wiring/factories.test.ts b/packages/backend-plugin-api/src/wiring/factories.test.ts index ae199d84e4..17b0eed55f 100644 --- a/packages/backend-plugin-api/src/wiring/factories.test.ts +++ b/packages/backend-plugin-api/src/wiring/factories.test.ts @@ -149,6 +149,7 @@ describe('createBackendModule', () => { type: 'module', pluginId: 'x', moduleId: 'y', + extensionPoints: [], init: { deps: expect.any(Object), func: expect.any(Function), diff --git a/packages/backend-plugin-api/src/wiring/factories.ts b/packages/backend-plugin-api/src/wiring/factories.ts index ecaccdc6de..c2ff1f84b9 100644 --- a/packages/backend-plugin-api/src/wiring/factories.ts +++ b/packages/backend-plugin-api/src/wiring/factories.ts @@ -159,13 +159,14 @@ export function createBackendPlugin( */ export interface BackendModuleConfig { /** - * The ID of this plugin. + * Should exactly match the `id` of the plugin that the module extends. * * @see {@link https://backstage.io/docs/backend-system/architecture/naming-patterns | Recommended naming patterns} */ pluginId: string; + /** - * Should exactly match the `id` of the plugin that the module extends. + * The ID of this module, used to identify the module and ensure that it is not installed twice. */ moduleId: string; register(reg: BackendModuleRegistrationPoints): void; @@ -194,10 +195,20 @@ export function createBackendModule( if (registrations) { return registrations; } + const extensionPoints: InternalBackendPluginRegistration['extensionPoints'] = + []; let init: InternalBackendModuleRegistration['init'] | undefined = undefined; c.register({ + registerExtensionPoint(ext, impl) { + if (init) { + throw new Error( + 'registerExtensionPoint called after registerInit', + ); + } + extensionPoints.push([ext, impl]); + }, registerInit(regInit) { if (init) { throw new Error('registerInit must only be called once'); @@ -220,6 +231,7 @@ export function createBackendModule( type: 'module', pluginId: c.pluginId, moduleId: c.moduleId, + extensionPoints, init, }, ]; diff --git a/packages/backend-plugin-api/src/wiring/types.ts b/packages/backend-plugin-api/src/wiring/types.ts index 028c2b87da..e2ed6ce387 100644 --- a/packages/backend-plugin-api/src/wiring/types.ts +++ b/packages/backend-plugin-api/src/wiring/types.ts @@ -59,6 +59,10 @@ export interface BackendPluginRegistrationPoints { * @public */ export interface BackendModuleRegistrationPoints { + registerExtensionPoint( + ref: ExtensionPoint, + impl: TExtensionPoint, + ): void; registerInit(options: { deps: { [name in keyof Deps]: ServiceRef | ExtensionPoint; @@ -105,6 +109,7 @@ export interface InternalBackendModuleRegistration { pluginId: string; moduleId: string; type: 'module'; + extensionPoints: Array, unknown]>; init: { deps: Record | ExtensionPoint>; func(deps: Record): Promise; From 8b85f546f7560c1f37bc03013bef16a03635ca0f Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 10 Aug 2023 19:10:13 +0200 Subject: [PATCH 02/16] backend-app-api: make sure modules are initialized in the correct order Signed-off-by: Patrik Oldsberg --- .../src/wiring/BackendInitializer.test.ts | 48 ++++++++++- .../src/wiring/BackendInitializer.ts | 82 +++++++++++++++---- 2 files changed, 115 insertions(+), 15 deletions(-) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index b05194f594..555ce63f15 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -21,7 +21,7 @@ import { createBackendPlugin, createBackendModule, } from '@backstage/backend-plugin-api'; -import { BackendInitializer } from './BackendInitializer'; +import { BackendInitializer, resolveInitChunks } from './BackendInitializer'; import { ServiceRegistry } from './ServiceRegistry'; import { rootLifecycleServiceFactory } from '../services/implementations'; @@ -176,3 +176,49 @@ describe('BackendInitializer', () => { ); }); }); + +describe('resolveInitChunks', () => { + it('should resolve flat chunks', () => { + const resolved = resolveInitChunks( + new Map( + Object.entries({ + a: { consumes: new Set(), provides: new Set(['a']) } as any, + b: { consumes: new Set(), provides: new Set(['b']) } as any, + c: { consumes: new Set(), provides: new Set(['c']) } as any, + d: { consumes: new Set(), provides: new Set(['d']) } as any, + }), + ), + ).map(chunk => Array.from(chunk.keys())); + expect(resolved).toEqual([['a', 'b', 'c', 'd']]); + }); + + it('should resolve nested chunks', () => { + const resolved = resolveInitChunks( + new Map( + Object.entries({ + a: { consumes: new Set(), provides: new Set(['a']) } as any, + b: { consumes: new Set(['a']), provides: new Set(['b', 'c']) } as any, + c: { consumes: new Set(['b']), provides: new Set() } as any, + d: { consumes: new Set(['c']), provides: new Set() } as any, + }), + ), + ).map(chunk => Array.from(chunk.keys())); + expect(resolved).toEqual([['a'], ['b'], ['c', 'd']]); + }); + + it('should detect circular dependencies', () => { + expect(() => + resolveInitChunks( + new Map( + Object.entries({ + a: { consumes: new Set(), provides: new Set(['a']) } as any, + b: { consumes: new Set(['a', 'b']), provides: new Set() } as any, + c: { consumes: new Set(['a', 'c']), provides: new Set() } as any, + }), + ), + ), + ).toThrow( + "Failed to resolve module initialization order, the following modules have circular dependencies, 'b', 'c'", + ); + }); +}); diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 3f1c73928b..19b933d96a 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -38,6 +38,58 @@ export interface BackendRegisterInit { }; } +/** @internal */ +export function resolveInitChunks( + registerInits: Map, +): Map[] { + let registerInitsToOrder = Array.from(registerInits); + const orderedRegisterInits = new Array>(); + + // Keep track of all deps that have been provided so far + const provided = new Set(); + + // Loop until we have ordered all registerInits + while (registerInitsToOrder.length > 0) { + const chunk = new Map(); + + // Find all registerInits that have all their deps provided + for (const [id, registerInit] of registerInitsToOrder) { + if ( + Array.from(registerInit.consumes).every(consumed => + provided.has(consumed), + ) + ) { + chunk.set(id, registerInit); + } + } + + // Add to the result set + orderedRegisterInits.push(chunk); + + // Register all the deps in the current chunk as provided + chunk.forEach(r => r.provides.forEach(p => provided.add(p))); + + // Remove the current chunk from the registerInits to order + const newRegisterInitsToOrder = registerInitsToOrder.filter( + ([id]) => !chunk.has(id), + ); + // Check that we have not hit a circular dependency + if ( + registerInitsToOrder.length !== 0 && + registerInitsToOrder.length === newRegisterInitsToOrder.length + ) { + throw new Error( + `Failed to resolve module initialization order, the following modules have circular dependencies, '${registerInitsToOrder + .map(r => r[0]) + .join("', '")}'`, + ); + } + registerInitsToOrder = newRegisterInitsToOrder; + } + + return orderedRegisterInits; +} + export class BackendInitializer { #startPromise?: Promise; #features = new Array(); @@ -210,21 +262,23 @@ export class BackendInitializer { await Promise.all( allPluginIds.map(async pluginId => { // Modules are initialized before plugins, so that they can provide extension to the plugin - const modules = moduleInits.get(pluginId) ?? []; - await Promise.all( - Array.from(modules).map(async ([moduleId, moduleInit]) => { - const moduleDeps = await this.#getInitDeps( - moduleInit.init.deps, - pluginId, - ); - await moduleInit.init.func(moduleDeps).catch(error => { - throw new ForwardedError( - `Module '${moduleId}' for plugin '${pluginId}' startup failed`, - error, + const modules = moduleInits.get(pluginId) ?? new Map(); + for (const chunk of resolveInitChunks(modules)) { + await Promise.all( + Array.from(chunk).map(async ([moduleId, moduleInit]) => { + const moduleDeps = await this.#getInitDeps( + moduleInit.init.deps, + pluginId, ); - }); - }), - ); + await moduleInit.init.func(moduleDeps).catch(error => { + throw new ForwardedError( + `Module '${moduleId}' for plugin '${pluginId}' startup failed`, + error, + ); + }); + }), + ); + } // Once all modules have been initialized, we can initialize the plugin itself const pluginInit = pluginInits.get(pluginId); From e2b6396a127488342d9b63743e40e29352e94d92 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 10 Aug 2023 19:11:39 +0200 Subject: [PATCH 03/16] catalog-backend-module-incremental-ingestion: move providers to extension point instead of options Signed-off-by: Patrik Oldsberg --- .changeset/perfect-trainers-invite.md | 5 + .../alpha-api-report.md | 17 ++- ...IncrementalIngestionEntityProvider.test.ts | 40 ++++-- ...oduleIncrementalIngestionEntityProvider.ts | 135 ++++++++++++------ .../src/module/index.ts | 6 +- .../src/run.ts | 36 +++-- 6 files changed, 168 insertions(+), 71 deletions(-) create mode 100644 .changeset/perfect-trainers-invite.md diff --git a/.changeset/perfect-trainers-invite.md b/.changeset/perfect-trainers-invite.md new file mode 100644 index 0000000000..6b543005cd --- /dev/null +++ b/.changeset/perfect-trainers-invite.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-incremental-ingestion': patch +--- + +Export new alpha `incrementalIngestionProvidersExtensionPoint` for registering incremental providers, rather than the providers being passed as options to the backend module. diff --git a/plugins/catalog-backend-module-incremental-ingestion/alpha-api-report.md b/plugins/catalog-backend-module-incremental-ingestion/alpha-api-report.md index b11ec43cf5..995c6e4893 100644 --- a/plugins/catalog-backend-module-incremental-ingestion/alpha-api-report.md +++ b/plugins/catalog-backend-module-incremental-ingestion/alpha-api-report.md @@ -4,16 +4,23 @@ ```ts import { BackendFeature } from '@backstage/backend-plugin-api'; +import { ExtensionPoint } from '@backstage/backend-plugin-api'; import { IncrementalEntityProvider } from '@backstage/plugin-catalog-backend-module-incremental-ingestion'; import { IncrementalEntityProviderOptions } from '@backstage/plugin-catalog-backend-module-incremental-ingestion'; // @alpha -export const catalogModuleIncrementalIngestionEntityProvider: (options: { - providers: { - provider: IncrementalEntityProvider; +export const catalogModuleIncrementalIngestionEntityProvider: () => BackendFeature; + +// @alpha +export interface IncrementalIngestionProviderExtensionPoint { + addProvider(config: { options: IncrementalEntityProviderOptions; - }[]; -}) => BackendFeature; + provider: IncrementalEntityProvider; + }): void; +} + +// @alpha +export const incrementalIngestionProvidersExtensionPoint: ExtensionPoint; // (No @packageDocumentation comment for this package) ``` diff --git a/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.test.ts b/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.test.ts index ae480048d1..7c3dfdf226 100644 --- a/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.test.ts +++ b/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.test.ts @@ -15,12 +15,18 @@ */ import { getVoidLogger } from '@backstage/backend-common'; -import { coreServices } from '@backstage/backend-plugin-api'; +import { + coreServices, + createBackendModule, +} from '@backstage/backend-plugin-api'; import { startTestBackend } from '@backstage/backend-test-utils'; import { ConfigReader } from '@backstage/config'; import { catalogProcessingExtensionPoint } from '@backstage/plugin-catalog-node/alpha'; import { IncrementalEntityProvider } from '../types'; -import { catalogModuleIncrementalIngestionEntityProvider } from './catalogModuleIncrementalIngestionEntityProvider'; +import { + catalogModuleIncrementalIngestionEntityProvider, + incrementalIngestionProvidersExtensionPoint, +} from './catalogModuleIncrementalIngestionEntityProvider'; describe('catalogModuleIncrementalIngestionEntityProvider', () => { it('should register provider at the catalog extension point', async () => { @@ -57,18 +63,26 @@ describe('catalogModuleIncrementalIngestionEntityProvider', () => { [coreServices.scheduler, scheduler], ], features: [ - catalogModuleIncrementalIngestionEntityProvider({ - providers: [ - { - provider: provider1, - options: { - burstInterval: { seconds: 1 }, - burstLength: { seconds: 1 }, - restLength: { seconds: 1 }, + catalogModuleIncrementalIngestionEntityProvider(), + createBackendModule({ + pluginId: 'catalog', + moduleId: 'incrementalTest', + register(env) { + env.registerInit({ + deps: { extension: incrementalIngestionProvidersExtensionPoint }, + async init({ extension }) { + extension.addProvider({ + provider: provider1, + options: { + burstInterval: { seconds: 1 }, + burstLength: { seconds: 1 }, + restLength: { seconds: 1 }, + }, + }); }, - }, - ], - }), + }); + }, + })(), ], }); diff --git a/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.ts b/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.ts index 78d28d7b07..81419d66ec 100644 --- a/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.ts +++ b/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.ts @@ -17,6 +17,7 @@ import { coreServices, createBackendModule, + createExtensionPoint, } from '@backstage/backend-plugin-api'; import { catalogProcessingExtensionPoint } from '@backstage/plugin-catalog-node/alpha'; import { @@ -25,56 +26,110 @@ import { } from '@backstage/plugin-catalog-backend-module-incremental-ingestion'; import { WrapperProviders } from './WrapperProviders'; +/** + * @alpha + * Interface for {@link incrementalIngestionProvidersExtensionPoint}. + */ +export interface IncrementalIngestionProviderExtensionPoint { + /** Adds a new incremental entity provider */ + addProvider(config: { + options: IncrementalEntityProviderOptions; + provider: IncrementalEntityProvider; + }): void; +} + +/** + * @alpha + * + * Extension point for registering incremental ingestion providers. + * The `catalogModuleIncrementalIngestionEntityProvider` must be installed for these providers to work. + * + * @example + * + * ```ts + * backend.add(createBackendModule({ + * pluginId: 'catalog', + * register(env) { + * env.registerInit({ + * deps: { + * extension: incrementalIngestionProvidersExtensionPoint, + * }, + * async init({ extension }) { + * extension.addProvider({ + * burstInterval: ..., + * burstLength: ..., + * restLength: ..., + * }, { + * next(context, cursor) { + * ... + * }, + * ... + * }) + * }) + * }) + * } + * })) + * ``` + */ +export const incrementalIngestionProvidersExtensionPoint = + createExtensionPoint({ + id: 'catalog.incrementalIngestionProvider.providers', + }); + /** * Registers the incremental entity provider with the catalog processing extension point. * * @alpha */ export const catalogModuleIncrementalIngestionEntityProvider = - createBackendModule( - (options: { - providers: Array<{ + createBackendModule({ + pluginId: 'catalog', + moduleId: 'incrementalIngestionEntityProvider', + register(env) { + const addedProviders = new Array<{ provider: IncrementalEntityProvider; options: IncrementalEntityProviderOptions; - }>; - }) => ({ - pluginId: 'catalog', - moduleId: 'incrementalIngestionEntityProvider', - register(env) { - env.registerInit({ - deps: { - catalog: catalogProcessingExtensionPoint, - config: coreServices.rootConfig, - database: coreServices.database, - httpRouter: coreServices.httpRouter, - logger: coreServices.logger, - scheduler: coreServices.scheduler, - }, - async init({ - catalog, + }>(); + + env.registerExtensionPoint(incrementalIngestionProvidersExtensionPoint, { + addProvider({ options, provider }) { + addedProviders.push({ options, provider }); + }, + }); + + env.registerInit({ + deps: { + catalog: catalogProcessingExtensionPoint, + config: coreServices.rootConfig, + database: coreServices.database, + httpRouter: coreServices.httpRouter, + logger: coreServices.logger, + scheduler: coreServices.scheduler, + }, + async init({ + catalog, + config, + database, + httpRouter, + logger, + scheduler, + }) { + const client = await database.getClient(); + + const providers = new WrapperProviders({ config, - database, - httpRouter, logger, + client, scheduler, - }) { - const client = await database.getClient(); + }); - const providers = new WrapperProviders({ - config, - logger, - client, - scheduler, - }); + for (const entry of addedProviders) { + const wrapped = providers.wrap(entry.provider, entry.options); + catalog.addEntityProvider(wrapped); + } - for (const entry of options.providers) { - const wrapped = providers.wrap(entry.provider, entry.options); - catalog.addEntityProvider(wrapped); - } - - httpRouter.use(await providers.adminRouter()); - }, - }); - }, - }), - ); + httpRouter.use(await providers.adminRouter()); + }, + }); + }, + }); diff --git a/plugins/catalog-backend-module-incremental-ingestion/src/module/index.ts b/plugins/catalog-backend-module-incremental-ingestion/src/module/index.ts index 9fcee99e01..37ecf60f7c 100644 --- a/plugins/catalog-backend-module-incremental-ingestion/src/module/index.ts +++ b/plugins/catalog-backend-module-incremental-ingestion/src/module/index.ts @@ -14,4 +14,8 @@ * limitations under the License. */ -export { catalogModuleIncrementalIngestionEntityProvider } from './catalogModuleIncrementalIngestionEntityProvider'; +export { + catalogModuleIncrementalIngestionEntityProvider, + incrementalIngestionProvidersExtensionPoint, + type IncrementalIngestionProviderExtensionPoint, +} from './catalogModuleIncrementalIngestionEntityProvider'; diff --git a/plugins/catalog-backend-module-incremental-ingestion/src/run.ts b/plugins/catalog-backend-module-incremental-ingestion/src/run.ts index e7e7a900b9..be1a6dcfc0 100644 --- a/plugins/catalog-backend-module-incremental-ingestion/src/run.ts +++ b/plugins/catalog-backend-module-incremental-ingestion/src/run.ts @@ -20,12 +20,16 @@ import { createBackend } from '@backstage/backend-defaults'; import { coreServices, + createBackendModule, createServiceFactory, } from '@backstage/backend-plugin-api'; import { ConfigReader } from '@backstage/config'; import { catalogPlugin } from '@backstage/plugin-catalog-backend/alpha'; import { IncrementalEntityProvider } from '.'; -import { catalogModuleIncrementalIngestionEntityProvider } from './alpha'; +import { + catalogModuleIncrementalIngestionEntityProvider, + incrementalIngestionProvidersExtensionPoint, +} from './alpha'; const provider: IncrementalEntityProvider = { getProviderName: () => 'test-provider', @@ -62,19 +66,27 @@ async function main() { }); backend.add(catalogPlugin()); + backend.add(catalogModuleIncrementalIngestionEntityProvider()); backend.add( - catalogModuleIncrementalIngestionEntityProvider({ - providers: [ - { - provider: provider, - options: { - burstInterval: { seconds: 1 }, - burstLength: { seconds: 10 }, - restLength: { seconds: 10 }, + createBackendModule({ + pluginId: 'catalog', + moduleId: 'incrementalTestProvider', + register(reg) { + reg.registerInit({ + deps: { extension: incrementalIngestionProvidersExtensionPoint }, + async init({ extension }) { + extension.addProvider({ + provider: provider, + options: { + burstInterval: { seconds: 1 }, + burstLength: { seconds: 10 }, + restLength: { seconds: 10 }, + }, + }); }, - }, - ], - }), + }); + }, + })(), ); await backend.start(); From 983a2388e715a9805c2a912945ccd33b52a9faf2 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 11 Aug 2023 09:56:22 +0200 Subject: [PATCH 04/16] backend-app-api: initial dependency tree helper Signed-off-by: Patrik Oldsberg --- .../src/lib/DependencyTree.test.ts | 124 ++++++++++++++++ .../backend-app-api/src/lib/DependencyTree.ts | 139 ++++++++++++++++++ 2 files changed, 263 insertions(+) create mode 100644 packages/backend-app-api/src/lib/DependencyTree.test.ts create mode 100644 packages/backend-app-api/src/lib/DependencyTree.ts diff --git a/packages/backend-app-api/src/lib/DependencyTree.test.ts b/packages/backend-app-api/src/lib/DependencyTree.test.ts new file mode 100644 index 0000000000..5364dd4257 --- /dev/null +++ b/packages/backend-app-api/src/lib/DependencyTree.test.ts @@ -0,0 +1,124 @@ +/* + * Copyright 2023 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { DependencyTree } from './DependencyTree'; + +describe('DependencyTree', () => { + it('should be empty', () => { + const empty = DependencyTree.fromMap({}); + expect(Array.from(empty.nodes)).toEqual([]); + expect(empty.findUnsatisfiedDeps()).toEqual([]); + expect(empty.detectCircularDependency()).toBeUndefined(); + }); + + it('should reject multiple producers', () => { + expect(() => + DependencyTree.fromMap({ + 1: { produces: ['a'] }, + 2: { produces: ['a'] }, + }), + ).toThrow( + "Dependency conflict detected, 'a' may not be produced by both '1' and '2'", + ); + }); + + it('should detect circular dependencies', () => { + expect( + DependencyTree.fromMap({ + 1: {}, + 2: {}, + 3: {}, + 4: {}, + }).detectCircularDependency(), + ).toBeUndefined(); + + expect( + DependencyTree.fromMap({ + 1: { produces: ['a'] }, + 2: { consumes: ['a'], produces: ['b', 'c'] }, + 3: { consumes: ['b'] }, + 4: { consumes: ['c'] }, + }).detectCircularDependency(), + ).toBeUndefined(); + + expect( + DependencyTree.fromMap({ + 1: { produces: ['a'], consumes: ['a'] }, + }).detectCircularDependency(), + ).toEqual(['1', '1']); + + expect( + DependencyTree.fromMap({ + 1: { produces: ['a'], consumes: ['b'] }, + 2: { produces: ['b'], consumes: ['a'] }, + }).detectCircularDependency(), + ).toEqual(['1', '2', '1']); + + expect( + DependencyTree.fromMap({ + 1: { produces: ['a'] }, + 2: { produces: ['b'], consumes: ['a', 'e'] }, + 3: { produces: ['c'], consumes: ['b'] }, + 4: { produces: ['d', 'e'], consumes: ['c', 'a'] }, + }).detectCircularDependency(), + ).toEqual(['2', '3', '4', '2']); + }); + + it('should find unsatisfied dependencies', () => { + expect( + DependencyTree.fromMap({ + 1: {}, + 2: {}, + 3: {}, + 4: {}, + }).findUnsatisfiedDeps(), + ).toEqual([]); + + expect( + DependencyTree.fromMap({ + 1: { produces: ['a'] }, + 2: { consumes: ['a'], produces: ['b', 'c'] }, + 3: { consumes: ['b'] }, + 4: { consumes: ['c'] }, + }).findUnsatisfiedDeps(), + ).toEqual([]); + + expect( + DependencyTree.fromMap({ + 1: { consumes: ['a'] }, + }).findUnsatisfiedDeps(), + ).toEqual([{ id: '1', unsatisfied: ['a'] }]); + + expect( + DependencyTree.fromMap({ + 1: { produces: ['a'], consumes: ['b'] }, + 2: { produces: ['b'], consumes: ['a', 'd', 'e'] }, + }).findUnsatisfiedDeps(), + ).toEqual([{ id: '2', unsatisfied: ['d', 'e'] }]); + + expect( + DependencyTree.fromMap({ + 1: { produces: ['a'] }, + 2: { produces: ['b'], consumes: ['a', 'd', 'e'] }, + 3: { produces: [], consumes: ['b'] }, + 4: { produces: [], consumes: ['c', 'a'] }, + }).findUnsatisfiedDeps(), + ).toEqual([ + { id: '2', unsatisfied: ['d', 'e'] }, + { id: '4', unsatisfied: ['c'] }, + ]); + }); +}); diff --git a/packages/backend-app-api/src/lib/DependencyTree.ts b/packages/backend-app-api/src/lib/DependencyTree.ts new file mode 100644 index 0000000000..80cf10ecb2 --- /dev/null +++ b/packages/backend-app-api/src/lib/DependencyTree.ts @@ -0,0 +1,139 @@ +/* + * Copyright 2023 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ConflictError, InputError } from '@backstage/errors'; + +interface NodeInput { + id: string; + consumes?: Iterable; + produces?: Iterable; +} + +/** @internal */ +class Node { + static from(input: NodeInput) { + return new Node( + input.id, + input.consumes ? new Set(input.consumes) : new Set(), + input.produces ? new Set(input.produces) : new Set(), + ); + } + + private constructor( + readonly id: string, + readonly consumes: Set, + readonly produces: Set, + ) {} +} + +/** @internal */ +export class DependencyTree { + static fromMap(nodes: Record>) { + return this.fromIterable( + Object.entries(nodes).map(([id, node]) => ({ id, ...node })), + ); + } + + static fromIterable(nodeInputs: Iterable) { + const nodes = new Map(); + for (const nodeInput of nodeInputs) { + const node = Node.from(nodeInput); + if (nodes.has(node.id)) { + throw new InputError(`Duplicate node with id ${node.id}`); + } + nodes.set(node.id, node); + } + + return new DependencyTree(nodes); + } + + #allProduced: Set; + #allConsumed: Set; + #producedBy: Map; + #consumedBy: Map>; + + private constructor(readonly nodes: Map) { + this.#allProduced = new Set(); + this.#allConsumed = new Set(); + this.#producedBy = new Map(); + this.#consumedBy = new Map(); + + for (const node of this.nodes.values()) { + for (const produced of node.produces) { + this.#allProduced.add(produced); + if (this.#producedBy.has(produced)) { + throw new ConflictError( + `Dependency conflict detected, '${produced}' may not be produced by both '${this.#producedBy.get( + produced, + )}' and '${node.id}'`, + ); + } + this.#producedBy.set(produced, node.id); + } + for (const consumed of node.consumes) { + this.#allConsumed.add(consumed); + if (!this.#consumedBy.get(consumed)?.add(node.id)) { + this.#consumedBy.set(consumed, new Set([node.id])); + } + } + } + } + + findUnsatisfiedDeps(): Array<{ id: string; unsatisfied: string[] }> { + const unsatisfiedDependencies = []; + for (const node of this.nodes.values()) { + const unsatisfied = Array.from(node.consumes).filter( + id => !this.#allProduced.has(id), + ); + if (unsatisfied.length > 0) { + unsatisfiedDependencies.push({ id: node.id, unsatisfied }); + } + } + return unsatisfiedDependencies; + } + + detectCircularDependency(): string[] | undefined { + for (const nodeId of this.nodes.keys()) { + const visited = new Set(); + const stack = new Array<[id: string, path: string[]]>([nodeId, [nodeId]]); + + while (stack.length > 0) { + const [id, path] = stack.pop()!; + if (visited.has(id)) { + continue; + } + visited.add(id); + const node = this.nodes.get(id); + if (node) { + for (const produced of node.produces) { + const consumers = this.#consumedBy.get(produced); + if (consumers) { + for (const consumer of consumers) { + if (consumer === nodeId) { + return [...path, nodeId]; + } + if (!visited.has(consumer)) { + stack.push([consumer, [...path, consumer]]); + } + } + } + } + } + } + } + return undefined; + } +} From b2fe28e483bdd4a114882304a43cc204341561de Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 11 Aug 2023 13:07:41 +0200 Subject: [PATCH 05/16] backend-app-api: implement topological traversal for dependency tree Signed-off-by: Patrik Oldsberg --- .../src/lib/DependencyTree.test.ts | 75 +++++++++++++++++++ .../backend-app-api/src/lib/DependencyTree.ts | 62 ++++++++++++++- 2 files changed, 136 insertions(+), 1 deletion(-) diff --git a/packages/backend-app-api/src/lib/DependencyTree.test.ts b/packages/backend-app-api/src/lib/DependencyTree.test.ts index 5364dd4257..d4cf8a863e 100644 --- a/packages/backend-app-api/src/lib/DependencyTree.test.ts +++ b/packages/backend-app-api/src/lib/DependencyTree.test.ts @@ -121,4 +121,79 @@ describe('DependencyTree', () => { { id: '4', unsatisfied: ['c'] }, ]); }); + + it('should traverse dependencies in topological order', async () => { + await expect( + DependencyTree.fromMap({ + 1: {}, + 2: {}, + 3: {}, + 4: {}, + }).parallelTopologicalTraversal(async id => id), + ).resolves.toEqual(['1', '2', '3', '4']); + + await expect( + DependencyTree.fromMap({ + 1: { produces: ['a'] }, + 2: { consumes: ['a'], produces: ['b', 'c'] }, + 3: { consumes: ['b'] }, + 4: { consumes: ['c'] }, + }).parallelTopologicalTraversal(async id => id), + ).resolves.toEqual(['1', '2', '3', '4']); + + await expect( + DependencyTree.fromMap({ + 1: { consumes: ['c'] }, + 2: { produces: ['c'], consumes: ['b'] }, + 3: { produces: ['b'], consumes: ['a'] }, + 4: { produces: ['a'] }, + }).parallelTopologicalTraversal(async id => id), + ).resolves.toEqual(['4', '3', '2', '1']); + + await expect( + DependencyTree.fromMap({ + 1: { produces: ['a'] }, + 2: { produces: ['b'], consumes: ['a'] }, + 3: { produces: ['c'], consumes: ['a'] }, + 4: { consumes: ['b'] }, + 5: { consumes: ['c'] }, + }).parallelTopologicalTraversal(async id => id), + ).resolves.toEqual(['1', '2', '3', '4', '5']); + + // Same as above, but with 2 being delayed + await expect( + DependencyTree.fromMap({ + 1: { produces: ['a'] }, + 2: { produces: ['b'], consumes: ['a'] }, + 3: { produces: ['c'], consumes: ['a'] }, + 4: { consumes: ['b'] }, + 5: { consumes: ['c'] }, + }).parallelTopologicalTraversal(async id => { + // When delaying 2 we expect 3 and 5 to complete before 2 and 4 + if (id === '2') { + await new Promise(resolve => setTimeout(resolve, 100)); + } + return id; + }), + ).resolves.toEqual(['1', '3', '5', '2', '4']); + + await expect( + DependencyTree.fromMap({ + 1: { produces: ['a'], consumes: ['a'] }, + }).parallelTopologicalTraversal(async id => id), + ).rejects.toThrow('Circular dependency detected'); + await expect( + DependencyTree.fromMap({ + 1: { produces: ['a'], consumes: ['b'] }, + 2: { produces: ['b'], consumes: ['a'] }, + }).parallelTopologicalTraversal(async id => id), + ).rejects.toThrow('Circular dependency detected'); + await expect( + DependencyTree.fromMap({ + 1: { produces: ['a'] }, + 2: { produces: ['c'], consumes: ['a', 'b'] }, + 3: { produces: ['b'], consumes: ['a', 'c'] }, + }).parallelTopologicalTraversal(async id => id), + ).rejects.toThrow('Circular dependency detected'); + }); }); diff --git a/packages/backend-app-api/src/lib/DependencyTree.ts b/packages/backend-app-api/src/lib/DependencyTree.ts index 80cf10ecb2..fe915cfa9d 100644 --- a/packages/backend-app-api/src/lib/DependencyTree.ts +++ b/packages/backend-app-api/src/lib/DependencyTree.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { ConflictError, InputError } from '@backstage/errors'; +import { ConflictError, ForwardedError, InputError } from '@backstage/errors'; interface NodeInput { id: string; @@ -136,4 +136,64 @@ export class DependencyTree { } return undefined; } + + async parallelTopologicalTraversal( + fn: (nodeId: string) => Promise, + ): Promise { + const allProduced = this.#allProduced; + const producedSoFar = new Set(); + const waiting = new Set(this.nodes.values()); + const visited = new Set(); + const results = new Array(); + let inFlight = 0; + + async function processMoreNodes() { + if (waiting.size === 0) { + return; + } + const nodesToProcess = []; + for (const node of waiting) { + let ready = true; + for (const consumed of node.consumes) { + if (allProduced.has(consumed) && !producedSoFar.has(consumed)) { + ready = false; + continue; + } + } + if (ready) { + nodesToProcess.push(node); + } + } + + for (const node of nodesToProcess) { + waiting.delete(node); + } + + if (nodesToProcess.length === 0 && inFlight === 0) { + // We expect the caller to check for circular dependencies before + // traversal, so this error should never happen + throw new Error('Circular dependency detected'); + } + + await Promise.all(nodesToProcess.map(processNode)); + } + + async function processNode(node: Node) { + visited.add(node.id); + inFlight += 1; + try { + const result = await fn(node.id); + results.push(result); + } catch (error) { + throw new ForwardedError(`Failed at ${node.id}`, error); + } + node.produces.forEach(produced => producedSoFar.add(produced)); + inFlight -= 1; + await processMoreNodes(); + } + + await processMoreNodes(); + + return results; + } } From 0474a4bb1609a2eff805184db1bc959597eb1004 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 11 Aug 2023 13:19:37 +0200 Subject: [PATCH 06/16] backend-app-api: use dependency tree to implement module initialization Signed-off-by: Patrik Oldsberg --- .../src/wiring/BackendInitializer.test.ts | 48 +-------- .../src/wiring/BackendInitializer.ts | 99 ++++++------------- 2 files changed, 32 insertions(+), 115 deletions(-) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index 555ce63f15..b05194f594 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -21,7 +21,7 @@ import { createBackendPlugin, createBackendModule, } from '@backstage/backend-plugin-api'; -import { BackendInitializer, resolveInitChunks } from './BackendInitializer'; +import { BackendInitializer } from './BackendInitializer'; import { ServiceRegistry } from './ServiceRegistry'; import { rootLifecycleServiceFactory } from '../services/implementations'; @@ -176,49 +176,3 @@ describe('BackendInitializer', () => { ); }); }); - -describe('resolveInitChunks', () => { - it('should resolve flat chunks', () => { - const resolved = resolveInitChunks( - new Map( - Object.entries({ - a: { consumes: new Set(), provides: new Set(['a']) } as any, - b: { consumes: new Set(), provides: new Set(['b']) } as any, - c: { consumes: new Set(), provides: new Set(['c']) } as any, - d: { consumes: new Set(), provides: new Set(['d']) } as any, - }), - ), - ).map(chunk => Array.from(chunk.keys())); - expect(resolved).toEqual([['a', 'b', 'c', 'd']]); - }); - - it('should resolve nested chunks', () => { - const resolved = resolveInitChunks( - new Map( - Object.entries({ - a: { consumes: new Set(), provides: new Set(['a']) } as any, - b: { consumes: new Set(['a']), provides: new Set(['b', 'c']) } as any, - c: { consumes: new Set(['b']), provides: new Set() } as any, - d: { consumes: new Set(['c']), provides: new Set() } as any, - }), - ), - ).map(chunk => Array.from(chunk.keys())); - expect(resolved).toEqual([['a'], ['b'], ['c', 'd']]); - }); - - it('should detect circular dependencies', () => { - expect(() => - resolveInitChunks( - new Map( - Object.entries({ - a: { consumes: new Set(), provides: new Set(['a']) } as any, - b: { consumes: new Set(['a', 'b']), provides: new Set() } as any, - c: { consumes: new Set(['a', 'c']), provides: new Set() } as any, - }), - ), - ), - ).toThrow( - "Failed to resolve module initialization order, the following modules have circular dependencies, 'b', 'c'", - ); - }); -}); diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 19b933d96a..e5aebb52fa 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -26,8 +26,9 @@ import { EnumerableServiceHolder, ServiceOrExtensionPoint } from './types'; // Direct internal import to avoid duplication // eslint-disable-next-line @backstage/no-forbidden-package-imports import { InternalBackendFeature } from '@backstage/backend-plugin-api/src/wiring/types'; -import { ForwardedError } from '@backstage/errors'; +import { ForwardedError, InputError } from '@backstage/errors'; import { featureDiscoveryServiceRef } from '@backstage/backend-plugin-api/alpha'; +import { DependencyTree } from '../lib/DependencyTree'; export interface BackendRegisterInit { consumes: Set; @@ -38,58 +39,6 @@ export interface BackendRegisterInit { }; } -/** @internal */ -export function resolveInitChunks( - registerInits: Map, -): Map[] { - let registerInitsToOrder = Array.from(registerInits); - const orderedRegisterInits = new Array>(); - - // Keep track of all deps that have been provided so far - const provided = new Set(); - - // Loop until we have ordered all registerInits - while (registerInitsToOrder.length > 0) { - const chunk = new Map(); - - // Find all registerInits that have all their deps provided - for (const [id, registerInit] of registerInitsToOrder) { - if ( - Array.from(registerInit.consumes).every(consumed => - provided.has(consumed), - ) - ) { - chunk.set(id, registerInit); - } - } - - // Add to the result set - orderedRegisterInits.push(chunk); - - // Register all the deps in the current chunk as provided - chunk.forEach(r => r.provides.forEach(p => provided.add(p))); - - // Remove the current chunk from the registerInits to order - const newRegisterInitsToOrder = registerInitsToOrder.filter( - ([id]) => !chunk.has(id), - ); - // Check that we have not hit a circular dependency - if ( - registerInitsToOrder.length !== 0 && - registerInitsToOrder.length === newRegisterInitsToOrder.length - ) { - throw new Error( - `Failed to resolve module initialization order, the following modules have circular dependencies, '${registerInitsToOrder - .map(r => r[0]) - .join("', '")}'`, - ); - } - registerInitsToOrder = newRegisterInitsToOrder; - } - - return orderedRegisterInits; -} - export class BackendInitializer { #startPromise?: Promise; #features = new Array(); @@ -262,22 +211,36 @@ export class BackendInitializer { await Promise.all( allPluginIds.map(async pluginId => { // Modules are initialized before plugins, so that they can provide extension to the plugin - const modules = moduleInits.get(pluginId) ?? new Map(); - for (const chunk of resolveInitChunks(modules)) { - await Promise.all( - Array.from(chunk).map(async ([moduleId, moduleInit]) => { - const moduleDeps = await this.#getInitDeps( - moduleInit.init.deps, - pluginId, - ); - await moduleInit.init.func(moduleDeps).catch(error => { - throw new ForwardedError( - `Module '${moduleId}' for plugin '${pluginId}' startup failed`, - error, - ); - }); - }), + const modules = moduleInits.get(pluginId); + if (modules) { + const tree = DependencyTree.fromIterable( + Array.from(modules).map(([id, { provides, consumes }]) => ({ + id, + provides: Array.from(provides).map(p => p.id), + consumes: Array.from(consumes).map(c => c.id), + })), ); + const circular = tree.detectCircularDependency(); + if (circular) { + throw new InputError( + `Circular dependency detected for modules of plugin '${pluginId}', '${circular.join( + "' -> '", + )}'`, + ); + } + await tree.parallelTopologicalTraversal(async moduleId => { + const moduleInit = modules.get(moduleId)!; + const moduleDeps = await this.#getInitDeps( + moduleInit.init.deps, + pluginId, + ); + await moduleInit.init.func(moduleDeps).catch(error => { + throw new ForwardedError( + `Module '${moduleId}' for plugin '${pluginId}' startup failed`, + error, + ); + }); + }); } // Once all modules have been initialized, we can initialize the plugin itself From ed90ce7f2c50eff52b17760d5f9a2be42f971b97 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 11 Aug 2023 14:00:00 +0200 Subject: [PATCH 07/16] backend-app-api: remove unique producer constraint from dependency tree Signed-off-by: Patrik Oldsberg --- .../backend-app-api/src/lib/DependencyTree.test.ts | 11 ----------- packages/backend-app-api/src/lib/DependencyTree.ts | 12 +----------- 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/packages/backend-app-api/src/lib/DependencyTree.test.ts b/packages/backend-app-api/src/lib/DependencyTree.test.ts index d4cf8a863e..88593754bc 100644 --- a/packages/backend-app-api/src/lib/DependencyTree.test.ts +++ b/packages/backend-app-api/src/lib/DependencyTree.test.ts @@ -24,17 +24,6 @@ describe('DependencyTree', () => { expect(empty.detectCircularDependency()).toBeUndefined(); }); - it('should reject multiple producers', () => { - expect(() => - DependencyTree.fromMap({ - 1: { produces: ['a'] }, - 2: { produces: ['a'] }, - }), - ).toThrow( - "Dependency conflict detected, 'a' may not be produced by both '1' and '2'", - ); - }); - it('should detect circular dependencies', () => { expect( DependencyTree.fromMap({ diff --git a/packages/backend-app-api/src/lib/DependencyTree.ts b/packages/backend-app-api/src/lib/DependencyTree.ts index fe915cfa9d..a11ce3cbaf 100644 --- a/packages/backend-app-api/src/lib/DependencyTree.ts +++ b/packages/backend-app-api/src/lib/DependencyTree.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { ConflictError, ForwardedError, InputError } from '@backstage/errors'; +import { ForwardedError, InputError } from '@backstage/errors'; interface NodeInput { id: string; @@ -62,26 +62,16 @@ export class DependencyTree { #allProduced: Set; #allConsumed: Set; - #producedBy: Map; #consumedBy: Map>; private constructor(readonly nodes: Map) { this.#allProduced = new Set(); this.#allConsumed = new Set(); - this.#producedBy = new Map(); this.#consumedBy = new Map(); for (const node of this.nodes.values()) { for (const produced of node.produces) { this.#allProduced.add(produced); - if (this.#producedBy.has(produced)) { - throw new ConflictError( - `Dependency conflict detected, '${produced}' may not be produced by both '${this.#producedBy.get( - produced, - )}' and '${node.id}'`, - ); - } - this.#producedBy.set(produced, node.id); } for (const consumed of node.consumes) { this.#allConsumed.add(consumed); From 8ade35b7806b782befa811b2ceffb68604adb2df Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 11 Aug 2023 14:00:31 +0200 Subject: [PATCH 08/16] backend-app-api: add module initialization test + fix Signed-off-by: Patrik Oldsberg --- .../src/wiring/BackendInitializer.test.ts | 77 ++++++++++++++++++- .../src/wiring/BackendInitializer.ts | 7 +- 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index b05194f594..b625db31c9 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -20,10 +20,15 @@ import { coreServices, createBackendPlugin, createBackendModule, + createExtensionPoint, } from '@backstage/backend-plugin-api'; import { BackendInitializer } from './BackendInitializer'; import { ServiceRegistry } from './ServiceRegistry'; -import { rootLifecycleServiceFactory } from '../services/implementations'; +import { + lifecycleServiceFactory, + loggerServiceFactory, + rootLifecycleServiceFactory, +} from '../services/implementations'; const rootRef = createServiceRef<{ x: number }>({ id: '1', @@ -44,6 +49,17 @@ class MockLogger { } } +const baseFactories = [ + lifecycleServiceFactory(), + rootLifecycleServiceFactory(), + createServiceFactory({ + service: coreServices.rootLogger, + deps: {}, + factory: () => new MockLogger(), + })(), + loggerServiceFactory(), +]; + describe('BackendInitializer', () => { it('should initialize root scoped services', async () => { const rootFactory = jest.fn(); @@ -75,6 +91,65 @@ describe('BackendInitializer', () => { expect(pluginFactory).not.toHaveBeenCalled(); }); + it('should initialize modules with extension points', async () => { + expect.assertions(3); + + const extensionPoint = createExtensionPoint<{ values: string[] }>({ + id: 'a', + }); + const init = new BackendInitializer(new ServiceRegistry(baseFactories)); + + init.add( + createBackendModule({ + pluginId: 'test', + moduleId: 'modA', + register(reg) { + reg.registerInit({ + deps: { extension: extensionPoint }, + async init({ extension }) { + expect(extension.values).toEqual(['b']); + extension.values.push('a'); + }, + }); + }, + })(), + ); + + init.add( + createBackendModule({ + pluginId: 'test', + moduleId: 'modB', + register(reg) { + const values = ['b']; + reg.registerExtensionPoint(extensionPoint, { values }); + reg.registerInit({ + deps: {}, + async init() { + expect(values).toEqual(['b', 'a', 'c']); + }, + }); + }, + })(), + ); + + init.add( + createBackendModule({ + pluginId: 'test', + moduleId: 'modC', + register(reg) { + reg.registerInit({ + deps: { extension: extensionPoint }, + async init({ extension }) { + expect(extension.values).toEqual(['b', 'a']); + extension.values.push('c'); + }, + }); + }, + })(), + ); + await init.start(); + }); + it('should forward errors when plugins fail to start', async () => { const init = new BackendInitializer(new ServiceRegistry([])); init.add( diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index e5aebb52fa..ce84c2be0f 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -216,8 +216,11 @@ export class BackendInitializer { const tree = DependencyTree.fromIterable( Array.from(modules).map(([id, { provides, consumes }]) => ({ id, - provides: Array.from(provides).map(p => p.id), - consumes: Array.from(consumes).map(c => c.id), + // Relationships are reversed at this point since we're only interested in the extension points. + // If a modules provides extension point A we want it to be initialized AFTER all modules + // that depend on extension point A, so that they can provide their extensions. + consumes: Array.from(provides).map(p => p.id), + produces: Array.from(consumes).map(c => c.id), })), ); const circular = tree.detectCircularDependency(); From dd89ede423a3df636f243468fdcabae812e0a2c7 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 11 Aug 2023 14:01:36 +0200 Subject: [PATCH 09/16] backend-app-api: add test for circular module dependency check Signed-off-by: Patrik Oldsberg --- .../src/wiring/BackendInitializer.test.ts | 44 +++++++++++++++++++ .../src/wiring/BackendInitializer.ts | 4 +- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts index b625db31c9..ba637b7b5b 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.test.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.test.ts @@ -250,4 +250,48 @@ describe('BackendInitializer', () => { "Module 'mod' for plugin 'test' is already registered", ); }); + + it('should reject modules with circular dependencies', async () => { + const extA = createExtensionPoint({ id: 'a' }); + const extB = createExtensionPoint({ id: 'b' }); + const init = new BackendInitializer( + new ServiceRegistry([ + rootLifecycleServiceFactory(), + createServiceFactory({ + service: coreServices.rootLogger, + deps: {}, + factory: () => new MockLogger(), + })(), + ]), + ); + init.add( + createBackendModule({ + pluginId: 'test', + moduleId: 'modA', + register(reg) { + reg.registerExtensionPoint(extA, 'a'); + reg.registerInit({ + deps: { ext: extB }, + async init() {}, + }); + }, + })(), + ); + init.add( + createBackendModule({ + pluginId: 'test', + moduleId: 'modB', + register(reg) { + reg.registerExtensionPoint(extB, 'b'); + reg.registerInit({ + deps: { ext: extA }, + async init() {}, + }); + }, + })(), + ); + await expect(init.start()).rejects.toThrow( + "Circular dependency detected for modules of plugin 'test', 'modA' -> 'modB' -> 'modA'", + ); + }); }); diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index ce84c2be0f..fe3120713e 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -26,7 +26,7 @@ import { EnumerableServiceHolder, ServiceOrExtensionPoint } from './types'; // Direct internal import to avoid duplication // eslint-disable-next-line @backstage/no-forbidden-package-imports import { InternalBackendFeature } from '@backstage/backend-plugin-api/src/wiring/types'; -import { ForwardedError, InputError } from '@backstage/errors'; +import { ForwardedError, ConflictError } from '@backstage/errors'; import { featureDiscoveryServiceRef } from '@backstage/backend-plugin-api/alpha'; import { DependencyTree } from '../lib/DependencyTree'; @@ -225,7 +225,7 @@ export class BackendInitializer { ); const circular = tree.detectCircularDependency(); if (circular) { - throw new InputError( + throw new ConflictError( `Circular dependency detected for modules of plugin '${pluginId}', '${circular.join( "' -> '", )}'`, From 6ed4759db18fd87a2249326dd35e47596105c624 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 11 Aug 2023 14:20:59 +0200 Subject: [PATCH 10/16] backend-app-api: refactor dependency tree to use values instead of IDs Signed-off-by: Patrik Oldsberg --- .../src/lib/DependencyTree.test.ts | 14 +- .../backend-app-api/src/lib/DependencyTree.ts | 122 +++++++++--------- .../src/wiring/BackendInitializer.ts | 39 +++--- 3 files changed, 87 insertions(+), 88 deletions(-) diff --git a/packages/backend-app-api/src/lib/DependencyTree.test.ts b/packages/backend-app-api/src/lib/DependencyTree.test.ts index 88593754bc..c76c23c36a 100644 --- a/packages/backend-app-api/src/lib/DependencyTree.test.ts +++ b/packages/backend-app-api/src/lib/DependencyTree.test.ts @@ -17,11 +17,13 @@ import { DependencyTree } from './DependencyTree'; describe('DependencyTree', () => { - it('should be empty', () => { + it('should be empty', async () => { const empty = DependencyTree.fromMap({}); - expect(Array.from(empty.nodes)).toEqual([]); expect(empty.findUnsatisfiedDeps()).toEqual([]); expect(empty.detectCircularDependency()).toBeUndefined(); + await expect( + empty.parallelTopologicalTraversal(async id => id), + ).resolves.toEqual([]); }); it('should detect circular dependencies', () => { @@ -89,14 +91,14 @@ describe('DependencyTree', () => { DependencyTree.fromMap({ 1: { consumes: ['a'] }, }).findUnsatisfiedDeps(), - ).toEqual([{ id: '1', unsatisfied: ['a'] }]); + ).toEqual([{ value: '1', unsatisfied: ['a'] }]); expect( DependencyTree.fromMap({ 1: { produces: ['a'], consumes: ['b'] }, 2: { produces: ['b'], consumes: ['a', 'd', 'e'] }, }).findUnsatisfiedDeps(), - ).toEqual([{ id: '2', unsatisfied: ['d', 'e'] }]); + ).toEqual([{ value: '2', unsatisfied: ['d', 'e'] }]); expect( DependencyTree.fromMap({ @@ -106,8 +108,8 @@ describe('DependencyTree', () => { 4: { produces: [], consumes: ['c', 'a'] }, }).findUnsatisfiedDeps(), ).toEqual([ - { id: '2', unsatisfied: ['d', 'e'] }, - { id: '4', unsatisfied: ['c'] }, + { value: '2', unsatisfied: ['d', 'e'] }, + { value: '4', unsatisfied: ['c'] }, ]); }); diff --git a/packages/backend-app-api/src/lib/DependencyTree.ts b/packages/backend-app-api/src/lib/DependencyTree.ts index a11ce3cbaf..addaa873a5 100644 --- a/packages/backend-app-api/src/lib/DependencyTree.ts +++ b/packages/backend-app-api/src/lib/DependencyTree.ts @@ -14,111 +14,109 @@ * limitations under the License. */ -import { ForwardedError, InputError } from '@backstage/errors'; - -interface NodeInput { - id: string; +interface NodeInput { + value: T; consumes?: Iterable; produces?: Iterable; } /** @internal */ -class Node { - static from(input: NodeInput) { - return new Node( - input.id, +class Node { + static from(input: NodeInput) { + return new Node( + input.value, input.consumes ? new Set(input.consumes) : new Set(), input.produces ? new Set(input.produces) : new Set(), ); } private constructor( - readonly id: string, + readonly value: T, readonly consumes: Set, readonly produces: Set, ) {} } /** @internal */ -export class DependencyTree { - static fromMap(nodes: Record>) { +export class DependencyTree { + static fromMap( + nodes: Record, 'value'>>, + ): DependencyTree { return this.fromIterable( - Object.entries(nodes).map(([id, node]) => ({ id, ...node })), + Object.entries(nodes).map(([key, node]) => ({ + value: String(key), + ...node, + })), ); } - static fromIterable(nodeInputs: Iterable) { - const nodes = new Map(); + static fromIterable( + nodeInputs: Iterable>, + ): DependencyTree { + const nodes = new Array>(); for (const nodeInput of nodeInputs) { - const node = Node.from(nodeInput); - if (nodes.has(node.id)) { - throw new InputError(`Duplicate node with id ${node.id}`); - } - nodes.set(node.id, node); + nodes.push(Node.from(nodeInput)); } return new DependencyTree(nodes); } + #nodes: Array>; #allProduced: Set; #allConsumed: Set; - #consumedBy: Map>; - private constructor(readonly nodes: Map) { + private constructor(nodes: Array>) { + this.#nodes = nodes; this.#allProduced = new Set(); this.#allConsumed = new Set(); - this.#consumedBy = new Map(); - for (const node of this.nodes.values()) { + for (const node of this.#nodes.values()) { for (const produced of node.produces) { this.#allProduced.add(produced); } for (const consumed of node.consumes) { this.#allConsumed.add(consumed); - if (!this.#consumedBy.get(consumed)?.add(node.id)) { - this.#consumedBy.set(consumed, new Set([node.id])); - } } } } - findUnsatisfiedDeps(): Array<{ id: string; unsatisfied: string[] }> { + findUnsatisfiedDeps(): Array<{ value: T; unsatisfied: string[] }> { const unsatisfiedDependencies = []; - for (const node of this.nodes.values()) { + for (const node of this.#nodes.values()) { const unsatisfied = Array.from(node.consumes).filter( id => !this.#allProduced.has(id), ); if (unsatisfied.length > 0) { - unsatisfiedDependencies.push({ id: node.id, unsatisfied }); + unsatisfiedDependencies.push({ value: node.value, unsatisfied }); } } return unsatisfiedDependencies; } - detectCircularDependency(): string[] | undefined { - for (const nodeId of this.nodes.keys()) { - const visited = new Set(); - const stack = new Array<[id: string, path: string[]]>([nodeId, [nodeId]]); + detectCircularDependency(): T[] | undefined { + for (const startNode of this.#nodes) { + const visited = new Set>(); + const stack = new Array<[node: Node, path: T[]]>([ + startNode, + [startNode.value], + ]); while (stack.length > 0) { - const [id, path] = stack.pop()!; - if (visited.has(id)) { + const [node, path] = stack.pop()!; + if (visited.has(node)) { continue; } - visited.add(id); - const node = this.nodes.get(id); - if (node) { - for (const produced of node.produces) { - const consumers = this.#consumedBy.get(produced); - if (consumers) { - for (const consumer of consumers) { - if (consumer === nodeId) { - return [...path, nodeId]; - } - if (!visited.has(consumer)) { - stack.push([consumer, [...path, consumer]]); - } - } + visited.add(node); + for (const produced of node.produces) { + const consumerNodes = this.#nodes.filter(other => + other.consumes.has(produced), + ); + for (const consumer of consumerNodes) { + if (consumer === startNode) { + return [...path, startNode.value]; + } + if (!visited.has(consumer)) { + stack.push([consumer, [...path, consumer.value]]); } } } @@ -127,14 +125,14 @@ export class DependencyTree { return undefined; } - async parallelTopologicalTraversal( - fn: (nodeId: string) => Promise, - ): Promise { + async parallelTopologicalTraversal( + fn: (value: T) => Promise, + ): Promise { const allProduced = this.#allProduced; const producedSoFar = new Set(); - const waiting = new Set(this.nodes.values()); - const visited = new Set(); - const results = new Array(); + const waiting = new Set(this.#nodes.values()); + const visited = new Set>(); + const results = new Array(); let inFlight = 0; async function processMoreNodes() { @@ -168,15 +166,13 @@ export class DependencyTree { await Promise.all(nodesToProcess.map(processNode)); } - async function processNode(node: Node) { - visited.add(node.id); + async function processNode(node: Node) { + visited.add(node); inFlight += 1; - try { - const result = await fn(node.id); - results.push(result); - } catch (error) { - throw new ForwardedError(`Failed at ${node.id}`, error); - } + + const result = await fn(node.value); + results.push(result); + node.produces.forEach(produced => producedSoFar.add(produced)); inFlight -= 1; await processMoreNodes(); diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index fe3120713e..1061ad8944 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -214,36 +214,37 @@ export class BackendInitializer { const modules = moduleInits.get(pluginId); if (modules) { const tree = DependencyTree.fromIterable( - Array.from(modules).map(([id, { provides, consumes }]) => ({ - id, + Array.from(modules).map(([moduleId, moduleInit]) => ({ + value: { moduleId, moduleInit }, // Relationships are reversed at this point since we're only interested in the extension points. // If a modules provides extension point A we want it to be initialized AFTER all modules // that depend on extension point A, so that they can provide their extensions. - consumes: Array.from(provides).map(p => p.id), - produces: Array.from(consumes).map(c => c.id), + consumes: Array.from(moduleInit.provides).map(p => p.id), + produces: Array.from(moduleInit.consumes).map(c => c.id), })), ); const circular = tree.detectCircularDependency(); if (circular) { throw new ConflictError( - `Circular dependency detected for modules of plugin '${pluginId}', '${circular.join( - "' -> '", - )}'`, + `Circular dependency detected for modules of plugin '${pluginId}', ${circular + .map(({ moduleId }) => `'${moduleId}'`) + .join(' -> ')}`, ); } - await tree.parallelTopologicalTraversal(async moduleId => { - const moduleInit = modules.get(moduleId)!; - const moduleDeps = await this.#getInitDeps( - moduleInit.init.deps, - pluginId, - ); - await moduleInit.init.func(moduleDeps).catch(error => { - throw new ForwardedError( - `Module '${moduleId}' for plugin '${pluginId}' startup failed`, - error, + await tree.parallelTopologicalTraversal( + async ({ moduleId, moduleInit }) => { + const moduleDeps = await this.#getInitDeps( + moduleInit.init.deps, + pluginId, ); - }); - }); + await moduleInit.init.func(moduleDeps).catch(error => { + throw new ForwardedError( + `Module '${moduleId}' for plugin '${pluginId}' startup failed`, + error, + ); + }); + }, + ); } // Once all modules have been initialized, we can initialize the plugin itself From a292c9d6d9e4bba777bba129bbc8c7fc71b7bc13 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 11 Aug 2023 14:34:17 +0200 Subject: [PATCH 11/16] backend-app-api: rename dependency tree produces -> provides Signed-off-by: Patrik Oldsberg --- .../src/lib/DependencyTree.test.ts | 68 +++++++++---------- .../backend-app-api/src/lib/DependencyTree.ts | 29 ++++---- .../src/wiring/BackendInitializer.ts | 2 +- 3 files changed, 47 insertions(+), 52 deletions(-) diff --git a/packages/backend-app-api/src/lib/DependencyTree.test.ts b/packages/backend-app-api/src/lib/DependencyTree.test.ts index c76c23c36a..70591aa295 100644 --- a/packages/backend-app-api/src/lib/DependencyTree.test.ts +++ b/packages/backend-app-api/src/lib/DependencyTree.test.ts @@ -38,8 +38,8 @@ describe('DependencyTree', () => { expect( DependencyTree.fromMap({ - 1: { produces: ['a'] }, - 2: { consumes: ['a'], produces: ['b', 'c'] }, + 1: { provides: ['a'] }, + 2: { consumes: ['a'], provides: ['b', 'c'] }, 3: { consumes: ['b'] }, 4: { consumes: ['c'] }, }).detectCircularDependency(), @@ -47,23 +47,23 @@ describe('DependencyTree', () => { expect( DependencyTree.fromMap({ - 1: { produces: ['a'], consumes: ['a'] }, + 1: { provides: ['a'], consumes: ['a'] }, }).detectCircularDependency(), ).toEqual(['1', '1']); expect( DependencyTree.fromMap({ - 1: { produces: ['a'], consumes: ['b'] }, - 2: { produces: ['b'], consumes: ['a'] }, + 1: { provides: ['a'], consumes: ['b'] }, + 2: { provides: ['b'], consumes: ['a'] }, }).detectCircularDependency(), ).toEqual(['1', '2', '1']); expect( DependencyTree.fromMap({ - 1: { produces: ['a'] }, - 2: { produces: ['b'], consumes: ['a', 'e'] }, - 3: { produces: ['c'], consumes: ['b'] }, - 4: { produces: ['d', 'e'], consumes: ['c', 'a'] }, + 1: { provides: ['a'] }, + 2: { provides: ['b'], consumes: ['a', 'e'] }, + 3: { provides: ['c'], consumes: ['b'] }, + 4: { provides: ['d', 'e'], consumes: ['c', 'a'] }, }).detectCircularDependency(), ).toEqual(['2', '3', '4', '2']); }); @@ -80,8 +80,8 @@ describe('DependencyTree', () => { expect( DependencyTree.fromMap({ - 1: { produces: ['a'] }, - 2: { consumes: ['a'], produces: ['b', 'c'] }, + 1: { provides: ['a'] }, + 2: { consumes: ['a'], provides: ['b', 'c'] }, 3: { consumes: ['b'] }, 4: { consumes: ['c'] }, }).findUnsatisfiedDeps(), @@ -95,17 +95,17 @@ describe('DependencyTree', () => { expect( DependencyTree.fromMap({ - 1: { produces: ['a'], consumes: ['b'] }, - 2: { produces: ['b'], consumes: ['a', 'd', 'e'] }, + 1: { provides: ['a'], consumes: ['b'] }, + 2: { provides: ['b'], consumes: ['a', 'd', 'e'] }, }).findUnsatisfiedDeps(), ).toEqual([{ value: '2', unsatisfied: ['d', 'e'] }]); expect( DependencyTree.fromMap({ - 1: { produces: ['a'] }, - 2: { produces: ['b'], consumes: ['a', 'd', 'e'] }, - 3: { produces: [], consumes: ['b'] }, - 4: { produces: [], consumes: ['c', 'a'] }, + 1: { provides: ['a'] }, + 2: { provides: ['b'], consumes: ['a', 'd', 'e'] }, + 3: { provides: [], consumes: ['b'] }, + 4: { provides: [], consumes: ['c', 'a'] }, }).findUnsatisfiedDeps(), ).toEqual([ { value: '2', unsatisfied: ['d', 'e'] }, @@ -125,8 +125,8 @@ describe('DependencyTree', () => { await expect( DependencyTree.fromMap({ - 1: { produces: ['a'] }, - 2: { consumes: ['a'], produces: ['b', 'c'] }, + 1: { provides: ['a'] }, + 2: { consumes: ['a'], provides: ['b', 'c'] }, 3: { consumes: ['b'] }, 4: { consumes: ['c'] }, }).parallelTopologicalTraversal(async id => id), @@ -135,17 +135,17 @@ describe('DependencyTree', () => { await expect( DependencyTree.fromMap({ 1: { consumes: ['c'] }, - 2: { produces: ['c'], consumes: ['b'] }, - 3: { produces: ['b'], consumes: ['a'] }, - 4: { produces: ['a'] }, + 2: { provides: ['c'], consumes: ['b'] }, + 3: { provides: ['b'], consumes: ['a'] }, + 4: { provides: ['a'] }, }).parallelTopologicalTraversal(async id => id), ).resolves.toEqual(['4', '3', '2', '1']); await expect( DependencyTree.fromMap({ - 1: { produces: ['a'] }, - 2: { produces: ['b'], consumes: ['a'] }, - 3: { produces: ['c'], consumes: ['a'] }, + 1: { provides: ['a'] }, + 2: { provides: ['b'], consumes: ['a'] }, + 3: { provides: ['c'], consumes: ['a'] }, 4: { consumes: ['b'] }, 5: { consumes: ['c'] }, }).parallelTopologicalTraversal(async id => id), @@ -154,9 +154,9 @@ describe('DependencyTree', () => { // Same as above, but with 2 being delayed await expect( DependencyTree.fromMap({ - 1: { produces: ['a'] }, - 2: { produces: ['b'], consumes: ['a'] }, - 3: { produces: ['c'], consumes: ['a'] }, + 1: { provides: ['a'] }, + 2: { provides: ['b'], consumes: ['a'] }, + 3: { provides: ['c'], consumes: ['a'] }, 4: { consumes: ['b'] }, 5: { consumes: ['c'] }, }).parallelTopologicalTraversal(async id => { @@ -170,20 +170,20 @@ describe('DependencyTree', () => { await expect( DependencyTree.fromMap({ - 1: { produces: ['a'], consumes: ['a'] }, + 1: { provides: ['a'], consumes: ['a'] }, }).parallelTopologicalTraversal(async id => id), ).rejects.toThrow('Circular dependency detected'); await expect( DependencyTree.fromMap({ - 1: { produces: ['a'], consumes: ['b'] }, - 2: { produces: ['b'], consumes: ['a'] }, + 1: { provides: ['a'], consumes: ['b'] }, + 2: { provides: ['b'], consumes: ['a'] }, }).parallelTopologicalTraversal(async id => id), ).rejects.toThrow('Circular dependency detected'); await expect( DependencyTree.fromMap({ - 1: { produces: ['a'] }, - 2: { produces: ['c'], consumes: ['a', 'b'] }, - 3: { produces: ['b'], consumes: ['a', 'c'] }, + 1: { provides: ['a'] }, + 2: { provides: ['c'], consumes: ['a', 'b'] }, + 3: { provides: ['b'], consumes: ['a', 'c'] }, }).parallelTopologicalTraversal(async id => id), ).rejects.toThrow('Circular dependency detected'); }); diff --git a/packages/backend-app-api/src/lib/DependencyTree.ts b/packages/backend-app-api/src/lib/DependencyTree.ts index addaa873a5..51567524d8 100644 --- a/packages/backend-app-api/src/lib/DependencyTree.ts +++ b/packages/backend-app-api/src/lib/DependencyTree.ts @@ -17,7 +17,7 @@ interface NodeInput { value: T; consumes?: Iterable; - produces?: Iterable; + provides?: Iterable; } /** @internal */ @@ -26,14 +26,14 @@ class Node { return new Node( input.value, input.consumes ? new Set(input.consumes) : new Set(), - input.produces ? new Set(input.produces) : new Set(), + input.provides ? new Set(input.provides) : new Set(), ); } private constructor( readonly value: T, readonly consumes: Set, - readonly produces: Set, + readonly provides: Set, ) {} } @@ -62,20 +62,15 @@ export class DependencyTree { } #nodes: Array>; - #allProduced: Set; - #allConsumed: Set; + #allProvided: Set; private constructor(nodes: Array>) { this.#nodes = nodes; - this.#allProduced = new Set(); - this.#allConsumed = new Set(); + this.#allProvided = new Set(); for (const node of this.#nodes.values()) { - for (const produced of node.produces) { - this.#allProduced.add(produced); - } - for (const consumed of node.consumes) { - this.#allConsumed.add(consumed); + for (const produced of node.provides) { + this.#allProvided.add(produced); } } } @@ -84,7 +79,7 @@ export class DependencyTree { const unsatisfiedDependencies = []; for (const node of this.#nodes.values()) { const unsatisfied = Array.from(node.consumes).filter( - id => !this.#allProduced.has(id), + id => !this.#allProvided.has(id), ); if (unsatisfied.length > 0) { unsatisfiedDependencies.push({ value: node.value, unsatisfied }); @@ -107,7 +102,7 @@ export class DependencyTree { continue; } visited.add(node); - for (const produced of node.produces) { + for (const produced of node.provides) { const consumerNodes = this.#nodes.filter(other => other.consumes.has(produced), ); @@ -128,7 +123,7 @@ export class DependencyTree { async parallelTopologicalTraversal( fn: (value: T) => Promise, ): Promise { - const allProduced = this.#allProduced; + const allProvided = this.#allProvided; const producedSoFar = new Set(); const waiting = new Set(this.#nodes.values()); const visited = new Set>(); @@ -143,7 +138,7 @@ export class DependencyTree { for (const node of waiting) { let ready = true; for (const consumed of node.consumes) { - if (allProduced.has(consumed) && !producedSoFar.has(consumed)) { + if (allProvided.has(consumed) && !producedSoFar.has(consumed)) { ready = false; continue; } @@ -173,7 +168,7 @@ export class DependencyTree { const result = await fn(node.value); results.push(result); - node.produces.forEach(produced => producedSoFar.add(produced)); + node.provides.forEach(produced => producedSoFar.add(produced)); inFlight -= 1; await processMoreNodes(); } diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 1061ad8944..fd5ced9158 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -220,7 +220,7 @@ export class BackendInitializer { // If a modules provides extension point A we want it to be initialized AFTER all modules // that depend on extension point A, so that they can provide their extensions. consumes: Array.from(moduleInit.provides).map(p => p.id), - produces: Array.from(moduleInit.consumes).map(c => c.id), + provides: Array.from(moduleInit.consumes).map(c => c.id), })), ); const circular = tree.detectCircularDependency(); From afe320acecb3196cc97196fa93527dd41910597d Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 11 Aug 2023 14:35:16 +0200 Subject: [PATCH 12/16] backend-app-api: rename DependencyTree -> DependencyGraph Signed-off-by: Patrik Oldsberg --- ...cyTree.test.ts => DependencyGraph.test.ts} | 42 +++++++++---------- .../{DependencyTree.ts => DependencyGraph.ts} | 8 ++-- .../src/wiring/BackendInitializer.ts | 4 +- 3 files changed, 27 insertions(+), 27 deletions(-) rename packages/backend-app-api/src/lib/{DependencyTree.test.ts => DependencyGraph.test.ts} (88%) rename packages/backend-app-api/src/lib/{DependencyTree.ts => DependencyGraph.ts} (97%) diff --git a/packages/backend-app-api/src/lib/DependencyTree.test.ts b/packages/backend-app-api/src/lib/DependencyGraph.test.ts similarity index 88% rename from packages/backend-app-api/src/lib/DependencyTree.test.ts rename to packages/backend-app-api/src/lib/DependencyGraph.test.ts index 70591aa295..6720adb5e6 100644 --- a/packages/backend-app-api/src/lib/DependencyTree.test.ts +++ b/packages/backend-app-api/src/lib/DependencyGraph.test.ts @@ -14,11 +14,11 @@ * limitations under the License. */ -import { DependencyTree } from './DependencyTree'; +import { DependencyGraph } from './DependencyGraph'; -describe('DependencyTree', () => { +describe('DependencyGraph', () => { it('should be empty', async () => { - const empty = DependencyTree.fromMap({}); + const empty = DependencyGraph.fromMap({}); expect(empty.findUnsatisfiedDeps()).toEqual([]); expect(empty.detectCircularDependency()).toBeUndefined(); await expect( @@ -28,7 +28,7 @@ describe('DependencyTree', () => { it('should detect circular dependencies', () => { expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: {}, 2: {}, 3: {}, @@ -37,7 +37,7 @@ describe('DependencyTree', () => { ).toBeUndefined(); expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'] }, 2: { consumes: ['a'], provides: ['b', 'c'] }, 3: { consumes: ['b'] }, @@ -46,20 +46,20 @@ describe('DependencyTree', () => { ).toBeUndefined(); expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'], consumes: ['a'] }, }).detectCircularDependency(), ).toEqual(['1', '1']); expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'], consumes: ['b'] }, 2: { provides: ['b'], consumes: ['a'] }, }).detectCircularDependency(), ).toEqual(['1', '2', '1']); expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'] }, 2: { provides: ['b'], consumes: ['a', 'e'] }, 3: { provides: ['c'], consumes: ['b'] }, @@ -70,7 +70,7 @@ describe('DependencyTree', () => { it('should find unsatisfied dependencies', () => { expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: {}, 2: {}, 3: {}, @@ -79,7 +79,7 @@ describe('DependencyTree', () => { ).toEqual([]); expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'] }, 2: { consumes: ['a'], provides: ['b', 'c'] }, 3: { consumes: ['b'] }, @@ -88,20 +88,20 @@ describe('DependencyTree', () => { ).toEqual([]); expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { consumes: ['a'] }, }).findUnsatisfiedDeps(), ).toEqual([{ value: '1', unsatisfied: ['a'] }]); expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'], consumes: ['b'] }, 2: { provides: ['b'], consumes: ['a', 'd', 'e'] }, }).findUnsatisfiedDeps(), ).toEqual([{ value: '2', unsatisfied: ['d', 'e'] }]); expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'] }, 2: { provides: ['b'], consumes: ['a', 'd', 'e'] }, 3: { provides: [], consumes: ['b'] }, @@ -115,7 +115,7 @@ describe('DependencyTree', () => { it('should traverse dependencies in topological order', async () => { await expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: {}, 2: {}, 3: {}, @@ -124,7 +124,7 @@ describe('DependencyTree', () => { ).resolves.toEqual(['1', '2', '3', '4']); await expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'] }, 2: { consumes: ['a'], provides: ['b', 'c'] }, 3: { consumes: ['b'] }, @@ -133,7 +133,7 @@ describe('DependencyTree', () => { ).resolves.toEqual(['1', '2', '3', '4']); await expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { consumes: ['c'] }, 2: { provides: ['c'], consumes: ['b'] }, 3: { provides: ['b'], consumes: ['a'] }, @@ -142,7 +142,7 @@ describe('DependencyTree', () => { ).resolves.toEqual(['4', '3', '2', '1']); await expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'] }, 2: { provides: ['b'], consumes: ['a'] }, 3: { provides: ['c'], consumes: ['a'] }, @@ -153,7 +153,7 @@ describe('DependencyTree', () => { // Same as above, but with 2 being delayed await expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'] }, 2: { provides: ['b'], consumes: ['a'] }, 3: { provides: ['c'], consumes: ['a'] }, @@ -169,18 +169,18 @@ describe('DependencyTree', () => { ).resolves.toEqual(['1', '3', '5', '2', '4']); await expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'], consumes: ['a'] }, }).parallelTopologicalTraversal(async id => id), ).rejects.toThrow('Circular dependency detected'); await expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'], consumes: ['b'] }, 2: { provides: ['b'], consumes: ['a'] }, }).parallelTopologicalTraversal(async id => id), ).rejects.toThrow('Circular dependency detected'); await expect( - DependencyTree.fromMap({ + DependencyGraph.fromMap({ 1: { provides: ['a'] }, 2: { provides: ['c'], consumes: ['a', 'b'] }, 3: { provides: ['b'], consumes: ['a', 'c'] }, diff --git a/packages/backend-app-api/src/lib/DependencyTree.ts b/packages/backend-app-api/src/lib/DependencyGraph.ts similarity index 97% rename from packages/backend-app-api/src/lib/DependencyTree.ts rename to packages/backend-app-api/src/lib/DependencyGraph.ts index 51567524d8..cafdbccc00 100644 --- a/packages/backend-app-api/src/lib/DependencyTree.ts +++ b/packages/backend-app-api/src/lib/DependencyGraph.ts @@ -38,10 +38,10 @@ class Node { } /** @internal */ -export class DependencyTree { +export class DependencyGraph { static fromMap( nodes: Record, 'value'>>, - ): DependencyTree { + ): DependencyGraph { return this.fromIterable( Object.entries(nodes).map(([key, node]) => ({ value: String(key), @@ -52,13 +52,13 @@ export class DependencyTree { static fromIterable( nodeInputs: Iterable>, - ): DependencyTree { + ): DependencyGraph { const nodes = new Array>(); for (const nodeInput of nodeInputs) { nodes.push(Node.from(nodeInput)); } - return new DependencyTree(nodes); + return new DependencyGraph(nodes); } #nodes: Array>; diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index fd5ced9158..06201dfb2b 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -28,7 +28,7 @@ import { EnumerableServiceHolder, ServiceOrExtensionPoint } from './types'; import { InternalBackendFeature } from '@backstage/backend-plugin-api/src/wiring/types'; import { ForwardedError, ConflictError } from '@backstage/errors'; import { featureDiscoveryServiceRef } from '@backstage/backend-plugin-api/alpha'; -import { DependencyTree } from '../lib/DependencyTree'; +import { DependencyGraph } from '../lib/DependencyGraph'; export interface BackendRegisterInit { consumes: Set; @@ -213,7 +213,7 @@ export class BackendInitializer { // Modules are initialized before plugins, so that they can provide extension to the plugin const modules = moduleInits.get(pluginId); if (modules) { - const tree = DependencyTree.fromIterable( + const tree = DependencyGraph.fromIterable( Array.from(modules).map(([moduleId, moduleInit]) => ({ value: { moduleId, moduleInit }, // Relationships are reversed at this point since we're only interested in the extension points. From b7e0362496e7b335ea57773d3f9aefdb90ee851a Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 11 Aug 2023 14:46:34 +0200 Subject: [PATCH 13/16] backend-app-api: some docs for DependencyGraph Signed-off-by: Patrik Oldsberg --- .../src/lib/DependencyGraph.ts | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/packages/backend-app-api/src/lib/DependencyGraph.ts b/packages/backend-app-api/src/lib/DependencyGraph.ts index cafdbccc00..8fd679f555 100644 --- a/packages/backend-app-api/src/lib/DependencyGraph.ts +++ b/packages/backend-app-api/src/lib/DependencyGraph.ts @@ -37,7 +37,10 @@ class Node { ) {} } -/** @internal */ +/** + * Internal helper to help validate and traverse a dependency graph. + * @internal + */ export class DependencyGraph { static fromMap( nodes: Record, 'value'>>, @@ -75,6 +78,7 @@ export class DependencyGraph { } } + // 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()) { @@ -88,6 +92,8 @@ 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. detectCircularDependency(): T[] | undefined { for (const startNode of this.#nodes) { const visited = new Set>(); @@ -120,6 +126,15 @@ export class DependencyGraph { return undefined; } + /** + * Traverses the dependency graph in topological order, calling the provided + * function for each node and waiting for it to resolve. + * + * The nodes are traversed in parallel, but in such a way that no node is + * visited before all of its dependencies. + * + * Dependencies of nodes that are not produced by any other nodes will be ignored. + */ async parallelTopologicalTraversal( fn: (value: T) => Promise, ): Promise { @@ -128,8 +143,9 @@ export class DependencyGraph { const waiting = new Set(this.#nodes.values()); const visited = new Set>(); const results = new Array(); - let inFlight = 0; + let inFlight = 0; // Keep track of how many callbacks are in flight, so that we know if we got stuck + // Find all nodes that have no dependencies that have not already been produced by visited nodes async function processMoreNodes() { if (waiting.size === 0) { return; @@ -161,6 +177,7 @@ export class DependencyGraph { await Promise.all(nodesToProcess.map(processNode)); } + // Process an individual node, and then add its produced dependencies to the set of available products async function processNode(node: Node) { visited.add(node); inFlight += 1; From d197f13172e08b1055368122edcde84fd590b272 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 14 Aug 2023 14:01:00 +0200 Subject: [PATCH 14/16] backend-app-api: restructure DependencyGraph tests Signed-off-by: Patrik Oldsberg --- .../src/lib/DependencyGraph.test.ts | 318 ++++++++++-------- 1 file changed, 175 insertions(+), 143 deletions(-) diff --git a/packages/backend-app-api/src/lib/DependencyGraph.test.ts b/packages/backend-app-api/src/lib/DependencyGraph.test.ts index 6720adb5e6..9fd6219ab3 100644 --- a/packages/backend-app-api/src/lib/DependencyGraph.test.ts +++ b/packages/backend-app-api/src/lib/DependencyGraph.test.ts @@ -26,165 +26,197 @@ describe('DependencyGraph', () => { ).resolves.toEqual([]); }); - it('should detect circular dependencies', () => { - expect( - DependencyGraph.fromMap({ - 1: {}, - 2: {}, - 3: {}, - 4: {}, - }).detectCircularDependency(), - ).toBeUndefined(); + describe('detectCircularDependency', () => { + it('should return undefined with not deps', () => { + expect( + DependencyGraph.fromMap({ + 1: {}, + 2: {}, + 3: {}, + 4: {}, + }).detectCircularDependency(), + ).toBeUndefined(); + }); - expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'] }, - 2: { consumes: ['a'], provides: ['b', 'c'] }, - 3: { consumes: ['b'] }, - 4: { consumes: ['c'] }, - }).detectCircularDependency(), - ).toBeUndefined(); + it('should return undefined with no circular deps', async () => { + expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'] }, + 2: { consumes: ['a'], provides: ['b', 'c'] }, + 3: { consumes: ['b'] }, + 4: { consumes: ['c'] }, + }).detectCircularDependency(), + ).toBeUndefined(); + }); - expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'], consumes: ['a'] }, - }).detectCircularDependency(), - ).toEqual(['1', '1']); + it('should detect an immediate circular dep', async () => { + expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'], consumes: ['a'] }, + }).detectCircularDependency(), + ).toEqual(['1', '1']); + }); - expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'], consumes: ['b'] }, - 2: { provides: ['b'], consumes: ['a'] }, - }).detectCircularDependency(), - ).toEqual(['1', '2', '1']); + it('should detect a small circular dep', async () => { + expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'], consumes: ['b'] }, + 2: { provides: ['b'], consumes: ['a'] }, + }).detectCircularDependency(), + ).toEqual(['1', '2', '1']); + }); - 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', '3', '4', '2']); + it('should detect a larger distant circular dep', async () => { + 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', '3', '4', '2']); + }); }); - it('should find unsatisfied dependencies', () => { - expect( - DependencyGraph.fromMap({ - 1: {}, - 2: {}, - 3: {}, - 4: {}, - }).findUnsatisfiedDeps(), - ).toEqual([]); + describe('findUnsatisfiedDeps', () => { + it('should return nothing with no deps', () => { + expect( + DependencyGraph.fromMap({ + 1: {}, + 2: {}, + 3: {}, + 4: {}, + }).findUnsatisfiedDeps(), + ).toEqual([]); + }); - expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'] }, - 2: { consumes: ['a'], provides: ['b', 'c'] }, - 3: { consumes: ['b'] }, - 4: { consumes: ['c'] }, - }).findUnsatisfiedDeps(), - ).toEqual([]); + it('should return nothing when all deps are satisfied', async () => { + expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'] }, + 2: { consumes: ['a'], provides: ['b', 'c'] }, + 3: { consumes: ['b'] }, + 4: { consumes: ['c'] }, + }).findUnsatisfiedDeps(), + ).toEqual([]); + }); - expect( - DependencyGraph.fromMap({ - 1: { consumes: ['a'] }, - }).findUnsatisfiedDeps(), - ).toEqual([{ value: '1', unsatisfied: ['a'] }]); + it('should find a single unsatisfied dep', async () => { + expect( + DependencyGraph.fromMap({ + 1: { consumes: ['a'] }, + }).findUnsatisfiedDeps(), + ).toEqual([{ value: '1', unsatisfied: ['a'] }]); + }); - expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'], consumes: ['b'] }, - 2: { provides: ['b'], consumes: ['a', 'd', 'e'] }, - }).findUnsatisfiedDeps(), - ).toEqual([{ value: '2', unsatisfied: ['d', 'e'] }]); + it('should find multiple unsatisfied deps for one node', async () => { + expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'], consumes: ['b'] }, + 2: { provides: ['b'], consumes: ['a', 'd', 'e'] }, + }).findUnsatisfiedDeps(), + ).toEqual([{ value: '2', unsatisfied: ['d', 'e'] }]); + }); - expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'] }, - 2: { provides: ['b'], consumes: ['a', 'd', 'e'] }, - 3: { provides: [], consumes: ['b'] }, - 4: { provides: [], consumes: ['c', 'a'] }, - }).findUnsatisfiedDeps(), - ).toEqual([ - { value: '2', unsatisfied: ['d', 'e'] }, - { value: '4', unsatisfied: ['c'] }, - ]); + it('should find multiple unsatisfied deps for multiple nodes', async () => { + expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'] }, + 2: { provides: ['b'], consumes: ['a', 'd', 'e'] }, + 3: { provides: [], consumes: ['b'] }, + 4: { provides: [], consumes: ['c', 'a'] }, + }).findUnsatisfiedDeps(), + ).toEqual([ + { value: '2', unsatisfied: ['d', 'e'] }, + { value: '4', unsatisfied: ['c'] }, + ]); + }); }); - it('should traverse dependencies in topological order', async () => { - await expect( - DependencyGraph.fromMap({ - 1: {}, - 2: {}, - 3: {}, - 4: {}, - }).parallelTopologicalTraversal(async id => id), - ).resolves.toEqual(['1', '2', '3', '4']); + describe('parallelTopologicalTraversal', () => { + it('should traverse with no deps', async () => { + await expect( + DependencyGraph.fromMap({ + 1: {}, + 2: {}, + 3: {}, + 4: {}, + }).parallelTopologicalTraversal(async id => id), + ).resolves.toEqual(['1', '2', '3', '4']); + }); - await expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'] }, - 2: { consumes: ['a'], provides: ['b', 'c'] }, - 3: { consumes: ['b'] }, - 4: { consumes: ['c'] }, - }).parallelTopologicalTraversal(async id => id), - ).resolves.toEqual(['1', '2', '3', '4']); + it('should traverse with a few deps', async () => { + await expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'] }, + 2: { consumes: ['a'], provides: ['b', 'c'] }, + 3: { consumes: ['b'] }, + 4: { consumes: ['c'] }, + }).parallelTopologicalTraversal(async id => id), + ).resolves.toEqual(['1', '2', '3', '4']); + }); - await expect( - DependencyGraph.fromMap({ - 1: { consumes: ['c'] }, - 2: { provides: ['c'], consumes: ['b'] }, - 3: { provides: ['b'], consumes: ['a'] }, - 4: { provides: ['a'] }, - }).parallelTopologicalTraversal(async id => id), - ).resolves.toEqual(['4', '3', '2', '1']); + it('should traverse in reverse', async () => { + await expect( + DependencyGraph.fromMap({ + 1: { consumes: ['c'] }, + 2: { provides: ['c'], consumes: ['b'] }, + 3: { provides: ['b'], consumes: ['a'] }, + 4: { provides: ['a'] }, + }).parallelTopologicalTraversal(async id => id), + ).resolves.toEqual(['4', '3', '2', '1']); + }); - await expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'] }, - 2: { provides: ['b'], consumes: ['a'] }, - 3: { provides: ['c'], consumes: ['a'] }, - 4: { consumes: ['b'] }, - 5: { consumes: ['c'] }, - }).parallelTopologicalTraversal(async id => id), - ).resolves.toEqual(['1', '2', '3', '4', '5']); + it('should execute in parallel', async () => { + await expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'] }, + 2: { provides: ['b'], consumes: ['a'] }, + 3: { provides: ['c'], consumes: ['a'] }, + 4: { consumes: ['b'] }, + 5: { consumes: ['c'] }, + }).parallelTopologicalTraversal(async id => id), + ).resolves.toEqual(['1', '2', '3', '4', '5']); - // Same as above, but with 2 being delayed - await expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'] }, - 2: { provides: ['b'], consumes: ['a'] }, - 3: { provides: ['c'], consumes: ['a'] }, - 4: { consumes: ['b'] }, - 5: { consumes: ['c'] }, - }).parallelTopologicalTraversal(async id => { - // When delaying 2 we expect 3 and 5 to complete before 2 and 4 - if (id === '2') { - await new Promise(resolve => setTimeout(resolve, 100)); - } - return id; - }), - ).resolves.toEqual(['1', '3', '5', '2', '4']); + // Same as above, but with 2 being delayed + await expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'] }, + 2: { provides: ['b'], consumes: ['a'] }, + 3: { provides: ['c'], consumes: ['a'] }, + 4: { consumes: ['b'] }, + 5: { consumes: ['c'] }, + }).parallelTopologicalTraversal(async id => { + // When delaying 2 we expect 3 and 5 to complete before 2 and 4 + if (id === '2') { + await new Promise(resolve => setTimeout(resolve, 100)); + } + return id; + }), + ).resolves.toEqual(['1', '3', '5', '2', '4']); + }); - await expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'], consumes: ['a'] }, - }).parallelTopologicalTraversal(async id => id), - ).rejects.toThrow('Circular dependency detected'); - await expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'], consumes: ['b'] }, - 2: { provides: ['b'], consumes: ['a'] }, - }).parallelTopologicalTraversal(async id => id), - ).rejects.toThrow('Circular dependency detected'); - await expect( - DependencyGraph.fromMap({ - 1: { provides: ['a'] }, - 2: { provides: ['c'], consumes: ['a', 'b'] }, - 3: { provides: ['b'], consumes: ['a', 'c'] }, - }).parallelTopologicalTraversal(async id => id), - ).rejects.toThrow('Circular dependency detected'); + it('should detect circular dependencies', async () => { + await expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'], consumes: ['a'] }, + }).parallelTopologicalTraversal(async id => id), + ).rejects.toThrow('Circular dependency detected'); + + await expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'], consumes: ['b'] }, + 2: { provides: ['b'], consumes: ['a'] }, + }).parallelTopologicalTraversal(async id => id), + ).rejects.toThrow('Circular dependency detected'); + + await expect( + DependencyGraph.fromMap({ + 1: { provides: ['a'] }, + 2: { provides: ['c'], consumes: ['a', 'b'] }, + 3: { provides: ['b'], consumes: ['a', 'c'] }, + }).parallelTopologicalTraversal(async id => id), + ).rejects.toThrow('Circular dependency detected'); + }); }); }); From de295fe8a8d7326ae7208a24802c2756e1c6d27d Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 14 Aug 2023 14:01:22 +0200 Subject: [PATCH 15/16] catalog-backend-module-msgraph: fix extension point example Signed-off-by: Patrik Oldsberg --- .../module/catalogModuleIncrementalIngestionEntityProvider.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.ts b/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.ts index 81419d66ec..4e94088ef6 100644 --- a/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.ts +++ b/plugins/catalog-backend-module-incremental-ingestion/src/module/catalogModuleIncrementalIngestionEntityProvider.ts @@ -49,6 +49,7 @@ export interface IncrementalIngestionProviderExtensionPoint { * ```ts * backend.add(createBackendModule({ * pluginId: 'catalog', + * moduleId: 'myIncrementalProvider', * register(env) { * env.registerInit({ * deps: { From 1fb03402eab70fd02e33aa8a70399f0c01faa7b8 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 14 Aug 2023 14:03:24 +0200 Subject: [PATCH 16/16] backend-app-api: make sure DependencyGraph.findUnsatisfiedDeps doesn't break on circular deps Signed-off-by: Patrik Oldsberg --- .../src/lib/DependencyGraph.test.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/backend-app-api/src/lib/DependencyGraph.test.ts b/packages/backend-app-api/src/lib/DependencyGraph.test.ts index 9fd6219ab3..65efac0252 100644 --- a/packages/backend-app-api/src/lib/DependencyGraph.test.ts +++ b/packages/backend-app-api/src/lib/DependencyGraph.test.ts @@ -109,6 +109,29 @@ describe('DependencyGraph', () => { ).toEqual([{ value: '1', unsatisfied: ['a'] }]); }); + it('should handle circular dependencies', async () => { + expect( + DependencyGraph.fromMap({ + 1: { consumes: ['a'], provides: ['a'] }, + }).findUnsatisfiedDeps(), + ).toEqual([]); + + expect( + DependencyGraph.fromMap({ + 1: { consumes: ['a'], provides: ['b'] }, + 2: { consumes: ['b'], provides: ['a'] }, + }).findUnsatisfiedDeps(), + ).toEqual([]); + + expect( + DependencyGraph.fromMap({ + 1: { consumes: ['a'] }, + 2: { consumes: ['b'], provides: ['c'] }, + 3: { consumes: ['c'], provides: ['a', 'b'] }, + }).findUnsatisfiedDeps(), + ).toEqual([]); + }); + it('should find multiple unsatisfied deps for one node', async () => { expect( DependencyGraph.fromMap({