From ad8ed6f91b2972a8374a0a84f0d926e0fd119552 Mon Sep 17 00:00:00 2001 From: Radoslaw Wielonski Date: Mon, 6 Dec 2021 10:48:06 +0100 Subject: [PATCH 01/11] feat: add ability to set custom errorHandler Signed-off-by: Radoslaw Wielonski --- .../service/lib/ServiceBuilderImpl.test.ts | 21 ++++++++++++++++++- .../src/service/lib/ServiceBuilderImpl.ts | 12 ++++++++--- packages/backend-common/src/service/types.ts | 11 +++++++++- 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts b/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts index bd97f444de..1d214d0bff 100644 --- a/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts +++ b/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts @@ -14,7 +14,8 @@ * limitations under the License. */ -import { applyCspDirectives } from './ServiceBuilderImpl'; +import { NextFunction, Request, Response } from 'express'; +import { applyCspDirectives, ServiceBuilderImpl } from './ServiceBuilderImpl'; describe('ServiceBuilderImpl', () => { describe('applyCspDirectives', () => { @@ -33,4 +34,22 @@ describe('ServiceBuilderImpl', () => { expect(result!['upgrade-insecure-requests']).toBeUndefined(); }); }); + + describe('setCustomErrorHandler', () => { + it('adds custom error handler', () => { + const serviceBuilder = new ServiceBuilderImpl(module); + const customErrorHandler = ( + error: Error, + req: Request, + res: Response, + next: NextFunction, + ) => {}; + serviceBuilder.setErrorHandler(customErrorHandler); + expect(serviceBuilder.errorHandler).toEqual(customErrorHandler); + }); + it('use default error handler', () => { + const serviceBuilder = new ServiceBuilderImpl(module); + expect(serviceBuilder.errorHandler).toBeUndefined(); + }); + }); }); diff --git a/packages/backend-common/src/service/lib/ServiceBuilderImpl.ts b/packages/backend-common/src/service/lib/ServiceBuilderImpl.ts index b085cccfd6..8d6404076f 100644 --- a/packages/backend-common/src/service/lib/ServiceBuilderImpl.ts +++ b/packages/backend-common/src/service/lib/ServiceBuilderImpl.ts @@ -17,7 +17,7 @@ import { Config } from '@backstage/config'; import compression from 'compression'; import cors from 'cors'; -import express, { Router } from 'express'; +import express, { Router, ErrorRequestHandler } from 'express'; import helmet from 'helmet'; import * as http from 'http'; import stoppable from 'stoppable'; @@ -25,7 +25,7 @@ import { Logger } from 'winston'; import { useHotCleanup } from '../../hot'; import { getRootLogger } from '../../logging'; import { - errorHandler, + errorHandler as defaultErrorHandler, notFoundHandler, requestLoggingHandler as defaultRequestLoggingHandler, } from '../../middleware'; @@ -66,6 +66,7 @@ export class ServiceBuilderImpl implements ServiceBuilder { private httpsSettings: HttpsSettings | undefined; private routers: [string, Router][]; private requestLoggingHandler: RequestLoggingHandlerFactory | undefined; + private errorHandler: ErrorRequestHandler | undefined; // Reference to the module where builder is created - needed for hot module // reloading private module: NodeModule; @@ -152,6 +153,11 @@ export class ServiceBuilderImpl implements ServiceBuilder { return this; } + setErrorHandler(errorHandler: ErrorRequestHandler) { + this.errorHandler = errorHandler; + return this; + } + async start(): Promise { const app = express(); const { port, host, logger, corsOptions, httpsSettings, helmetOptions } = @@ -169,7 +175,7 @@ export class ServiceBuilderImpl implements ServiceBuilder { app.use(root, route); } app.use(notFoundHandler()); - app.use(errorHandler()); + app.use(this.errorHandler ?? defaultErrorHandler()); const server: http.Server = httpsSettings ? await createHttpsServer(app, httpsSettings, logger) diff --git a/packages/backend-common/src/service/types.ts b/packages/backend-common/src/service/types.ts index 2ad379f31e..37bfec3c43 100644 --- a/packages/backend-common/src/service/types.ts +++ b/packages/backend-common/src/service/types.ts @@ -16,7 +16,7 @@ import { Config } from '@backstage/config'; import cors from 'cors'; -import { Router, RequestHandler } from 'express'; +import { Router, RequestHandler, ErrorRequestHandler } from 'express'; import { Server } from 'http'; import { Logger } from 'winston'; @@ -98,6 +98,15 @@ export type ServiceBuilder = { requestLoggingHandler: RequestLoggingHandlerFactory, ): ServiceBuilder; + /** + * Set the error handler + * + * If no handler is given the default one is used + * + * @param errorHandler - an error handler + */ + setErrorHandler(errorHandler: ErrorRequestHandler): ServiceBuilder; + /** * Starts the server using the given settings. */ From 5a008576c421ba16b6ab19fa29e122470969cfb7 Mon Sep 17 00:00:00 2001 From: Radoslaw Wielonski Date: Mon, 6 Dec 2021 10:56:14 +0100 Subject: [PATCH 02/11] docs: add note to changeset Signed-off-by: Radoslaw Wielonski --- .changeset/hot-toys-grab.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/hot-toys-grab.md diff --git a/.changeset/hot-toys-grab.md b/.changeset/hot-toys-grab.md new file mode 100644 index 0000000000..a2d4faa469 --- /dev/null +++ b/.changeset/hot-toys-grab.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +Add possibility to use custom error handler From f0064d4ee3b118f1d11860ce024be5d2298b43c3 Mon Sep 17 00:00:00 2001 From: Radoslaw Wielonski Date: Mon, 6 Dec 2021 15:40:02 +0100 Subject: [PATCH 03/11] fix: fix tests for custom error handler Signed-off-by: Radoslaw Wielonski --- .../src/service/lib/ServiceBuilderImpl.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts b/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts index 1d214d0bff..665eaa6550 100644 --- a/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts +++ b/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts @@ -43,12 +43,17 @@ describe('ServiceBuilderImpl', () => { req: Request, res: Response, next: NextFunction, - ) => {}; + ) => { + console.log(req, res); + next(error); + }; serviceBuilder.setErrorHandler(customErrorHandler); + // @ts-ignore check private attribute expect(serviceBuilder.errorHandler).toEqual(customErrorHandler); }); it('use default error handler', () => { const serviceBuilder = new ServiceBuilderImpl(module); + // @ts-ignore check private attribute expect(serviceBuilder.errorHandler).toBeUndefined(); }); }); From 5a59f5507e2f6be0d61d1a68488007d6b4522e66 Mon Sep 17 00:00:00 2001 From: Radoslaw Wielonski Date: Tue, 7 Dec 2021 11:35:08 +0100 Subject: [PATCH 04/11] feat: add flag for default error handler Signed-off-by: Radoslaw Wielonski --- .../src/service/lib/ServiceBuilderImpl.test.ts | 3 ++- .../src/service/lib/ServiceBuilderImpl.ts | 16 +++++++++++++++- packages/backend-common/src/service/types.ts | 7 +++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts b/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts index 665eaa6550..10363f9d1b 100644 --- a/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts +++ b/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts @@ -40,11 +40,12 @@ describe('ServiceBuilderImpl', () => { const serviceBuilder = new ServiceBuilderImpl(module); const customErrorHandler = ( error: Error, + // @ts-ignore req: Request, + // @ts-ignore res: Response, next: NextFunction, ) => { - console.log(req, res); next(error); }; serviceBuilder.setErrorHandler(customErrorHandler); diff --git a/packages/backend-common/src/service/lib/ServiceBuilderImpl.ts b/packages/backend-common/src/service/lib/ServiceBuilderImpl.ts index 8d6404076f..54f145539f 100644 --- a/packages/backend-common/src/service/lib/ServiceBuilderImpl.ts +++ b/packages/backend-common/src/service/lib/ServiceBuilderImpl.ts @@ -67,6 +67,7 @@ export class ServiceBuilderImpl implements ServiceBuilder { private routers: [string, Router][]; private requestLoggingHandler: RequestLoggingHandlerFactory | undefined; private errorHandler: ErrorRequestHandler | undefined; + private useDefaultErrorHandler: boolean; // Reference to the module where builder is created - needed for hot module // reloading private module: NodeModule; @@ -74,6 +75,7 @@ export class ServiceBuilderImpl implements ServiceBuilder { constructor(moduleRef: NodeModule) { this.routers = []; this.module = moduleRef; + this.useDefaultErrorHandler = true; } loadConfig(config: Config): ServiceBuilder { @@ -158,6 +160,11 @@ export class ServiceBuilderImpl implements ServiceBuilder { return this; } + disableDefaultErrorHandler() { + this.useDefaultErrorHandler = false; + return this; + } + async start(): Promise { const app = express(); const { port, host, logger, corsOptions, httpsSettings, helmetOptions } = @@ -175,7 +182,14 @@ export class ServiceBuilderImpl implements ServiceBuilder { app.use(root, route); } app.use(notFoundHandler()); - app.use(this.errorHandler ?? defaultErrorHandler()); + + if (this.errorHandler) { + app.use(this.errorHandler); + } + + if (this.useDefaultErrorHandler) { + app.use(defaultErrorHandler()); + } const server: http.Server = httpsSettings ? await createHttpsServer(app, httpsSettings, logger) diff --git a/packages/backend-common/src/service/types.ts b/packages/backend-common/src/service/types.ts index 37bfec3c43..3ec4c7aa0e 100644 --- a/packages/backend-common/src/service/types.ts +++ b/packages/backend-common/src/service/types.ts @@ -107,6 +107,13 @@ export type ServiceBuilder = { */ setErrorHandler(errorHandler: ErrorRequestHandler): ServiceBuilder; + /** + * Disable default error handler + * + * If it's not called, default error handler is used + */ + disableDefaultErrorHandler(): ServiceBuilder; + /** * Starts the server using the given settings. */ From af77b33895ad446820aaefbdd767b261e3aa0055 Mon Sep 17 00:00:00 2001 From: Radoslaw Wielonski Date: Tue, 7 Dec 2021 11:36:01 +0100 Subject: [PATCH 05/11] docs: update api-report.md with error handler changes Signed-off-by: Radoslaw Wielonski --- packages/backend-common/api-report.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/backend-common/api-report.md b/packages/backend-common/api-report.md index 9c66c0573e..01c714b36e 100644 --- a/packages/backend-common/api-report.md +++ b/packages/backend-common/api-report.md @@ -561,6 +561,8 @@ export type ServiceBuilder = { setRequestLoggingHandler( requestLoggingHandler: RequestLoggingHandlerFactory, ): ServiceBuilder; + setErrorHandler(errorHandler: ErrorRequestHandler): ServiceBuilder; + disableDefaultErrorHandler(): ServiceBuilder; start(): Promise; }; From 06d80ac7a657b5fcf152a6e9a6d9673e92440c37 Mon Sep 17 00:00:00 2001 From: Radoslaw Wielonski Date: Tue, 7 Dec 2021 12:02:58 +0100 Subject: [PATCH 06/11] refactor: remove ts-ignore comments from tests Signed-off-by: Radoslaw Wielonski --- .../src/service/lib/ServiceBuilderImpl.test.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts b/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts index 10363f9d1b..853c7d9a9e 100644 --- a/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts +++ b/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts @@ -40,21 +40,17 @@ describe('ServiceBuilderImpl', () => { const serviceBuilder = new ServiceBuilderImpl(module); const customErrorHandler = ( error: Error, - // @ts-ignore - req: Request, - // @ts-ignore - res: Response, + _req: Request, + _res: Response, next: NextFunction, ) => { next(error); }; serviceBuilder.setErrorHandler(customErrorHandler); - // @ts-ignore check private attribute expect(serviceBuilder.errorHandler).toEqual(customErrorHandler); }); it('use default error handler', () => { const serviceBuilder = new ServiceBuilderImpl(module); - // @ts-ignore check private attribute expect(serviceBuilder.errorHandler).toBeUndefined(); }); }); From 6026ebb680ccae8306b7699c8b02564a60e5b37b Mon Sep 17 00:00:00 2001 From: Radoslaw Wielonski Date: Tue, 7 Dec 2021 12:24:14 +0100 Subject: [PATCH 07/11] refactor: use prototype to access private properties and avoid // @ts-ignore Signed-off-by: Radoslaw Wielonski --- .../src/service/lib/ServiceBuilderImpl.test.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts b/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts index 853c7d9a9e..6229cbb683 100644 --- a/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts +++ b/packages/backend-common/src/service/lib/ServiceBuilderImpl.test.ts @@ -36,8 +36,15 @@ describe('ServiceBuilderImpl', () => { }); describe('setCustomErrorHandler', () => { + it('check if custom error handler is undefined', () => { + const serviceBuilder = new ServiceBuilderImpl(module); + const serviceBuilderProto = Object.getPrototypeOf(serviceBuilder); + expect(serviceBuilderProto.errorHandler).toBeUndefined(); + }); + it('adds custom error handler', () => { const serviceBuilder = new ServiceBuilderImpl(module); + const serviceBuilderProto = Object.getPrototypeOf(serviceBuilder); const customErrorHandler = ( error: Error, _req: Request, @@ -46,12 +53,8 @@ describe('ServiceBuilderImpl', () => { ) => { next(error); }; - serviceBuilder.setErrorHandler(customErrorHandler); - expect(serviceBuilder.errorHandler).toEqual(customErrorHandler); - }); - it('use default error handler', () => { - const serviceBuilder = new ServiceBuilderImpl(module); - expect(serviceBuilder.errorHandler).toBeUndefined(); + serviceBuilderProto.setErrorHandler(customErrorHandler); + expect(serviceBuilderProto.errorHandler).toEqual(customErrorHandler); }); }); }); From a07e0f5f06bdf835b0e7139592aa4abaaf3f2026 Mon Sep 17 00:00:00 2001 From: Radoslaw Wielonski Date: Wed, 8 Dec 2021 13:15:25 +0100 Subject: [PATCH 08/11] docs: update documentation of setErrorHandler method Signed-off-by: Radoslaw Wielonski --- packages/backend-common/src/service/types.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/backend-common/src/service/types.ts b/packages/backend-common/src/service/types.ts index 3ec4c7aa0e..07075b4d16 100644 --- a/packages/backend-common/src/service/types.ts +++ b/packages/backend-common/src/service/types.ts @@ -99,9 +99,10 @@ export type ServiceBuilder = { ): ServiceBuilder; /** - * Set the error handler + * Sets an additional errorHandler to run before the defaultErrorHandler. * - * If no handler is given the default one is used + * If we want to use only custom errorHandler without defaultErrorHandler we need to + * disable the defaultErrorHandler by invoking disableDefaultErrorHandler() * * @param errorHandler - an error handler */ From d2ad94df9734340051ef53a9baffda2f1b39b81a Mon Sep 17 00:00:00 2001 From: radoslaw-wielonski-nc <86358570+radoslaw-wielonski-nc@users.noreply.github.com> Date: Wed, 8 Dec 2021 15:07:24 +0100 Subject: [PATCH 09/11] Update packages/backend-common/src/service/types.ts Co-authored-by: Johan Haals Signed-off-by: Radoslaw Wielonski --- packages/backend-common/src/service/types.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/backend-common/src/service/types.ts b/packages/backend-common/src/service/types.ts index 07075b4d16..17ce24c58d 100644 --- a/packages/backend-common/src/service/types.ts +++ b/packages/backend-common/src/service/types.ts @@ -111,7 +111,6 @@ export type ServiceBuilder = { /** * Disable default error handler * - * If it's not called, default error handler is used */ disableDefaultErrorHandler(): ServiceBuilder; From 0105e3cd6447d6b181b0e1362c98efd49b4e0fce Mon Sep 17 00:00:00 2001 From: Radoslaw Wielonski Date: Wed, 8 Dec 2021 15:11:07 +0100 Subject: [PATCH 10/11] docs: update documentation of setErrorHandler method Signed-off-by: Radoslaw Wielonski --- packages/backend-common/src/service/types.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/backend-common/src/service/types.ts b/packages/backend-common/src/service/types.ts index 17ce24c58d..2f2a3e2f4c 100644 --- a/packages/backend-common/src/service/types.ts +++ b/packages/backend-common/src/service/types.ts @@ -101,8 +101,8 @@ export type ServiceBuilder = { /** * Sets an additional errorHandler to run before the defaultErrorHandler. * - * If we want to use only custom errorHandler without defaultErrorHandler we need to - * disable the defaultErrorHandler by invoking disableDefaultErrorHandler() + * For execution of only the custom error handler make sure to also invoke disableDefaultErrorHandler() + * otherwise the defaultErrorHandler is executed at the end of the error middleware chain. * * @param errorHandler - an error handler */ From aec2c96c88ea39e5a1194ea44729ef951b1be287 Mon Sep 17 00:00:00 2001 From: Radoslaw Wielonski Date: Wed, 8 Dec 2021 15:13:37 +0100 Subject: [PATCH 11/11] docs: update documentation of disableDefaultErrorHandler method Signed-off-by: Radoslaw Wielonski --- packages/backend-common/src/service/types.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/backend-common/src/service/types.ts b/packages/backend-common/src/service/types.ts index 2f2a3e2f4c..3e94196006 100644 --- a/packages/backend-common/src/service/types.ts +++ b/packages/backend-common/src/service/types.ts @@ -109,8 +109,7 @@ export type ServiceBuilder = { setErrorHandler(errorHandler: ErrorRequestHandler): ServiceBuilder; /** - * Disable default error handler - * + * Disables the default error handler */ disableDefaultErrorHandler(): ServiceBuilder;