From 1f384c56443fc553ca6e3f72db009a1a524d1815 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 2 Sep 2022 11:24:17 +0200 Subject: [PATCH 1/4] backend-app-api: nicer error messaging when service deps are missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Signed-off-by: Patrik Oldsberg --- .changeset/tiny-rocks-flash.md | 5 ++++ .../src/wiring/ServiceRegistry.test.ts | 20 ++++++++++++++++ .../src/wiring/ServiceRegistry.ts | 24 ++++++++++++++----- 3 files changed, 43 insertions(+), 6 deletions(-) create mode 100644 .changeset/tiny-rocks-flash.md diff --git a/.changeset/tiny-rocks-flash.md b/.changeset/tiny-rocks-flash.md new file mode 100644 index 0000000000..32b467a6af --- /dev/null +++ b/.changeset/tiny-rocks-flash.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': patch +--- + +Improved error messaging when failing to instantiate services. diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts index d5b688c0de..94ec77eea8 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts @@ -220,4 +220,24 @@ describe('ServiceRegistry', () => { expect(innerFactory).toHaveBeenCalledWith('catalog'); expect(innerFactory).toHaveBeenCalledWith('scaffolder'); }); + + it('should throw if dependencies are not available', async () => { + const myFactory = createServiceFactory({ + service: ref1, + deps: { dep: ref2 }, + async factory({ dep }) { + return async pluginId => { + const d = await dep(pluginId); + return { x: d.x, pluginId }; + }; + }, + }); + + const registry = new ServiceRegistry([myFactory]); + const factory = registry.get(ref1)!; + + await expect(factory('catalog')).rejects.toThrow( + "Failed to instantiate service '1' for 'catalog'. The following dependent services are missing: '2'", + ); + }); }); diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.ts index 9df092db42..f8d99bffd8 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts @@ -56,12 +56,24 @@ export class ServiceRegistry { let implementation = this.#implementations.get(factory); if (!implementation) { - const factoryDeps = Object.fromEntries( - Object.entries(factory.deps).map(([name, serviceRef]) => [ - name, - this.get(serviceRef)!, // TODO: throw - ]), - ); + const missingRefs = new Array>(); + const factoryDeps: { [name in string]: FactoryFunc } = {}; + + for (const [name, serviceRef] of Object.entries(factory.deps)) { + const target = this.get(serviceRef); + if (!target) { + missingRefs.push(serviceRef); + } else { + factoryDeps[name] = target; + } + } + + if (missingRefs.length) { + const missing = missingRefs.map(r => `'${r.id}'`).join(', '); + throw new Error( + `Failed to instantiate service '${ref.id}' for '${pluginId}'. The following dependent services are missing: ${missing}`, + ); + } implementation = { factoryFunc: factory.factory(factoryDeps), From 181c066d2d7c171b96ff3b06a444e217ef051f3b Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 2 Sep 2022 11:44:17 +0200 Subject: [PATCH 2/4] backend-app-api: improved error messaging when service factories throw errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Signed-off-by: Patrik Oldsberg --- packages/backend-app-api/package.json | 1 + .../src/wiring/ServiceRegistry.test.ts | 38 ++++++++++++++++++- .../src/wiring/ServiceRegistry.ts | 29 ++++++++++++-- yarn.lock | 1 + 4 files changed, 65 insertions(+), 4 deletions(-) diff --git a/packages/backend-app-api/package.json b/packages/backend-app-api/package.json index 0ae48d4e9c..f6c2b9f3b9 100644 --- a/packages/backend-app-api/package.json +++ b/packages/backend-app-api/package.json @@ -36,6 +36,7 @@ "@backstage/backend-common": "^0.15.1-next.1", "@backstage/backend-plugin-api": "^0.1.2-next.0", "@backstage/backend-tasks": "^0.3.5-next.0", + "@backstage/errors": "^1.1.0", "@backstage/plugin-permission-node": "^0.6.5-next.1", "express": "^4.17.1", "express-promise-router": "^4.1.0", diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts index 94ec77eea8..c75167ea3b 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts @@ -237,7 +237,43 @@ describe('ServiceRegistry', () => { const factory = registry.get(ref1)!; await expect(factory('catalog')).rejects.toThrow( - "Failed to instantiate service '1' for 'catalog'. The following dependent services are missing: '2'", + "Failed to instantiate service '1' for 'catalog' because the following dependent services are missing: '2'", + ); + }); + + it('should decorate error messages thrown by the top-level factory function', async () => { + const myFactory = createServiceFactory({ + service: ref1, + deps: {}, + factory() { + throw new Error('top-level error'); + }, + }); + + const registry = new ServiceRegistry([myFactory]); + const factory = registry.get(ref1)!; + + await expect(factory('catalog')).rejects.toThrow( + "Failed to instantiate service '1' because the top-level factory function threw an error, Error: top-level error", + ); + }); + + it('should decorate error messages thrown by the plugin-level factory function', async () => { + const myFactory = createServiceFactory({ + service: ref1, + deps: {}, + async factory() { + return pluginId => { + throw new Error(`error in plugin ${pluginId}`); + }; + }, + }); + + const registry = new ServiceRegistry([myFactory]); + const factory = registry.get(ref1)!; + + await expect(factory('catalog')).rejects.toThrow( + "Failed to instantiate service '1' for 'catalog' because the factory function threw an error, Error: error in plugin catalog", ); }); }); diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.ts index f8d99bffd8..fc79bad14a 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts @@ -18,6 +18,7 @@ import { FactoryFunc, ServiceRef, } from '@backstage/backend-plugin-api'; +import { stringifyError } from '@backstage/errors'; export class ServiceRegistry { readonly #providedFactories: Map; @@ -71,12 +72,22 @@ export class ServiceRegistry { if (missingRefs.length) { const missing = missingRefs.map(r => `'${r.id}'`).join(', '); throw new Error( - `Failed to instantiate service '${ref.id}' for '${pluginId}'. The following dependent services are missing: ${missing}`, + `Failed to instantiate service '${ref.id}' for '${pluginId}' because the following dependent services are missing: ${missing}`, ); } implementation = { - factoryFunc: factory.factory(factoryDeps), + factoryFunc: Promise.resolve() + .then(() => factory!.factory(factoryDeps)) + .catch(error => { + throw new Error( + `Failed to instantiate service '${ + ref.id + }' because the top-level factory function threw an error, ${stringifyError( + error, + )}`, + ); + }), byPlugin: new Map(), }; @@ -85,7 +96,19 @@ export class ServiceRegistry { let result = implementation.byPlugin.get(pluginId) as Promise; if (!result) { - result = implementation.factoryFunc.then(func => func(pluginId)); + result = implementation.factoryFunc.then(func => + Promise.resolve() + .then(() => func(pluginId)) + .catch(error => { + throw new Error( + `Failed to instantiate service '${ + ref.id + }' for '${pluginId}' because the factory function threw an error, ${stringifyError( + error, + )}`, + ); + }), + ); implementation.byPlugin.set(pluginId, result); } diff --git a/yarn.lock b/yarn.lock index f0a567bfe3..1313484b21 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2846,6 +2846,7 @@ __metadata: "@backstage/backend-plugin-api": ^0.1.2-next.0 "@backstage/backend-tasks": ^0.3.5-next.0 "@backstage/cli": ^0.19.0-next.1 + "@backstage/errors": ^1.1.0 "@backstage/plugin-permission-node": ^0.6.5-next.1 express: ^4.17.1 express-promise-router: ^4.1.0 From 06b37e817edae72ca15e9dee357f5c0d4ac9066d Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 2 Sep 2022 11:52:00 +0200 Subject: [PATCH 3/4] backend-app-api: improved error messaging when default factory loaders throw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Signed-off-by: Patrik Oldsberg --- .../src/wiring/ServiceRegistry.test.ts | 16 ++++++++++++++++ .../src/wiring/ServiceRegistry.ts | 14 ++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts index c75167ea3b..2c17606a16 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts @@ -276,4 +276,20 @@ describe('ServiceRegistry', () => { "Failed to instantiate service '1' for 'catalog' because the factory function threw an error, Error: error in plugin catalog", ); }); + + it('should decorate error messages thrown by default factory loaders', async () => { + const ref = createServiceRef({ + id: '1', + defaultFactory() { + throw new Error('default factory error'); + }, + }); + + const registry = new ServiceRegistry([]); + const factory = registry.get(ref)!; + + await expect(factory('catalog')).rejects.toThrow( + "Failed to instantiate service '1' because the default factory loader threw an error, Error: default factory error", + ); + }); }); diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.ts index fc79bad14a..5592a0f0eb 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.ts @@ -48,11 +48,21 @@ export class ServiceRegistry { if (!factory) { let loadedFactory = this.#loadedDefaultFactories.get(defaultFactory!); if (!loadedFactory) { - loadedFactory = defaultFactory!(ref) as Promise; + loadedFactory = Promise.resolve().then( + () => defaultFactory!(ref) as Promise, + ); this.#loadedDefaultFactories.set(defaultFactory!, loadedFactory); } // NOTE: This await is safe as long as #providedFactories is not mutated. - factory = await loadedFactory; + factory = await loadedFactory.catch(error => { + throw new Error( + `Failed to instantiate service '${ + ref.id + }' because the default factory loader threw an error, ${stringifyError( + error, + )}`, + ); + }); } let implementation = this.#implementations.get(factory); From 6b730da35cf51a5c97b1059af47161748b6e5736 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Fri, 2 Sep 2022 11:56:10 +0200 Subject: [PATCH 4/4] backstage-app-api: add test for missing transitive service dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Signed-off-by: Patrik Oldsberg --- .../src/wiring/ServiceRegistry.test.ts | 37 ++++++++++++++++--- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts index 2c17606a16..a00f6c0b4b 100644 --- a/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts +++ b/packages/backend-app-api/src/wiring/ServiceRegistry.test.ts @@ -225,11 +225,8 @@ describe('ServiceRegistry', () => { const myFactory = createServiceFactory({ service: ref1, deps: { dep: ref2 }, - async factory({ dep }) { - return async pluginId => { - const d = await dep(pluginId); - return { x: d.x, pluginId }; - }; + async factory() { + throw new Error('ignored'); }, }); @@ -241,6 +238,36 @@ describe('ServiceRegistry', () => { ); }); + it('should throw if dependencies are not available 2', async () => { + const refA = createServiceRef({ id: 'a' }); + const refB = createServiceRef({ id: 'b' }); + const refC = createServiceRef({ id: 'c' }); + const refD = createServiceRef({ id: 'd' }); + + const factoryA = createServiceFactory({ + service: refA, + deps: { b: refB }, + async factory({ b }) { + return async pluginId => b(pluginId); + }, + }); + + const factoryB = createServiceFactory({ + service: refB, + deps: { c: refC, d: refD }, + async factory() { + throw new Error('ignored'); + }, + }); + + const registry = new ServiceRegistry([factoryA, factoryB]); + const factory = registry.get(refA)!; + + await expect(factory('catalog')).rejects.toThrow( + "Failed to instantiate service 'a' for 'catalog' because the factory function threw an error, Error: Failed to instantiate service 'b' for 'catalog' because the following dependent services are missing: 'c', 'd'", + ); + }); + it('should decorate error messages thrown by the top-level factory function', async () => { const myFactory = createServiceFactory({ service: ref1,