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: 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 diff --git a/src/middleware/inject_user_function_error_handle_middleware_chain.ts b/src/middleware/inject_user_function_error_handle_middleware_chain.ts new file mode 100644 index 00000000..fe3afd1b --- /dev/null +++ b/src/middleware/inject_user_function_error_handle_middleware_chain.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 {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; + +/** + * 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 injectUserFunctionErrorHandleMiddlewareChain = ( + frameworkApp: Express, + userFunction: HandlerFunction +) => { + // 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; + } + + // 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; + } + + frameworkApp.use(middleware.handle); + } +}; + +/** + * 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) + ); +}; + +/** + * 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 (let index = 0; index < middlewares.length; index++) { + const middleware = middlewares[index]; + if ( + middleware.handle && + middleware.handle.length === EXPRESS_ERROR_HANDLE_PARAM_LENGTH + ) { + return index; + } + } + + return null; +}; 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/server.ts b/src/server.ts index 9a9b3acc..6734637b 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 {injectUserFunctionErrorHandleMiddlewareChain} from './middleware/inject_user_function_error_handle_middleware_chain'; /** * Creates and configures an Express application and returns an HTTP server @@ -172,5 +173,9 @@ export function getServer( app.post('/*', requestHandler); } + if (options.propagateFrameworkErrors) { + injectUserFunctionErrorHandleMiddlewareChain(app, userFunction); + } + return http.createServer(app); } 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/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); + }); }); 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/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..944123e9 --- /dev/null +++ b/test/middleware/inject_user_function_error_handle_middleware_chain.ts @@ -0,0 +1,94 @@ +import * as assert from 'assert'; +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', () => { + 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(userAppErrorHandleMiddleware); + userApp.use(userAppNormalMiddleware); + userApp.use(userAppFollowUpErrorMiddleware); + + injectUserFunctionErrorHandleMiddlewareChain(frameworkApp, userApp); + const frameworkAppMiddlewareNames = getMiddlewareNames(frameworkApp); + + 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(frameworkAppMiddlewareNames, [ + 'query', + 'expressInit', + 'userAppErrorHandleMiddleware', + 'userAppFollowUpErrorMiddleware', + ]); + }); + + it('user app without error handle middleware chain not injected into framework app', () => { + const frameworkApp = express(); + const userApp = express(); + userApp.use(userAppNormalMiddleware); + + injectUserFunctionErrorHandleMiddlewareChain(frameworkApp, userApp); + + assert.strictEqual('_router' in frameworkApp, false); + }); + + 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!'); + }; + + injectUserFunctionErrorHandleMiddlewareChain(frameworkApp, userApp); + + assert.strictEqual('_router' in frameworkApp, false); + }); +}); 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); + }); });