Fix async express middleware to properly handle error cases

[humphd]

We have a lot of express end-points that use async functions:

$ rg "async \(req"
src/backend/web/routes/stats.js
8:const statsRoute = (statsPeriod) => async (req, res) => {

src/backend/web/routes/planet.js
63:router.get('/', async (req, res) => {

src/backend/web/routes/query.js
9:router.get('/', validateQuery(), async (req, res) => {

src/backend/web/routes/user.js
19:router.get('/feeds', protect(), async (req, res) => {

src/backend/web/routes/posts.js
10:posts.get('/', validatePostsQuery(), async (req, res) => {
76:posts.get('/:id', validatePostsIdParam(), async (req, res) => {

src/backend/web/routes/feed.js
29:const feedRoute = (type) => async (req, res) => {
110:router.get('/opml', async (req, res) => {
145:router.get('/wiki', async (req, res) => {

src/backend/web/validation.js
5:  return async (req, res, next) => {

src/backend/web/routes/feeds.js
11:feeds.get('/', async (req, res) => {
35:feeds.get('/:id', validateFeedsIdParam(), async (req, res) => {
56:feeds.put('/:id/flag', protectAdmin(), validateFeedsIdParam(), async (req, res) => {
77:feeds.post('/', protect(), validateNewFeed(), async (req, res) => {
100:feeds.delete('/cache', protectAdmin(true), async (req, res) => {
112:feeds.delete('/:id', validateFeedsIdParam(), protect(), async (req, res) => {
133:feeds.delete('/:id/flag', protectAdmin(), validateFeedsIdParam(), async (req, res) => {

Express 4.x doesn't quite do async properly, in that you have to manually deal with the error case and call next(err) manually so that errors propagate down the middleware chain to the eventual default error handler.

The trick is to turn code like this:

router.get('/:id', async (req, res) => {
  const { id } = req.params;
  const data = await getSomething(id);
  res.json(data);
});

into something like this:

router.get('/:id', async (req, res, next) => {
  try {
    const { id } = req.params;
    const data = await getSomething(id);
    res.json(data);
  } catch(err) {
    next(err);
  }
});

Basically, we have to either deal with the error case in the route (i.e., don't let it go further in any code path), or call next(err) ourselves when things blow up. I haven't looked at all of our routes to see if they need changing, so this is a bug to audit things.

[HyperTHD]

So if I'm understanding this correctly, we'd fix up the backend routes so that they explicitly call next with the error?

For reference, here's our logout route in auth.js

[humphd]

In the code you listed above, we aren't using async, so this doesn't apply. However, in this case we do need to do something different from the usual error handler: we want to redirect.

[HyperTHD]

One thing I noticed in all the asynchronous function callbacks is that many of them do not call next(err). Is this something that needs to be fixed for all of these cases to fix their error handling?

[humphd]

Ideally we should have all errors go through a common error handler so that we can log it in production, see https://github.com/Seneca-CDOT/satellite/blob/master/index.js#L99-L119.

Ideally, the pattern would be something like this:

router.get('/whatever', async (req, res, next) => {
  try {
    res.json(await trySomething());
  } catch(err) {
    // you could also create a better external error to use here vs. exposing details about the internal problem...
    next(err);
  }
});

Then you