From c44dc6225ffb0eefa55571e3a5c7f7bc0bd2228d Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Mon, 9 Jan 2023 16:04:25 +0100 Subject: [PATCH] backend-common: reimplement middleware using MiddlewareFactory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Co-authored-by: blam Co-authored-by: Johan Haals Signed-off-by: Patrik Oldsberg --- packages/backend-app-api/src/index.ts | 1 + packages/backend-common/package.json | 1 + .../src/middleware/errorHandler.ts | 84 ++----------------- .../src/middleware/notFoundHandler.ts | 13 +-- .../src/middleware/requestLoggingHandler.ts | 18 ++-- yarn.lock | 1 + 6 files changed, 26 insertions(+), 92 deletions(-) diff --git a/packages/backend-app-api/src/index.ts b/packages/backend-app-api/src/index.ts index 02633f3732..a8cc079d47 100644 --- a/packages/backend-app-api/src/index.ts +++ b/packages/backend-app-api/src/index.ts @@ -20,5 +20,6 @@ * @packageDocumentation */ +export * from './lib/http'; export * from './wiring'; export * from './services/implementations'; diff --git a/packages/backend-common/package.json b/packages/backend-common/package.json index aff5e56abb..9c16472263 100644 --- a/packages/backend-common/package.json +++ b/packages/backend-common/package.json @@ -34,6 +34,7 @@ "test:kubernetes": "backstage-cli package test -t KubernetesContainerRunner --no-watch" }, "dependencies": { + "@backstage/backend-app-api": "workspace:^", "@backstage/backend-plugin-api": "workspace:^", "@backstage/cli-common": "workspace:^", "@backstage/config": "workspace:^", diff --git a/packages/backend-common/src/middleware/errorHandler.ts b/packages/backend-common/src/middleware/errorHandler.ts index 04b3f92e26..99ede75ab0 100644 --- a/packages/backend-common/src/middleware/errorHandler.ts +++ b/packages/backend-common/src/middleware/errorHandler.ts @@ -14,19 +14,11 @@ * limitations under the License. */ -import { - AuthenticationError, - ConflictError, - ErrorResponseBody, - InputError, - NotAllowedError, - NotFoundError, - NotModifiedError, - serializeError, -} from '@backstage/errors'; -import { ErrorRequestHandler, NextFunction, Request, Response } from 'express'; +import { ErrorRequestHandler } from 'express'; import { LoggerService } from '@backstage/backend-plugin-api'; import { getRootLogger } from '../logging'; +import { ConfigReader } from '@backstage/config'; +import { MiddlewareFactory } from '@backstage/backend-app-api'; /** * Options passed to the {@link errorHandler} middleware. @@ -73,69 +65,11 @@ export type ErrorHandlerOptions = { export function errorHandler( options: ErrorHandlerOptions = {}, ): ErrorRequestHandler { - const showStackTraces = - options.showStackTraces ?? process.env.NODE_ENV === 'development'; - - const logger = (options.logger || getRootLogger()).child({ - type: 'errorHandler', + return MiddlewareFactory.create({ + config: new ConfigReader({}), + logger: options.logger ?? getRootLogger(), + }).error({ + logAllErrors: options.logClientErrors, + showStackTraces: options.showStackTraces, }); - - return (error: Error, req: Request, res: Response, next: NextFunction) => { - const statusCode = getStatusCode(error); - if (options.logClientErrors || statusCode >= 500) { - logger.error(`Request failed with status ${statusCode}`, error); - } - - if (res.headersSent) { - // If the headers have already been sent, do not send the response again - // as this will throw an error in the backend. - next(error); - return; - } - - const body: ErrorResponseBody = { - error: serializeError(error, { includeStack: showStackTraces }), - request: { method: req.method, url: req.url }, - response: { statusCode }, - }; - - res.status(statusCode).json(body); - }; -} - -function getStatusCode(error: Error): number { - // Look for common http library status codes - const knownStatusCodeFields = ['statusCode', 'status']; - for (const field of knownStatusCodeFields) { - const statusCode = (error as any)[field]; - if ( - typeof statusCode === 'number' && - (statusCode | 0) === statusCode && // is whole integer - statusCode >= 100 && - statusCode <= 599 - ) { - return statusCode; - } - } - - // Handle well-known error types - switch (error.name) { - case NotModifiedError.name: - return 304; - case InputError.name: - return 400; - case AuthenticationError.name: - return 401; - case NotAllowedError.name: - return 403; - case NotFoundError.name: - return 404; - case ConflictError.name: - return 409; - default: - break; - } - - // Fall back to internal server error - return 500; } diff --git a/packages/backend-common/src/middleware/notFoundHandler.ts b/packages/backend-common/src/middleware/notFoundHandler.ts index 2d8b0ef3d0..4f29b4f824 100644 --- a/packages/backend-common/src/middleware/notFoundHandler.ts +++ b/packages/backend-common/src/middleware/notFoundHandler.ts @@ -14,7 +14,10 @@ * limitations under the License. */ -import { NextFunction, Request, RequestHandler, Response } from 'express'; +import { MiddlewareFactory } from '@backstage/backend-app-api'; +import { ConfigReader } from '@backstage/config'; +import { RequestHandler } from 'express'; +import { getRootLogger } from '../logging'; /** * Express middleware to handle requests for missing routes. @@ -26,8 +29,8 @@ import { NextFunction, Request, RequestHandler, Response } from 'express'; * @returns An Express request handler */ export function notFoundHandler(): RequestHandler { - /* eslint-disable @typescript-eslint/no-unused-vars */ - return (_request: Request, response: Response, _next: NextFunction) => { - response.status(404).end(); - }; + return MiddlewareFactory.create({ + config: new ConfigReader({}), + logger: getRootLogger(), + }).notFound(); } diff --git a/packages/backend-common/src/middleware/requestLoggingHandler.ts b/packages/backend-common/src/middleware/requestLoggingHandler.ts index 1618f43ced..a03e3c31f6 100644 --- a/packages/backend-common/src/middleware/requestLoggingHandler.ts +++ b/packages/backend-common/src/middleware/requestLoggingHandler.ts @@ -14,10 +14,11 @@ * limitations under the License. */ +import { MiddlewareFactory } from '@backstage/backend-app-api'; import { RequestHandler } from 'express'; import { LoggerService } from '@backstage/backend-plugin-api'; -import morgan from 'morgan'; import { getRootLogger } from '../logging'; +import { ConfigReader } from '@backstage/config'; /** * Logs incoming requests. @@ -27,15 +28,8 @@ import { getRootLogger } from '../logging'; * @returns An Express request handler */ export function requestLoggingHandler(logger?: LoggerService): RequestHandler { - const actualLogger = (logger || getRootLogger()).child({ - type: 'incomingRequest', - }); - - return morgan('combined', { - stream: { - write(message: string) { - actualLogger.info(message.trimEnd()); - }, - }, - }); + return MiddlewareFactory.create({ + config: new ConfigReader({}), + logger: logger ?? getRootLogger(), + }).logging(); } diff --git a/yarn.lock b/yarn.lock index ed60523406..fe2bc0bcca 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3413,6 +3413,7 @@ __metadata: version: 0.0.0-use.local resolution: "@backstage/backend-common@workspace:packages/backend-common" dependencies: + "@backstage/backend-app-api": "workspace:^" "@backstage/backend-plugin-api": "workspace:^" "@backstage/backend-test-utils": "workspace:^" "@backstage/cli": "workspace:^"