Hardening REST endpoints

[sonallux]

Currently, all REST endpoints of the typescript microservices do not have an appropriate error handling for the following two error scenarios:

1. a non async request handling function is throwing an Error
2. an async (or a Promise returning) request handling function is returning a rejected promise, which happens when an Error is thrown inside an async function or an awaited Promise is rejected.

Error handling in express

Express can handle thrown Errors and Errors passed to the next function. It does allow registering special error handlers to handle those caught errors. We do not register any error handlers currently, but there is always a default error handler registered, which will respond with a status 500 and the stack trace of the error in the body. Sending the stack trace can be turned off by setting the NODE_ENV environment variable to production, then only the stringified status code is present in the response body (e.g. Internal Server Error).

Scenario 1

Those errors are caught by express automatically, and the default error handler will create a 500 response with the stack trace in the body. Having the stack trace in the body is great for development, but it is also exposing a lot of internal details.

So scenario 1 is actually already handled by express. Adding our own error handler that returns just the error message and not the full stack trace might be an option to consider. But I am also ok with the default error handler.

Scenario 2

As express does not have support for handling promises, the famous unhandled promise rejection error will occur when a rejected promise is returned in a request handler. When this happens also no response is sent to the user. To be able to handle a rejected promise with the express error handler the next function can be set as the promise catch handler:

app.get('/', function (req, res, next) {
  handleRequest(req, res) //returns a promise
    .catch(next) // Errors will be passed to express error handler.
})

For solving scenario 2 I would propose to add this little wrapper function, which might be also a good candidate for adding to @jvalue/node-dry. This function does nothing for non promise returning request handlers, so it is safe to add it to every request handler.

function asyncHandler (handler: (req: express.Request, res: express.Response, next?: express.NextFunction) => void | Promise<void>): express.RequestHandler {
  function wrapper (req: express.Request, res: express.Response, next: express.NextFunction): void {
    const handlerResult = handler(req, res, next)
    Promise.resolve(handlerResult).catch(next)
  }
  return wrapper
}

Then all (async) request handler must be wrapped with the asyncHandler:

app.get('/promise', asyncHandler(handleRequest))

function handleRequest(req: express.Request, res: express.Response): Promise<void> {...}

Alternative solution

There is an express-async-router library that wraps the express Router and adds handling for Promises. But the library seems to be unmaintained as there has been no activity in the repository in the last two years. Therefore, I would not go with this solution.

---

@georg-schwarz @mathiaszinnen @lunedis @MBuchalik what do you think about this? Especially what should the response contain when such an Error happens?

[georg-schwarz]

Great issue description, I like it!

So I think we exclude handled errors, like input validation, etc., from this discussion because they respond with other status. So basically in a perfect world the remaining cases point to bugs in the implementation or non-handled inconsistencies and thus should return status 500. Did I frame the context of the issue correctly?

Then it is important that the stacktrace is not lost, so we can fix it. So at least the error has to be logged. Returning it to the caller and exposing implementation details doesn't make too much sense in my opinion, though in dev mode it might be an an option like express already does. In production mode I would return a generic error message.

BTW this is an excellent example we want to standardize across all services to provide the feel of a unified API. We might want to do that separately in an API guideline. ;-)

[sonallux]

So I think we exclude handled errors, like input validation, etc., from this discussion because they respond with other status. So basically in a perfect world the remaining cases point to bugs in the implementation or non-handled inconsistencies and thus should return status 500. Did I frame the context of the issue correctly?

Yes, correct.

[sonallux]

Scenario 2, that returned Promises are not handled, could actually be detected by the @typescript-eslint/no-misused-promises linter rule. But as the express type definitions are not strict enough, this does currently not work. I have already created a PR DefinitelyTyped/DefinitelyTyped#49677 to fix the type issue. So once that is merged and we update @types/express we will get linter warnings about this issue.

[dwrss]

@jsone-studios PR merged.

[sonallux]

@georg-schwarz how do we proceed with this? Are you going to create a new repository for a node-dry-express library?

[georg-schwarz]

I suggest the following procedure: lets first do a regular PR in one of the services and check if it works. Afterward, we will outsource to a new repo.

This will prevent us from going back and forth between lib and services.

[sonallux]

Closing, because this has been resolved in all microservices.