From c33308a72b05181e5acc17c63f6688c5a2a3bebb Mon Sep 17 00:00:00 2001 From: Gregory Gaines Date: Sun, 30 Mar 2025 00:51:49 -0400 Subject: [PATCH 1/8] feat: create propagate error to client error handle middleware --- .../propagate_error_to_client_error_handle.ts | 101 ++++++++++++++++++ .../propagate_error_to_client_error_handle.ts | 68 ++++++++++++ 2 files changed, 169 insertions(+) create mode 100644 src/middleware/propagate_error_to_client_error_handle.ts create mode 100644 test/middleware/propagate_error_to_client_error_handle.ts diff --git a/src/middleware/propagate_error_to_client_error_handle.ts b/src/middleware/propagate_error_to_client_error_handle.ts new file mode 100644 index 00000000..74101793 --- /dev/null +++ b/src/middleware/propagate_error_to_client_error_handle.ts @@ -0,0 +1,101 @@ +// Copyright 2019 Google LLC +// +// 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. +import {Request, Response, NextFunction, Express} from 'express'; +import {HandlerFunction} from '../functions'; +import {ILayer} from 'express-serve-static-core'; + +/** + * Common properties that exists on Express object instances. Extracted by calling + * `Object.getOwnPropertyNames` on an express instance. + */ +const COMMON_EXPRESS_OBJECT_PROPERTIES = [ + '_router', + 'use', + 'get', + 'post', + 'put', + 'delete', +]; + +/** The number of parameters on an express error handler. */ +const EXPRESS_ERROR_HANDLE_PARAM_LENGTH = 4; + +/** A express app error handle. */ +interface ErrorHandle { + (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. + * @param userFunction - User handler function + */ +export const createPropagateErrorToClientErrorHandleMiddleware = ( + userFunction: HandlerFunction +): ErrorHandle => { + const userFunctionErrorHandle = + getFirstUserFunctionErrorHandleMiddleware(userFunction); + + return function ( + err: Error, + req: Request, + res: Response, + next: NextFunction + ) { + // Propagate error to user function error handle. + if (userFunctionErrorHandle) { + return userFunctionErrorHandle(err, req, res, next); + } + + // Propagate error to default Express error handle. + return next(); + }; +}; + +/** + * Returns the first user handler function defined error handle, if available. + * @param userFunction - User handler function + */ +const getFirstUserFunctionErrorHandleMiddleware = ( + userFunction: HandlerFunction +): ErrorHandle | null => { + if (!isExpressApp(userFunction)) { + return null; + } + + const middlewares: ILayer[] = (userFunction as Express)._router.stack; + for (const middleware of middlewares) { + if ( + middleware.handle && + middleware.handle.length === EXPRESS_ERROR_HANDLE_PARAM_LENGTH + ) { + return middleware.handle as unknown as ErrorHandle; + } + } + + 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 --git a/test/middleware/propagate_error_to_client_error_handle.ts b/test/middleware/propagate_error_to_client_error_handle.ts new file mode 100644 index 00000000..81d1074e --- /dev/null +++ b/test/middleware/propagate_error_to_client_error_handle.ts @@ -0,0 +1,68 @@ +import * as assert from 'assert'; +import * as sinon from 'sinon'; +import {NextFunction} from 'express'; +import {Request, Response} from '../../src'; +import {createPropagateErrorToClientErrorHandleMiddleware} from '../../src/middleware/propagate_error_to_client_error_handle'; +import * as express from 'express'; + +describe('propagateErrorToClientErrorHandleMiddleware', () => { + let request: Request; + let response: Response; + let next: NextFunction; + let errListener = () => {}; + let error: Error; + beforeEach(() => { + error = Error('Something went wrong!'); + request = { + setTimeout: sinon.spy(), + on: sinon.spy(), + } as unknown as Request; + response = { + on: sinon.spy(), + } as unknown as Response; + next = sinon.spy(); + errListener = sinon.spy(); + }); + + it('user express app with error handle calls user function app error handle', () => { + const app = express(); + app.use( + ( + _err: Error, + _req: Request, + _res: Response, + _next: NextFunction + ): any => { + errListener(); + } + ); + + const middleware = createPropagateErrorToClientErrorHandleMiddleware(app); + middleware(error, request, response, next); + + assert.strictEqual((errListener as sinon.SinonSpy).called, true); + assert.strictEqual((next as sinon.SinonSpy).called, false); + }); + + it('user express app without error handle calls default express error handle', () => { + const app = express(); + + const middleware = createPropagateErrorToClientErrorHandleMiddleware(app); + middleware(error, request, response, next); + + assert.strictEqual((errListener as sinon.SinonSpy).called, false); + assert.strictEqual((next as sinon.SinonSpy).called, true); + }); + + it('non-express user app calls default express error handle', () => { + const app = (_req: Request, res: Response) => { + res.send('Hello, World!'); + }; + + const middleware = createPropagateErrorToClientErrorHandleMiddleware(app); + middleware(error, request, response, next); + + assert.strictEqual((errListener as sinon.SinonSpy).called, false); + assert.strictEqual((next as sinon.SinonSpy).called, true); + }); +}); From 9ad6a088b7f4bce9464c313864a16623278a3313 Mon Sep 17 00:00:00 2001 From: Gregory Gaines Date: Sun, 30 Mar 2025 01:26:34 -0400 Subject: [PATCH 2/8] feat: add propagate errors to client cli option --- src/options.ts | 23 +++++++++++++++++++- src/testing.ts | 1 + test/integration/legacy_event.ts | 1 + test/options.ts | 37 ++++++++++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 1 deletion(-) diff --git a/src/options.ts b/src/options.ts index a0e9c89f..a3719f50 100644 --- a/src/options.ts +++ b/src/options.ts @@ -15,7 +15,7 @@ import * as minimist from 'minimist'; import * as semver from 'semver'; import {resolve} from 'path'; -import {SignatureType, isValidSignatureType} from './types'; +import {isValidSignatureType, SignatureType} from './types'; /** * Error thrown when an invalid option is provided. @@ -60,6 +60,10 @@ export interface FrameworkOptions { * Routes that should return a 404 without invoking the function. */ ignoredRoutes: string | null; + /** + * Whether or not to propagate framework errors to the client. + */ + propagateFrameworkErrors: boolean; } /** @@ -167,6 +171,18 @@ const ExecutionIdOption = new ConfigurableOption( } ); +const PropagateFrameworkErrorsOption = new ConfigurableOption( + 'propagate-framework-errors', + 'PROPAGATE_FRAMEWORK_ERRORS', + false, + x => { + return ( + (typeof x === 'boolean' && x) || + (typeof x === 'string' && x.toLowerCase() === 'true') + ); + } +); + export const helpText = `Example usage: functions-framework --target=helloWorld --port=8080 Documentation: @@ -191,6 +207,7 @@ export const parseOptions = ( SourceLocationOption.cliOption, TimeoutOption.cliOption, IgnoredRoutesOption.cliOption, + PropagateFrameworkErrorsOption.cliOption, ], }); return { @@ -202,5 +219,9 @@ export const parseOptions = ( printHelp: cliArgs[2] === '-h' || cliArgs[2] === '--help', enableExecutionId: ExecutionIdOption.parse(argv, envVars), ignoredRoutes: IgnoredRoutesOption.parse(argv, envVars), + propagateFrameworkErrors: PropagateFrameworkErrorsOption.parse( + argv, + envVars + ), }; }; diff --git a/src/testing.ts b/src/testing.ts index 2fda93c2..1e4298b1 100644 --- a/src/testing.ts +++ b/src/testing.ts @@ -57,5 +57,6 @@ export const getTestServer = (functionName: string): Server => { sourceLocation: '', printHelp: false, ignoredRoutes: null, + propagateFrameworkErrors: false, }); }; diff --git a/test/integration/legacy_event.ts b/test/integration/legacy_event.ts index 231d0f62..3a1fd72d 100644 --- a/test/integration/legacy_event.ts +++ b/test/integration/legacy_event.ts @@ -41,6 +41,7 @@ const testOptions = { sourceLocation: '', printHelp: false, ignoredRoutes: null, + propagateFrameworkErrors: false, }; describe('Event Function', () => { diff --git a/test/options.ts b/test/options.ts index b6279485..cab3a7a4 100644 --- a/test/options.ts +++ b/test/options.ts @@ -61,6 +61,7 @@ describe('parseOptions', () => { enableExecutionId: false, timeoutMilliseconds: 0, ignoredRoutes: null, + propagateFrameworkErrors: false, }, }, { @@ -77,6 +78,7 @@ describe('parseOptions', () => { '--timeout', '6', '--ignored-routes=banana', + '--propagate-framework-errors=true', ], envVars: {}, expectedOptions: { @@ -88,6 +90,7 @@ describe('parseOptions', () => { enableExecutionId: false, timeoutMilliseconds: 6000, ignoredRoutes: 'banana', + propagateFrameworkErrors: true, }, }, { @@ -100,6 +103,7 @@ describe('parseOptions', () => { FUNCTION_SOURCE: '/source', CLOUD_RUN_TIMEOUT_SECONDS: '2', IGNORED_ROUTES: '', + PROPAGATE_FRAMEWORK_ERRORS: 'true', }, expectedOptions: { port: '1234', @@ -110,6 +114,7 @@ describe('parseOptions', () => { enableExecutionId: false, timeoutMilliseconds: 2000, ignoredRoutes: '', + propagateFrameworkErrors: true, }, }, { @@ -125,6 +130,7 @@ describe('parseOptions', () => { '--source=/source', '--timeout=3', '--ignored-routes=avocado', + '--propagate-framework-errors', ], envVars: { PORT: '4567', @@ -133,6 +139,7 @@ describe('parseOptions', () => { FUNCTION_SOURCE: '/somewhere/else', CLOUD_RUN_TIMEOUT_SECONDS: '5', IGNORED_ROUTES: 'banana', + PROPAGATE_FRAMEWORK_ERRORS: 'false', }, expectedOptions: { port: '1234', @@ -143,6 +150,7 @@ describe('parseOptions', () => { enableExecutionId: false, timeoutMilliseconds: 3000, ignoredRoutes: 'avocado', + propagateFrameworkErrors: false, }, }, ]; @@ -236,4 +244,33 @@ describe('parseOptions', () => { }); }); }); + + it('default disable propagate framework errors', () => { + const options = parseOptions(['bin/node', '/index.js'], {}); + assert.strictEqual(options.propagateFrameworkErrors, false); + }); + + it('disable propagate framework errors by cli flag', () => { + const options = parseOptions( + ['bin/node', '/index.js', '--propagate-framework-errors=false'], + {} + ); + assert.strictEqual(options.propagateFrameworkErrors, false); + }); + + it('enable propagate framework errors by cli flag', () => { + const options = parseOptions( + ['bin/node', '/index.js', '--propagate-framework-errors=true'], + {} + ); + assert.strictEqual(options.propagateFrameworkErrors, true); + }); + + it('disable propagate framework errors by env var', () => { + const envVars = { + PROPAGATE_FRAMEWORK_ERRORS: 'False', + }; + const options = parseOptions(cliOpts, envVars); + assert.strictEqual(options.propagateFrameworkErrors, false); + }); }); From e20f8237405289c8336c09557f6b3e853d7d1c33 Mon Sep 17 00:00:00 2001 From: Gregory Gaines Date: Sun, 30 Mar 2025 01:28:29 -0400 Subject: [PATCH 3/8] fix: Fix gts errors --- src/middleware/propagate_error_to_client_error_handle.ts | 1 + test/middleware/propagate_error_to_client_error_handle.ts | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/middleware/propagate_error_to_client_error_handle.ts b/src/middleware/propagate_error_to_client_error_handle.ts index 74101793..a4cfc213 100644 --- a/src/middleware/propagate_error_to_client_error_handle.ts +++ b/src/middleware/propagate_error_to_client_error_handle.ts @@ -33,6 +33,7 @@ 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; } diff --git a/test/middleware/propagate_error_to_client_error_handle.ts b/test/middleware/propagate_error_to_client_error_handle.ts index 81d1074e..3f6254d3 100644 --- a/test/middleware/propagate_error_to_client_error_handle.ts +++ b/test/middleware/propagate_error_to_client_error_handle.ts @@ -28,10 +28,15 @@ describe('propagateErrorToClientErrorHandleMiddleware', () => { const app = express(); app.use( ( + // 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 => { errListener(); } From 2fc508c2bbb9567375e2d39e3f1d24bc5a204109 Mon Sep 17 00:00:00 2001 From: Gregory Gaines Date: Sun, 30 Mar 2025 02:41:32 -0400 Subject: [PATCH 4/8] feat: propagate framework errors to client when option enabled --- src/server.ts | 7 ++++ test/integration/http.ts | 87 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/src/server.ts b/src/server.ts index 9a9b3acc..87197afc 100644 --- a/src/server.ts +++ b/src/server.ts @@ -26,6 +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'; /** * Creates and configures an Express application and returns an HTTP server @@ -172,5 +173,11 @@ export function getServer( app.post('/*', requestHandler); } + if (options.propagateFrameworkErrors) { + const errorHandleMiddleware = + createPropagateErrorToClientErrorHandleMiddleware(userFunction); + app.use(errorHandleMiddleware); + } + return http.createServer(app); } diff --git a/test/integration/http.ts b/test/integration/http.ts index b510fbaa..0954e145 100644 --- a/test/integration/http.ts +++ b/test/integration/http.ts @@ -18,6 +18,9 @@ import * as supertest from 'supertest'; import * as functions from '../../src/index'; import {getTestServer} from '../../src/testing'; +import {Request, Response, NextFunction} from 'express'; +import * as express from 'express'; +import {getServer} from '../../src/server'; describe('HTTP Function', () => { let callCount = 0; @@ -111,4 +114,88 @@ describe('HTTP Function', () => { assert.strictEqual(callCount, test.expectedCallCount); }); }); + + it('default error handler', async () => { + const app = express(); + app.post('/foo', async (req, res) => { + res.send('Foo!'); + }); + app.use( + // eslint-disable-next-line @typescript-eslint/no-unused-vars + (_err: Error, _req: Request, res: Response, _next: NextFunction) => { + res.status(500).send('Caught error!'); + } + ); + functions.http('testHttpFunction', app); + const malformedBody = '{"key": "value", "anotherKey": }'; + const st = supertest( + getServer(app, { + port: '', + target: '', + sourceLocation: '', + signatureType: 'http', + printHelp: false, + enableExecutionId: false, + timeoutMilliseconds: 0, + ignoredRoutes: null, + propagateFrameworkErrors: false, + }) + ); + const resBody = + '\n' + + '\n' + + '\n' + + '\n' + + 'Error\n' + + '\n' + + '\n' + + '
SyntaxError: Unexpected token '}', ..."therKey": }" is not valid JSON
   at JSON.parse (<anonymous>)
   at parse (/Users/gregei/IdeaProjects/functions-framework-nodejs/node_modules/body-parser/lib/types/json.js:92:19)
   at /Users/gregei/IdeaProjects/functions-framework-nodejs/node_modules/body-parser/lib/read.js:128:18
   at AsyncResource.runInAsyncScope (node:async_hooks:211:14)
   at invokeCallback (/Users/gregei/IdeaProjects/functions-framework-nodejs/node_modules/raw-body/index.js:238:16)
   at done (/Users/gregei/IdeaProjects/functions-framework-nodejs/node_modules/raw-body/index.js:227:7)
   at IncomingMessage.onEnd (/Users/gregei/IdeaProjects/functions-framework-nodejs/node_modules/raw-body/index.js:287:7)
   at IncomingMessage.emit (node:events:518:28)
   at IncomingMessage.emit (node:domain:552:15)
   at endReadableNT (node:internal/streams/readable:1698:12)
   at process.processTicksAndRejections (node:internal/process/task_queues:90:21)
\n' + + '\n' + + '\n'; + + const response = await st + .post('/foo') + .set('Content-Type', 'application/json') + .send(malformedBody); + + assert.strictEqual(response.status, 400); + assert.equal(response.text, resBody); + }); + + it('user application error handler', async () => { + const app = express(); + const resBody = 'Caught error!'; + app.post('/foo', async (req, res) => { + res.send('Foo!'); + }); + app.use( + // eslint-disable-next-line @typescript-eslint/no-unused-vars + (_err: Error, _req: Request, res: Response, _next: NextFunction) => { + res.status(500).send(resBody); + } + ); + functions.http('testHttpFunction', app); + const malformedBody = '{"key": "value", "anotherKey": }'; + const st = supertest( + getServer(app, { + port: '', + target: '', + sourceLocation: '', + signatureType: 'http', + printHelp: false, + enableExecutionId: false, + timeoutMilliseconds: 0, + ignoredRoutes: null, + propagateFrameworkErrors: true, + }) + ); + + const response = await st + .post('/foo') + .set('Content-Type', 'application/json') + .send(malformedBody); + + assert.strictEqual(response.status, 500); + assert.equal(response.text, resBody); + }); }); From ed2668ecbe9a21f37e9bbd998156890b306da6a5 Mon Sep 17 00:00:00 2001 From: Gregory Gaines Date: Sun, 30 Mar 2025 02:48:07 -0400 Subject: [PATCH 5/8] chore: update readme to contain propagate framework errors cli option --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 88144492..f5f7d245 100644 --- a/README.md +++ b/README.md @@ -183,6 +183,7 @@ ignored. | `--source` | `FUNCTION_SOURCE` | The path to the directory of your function. Default: `cwd` (the current working directory) | | `--log-execution-id`| `LOG_EXECUTION_ID` | Enables execution IDs in logs, either `true` or `false`. When not specified, default to disable. Requires Node.js 13.0.0 or later. | | `--ignored-routes`| `IGNORED_ROUTES` | A route expression for requests that should not be routed the function. An empty 404 response will be returned. This is set to `/favicon.ico|/robots.txt` by default for `http` functions. | +| `--propagate-framework-errors` | `PROPAGATE_FRAMEWORK_ERRORS` | Enables propagating framework errors to the client application error handler, either `true` or `false`. When not specified, default to disable.| You can set command-line flags in your `package.json` via the `start` script. For example: From eb6184204e4c57c1dcdb4d693687535cf6c45acb Mon Sep 17 00:00:00 2001 From: Gregory Gaines Date: Sun, 30 Mar 2025 03:51:51 -0400 Subject: [PATCH 6/8] chore: Add docs for propagating framework errors --- docs/README.md | 1 + docs/propagate-internal-framework-errors.md | 81 +++++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 docs/propagate-internal-framework-errors.md diff --git a/docs/README.md b/docs/README.md index af449ae4..8fae5673 100644 --- a/docs/README.md +++ b/docs/README.md @@ -9,6 +9,7 @@ This directory contains advanced docs around the Functions Framework. - [Running and Deploying Docker Containers](docker.md) - [Writing a Function in Typescript](typescript.md) - [ES Modules](esm/README.md) +- [Propagate internal framework errors](propagate-internal-framework-errors.md) ## Generated Docs diff --git a/docs/propagate-internal-framework-errors.md b/docs/propagate-internal-framework-errors.md new file mode 100644 index 00000000..925c08fa --- /dev/null +++ b/docs/propagate-internal-framework-errors.md @@ -0,0 +1,81 @@ +# Propagate Internal Framework Errors + +The Functions Framework normally sends express level errors to the default express error handler which sends the error to the calling client with an optional stack trace if in a non-prod environment. + +## Example + +```ts +const app = express(); + +app.post("/", (req, res) => { +... +}); + +// User error handler +app.use((err, req, res, next) => { + logger.log(err); + res.send("Caught error!"); +}); + +functions.http("helloWorld, app); +``` + +```ts +// Post request with bad JSON +http.post("/", "{"id": "Hello}"); +``` + +Default express error handler: + +``` +SyntaxError: Expected double-quoted property name in JSON at position 20 (line 3 column 1) + at JSON.parse () + at parse (functions-framework-nodejs/node_modules/body-parser/lib/types/json.js:92:19) + at functions-framework-nodejs/node_modules/body-parser/lib/read.js:128:18 + at AsyncResource.runInAsyncScope (node:async_hooks:211:14) + at invokeCallback (functions-framework-nodejs/node_modules/raw-body/index.js:238:16) + at done (functions-framework-nodejs/node_modules/raw-body/index.js:227:7) + at IncomingMessage.onEnd (functions-framework-nodejs/node_modules/raw-body/index.js:287:7) + at IncomingMessage.emit (node:events:518:28) + at endReadableNT (node:internal/streams/readable:1698:12) + at process.processTicksAndRejections (node:internal/process/task_queues:90:21) +``` + +## Propagating Errors + +If you want to propgate internal express level errors to your application, enabling the propagate option and defining a custom error handler will allow your application to receive errors: + +1. In your `package.json`, specify `--propagate-framework-errors=true"` for the `functions-framework`: + +```sh +{ + "scripts": { + "start": "functions-framework --target=helloWorld --propagate-framework-errors=true" + } +} +``` + +2. Define a express error handler: + +```ts +const app = express(); + +// User error handler +app.use((err, req, res, next) => { + logger.log(err); + res.send("Caught error!"); +}); +``` + +Now your application will receive internal express level errors! + +```ts +// Post request with bad JSON +http.post("/", "{"id": "Hello}"); +``` + +The custom error handler logic executes: + +``` +Caught error! +``` \ No newline at end of file From 90ea72e6793614c75d9b93799e8e590e199dac03 Mon Sep 17 00:00:00 2001 From: Gregory Gaines Date: Sun, 30 Mar 2025 12:25:06 -0400 Subject: [PATCH 7/8] fix: Inject user function error handle middleware chain into framework instead of just the middleware --- ...function_error_handle_middleware_chain.ts} | 96 ++++++++++--------- src/server.ts | 6 +- ..._function_error_handle_middleware_chain.ts | 77 +++++++++++++++ .../propagate_error_to_client_error_handle.ts | 73 -------------- 4 files changed, 128 insertions(+), 124 deletions(-) rename src/middleware/{propagate_error_to_client_error_handle.ts => inject_user_function_error_handle_middleware_chain.ts} (56%) create mode 100644 test/middleware/inject_user_function_error_handle_middleware_chain.ts delete mode 100644 test/middleware/propagate_error_to_client_error_handle.ts diff --git a/src/middleware/propagate_error_to_client_error_handle.ts b/src/middleware/inject_user_function_error_handle_middleware_chain.ts similarity index 56% rename from src/middleware/propagate_error_to_client_error_handle.ts rename to src/middleware/inject_user_function_error_handle_middleware_chain.ts index a4cfc213..84fd0299 100644 --- a/src/middleware/propagate_error_to_client_error_handle.ts +++ b/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 --git a/src/server.ts b/src/server.ts index 87197afc..6734637b 100644 --- a/src/server.ts +++ b/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 --git a/test/middleware/inject_user_function_error_handle_middleware_chain.ts b/test/middleware/inject_user_function_error_handle_middleware_chain.ts new file mode 100644 index 00000000..f565120f --- /dev/null +++ b/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); + }); +}); diff --git a/test/middleware/propagate_error_to_client_error_handle.ts b/test/middleware/propagate_error_to_client_error_handle.ts deleted file mode 100644 index 3f6254d3..00000000 --- a/test/middleware/propagate_error_to_client_error_handle.ts +++ /dev/null @@ -1,73 +0,0 @@ -import * as assert from 'assert'; -import * as sinon from 'sinon'; -import {NextFunction} from 'express'; -import {Request, Response} from '../../src'; -import {createPropagateErrorToClientErrorHandleMiddleware} from '../../src/middleware/propagate_error_to_client_error_handle'; -import * as express from 'express'; - -describe('propagateErrorToClientErrorHandleMiddleware', () => { - let request: Request; - let response: Response; - let next: NextFunction; - let errListener = () => {}; - let error: Error; - beforeEach(() => { - error = Error('Something went wrong!'); - request = { - setTimeout: sinon.spy(), - on: sinon.spy(), - } as unknown as Request; - response = { - on: sinon.spy(), - } as unknown as Response; - next = sinon.spy(); - errListener = sinon.spy(); - }); - - it('user express app with error handle calls user function app error handle', () => { - const app = express(); - app.use( - ( - // 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 => { - errListener(); - } - ); - - const middleware = createPropagateErrorToClientErrorHandleMiddleware(app); - middleware(error, request, response, next); - - assert.strictEqual((errListener as sinon.SinonSpy).called, true); - assert.strictEqual((next as sinon.SinonSpy).called, false); - }); - - it('user express app without error handle calls default express error handle', () => { - const app = express(); - - const middleware = createPropagateErrorToClientErrorHandleMiddleware(app); - middleware(error, request, response, next); - - assert.strictEqual((errListener as sinon.SinonSpy).called, false); - assert.strictEqual((next as sinon.SinonSpy).called, true); - }); - - it('non-express user app calls default express error handle', () => { - const app = (_req: Request, res: Response) => { - res.send('Hello, World!'); - }; - - const middleware = createPropagateErrorToClientErrorHandleMiddleware(app); - middleware(error, request, response, next); - - assert.strictEqual((errListener as sinon.SinonSpy).called, false); - assert.strictEqual((next as sinon.SinonSpy).called, true); - }); -}); From 9243eb026bb7bab7f1ecc73ade74c65d1e9bee4b Mon Sep 17 00:00:00 2001 From: Gregory Gaines Date: Mon, 31 Mar 2025 12:38:56 -0400 Subject: [PATCH 8/8] test: Tighten test cases --- ..._function_error_handle_middleware_chain.ts | 15 ++- ..._function_error_handle_middleware_chain.ts | 97 +++++++++++-------- 2 files changed, 63 insertions(+), 49 deletions(-) diff --git a/src/middleware/inject_user_function_error_handle_middleware_chain.ts b/src/middleware/inject_user_function_error_handle_middleware_chain.ts index 84fd0299..fe3afd1b 100644 --- a/src/middleware/inject_user_function_error_handle_middleware_chain.ts +++ b/src/middleware/inject_user_function_error_handle_middleware_chain.ts @@ -54,15 +54,12 @@ export const injectUserFunctionErrorHandleMiddlewareChain = ( return; } - // 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++ - ) { - const middleware = middlewares[index]; - + // Inject middleware chain starting from the first error handle + // middleware into the framework app. + const middlewares = (userFunction as Express)._router.stack.slice( + firstErrorHandleMiddlewareIndex + ); + for (const middleware of middlewares) { // We don't care about routes. if (middleware.route) { continue; diff --git a/test/middleware/inject_user_function_error_handle_middleware_chain.ts b/test/middleware/inject_user_function_error_handle_middleware_chain.ts index f565120f..944123e9 100644 --- a/test/middleware/inject_user_function_error_handle_middleware_chain.ts +++ b/test/middleware/inject_user_function_error_handle_middleware_chain.ts @@ -1,70 +1,87 @@ import * as assert from 'assert'; -import {NextFunction} from 'express'; +import {Express, 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 userAppErrorHandleMiddleware = ( + /* eslint-disable @typescript-eslint/no-unused-vars */ + _err: Error, + _req: Request, + _res: Response, + _next: NextFunction + ) => {}; + const userAppFollowUpErrorMiddleware = ( + /* eslint-disable @typescript-eslint/no-unused-vars */ + _err: Error, + _req: Request, + _res: Response, + _next: NextFunction + ) => {}; + const userAppNormalMiddleware = ( + /* eslint-disable @typescript-eslint/no-unused-vars */ + _req: Request, + _res: Response, + _next: NextFunction + ) => {}; + + const getMiddlewareNames = (app: Express) => { + return app._router.stack.map((middleware: ILayer) => middleware.name); + }; + + it('user app with error handle middleware injects middleware chain 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 - ) {} + userApp.use(userAppErrorHandleMiddleware); + userApp.use(userAppNormalMiddleware); + userApp.use(userAppFollowUpErrorMiddleware); injectUserFunctionErrorHandleMiddlewareChain(frameworkApp, userApp); + const frameworkAppMiddlewareNames = getMiddlewareNames(frameworkApp); - const appAMiddleware = frameworkApp._router.stack; - const appAMiddlewareNames = appAMiddleware.map( - (middleware: ILayer) => middleware.name + assert.deepStrictEqual(frameworkAppMiddlewareNames, [ + 'query', + 'expressInit', + 'userAppErrorHandleMiddleware', + 'userAppNormalMiddleware', + 'userAppFollowUpErrorMiddleware', + ]); + }); + + it('user app with error handle middleware ignores routes and injects middleware chain into framework app', () => { + const frameworkApp = express(); + const userApp = express(); + userApp.use(userAppErrorHandleMiddleware); + userApp.post( + '/foo', + (_req: Request, _res: Response, _next: NextFunction) => {} ); + userApp.use(userAppFollowUpErrorMiddleware); + + injectUserFunctionErrorHandleMiddlewareChain(frameworkApp, userApp); + const frameworkAppMiddlewareNames = getMiddlewareNames(frameworkApp); - assert.deepStrictEqual(appAMiddlewareNames, [ + assert.deepStrictEqual(frameworkAppMiddlewareNames, [ 'query', 'expressInit', - 'appBErrorHandle', - 'appBFollowUpMiddleware', + 'userAppErrorHandleMiddleware', + 'userAppFollowUpErrorMiddleware', ]); }); - it('user app without error handle not injected into framework app', () => { + it('user app without error handle middleware chain 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 - ) {} + userApp.use(userAppNormalMiddleware); injectUserFunctionErrorHandleMiddlewareChain(frameworkApp, userApp); assert.strictEqual('_router' in frameworkApp, false); }); - it('non-express user app not injected into framework app', () => { + it('non-express user app middleware chain not injected into framework app', () => { const frameworkApp = express(); const userApp = (_req: Request, res: Response) => { res.send('Hello, World!');