From cf41eedf435032146b46f9f95db5b584a225d0cd Mon Sep 17 00:00:00 2001 From: Patrick Jungermann Date: Fri, 18 Nov 2022 04:22:25 +0100 Subject: [PATCH] chore(events): apply post-merge comments Introduces a new interface `RequestDetails` to abstract `Request` providing access to request body and headers. **BREAKING:** Replace `request: Request` with `request: RequestDetails` at `RequestValidator`. **BREAKING:** Remove required field `router` at `HttpPostIngressEventPublisher.fromConfig` and replace it with `bind(router: Router)`. Additionally, the path prefix `/http` will be added inside `HttpPostIngressEventPublisher`. Relates-to: PR #13931 Signed-off-by: Patrick Jungermann --- .changeset/fluffy-walls-approve.md | 21 +++++++++++ .changeset/nine-ears-whisper.md | 9 +++++ packages/backend/src/plugins/events.ts | 4 +- plugins/events-backend/README.md | 2 +- plugins/events-backend/api-report.md | 3 +- .../src/service/EventsPlugin.ts | 26 +++++-------- .../HttpPostIngressEventPublisher.test.ts | 37 +++++++------------ .../http/HttpPostIngressEventPublisher.ts | 22 +++++++---- plugins/events-node/api-report.md | 9 ++++- plugins/events-node/package.json | 4 +- .../src/api/http/validation/RequestDetails.ts | 29 +++++++++++++++ .../api/http/validation/RequestValidator.ts | 4 +- .../src/api/http/validation/index.ts | 1 + yarn.lock | 2 - 14 files changed, 112 insertions(+), 61 deletions(-) create mode 100644 .changeset/fluffy-walls-approve.md create mode 100644 .changeset/nine-ears-whisper.md create mode 100644 plugins/events-node/src/api/http/validation/RequestDetails.ts diff --git a/.changeset/fluffy-walls-approve.md b/.changeset/fluffy-walls-approve.md new file mode 100644 index 0000000000..968f5cc402 --- /dev/null +++ b/.changeset/fluffy-walls-approve.md @@ -0,0 +1,21 @@ +--- +'@backstage/plugin-events-backend': minor +--- + +**BREAKING:** Remove required field `router` at `HttpPostIngressEventPublisher.fromConfig` +and replace it with `bind(router: Router)`. +Additionally, the path prefix `/http` will be added inside `HttpPostIngressEventPublisher`. + +```diff +// at packages/backend/src/plugins/events.ts + const eventsRouter = Router(); +- const httpRouter = Router(); +- eventsRouter.use('/http', httpRouter); + + const http = HttpPostIngressEventPublisher.fromConfig({ + config: env.config, + logger: env.logger, +- router: httpRouter, + }); ++ http.bind(eventsRouter); +``` diff --git a/.changeset/nine-ears-whisper.md b/.changeset/nine-ears-whisper.md new file mode 100644 index 0000000000..a1427158af --- /dev/null +++ b/.changeset/nine-ears-whisper.md @@ -0,0 +1,9 @@ +--- +'@backstage/plugin-events-backend': patch +'@backstage/plugin-events-node': minor +--- + +Introduce a new interface `RequestDetails` to abstract `Request` +providing access to request body and headers. + +**BREAKING:** Replace `request: Request` with `request: RequestDetails` at `RequestValidator`. diff --git a/packages/backend/src/plugins/events.ts b/packages/backend/src/plugins/events.ts index 2ada92a5b6..7427d34217 100644 --- a/packages/backend/src/plugins/events.ts +++ b/packages/backend/src/plugins/events.ts @@ -27,14 +27,12 @@ export default async function createPlugin( subscribers: EventSubscriber[], ): Promise { const eventsRouter = Router(); - const httpRouter = Router(); - eventsRouter.use('/http', httpRouter); const http = HttpPostIngressEventPublisher.fromConfig({ config: env.config, logger: env.logger, - router: httpRouter, }); + http.bind(eventsRouter); await new EventsBackend(env.logger) .addPublishers(http) diff --git a/plugins/events-backend/README.md b/plugins/events-backend/README.md index f71df21517..cef8ac7330 100644 --- a/plugins/events-backend/README.md +++ b/plugins/events-backend/README.md @@ -170,8 +170,8 @@ const http = HttpPostIngressEventPublisher.fromConfig({ }, }, logger: env.logger, - router: httpRouter, }); +http.bind(router); await new EventsBackend(env.logger) .addPublishers(http) diff --git a/plugins/events-backend/api-report.md b/plugins/events-backend/api-report.md index 4154168fa1..69d7600ed3 100644 --- a/plugins/events-backend/api-report.md +++ b/plugins/events-backend/api-report.md @@ -33,6 +33,8 @@ export const eventsPlugin: (options?: undefined) => BackendFeature; // @public export class HttpPostIngressEventPublisher implements EventPublisher { + // (undocumented) + bind(router: express.Router): void; // (undocumented) static fromConfig(env: { config: Config; @@ -40,7 +42,6 @@ export class HttpPostIngressEventPublisher implements EventPublisher { [topic: string]: Omit; }; logger: Logger; - router: express.Router; }): HttpPostIngressEventPublisher; // (undocumented) setEventBroker(eventBroker: EventBroker): Promise; diff --git a/plugins/events-backend/src/service/EventsPlugin.ts b/plugins/events-backend/src/service/EventsPlugin.ts index d5a38a7fd7..65e7770d87 100644 --- a/plugins/events-backend/src/service/EventsPlugin.ts +++ b/plugins/events-backend/src/service/EventsPlugin.ts @@ -90,14 +90,11 @@ export const eventsPlugin = createBackendPlugin({ env.registerInit({ deps: { config: configServiceRef, - httpRouter: httpRouterServiceRef, logger: loggerServiceRef, + router: httpRouterServiceRef, }, - async init({ config, httpRouter, logger }) { + async init({ config, logger, router }) { const winstonLogger = loggerToWinstonLogger(logger); - const eventsRouter = Router(); - const router = Router(); - eventsRouter.use('/http', router); const ingresses = Object.fromEntries( extensionPoint.httpPostIngresses.map(ingress => [ @@ -108,23 +105,20 @@ export const eventsPlugin = createBackendPlugin({ const http = HttpPostIngressEventPublisher.fromConfig({ config, - logger: winstonLogger, - router, ingresses, + logger: winstonLogger, }); + const eventsRouter = Router(); + http.bind(eventsRouter); + router.use(eventsRouter); - if (!extensionPoint.eventBroker) { - extensionPoint.setEventBroker(new InMemoryEventBroker(winstonLogger)); - } + const eventBroker = + extensionPoint.eventBroker ?? new InMemoryEventBroker(winstonLogger); - extensionPoint.eventBroker!.subscribe(extensionPoint.subscribers); + eventBroker.subscribe(extensionPoint.subscribers); [extensionPoint.publishers, http] .flat() - .forEach(publisher => - publisher.setEventBroker(extensionPoint.eventBroker!), - ); - - httpRouter.use(eventsRouter); + .forEach(publisher => publisher.setEventBroker(eventBroker)); }, }); }, diff --git a/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.test.ts b/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.test.ts index 3d93e347b3..28f3ddcac4 100644 --- a/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.test.ts +++ b/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { errorHandler, getVoidLogger } from '@backstage/backend-common'; +import { getVoidLogger } from '@backstage/backend-common'; import { ConfigReader } from '@backstage/config'; import { TestEventBroker } from '@backstage/plugin-events-backend-test-utils'; import express from 'express'; @@ -35,37 +35,35 @@ describe('HttpPostIngressEventPublisher', () => { }); const router = Router(); - router.use(express.json()); - router.use(errorHandler()); const app = express().use(router); const publisher = HttpPostIngressEventPublisher.fromConfig({ config, - logger, - router, ingresses: { testB: {}, }, + logger, }); + publisher.bind(router); const eventBroker = new TestEventBroker(); await publisher.setEventBroker(eventBroker); const notFoundResponse = await request(app) - .post('/unknown') + .post('/http/unknown') .timeout(100) .send({ test: 'data' }); expect(notFoundResponse.status).toBe(404); const response1 = await request(app) - .post('/testA') + .post('/http/testA') .set('X-Custom-Header', 'test-value') .timeout(100) .send({ testA: 'data' }); expect(response1.status).toBe(202); const response2 = await request(app) - .post('/testB') + .post('/http/testB') .set('X-Custom-Header', 'test-value') .timeout(100) .send({ testB: 'data' }); @@ -100,14 +98,10 @@ describe('HttpPostIngressEventPublisher', () => { }); const router = Router(); - router.use(express.json()); - router.use(errorHandler()); const app = express().use(router); const publisher = HttpPostIngressEventPublisher.fromConfig({ config, - logger, - router, ingresses: { testB: { validator: async (req, context) => { @@ -148,26 +142,28 @@ describe('HttpPostIngressEventPublisher', () => { }, }, }, + logger, }); + publisher.bind(router); const eventBroker = new TestEventBroker(); await publisher.setEventBroker(eventBroker); const response1 = await request(app) - .post('/testA') + .post('/http/testA') .timeout(100) .send({ test: 'data' }); expect(response1.status).toBe(202); const response2 = await request(app) - .post('/testB') + .post('/http/testB') .timeout(100) .send({ test: 'data' }); expect(response2.status).toBe(400); expect(response2.body).toEqual({ message: 'wrong signature' }); const response3 = await request(app) - .post('/testB') + .post('/http/testB') .set('X-Test-Signature', 'wrong') .timeout(100) .send({ test: 'data' }); @@ -175,21 +171,21 @@ describe('HttpPostIngressEventPublisher', () => { expect(response3.body).toEqual({ message: 'wrong signature' }); const response4 = await request(app) - .post('/testB') + .post('/http/testB') .set('X-Test-Signature', 'testB-signature') .timeout(100) .send({ test: 'data' }); expect(response4.status).toBe(202); const response5 = await request(app) - .post('/testC') + .post('/http/testC') .timeout(100) .send({ test: 'data' }); expect(response5.status).toBe(404); expect(response5.body).toEqual({}); const response6 = await request(app) - .post('/testD') + .post('/http/testD') .timeout(100) .send({ test: 'data' }); expect(response6.status).toBe(403); @@ -210,15 +206,10 @@ describe('HttpPostIngressEventPublisher', () => { it('without configuration', async () => { const config = new ConfigReader({}); - const router = Router(); - router.use(express.json()); - router.use(errorHandler()); - expect(() => HttpPostIngressEventPublisher.fromConfig({ config, logger, - router, }), ).not.toThrow(); }); diff --git a/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.ts b/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.ts index 64b7140b54..b5a6ccbca1 100644 --- a/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.ts +++ b/plugins/events-backend/src/service/http/HttpPostIngressEventPublisher.ts @@ -41,7 +41,6 @@ export class HttpPostIngressEventPublisher implements EventPublisher { config: Config; ingresses?: { [topic: string]: Omit }; logger: Logger; - router: express.Router; }): HttpPostIngressEventPublisher { const topics = env.config.getOptionalStringArray('events.http.topics') ?? []; @@ -55,15 +54,18 @@ export class HttpPostIngressEventPublisher implements EventPublisher { } }); - return new HttpPostIngressEventPublisher(env.logger, env.router, ingresses); + return new HttpPostIngressEventPublisher(env.logger, ingresses); } private constructor( - private logger: Logger, - router: express.Router, - ingresses: { [topic: string]: Omit }, - ) { - router.use(this.createRouter(ingresses)); + private readonly logger: Logger, + private readonly ingresses: { + [topic: string]: Omit; + }, + ) {} + + bind(router: express.Router): void { + router.use('/http', this.createRouter(this.ingresses)); } async setEventBroker(eventBroker: EventBroker): Promise { @@ -92,8 +94,12 @@ export class HttpPostIngressEventPublisher implements EventPublisher { const path = `/${topic}`; router.post(path, async (request, response) => { + const requestDetails = { + body: request.body, + headers: request.headers, + }; const context = new RequestValidationContextImpl(); - await validator?.(request, context); + await validator?.(requestDetails, context); if (context.wasRejected()) { response .status(context.rejectionDetails!.status) diff --git a/plugins/events-node/api-report.md b/plugins/events-node/api-report.md index 1054211cbc..51136fd301 100644 --- a/plugins/events-node/api-report.md +++ b/plugins/events-node/api-report.md @@ -4,7 +4,6 @@ ```ts import { ExtensionPoint } from '@backstage/backend-plugin-api'; -import { Request as Request_2 } from 'express'; // @public export interface EventBroker { @@ -74,6 +73,12 @@ export interface HttpPostIngressOptions { validator?: RequestValidator; } +// @public (undocumented) +export interface RequestDetails { + body: unknown; + headers: Record; +} + // @public export interface RequestRejectionDetails { // (undocumented) @@ -89,7 +94,7 @@ export interface RequestValidationContext { // @public export type RequestValidator = ( - request: Request_2, + request: RequestDetails, context: RequestValidationContext, ) => Promise; diff --git a/plugins/events-node/package.json b/plugins/events-node/package.json index d193f33b00..978037f8ff 100644 --- a/plugins/events-node/package.json +++ b/plugins/events-node/package.json @@ -24,9 +24,7 @@ "postpack": "backstage-cli package postpack" }, "dependencies": { - "@backstage/backend-plugin-api": "workspace:^", - "@types/express": "^4.17.6", - "express": "^4.17.1" + "@backstage/backend-plugin-api": "workspace:^" }, "devDependencies": { "@backstage/cli": "workspace:^" diff --git a/plugins/events-node/src/api/http/validation/RequestDetails.ts b/plugins/events-node/src/api/http/validation/RequestDetails.ts new file mode 100644 index 0000000000..83c2669503 --- /dev/null +++ b/plugins/events-node/src/api/http/validation/RequestDetails.ts @@ -0,0 +1,29 @@ +/* + * Copyright 2022 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. + */ + +/** + * @public + */ +export interface RequestDetails { + /** + * Request body. JSON payloads have been parsed already. + */ + body: unknown; + /** + * Key-value pairs of header names and values. Header names are lower-cased. + */ + headers: Record; +} diff --git a/plugins/events-node/src/api/http/validation/RequestValidator.ts b/plugins/events-node/src/api/http/validation/RequestValidator.ts index b419b855cf..3008db03ae 100644 --- a/plugins/events-node/src/api/http/validation/RequestValidator.ts +++ b/plugins/events-node/src/api/http/validation/RequestValidator.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Request } from 'express'; +import { RequestDetails } from './RequestDetails'; import { RequestValidationContext } from './RequestValidationContext'; /** @@ -29,6 +29,6 @@ import { RequestValidationContext } from './RequestValidationContext'; * @public */ export type RequestValidator = ( - request: Request, + request: RequestDetails, context: RequestValidationContext, ) => Promise; diff --git a/plugins/events-node/src/api/http/validation/index.ts b/plugins/events-node/src/api/http/validation/index.ts index 95f2d474eb..11f2e9f1f6 100644 --- a/plugins/events-node/src/api/http/validation/index.ts +++ b/plugins/events-node/src/api/http/validation/index.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +export type { RequestDetails } from './RequestDetails'; export type { RequestRejectionDetails } from './RequestRejectionDetails'; export type { RequestValidationContext } from './RequestValidationContext'; export type { RequestValidator } from './RequestValidator'; diff --git a/yarn.lock b/yarn.lock index b1531e5db9..b9fdd7f050 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5232,8 +5232,6 @@ __metadata: dependencies: "@backstage/backend-plugin-api": "workspace:^" "@backstage/cli": "workspace:^" - "@types/express": ^4.17.6 - express: ^4.17.1 languageName: unknown linkType: soft