fix: Inject user function error handle middleware chain into framework instead of just the middleware

Author: gregorygaines

Diff for: src/middleware/propagate_error_to_client_error_handle.ts renamed to src/middleware/inject_user_function_error_handle_middleware_chain.ts

@@ -11,7 +11,7 @@
 // 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.
-import {Request, Response, NextFunction, Express} from 'express';
+import {Express} from 'express';
 import {HandlerFunction} from '../functions';
 import {ILayer} from 'express-serve-static-core';
 
@@ -31,72 +31,74 @@ const COMMON_EXPRESS_OBJECT_PROPERTIES = [
 /** The number of parameters on an express error handler. */
 const EXPRESS_ERROR_HANDLE_PARAM_LENGTH = 4;
 
-/** A express app error handle. */
-interface ErrorHandle {
-  // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  (err: Error, req: Request, res: Response, next: NextFunction): any;
-}
-
 /**
- * Express middleware to propagate framework errors to the user function error handle.
- * This enables users to handle framework errors that would otherwise be handled by the
- * default Express error handle. If the user function doesn't have an error handle,
- * it falls back to the default Express error handle.
+ * Injects the user function error handle middleware and its subsequent middleware
+ * chain into the framework. This enables users to handle framework errors that would
+ * otherwise be handled by the default Express error handle.
+ * @param frameworkApp - Framework app
  * @param userFunction - User handler function
  */
-export const createPropagateErrorToClientErrorHandleMiddleware = (
+export const injectUserFunctionErrorHandleMiddlewareChain = (
+  frameworkApp: Express,
   userFunction: HandlerFunction
-): ErrorHandle => {
-  const userFunctionErrorHandle =
-    getFirstUserFunctionErrorHandleMiddleware(userFunction);
+) => {
+  // Check if user function is an express app that can register middleware.
+  if (!isExpressApp(userFunction)) {
+    return;
+  }
+
+  // Get the index of the user's first error handle middleware.
+  const firstErrorHandleMiddlewareIndex =
+    getFirstUserFunctionErrorHandleMiddlewareIndex(userFunction);
+  if (!firstErrorHandleMiddlewareIndex) {
+    return;
+  }
 
-  return function (
-    err: Error,
-    req: Request,
-    res: Response,
-    next: NextFunction
+  // Inject their error handle middleware chain into the framework app.
+  const middlewares = (userFunction as Express)._router.stack;
+  for (
+    let index = firstErrorHandleMiddlewareIndex;
+    index < middlewares.length;
+    index++
   ) {
-    // Propagate error to user function error handle.
-    if (userFunctionErrorHandle) {
-      return userFunctionErrorHandle(err, req, res, next);
+    const middleware = middlewares[index];
+
+    // We don't care about routes.
+    if (middleware.route) {
+      continue;
     }
 
-    // Propagate error to default Express error handle.
-    return next();
-  };
+    frameworkApp.use(middleware.handle);
+  }
 };
 
 /**
- * Returns the first user handler function defined error handle, if available.
- * @param userFunction - User handler function
+ * Returns if the user function contains common properties of an Express app.
+ * @param userFunction
  */
-const getFirstUserFunctionErrorHandleMiddleware = (
-  userFunction: HandlerFunction
-): ErrorHandle | null => {
-  if (!isExpressApp(userFunction)) {
-    return null;
-  }
+const isExpressApp = (userFunction: HandlerFunction): boolean => {
+  const userFunctionProperties = Object.getOwnPropertyNames(userFunction);
+  return COMMON_EXPRESS_OBJECT_PROPERTIES.every(prop =>
+    userFunctionProperties.includes(prop)
+  );
+};
 
+/**
+ * Returns the index of the first error handle middleware in the user function.
+ */
+const getFirstUserFunctionErrorHandleMiddlewareIndex = (
+  userFunction: HandlerFunction
+): number | null => {
   const middlewares: ILayer[] = (userFunction as Express)._router.stack;
-  for (const middleware of middlewares) {
+  for (let index = 0; index < middlewares.length; index++) {
+    const middleware = middlewares[index];
     if (
       middleware.handle &&
       middleware.handle.length === EXPRESS_ERROR_HANDLE_PARAM_LENGTH
     ) {
-      return middleware.handle as unknown as ErrorHandle;
+      return index;
     }
   }
 
   return null;
 };
-
-/**
- * Returns if the user function contains common properties of an Express app.
- * @param userFunction
- */
-const isExpressApp = (userFunction: HandlerFunction): boolean => {
-  const userFunctionProperties = Object.getOwnPropertyNames(userFunction);
-  return COMMON_EXPRESS_OBJECT_PROPERTIES.every(prop =>
-    userFunctionProperties.includes(prop)
-  );
-};

Diff for: src/server.ts

@@ -26,7 +26,7 @@ import {wrapUserFunction} from './function_wrappers';
 import {asyncLocalStorageMiddleware} from './async_local_storage';
 import {executionContextMiddleware} from './execution_context';
 import {FrameworkOptions} from './options';
-import {createPropagateErrorToClientErrorHandleMiddleware} from './middleware/propagate_error_to_client_error_handle';
+import {injectUserFunctionErrorHandleMiddlewareChain} from './middleware/inject_user_function_error_handle_middleware_chain';
 
 /**
  * Creates and configures an Express application and returns an HTTP server
@@ -174,9 +174,7 @@ export function getServer(
   }
 
   if (options.propagateFrameworkErrors) {
-    const errorHandleMiddleware =
-      createPropagateErrorToClientErrorHandleMiddleware(userFunction);
-    app.use(errorHandleMiddleware);
+    injectUserFunctionErrorHandleMiddlewareChain(app, userFunction);
   }
 
   return http.createServer(app);

Diff for: test/middleware/inject_user_function_error_handle_middleware_chain.ts

@@ -0,0 +1,77 @@
+import * as assert from 'assert';
+import {NextFunction} from 'express';
+import {Request, Response} from '../../src';
+import * as express from 'express';
+import {injectUserFunctionErrorHandleMiddlewareChain} from '../../src/middleware/inject_user_function_error_handle_middleware_chain';
+import {ILayer} from 'express-serve-static-core';
+
+describe('injectUserFunctionErrorHandleMiddlewareChain', () => {
+  it('user app with error handle middleware injects into framework app', () => {
+    const frameworkApp = express();
+    const userApp = express();
+    userApp.use(appBErrorHandle);
+    function appBErrorHandle(
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _err: Error,
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _req: Request,
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _res: Response,
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _next: NextFunction
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    ): any {}
+    userApp.use(appBFollowUpMiddleware);
+    function appBFollowUpMiddleware(
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _req: Request,
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _res: Response,
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _next: NextFunction
+    ) {}
+
+    injectUserFunctionErrorHandleMiddlewareChain(frameworkApp, userApp);
+
+    const appAMiddleware = frameworkApp._router.stack;
+    const appAMiddlewareNames = appAMiddleware.map(
+      (middleware: ILayer) => middleware.name
+    );
+
+    assert.deepStrictEqual(appAMiddlewareNames, [
+      'query',
+      'expressInit',
+      'appBErrorHandle',
+      'appBFollowUpMiddleware',
+    ]);
+  });
+
+  it('user app without error handle not injected into framework app', () => {
+    const frameworkApp = express();
+    const userApp = express();
+    userApp.use(appBFollowUpMiddleware);
+    function appBFollowUpMiddleware(
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _req: Request,
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _res: Response,
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _next: NextFunction
+    ) {}
+
+    injectUserFunctionErrorHandleMiddlewareChain(frameworkApp, userApp);
+
+    assert.strictEqual('_router' in frameworkApp, false);
+  });
+
+  it('non-express user app not injected into framework app', () => {
+    const frameworkApp = express();
+    const userApp = (_req: Request, res: Response) => {
+      res.send('Hello, World!');
+    };
+
+    injectUserFunctionErrorHandleMiddlewareChain(frameworkApp, userApp);
+
+    assert.strictEqual('_router' in frameworkApp, false);
+  });
+});