Proposal: Refactor Server Error Handling

[jniles]

The current state of error handling on the server is pretty poor. Here are some problems with it:

1. The code is difficult to read.
2. We are attaching objects to the native Node req object on every request, even if they are never used. This has performance implications and makes req.codes seem like a magic object.
3. Errors are difficult to write (e.g. next(new req.codes.ERR_SOME_ERROR())).
4. The error stack property doesn't work, making it impossible to use it to trace errors.

I propose the following refactor. Errors will be defined in files found in server/lib/errors/*.js. We will implement a subset of the error HTTP status codes in a few files. Specifically, this subset:

1. BadRequest (400)
2. Unauthorized (401)
3. Forbidden (403)
4. NotFound (404)
5. InternalServerError (500)

These would appear in files named like the above, for example /server/lib/errors/NotFound.js. These files would contain a single function, exported via module.exports. See below for a sample:

var util = require('util');

// implement the 404 NOT FOUND error function
function NotFound(description) {
  'use strict';

  // make sure we have a working stack trace
  Error.captureStackTrace(this, this.constructor);

  // HTTP status code and translation key for the client.
  this.status = 404;
  this.code = 'ERR_NOT_FOUND';

  // description allows developers to attach a personalized debugging message
  this.description = description || '';
}

util.inherits(NotFound, Error);

module.exports = NotFound;

A developer can then selectively import the errors they need and use them as follows:

const NotFound = require('../lib/errors/NotFound');

exports.detail = function detail(req, res, next) {

  const sql = 'SELECT some_col FROM some_table WHERE id = ?;';

  db.exec(sql, [ req.params.id ])
  .then(function (rows) {

    // whoops!  Couldn't find the record!
    if (rows.length === 0) {
      throw NotFound(`Could not find a record in some_table with id: ${req.params.id}`);
    }

    // all okay!  Send the response to the client
    res.status(200).json(rows[0]);
 })
 .catch(next)
 .done();
};

This is obviously much cleaner to write and read. We avoid most of the problems pointed out at the top of this issue, and actually get clean stack traces!

Error inheritance inspired by this gist.

[sfount]

We are attaching objects to the native Node req object on every request, even if they are never used. This has performance implications and makes req.codes seem like a magic object.

I've noticed this during a number of code reviews - I think this is a very important change and should be included as soon as possible so that it can be applied to everyone's code. I will be referencing this issue and it can be scheduled to be tackled ASAP.

[jniles]

Sounds good. Should I work on a PR with the errors pre-made, then developers can rebase and begin using them?

[sfount]

Yes - this should be tackled before it becomes a larger problem; the problem described in this pull request is shown here

I suggest a pull request demonstrating the structure of this proposal is made today - once reviewed it can be merged as soon as possible and future reviews will be able to reference that code as a good template to follow. Cheers.