fix(event-handler): run global middleware on all requests for REST API (#4507)

Author: svozza

packages/event-handler/src/rest/Router.ts

@@ -226,33 +226,40 @@ class Router {
 
       const route = this.routeRegistry.resolve(method, path);
 
-      if (route === null) {
-        throw new NotFoundError(`Route ${path} for method ${method} not found`);
-      }
-
-      const handler =
-        options?.scope != null
-          ? route.handler.bind(options.scope)
-          : route.handler;
-
       const handlerMiddleware: Middleware = async (params, reqCtx, next) => {
-        const handlerResult = await handler(params, reqCtx);
-        reqCtx.res = handlerResultToWebResponse(
-          handlerResult,
-          reqCtx.res.headers
-        );
+        if (route === null) {
+          const notFoundRes = await this.handleError(
+            new NotFoundError(`Route ${path} for method ${method} not found`),
+            { ...reqCtx, scope: options?.scope }
+          );
+          reqCtx.res = handlerResultToWebResponse(
+            notFoundRes,
+            reqCtx.res.headers
+          );
+        } else {
+          const handler =
+            options?.scope != null
+              ? route.handler.bind(options.scope)
+              : route.handler;
+
+          const handlerResult = await handler(params, reqCtx);
+          reqCtx.res = handlerResultToWebResponse(
+            handlerResult,
+            reqCtx.res.headers
+          );
+        }
 
         await next();
       };
 
       const middleware = composeMiddleware([
         ...this.middleware,
-        ...route.middleware,
+        ...(route?.middleware ?? []),
         handlerMiddleware,
       ]);
 
       const middlewareResult = await middleware(
-        route.params,
+        route?.params ?? {},
         requestContext,
         () => Promise.resolve()
       );

packages/event-handler/src/rest/converters.ts

@@ -140,7 +140,14 @@ export const handlerResultToWebResponse = (
   resHeaders?: Headers
 ): Response => {
   if (response instanceof Response) {
-    return response;
+    const headers = new Headers(resHeaders);
+    for (const [key, value] of response.headers.entries()) {
+      headers.set(key, value);
+    }
+    return new Response(response.body, {
+      status: response.status,
+      headers,
+    });
   }
 
   const headers = new Headers(resHeaders);

packages/event-handler/tests/unit/rest/Router/decorators.test.ts

@@ -256,29 +256,32 @@ describe('Class: Router - Decorators', () => {
       });
     });
 
-    it('works with notFound decorator', async () => {
+    it('works with notFound decorator and preserves scope', async () => {
       // Prepare
       const app = new Router();
 
       class Lambda {
+        public scope = 'scoped';
+
         @app.notFound()
         public async handleNotFound(error: NotFoundError) {
           return {
             statusCode: HttpErrorCodes.NOT_FOUND,
             error: 'Not Found',
-            message: `Decorated: ${error.message}`,
+            message: `${this.scope}: ${error.message}`,
           };
         }
 
         public async handler(event: unknown, _context: Context) {
-          return app.resolve(event, _context);
+          return app.resolve(event, _context, { scope: this });
         }
       }
 
       const lambda = new Lambda();
+      const handler = lambda.handler.bind(lambda);
 
       // Act
-      const result = await lambda.handler(
+      const result = await handler(
         createTestEvent('/nonexistent', 'GET'),
         context
       );
@@ -289,7 +292,7 @@ describe('Class: Router - Decorators', () => {
         body: JSON.stringify({
           statusCode: HttpErrorCodes.NOT_FOUND,
           error: 'Not Found',
-          message: 'Decorated: Route /nonexistent for method GET not found',
+          message: 'scoped: Route /nonexistent for method GET not found',
         }),
         headers: { 'content-type': 'application/json' },
         isBase64Encoded: false,

packages/event-handler/tests/unit/rest/Router/middleware.test.ts

@@ -421,6 +421,33 @@ describe('Class: Router - Middleware', () => {
       });
     });
 
+    it('headers set by handler before next() take precedence', async () => {
+      // Prepare
+      const app = new Router();
+
+      app.use(async (_params, reqCtx, next) => {
+        reqCtx.res.headers.set('x-before-handler', 'middleware-value');
+        await next();
+      });
+
+      app.get('/handler-precedence', async () => {
+        const response = Response.json({ success: true });
+        response.headers.set('x-before-handler', 'handler-value');
+        return response;
+      });
+
+      // Act
+      const result = await app.resolve(
+        createTestEvent('/handler-precedence', 'GET'),
+        context
+      );
+
+      // Assess
+      expect(result.statusCode).toBe(200);
+      expect(result.headers?.['content-type']).toBe('application/json');
+      expect(result.headers?.['x-before-handler']).toBe('handler-value');
+    });
+
     it('overwrites headers when set later in the middleware stack', async () => {
       // Prepare
       const app = new Router();
@@ -455,6 +482,24 @@ describe('Class: Router - Middleware', () => {
       });
     });
 
+    it('executes global middleware even when route is not found', async () => {
+      // Prepare
+      const app = new Router();
+      const executionOrder: string[] = [];
+
+      const globalMiddleware = createTrackingMiddleware(
+        'global',
+        executionOrder
+      );
+      app.use(globalMiddleware);
+
+      // Act
+      await app.resolve(createTestEvent('/nonexistent', 'GET'), context);
+
+      // Assess
+      expect(executionOrder).toEqual(['global-start', 'global-end']);
+    });
+
     it('works with class decorators and preserves scope access', async () => {
       // Prepare
       const app = new Router();

packages/event-handler/tests/unit/rest/converters.test.ts

@@ -549,17 +549,6 @@ describe('Converters', () => {
   });
 
   describe('handlerResultToResponse', () => {
-    it('returns Response object as-is', () => {
-      // Prepare
-      const response = new Response('Hello', { status: 201 });
-
-      // Act
-      const result = handlerResultToWebResponse(response);
-
-      // Assess
-      expect(result).toBe(response);
-    });
-
     it('converts APIGatewayProxyResult to Response', async () => {
       // Prepare
       const proxyResult = {
@@ -678,5 +667,25 @@ describe('Converters', () => {
       // Assess
       expect(result.headers.get('content-type')).toBe('text/plain');
     });
+
+    it('merges headers from resHeaders with Response object, headers from Response take precedence', () => {
+      // Prepare
+      const response = new Response('Hello', {
+        headers: { 'content-type': 'text/plain' },
+      });
+      const resHeaders = new Headers({
+        'x-custom': 'value',
+        'content-type': 'application/json', // should not override existing
+      });
+
+      // Act
+      const result = handlerResultToWebResponse(response, resHeaders);
+
+      // Assess
+      expect(result.headers.get('content-type')).toBe('text/plain');
+      expect(result.headers.get('x-custom')).toBe('value');
+      expect(result.status).toBe(200);
+      expect(result.text()).resolves.toBe('Hello');
+    });
   });
 });