Logging and error middleware

Question

I'm looking for code review on the following basic log/error middleware. The logging middleware uses logger.js which has a bunch of methods like log.error() (which makes a request out to error tracking service like Sentry).

The handleErrors middleware is intended to handle any errors that occur in any of the api routes. So, for example, I have a route /api/users and if an error occurs in that request, the catch will execute and pass along the error to the handleErrors middleware.

const getUsers = (req, res, next) => {
  return apiHttpClient.get(req, '/api/users')
    .then(users => res.status(HttpStatus.OK).json(users))
    .catch(err => next(err));
};

The error middleware should create an error context for the client without exposing the specific error. So for example, if a 400 error occurs, the message sent to the browser would look like:

{
  "message": "Bad Request",
  "status": 400
}    

But depending on if the NODE_ENV is production, the logging middleware should log out the specific error:

{
  "name":"Example",
  "hostname":"",
  "pid":18322,
  "level":40,
  "payload":{
    "name":"BadRequest",
    "message":"A specific error message",
    "statusCode":400,
    "errorCode":400
  },
  "msg":"400: A specific error message",
  "time":"2017-10-22T15:26:06.824Z",
  "v":0
}

Usage

express.js

const { logErrors, handleErrors } = require('./errorMiddleware');

// other middleware
// api routes

app.use(logErrors())
app.use(handleErrors());

errorMiddleware.js

const log = require('../../logger');
const HttpStatus = require('http-status-codes');

const isServerError = (statusCode) => {
  return statusCode >= HttpStatus.INTERNAL_SERVER_ERROR;
};

const isProdEnv = (env) => {
  return env === 'production';
};

const getStatusCode = (statusCode) => {
  return statusCode || HttpStatus.INTERNAL_SERVER_ERROR;
};

const createErrorContext = (statusCode) => {
  return {
    message: HttpStatus.getStatusText(statusCode),
    status: statusCode
  };
};

const createErrorMessage = (err) => {
  const statusCode = getStatusCode(err.statusCode);
  return `${statusCode}: ${err.message}`;
};

const deleteErrorStack = (env, err) => {
  if (isProdEnv(env)) delete err.stack;
};

const getLogLevel = (statusCode) => {
  return isServerError(statusCode) ? 'error' : 'warn';
};

const logErrors = () => {
  return (err, req, res, next) => {
    const statusCode = getStatusCode(err.statusCode);
    const logLevel = getLogLevel(statusCode);
    const errMessage = createErrorMessage(err);
    const extraData = req.path;

    log[logLevel](errMessage, err, extraData);

    next(err);
  };
};

const handleErrors = () => {
  return (err, req, res, next) => {
    const statusCode = getStatusCode(err.statusCode);
    const errorContext = createErrorContext(statusCode);

    deleteErrorStack(req.app.get('env'), err);
    res.status(statusCode).json(errorContext);
  };
};

module.exports = {
  logErrors: logErrors,
  handleErrors: handleErrors
};

Answer 1

Some thoughts:

• Consider making your middleware package into a proper (private?) npm package and consume it as such in the main application. This would also allow better reuse across other applications that may need this behavior.
• const log = require('../../logger'); This relative reference seems problematic. I would guess your logger should also be a proper npm package (or in same package as your middleware if this is a custom logger). At a very minimum, I would think you would want to use path or similar to get to an absolute file reference based on the location of your middleware file to the working directory.
• Your methodology for determining 'production' environment seems fragile. Why is it up to your middleware package to have a hard-coded string in it that determines whether production behavior is to be expressed?
• The deleteErrorStack() function seems superfluos and actually obfuscates the logic of what is happening in the handleErrors function. Why not just have this single line of code inside the handleErrors function so intent is much clearer.

For example:

const handleErrors = () => {
  return (err, req, res, next) => {
    const statusCode = getStatusCode(err.statusCode);
    const errorContext = createErrorContext(statusCode);

    if(isProdEnv(req.app.get('env')) {
        delete err.stack;
    }
    res.status(statusCode).json(errorContext);
  };
};

• One could possibly argue that the deletion of the stack trace information from err is logic this should exist within your context creation method, as this is really a display concern, not an error handling concern.