From ec547b803291f3c7a698c3a8282b3a27d2119c9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Sat, 21 Dec 2024 15:51:38 +0100 Subject: [PATCH 1/2] add error handler middleware in the plugin router MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/cyan-frogs-count.md | 6 ++++ .changeset/shiny-walls-press.md | 7 +++++ .../httpRouter/httpRouterServiceFactory.ts | 20 +++++++++++-- .../src/module/WrapperProviders.ts | 1 - .../src/router/routes.ts | 11 ------- .../src/service/IncrementalCatalogBuilder.ts | 1 - .../devtools-backend/src/service/router.ts | 6 +--- .../src/routes/resourceRoutes.test.ts | 30 +++++++++---------- plugins/notifications-backend/dev/index.ts | 8 +---- 9 files changed, 48 insertions(+), 42 deletions(-) create mode 100644 .changeset/cyan-frogs-count.md create mode 100644 .changeset/shiny-walls-press.md diff --git a/.changeset/cyan-frogs-count.md b/.changeset/cyan-frogs-count.md new file mode 100644 index 0000000000..b06a07468a --- /dev/null +++ b/.changeset/cyan-frogs-count.md @@ -0,0 +1,6 @@ +--- +'@backstage/plugin-catalog-backend-module-incremental-ingestion': patch +'@backstage/plugin-devtools-backend': patch +--- + +Remove the error handler middleware, since that is now provided by the framework diff --git a/.changeset/shiny-walls-press.md b/.changeset/shiny-walls-press.md new file mode 100644 index 0000000000..aa93779d85 --- /dev/null +++ b/.changeset/shiny-walls-press.md @@ -0,0 +1,7 @@ +--- +'@backstage/backend-defaults': minor +--- + +**BREAKING**: Ensure that an error handler middleware exists at the end of each plugin `httpRouter` handler chain. This makes it so that exceptions thrown by plugin routes are caught and encoded in the standard error format. + +If you were using the standard `MiddlewareFactory` just to put an `error` middleware in you router, you can now remove that at your earliest convenience since it's redundant. If you have custom error handlers in your plugin router, those will continue to function as previously. If you were relying on thrown errors propagating all the way down to the root HTTP router, you will find that they no longer do that, and may want to hoist your error handling up to the plugin level instead. diff --git a/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts index 6f8792aa3d..1f2b989018 100644 --- a/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts @@ -27,6 +27,7 @@ import { createCredentialsBarrier, createAuthIntegrationRouter, } from './http'; +import { MiddlewareFactory } from '../rootHttpRouter'; /** * HTTP route registration for plugins. @@ -47,8 +48,17 @@ export const httpRouterServiceFactory = createServiceFactory({ rootHttpRouter: coreServices.rootHttpRouter, auth: coreServices.auth, httpAuth: coreServices.httpAuth, + logger: coreServices.logger, }, - async factory({ auth, httpAuth, config, plugin, rootHttpRouter, lifecycle }) { + async factory({ + auth, + httpAuth, + config, + plugin, + rootHttpRouter, + lifecycle, + logger, + }) { const router = PromiseRouter(); rootHttpRouter.use(`/api/${plugin.getId()}`, router); @@ -63,9 +73,15 @@ export const httpRouterServiceFactory = createServiceFactory({ router.use(credentialsBarrier.middleware); router.use(createCookieAuthRefreshMiddleware({ auth, httpAuth })); + const pluginRoutes = PromiseRouter(); + router.use(pluginRoutes); + + const middleware = MiddlewareFactory.create({ config, logger }); + router.use(middleware.error()); + return { use(handler: Handler): void { - router.use(handler); + pluginRoutes.use(handler); }, addAuthPolicy(policy: HttpRouterServiceAuthPolicy): void { credentialsBarrier.addAuthPolicy(policy); diff --git a/plugins/catalog-backend-module-incremental-ingestion/src/module/WrapperProviders.ts b/plugins/catalog-backend-module-incremental-ingestion/src/module/WrapperProviders.ts index 2f8a8df3ba..11b9619dc5 100644 --- a/plugins/catalog-backend-module-incremental-ingestion/src/module/WrapperProviders.ts +++ b/plugins/catalog-backend-module-incremental-ingestion/src/module/WrapperProviders.ts @@ -79,7 +79,6 @@ export class WrapperProviders { return new IncrementalProviderRouter( new IncrementalIngestionDatabaseManager({ client: this.options.client }), this.options.logger, - this.options.config, ).createRouter(); } diff --git a/plugins/catalog-backend-module-incremental-ingestion/src/router/routes.ts b/plugins/catalog-backend-module-incremental-ingestion/src/router/routes.ts index ad8d6ce103..ca6f600840 100644 --- a/plugins/catalog-backend-module-incremental-ingestion/src/router/routes.ts +++ b/plugins/catalog-backend-module-incremental-ingestion/src/router/routes.ts @@ -18,22 +18,17 @@ import express from 'express'; import Router from 'express-promise-router'; import { IncrementalIngestionDatabaseManager } from '../database/IncrementalIngestionDatabaseManager'; import { LoggerService } from '@backstage/backend-plugin-api'; -import { MiddlewareFactory } from '@backstage/backend-defaults/rootHttpRouter'; -import { Config } from '@backstage/config'; export class IncrementalProviderRouter { private manager: IncrementalIngestionDatabaseManager; private logger: LoggerService; - private config: Config; constructor( manager: IncrementalIngestionDatabaseManager, logger: LoggerService, - config: Config, ) { this.manager = manager; this.logger = logger; - this.config = config; } createRouter(): express.Router { @@ -253,12 +248,6 @@ export class IncrementalProviderRouter { }, ); - const middleware = MiddlewareFactory.create({ - logger: this.logger, - config: this.config, - }); - router.use(middleware.error()); - return router; } } diff --git a/plugins/catalog-backend-module-incremental-ingestion/src/service/IncrementalCatalogBuilder.ts b/plugins/catalog-backend-module-incremental-ingestion/src/service/IncrementalCatalogBuilder.ts index ed4414b68b..6ea2f10a64 100644 --- a/plugins/catalog-backend-module-incremental-ingestion/src/service/IncrementalCatalogBuilder.ts +++ b/plugins/catalog-backend-module-incremental-ingestion/src/service/IncrementalCatalogBuilder.ts @@ -66,7 +66,6 @@ export class IncrementalCatalogBuilder { const incrementalAdminRouter = await new IncrementalProviderRouter( this.manager, routerLogger, - this.env.config, ).createRouter(); return { incrementalAdminRouter }; diff --git a/plugins/devtools-backend/src/service/router.ts b/plugins/devtools-backend/src/service/router.ts index adcb79c4cd..ad77495b36 100644 --- a/plugins/devtools-backend/src/service/router.ts +++ b/plugins/devtools-backend/src/service/router.ts @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + import { AuthorizeResult } from '@backstage/plugin-permission-common'; import { devToolsConfigReadPermission, @@ -20,7 +21,6 @@ import { devToolsInfoReadPermission, devToolsPermissions, } from '@backstage/plugin-devtools-common'; - import { DevToolsBackendApi } from '../api'; import { NotAllowedError } from '@backstage/errors'; import Router from 'express-promise-router'; @@ -33,7 +33,6 @@ import { PermissionsService, RootConfigService, } from '@backstage/backend-plugin-api'; -import { MiddlewareFactory } from '@backstage/backend-defaults/rootHttpRouter'; /** * @internal @@ -121,8 +120,5 @@ export async function createRouter( response.status(200).json(health); }); - const middleware = MiddlewareFactory.create({ logger, config }); - - router.use(middleware.error()); return router; } diff --git a/plugins/kubernetes-backend/src/routes/resourceRoutes.test.ts b/plugins/kubernetes-backend/src/routes/resourceRoutes.test.ts index 01da602232..7ace17e805 100644 --- a/plugins/kubernetes-backend/src/routes/resourceRoutes.test.ts +++ b/plugins/kubernetes-backend/src/routes/resourceRoutes.test.ts @@ -169,7 +169,7 @@ describe('resourcesRoutes', () => { error: { name: 'InputError', message: 'entity is a required field' }, request: { method: 'POST', - url: '/api/kubernetes/resources/workloads/query', + url: '/resources/workloads/query', }, response: { statusCode: 400 }, }); @@ -193,7 +193,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/workloads/query', + url: '/resources/workloads/query', }, response: { statusCode: 400 }, }); @@ -216,7 +216,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/workloads/query', + url: '/resources/workloads/query', }, response: { statusCode: 400 }, }); @@ -240,7 +240,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/workloads/query', + url: '/resources/workloads/query', }, response: { statusCode: 401 }, }); @@ -264,7 +264,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/workloads/query', + url: '/resources/workloads/query', }, response: { statusCode: 401 }, }); @@ -287,7 +287,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/workloads/query', + url: '/resources/workloads/query', }, response: { statusCode: 500 }, }); @@ -346,7 +346,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/custom/query', + url: '/resources/custom/query', }, response: { statusCode: 400 }, }); @@ -370,7 +370,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/custom/query', + url: '/resources/custom/query', }, response: { statusCode: 400 }, }); @@ -394,7 +394,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/custom/query', + url: '/resources/custom/query', }, response: { statusCode: 400 }, }); @@ -420,7 +420,7 @@ describe('resourcesRoutes', () => { error: { name: 'InputError', message: 'entity is a required field' }, request: { method: 'POST', - url: '/api/kubernetes/resources/custom/query', + url: '/resources/custom/query', }, response: { statusCode: 400 }, }); @@ -451,7 +451,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/custom/query', + url: '/resources/custom/query', }, response: { statusCode: 400 }, }); @@ -481,7 +481,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/custom/query', + url: '/resources/custom/query', }, response: { statusCode: 400 }, }); @@ -512,7 +512,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/custom/query', + url: '/resources/custom/query', }, response: { statusCode: 401 }, }); @@ -543,7 +543,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/custom/query', + url: '/resources/custom/query', }, response: { statusCode: 401 }, }); @@ -573,7 +573,7 @@ describe('resourcesRoutes', () => { }, request: { method: 'POST', - url: '/api/kubernetes/resources/custom/query', + url: '/resources/custom/query', }, response: { statusCode: 500 }, }); diff --git a/plugins/notifications-backend/dev/index.ts b/plugins/notifications-backend/dev/index.ts index f1fe453457..936c31f211 100644 --- a/plugins/notifications-backend/dev/index.ts +++ b/plugins/notifications-backend/dev/index.ts @@ -26,7 +26,6 @@ import { } from '@backstage/plugin-notifications-common'; import express, { Response } from 'express'; import Router from 'express-promise-router'; -import { MiddlewareFactory } from '@backstage/backend-defaults/rootHttpRouter'; const randomSeverity = (): NotificationSeverity => { return notificationSeverities[ @@ -77,12 +76,8 @@ const notificationsDebug = createBackendPlugin({ deps: { notifications: notificationService, httpRouter: coreServices.httpRouter, - config: coreServices.rootConfig, - logger: coreServices.logger, }, - async init({ notifications, httpRouter, config, logger }) { - const middleware = MiddlewareFactory.create({ config, logger }); - + async init({ notifications, httpRouter }) { const router = Router(); router.use(express.json()); router.post('/', async (_, res: Response) => { @@ -100,7 +95,6 @@ const notificationsDebug = createBackendPlugin({ }); res.status(200).send({ status: 'ok' }); }); - router.use(middleware.error()); httpRouter.use(router); httpRouter.addAuthPolicy({ From 46c2f6eda2bd80497f1e3a3842656fb6c331d299 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Sat, 21 Dec 2024 19:08:02 +0100 Subject: [PATCH 2/2] feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/shiny-walls-press.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/shiny-walls-press.md b/.changeset/shiny-walls-press.md index aa93779d85..29fe54afa2 100644 --- a/.changeset/shiny-walls-press.md +++ b/.changeset/shiny-walls-press.md @@ -2,6 +2,6 @@ '@backstage/backend-defaults': minor --- -**BREAKING**: Ensure that an error handler middleware exists at the end of each plugin `httpRouter` handler chain. This makes it so that exceptions thrown by plugin routes are caught and encoded in the standard error format. +Ensure that an error handler middleware exists at the end of each plugin `httpRouter` handler chain. This makes it so that exceptions thrown by plugin routes are caught and encoded in the standard error format. If you were using the standard `MiddlewareFactory` just to put an `error` middleware in you router, you can now remove that at your earliest convenience since it's redundant. If you have custom error handlers in your plugin router, those will continue to function as previously. If you were relying on thrown errors propagating all the way down to the root HTTP router, you will find that they no longer do that, and may want to hoist your error handling up to the plugin level instead.