Merge pull request #195 from trouble/feature/#128-standardize-error-handler

Feature/#128 standardize error handler

src/collections/graphql/resolvers/find.js

@@ -17,7 +17,6 @@ const findResolver = collection => async (_, args, context) => {
   };
 
   const results = await find(options);
-
   return results;
 };
 

src/collections/requestHandlers/create.js

@@ -1,9 +1,8 @@
 const httpStatus = require('http-status');
-const formatErrorResponse = require('../../express/responses/formatError');
 const formatSuccessResponse = require('../../express/responses/formatSuccess');
 const { create } = require('../operations');
 
-const createHandler = async (req, res) => {
+const createHandler = async (req, res, next) => {
   try {
     const doc = await create({
       req,
@@ -17,7 +16,7 @@ const createHandler = async (req, res) => {
       doc,
     });
   } catch (error) {
-    return res.status(error.status || httpStatus.INTERNAL_SERVER_ERROR).json(formatErrorResponse(error));
+    return next(error);
   }
 };
 

src/collections/requestHandlers/delete.js

@@ -2,7 +2,7 @@ const httpStatus = require('http-status');
 const { NotFound } = require('../../errors');
 const { deleteQuery } = require('../operations');
 
-const deleteHandler = async (req, res) => {
+const deleteHandler = async (req, res, next) => {
   try {
     const doc = await deleteQuery({
       req,
@@ -16,8 +16,8 @@ const deleteHandler = async (req, res) => {
     }
 
     return res.status(httpStatus.OK).send(doc);
-  } catch (err) {
-    return res.status(httpStatus.INTERNAL_SERVER_ERROR).json(err);
+  } catch (error) {
+    return next(error);
   }
 };
 

src/collections/requestHandlers/find.js

@@ -1,7 +1,7 @@
 const httpStatus = require('http-status');
 const { find } = require('../operations');
 
-const findHandler = async (req, res) => {
+const findHandler = async (req, res, next) => {
   try {
     const options = {
       req,
@@ -17,8 +17,8 @@ const findHandler = async (req, res) => {
     const result = await find(options);
 
     return res.status(httpStatus.OK).json(result);
-  } catch (err) {
-    return res.status(400).json(err);
+  } catch (error) {
+    return next(error);
   }
 };
 

src/collections/requestHandlers/findByID.js

@@ -1,8 +1,6 @@
-const httpStatus = require('http-status');
 const { findByID } = require('../operations');
-const formatErrorResponse = require('../../express/responses/formatError');
 
-const findByIDHandler = async (req, res) => {
+const findByIDHandler = async (req, res, next) => {
   const options = {
     req,
     Model: req.collection.Model,
@@ -14,8 +12,8 @@ const findByIDHandler = async (req, res) => {
   try {
     const doc = await findByID(options);
     return res.json(doc);
-  } catch (err) {
-    return res.status(err.status || httpStatus.INTERNAL_SERVER_ERROR).json(formatErrorResponse(err));
+  } catch (error) {
+    return next(error);
   }
 };
 

src/collections/requestHandlers/update.js

@@ -1,9 +1,8 @@
 const httpStatus = require('http-status');
-const formatErrorResponse = require('../../express/responses/formatError');
 const formatSuccessResponse = require('../../express/responses/formatSuccess');
 const { update } = require('../operations');
 
-const updateHandler = async (req, res) => {
+const updateHandler = async (req, res, next) => {
   try {
     const doc = await update({
       req,
@@ -18,7 +17,7 @@ const updateHandler = async (req, res) => {
       doc,
     });
   } catch (error) {
-    return res.status(error.status || httpStatus.INTERNAL_SERVER_ERROR).json(formatErrorResponse(error));
+    return next(error);
   }
 };
 

src/errors/index.js

@@ -7,9 +7,11 @@ const MissingGlobalLabel = require('./MissingGlobalLabel');
 const NotFound = require('./NotFound');
 const Forbidden = require('./Forbidden');
 const ValidationError = require('./ValidationError');
+const errorHandler = require('../express/middleware/errorHandler');
 const MissingFile = require('./MissingFile');
 
 module.exports = {
+  errorHandler,
   APIError,
   AuthenticationError,
   DuplicateCollection,

src/express/middleware/errorHandler.js

@@ -0,0 +1,30 @@
+const httpStatus = require('http-status');
+const formatErrorResponse = require('../responses/formatError');
+
+const errorHandler = config => async (err, req, res, next) => {
+  const data = formatErrorResponse(err);
+  let response;
+  let status = err.status || httpStatus.INTERNAL_SERVER_ERROR;
+
+  // TODO: use payload logging
+  console.error(err.stack);
+
+  if (config.debug && config.debug === true) {
+    data.stack = err.stack;
+  }
+  response = {
+    ...data,
+  };
+
+  if (req.collection && typeof req.collection.config.hooks.afterError === 'function') {
+    ({ response, status } = await req.collection.config.hooks.afterError(err, response) || { response, status });
+  }
+  if (typeof config.hooks.afterError === 'function') {
+    ({ response, status } = await config.hooks.afterError(err, response) || { response, status });
+  }
+
+  res.status(status)
+    .send(response);
+};
+
+module.exports = errorHandler;

src/express/middleware/errorHandler.spec.js

@@ -0,0 +1,110 @@
+const errorHandler = require('./errorHandler');
+const APIError = require('../../errors/APIError');
+
+const testError = new APIError('test error', 503);
+
+const mockResponse = () => {
+  const res = {};
+  res.status = jest.fn()
+    .mockReturnValue(res);
+  res.send = jest.fn()
+    .mockReturnValue(res);
+  return res;
+};
+
+const mockRequest = async () => {
+  const req = {};
+  req.collection = {
+    config: {
+      hooks: {},
+    },
+  };
+  req.collection.config.hooks.afterError = await jest.fn();
+  return req;
+};
+
+describe('errorHandler', () => {
+  let res;
+  let req;
+  beforeAll(async (done) => {
+    res = mockResponse();
+    req = await mockRequest();
+    done();
+  });
+
+  it('should send the response with the error', async () => {
+    const handler = errorHandler({ debug: true, hooks: {}});
+    await handler(testError, req, res);
+    expect(res.send)
+      .toHaveBeenCalledWith(
+        expect.objectContaining({ errors: [{ message: 'test error' }] }),
+      );
+  });
+
+  it('should include stack trace when config debug is on', async () => {
+    const handler = errorHandler({ debug: true, hooks: {} });
+    await handler(testError, req, res);
+    expect(res.send)
+      .toHaveBeenCalledWith(
+        expect.objectContaining({ stack: expect.any(String) }),
+      );
+  });
+
+  it('should not include stack trace when config debug is not set', async () => {
+    const handler = errorHandler({hooks: {}});
+    await handler(testError, req, res);
+    expect(res.send)
+      .toHaveBeenCalledWith(
+        expect.not.objectContaining({ stack: expect.any(String) }),
+      );
+  });
+
+  it('should not include stack trace when config debug is false', async () => {
+    const handler = errorHandler({ debug: false, hooks: {} });
+    await handler(testError, req, res);
+    expect(res.send)
+      .toHaveBeenCalledWith(
+        expect.not.objectContaining({ stack: expect.any(String) }),
+      );
+  });
+
+  it('should show the status code when given an error with a code', async () => {
+    const handler = errorHandler({ debug: false, hooks: {} });
+    await handler(testError, req, res);
+    expect(res.status)
+      .toHaveBeenCalledWith(
+        503,
+      );
+  });
+
+  it('should default to 500 when an error does not have a status code', async () => {
+    const handler = errorHandler({ debug: false, hooks: {} });
+    testError.status = undefined;
+    await handler(testError, req, res);
+    expect(res.status)
+      .toHaveBeenCalledWith(
+        500,
+      );
+  });
+
+  it('should call payload config afterError hook', async () => {
+    const afterError = jest.fn();
+    const handler = errorHandler({
+      debug: false,
+      hooks: { afterError },
+    });
+    await handler(testError, req, res);
+    expect(afterError)
+      .toHaveBeenCalled();
+  });
+
+  it('should call collection config afterError hook', async () => {
+    const handler = errorHandler({
+      debug: false,
+      hooks: {},
+    });
+    await handler(testError, req, res);
+    expect(req.collection.config.hooks.afterError)
+      .toHaveBeenCalled();
+  });
+});

src/express/responses/formatError.js

@@ -1,9 +1,11 @@
 const formatErrorResponse = (incoming) => {
   if (incoming) {
+    // mongoose
     if (incoming.errors) {
       return {
         errors: Object.keys(incoming.errors).reduce((acc, key) => {
           acc.push({
+            field: incoming.errors[key].path,
             message: incoming.errors[key].message,
           });
           return acc;

src/graphql/errorHandler.js

@@ -0,0 +1,22 @@
+const errorHandler = async (info, debug, afterErrorHook) => {
+  return Promise.all(info.result.errors.map(async (err) => {
+    // TODO: use payload logging
+    console.error(err.stack);
+
+    let response = {
+      ...err,
+    };
+
+    if (afterErrorHook) {
+      ({ response } = await afterErrorHook(err, response) || { response });
+    }
+
+    if (debug && debug === true) {
+      response.stack = err.stack;
+    }
+
+    return response;
+  }));
+};
+
+module.exports = errorHandler;

src/graphql/index.js

@@ -10,6 +10,7 @@ const buildFallbackLocaleInputType = require('./schema/buildFallbackLocaleInputT
 const initCollections = require('../collections/graphql/init');
 const initGlobals = require('../globals/graphql/init');
 const buildWhereInputType = require('./schema/buildWhereInputType');
+const errorHandler = require('./errorHandler');
 
 class GraphQL {
   constructor(init) {
@@ -23,8 +24,14 @@ class GraphQL {
       fallbackLocaleInputType: buildFallbackLocaleInputType(this.config.localization),
     };
 
-    this.Query = { name: 'Query', fields: {} };
-    this.Mutation = { name: 'Mutation', fields: {} };
+    this.Query = {
+      name: 'Query',
+      fields: {},
+    };
+    this.Mutation = {
+      name: 'Mutation',
+      fields: {},
+    };
 
     this.buildBlockType = buildBlockType.bind(this);
     this.buildMutationInputType = buildMutationInputType.bind(this);
@@ -51,9 +58,34 @@ class GraphQL {
 
     const query = new GraphQLObjectType(this.Query);
     const mutation = new GraphQLObjectType(this.Mutation);
-    const schema = new GraphQLSchema({ query, mutation });
+    const schema = new GraphQLSchema({
+      query,
+      mutation,
+    });
 
-    return graphQLHTTP({ schema });
+    let errorExtensions = [];
+    let errorExtensionIteration = 0;
+
+    const extensions = async (info) => {
+      const { result } = info;
+      if (result.errors) {
+        const afterErrorHook = typeof this.config.hooks.afterError === 'function' ? this.config.hooks.afterError : null;
+        errorExtensions = await errorHandler(info, this.config.debug, afterErrorHook);
+      }
+      return null;
+    };
+
+    return graphQLHTTP({
+      schema,
+      customFormatErrorFn: () => {
+        const response = {
+          ...errorExtensions[errorExtensionIteration],
+        };
+        errorExtensionIteration += 1;
+        return response;
+      },
+      extensions,
+    });
   }
 }
 

src/index.js

@@ -15,6 +15,7 @@ const GraphQL = require('./graphql');
 const sanitizeConfig = require('./utilities/sanitizeConfig');
 const buildEmail = require('./email/build');
 const identifyAPI = require('./express/middleware/identifyAPI');
+const errorHandler = require('./express/middleware/errorHandler');
 
 class Payload {
   constructor(options) {
@@ -69,6 +70,8 @@ class Payload {
 
     // Enable static routes for all collections permitting upload
     this.initStatic();
+
+    this.router.use(errorHandler(this.config));
   }
 
   async sendEmail(message) {

src/utilities/sanitizeConfig.js

@@ -28,6 +28,7 @@ const sanitizeConfig = (config) => {
   };
 
   sanitizedConfig.components = { ...(config.components || {}) };
+  sanitizedConfig.hooks = { ...(config.hooks || {}) };
 
   return sanitizedConfig;
 };