error handling middleware refactor and users error handling changed to

calling next with the error

app/api/users/routes.js

@@ -16,10 +16,10 @@ export default (app) => {
       password: Joi.string(),
       role: Joi.string().valid('admin', 'editor')
     }).required()),
-    (req, res) => {
+    (req, res, next) => {
       users.save(req.body, req.user, getDomain(req))
       .then(response => res.json(response))
-      .catch(res.error);
+      .catch(next);
     });
 
   app.post('/api/users/new',
@@ -29,50 +29,50 @@ export default (app) => {
       email: Joi.string().required(),
       role: Joi.string().valid('admin', 'editor').required()
     }).required()),
-    (req, res) => {
+    (req, res, next) => {
       users.newUser(req.body, getDomain(req))
       .then(response => res.json(response))
-      .catch(res.error);
+      .catch(next);
     });
 
   app.post(
     '/api/recoverpassword',
     validateRequest(Joi.object().keys({
       email: Joi.string().required(),
     }).required()),
-    (req, res) => {
+    (req, res, next) => {
       users.recoverPassword(req.body.email, getDomain(req))
       .then(() => res.json('OK'))
-      .catch(res.error);
+      .catch(next);
     }
   );
 
   app.post('/api/resetpassword',
     validateRequest(Joi.object().keys({
       key: Joi.string().required()
     }).required()),
-    (req, res) => {
+    (req, res, next) => {
       users.resetPassword(req.body)
       .then(response => res.json(response))
-      .catch(res.error);
+      .catch(next);
     }
   );
 
-  app.get('/api/users', needsAuthorization(), (req, res) => {
+  app.get('/api/users', needsAuthorization(), (req, res, next) => {
     users.get()
     .then(response => res.json(response))
-    .catch(res.error);
+    .catch(next);
   });
 
   app.delete('/api/users',
     needsAuthorization(),
     validateRequest(Joi.object().keys({
       _id: Joi.string().required()
     }).required(), 'query'),
-    (req, res) => {
+    (req, res, next) => {
       users.delete(req.query._id, req.user)
       .then(response => res.json(response))
-      .catch(res.error);
+      .catch(next);
     }
   );
 };

app/api/users/specs/routes.spec.js

@@ -63,15 +63,13 @@ describe('users routes', () => {
         .catch(catchErrors(done));
       });
 
-      it('should return an error if recover password fails', (done) => {
-        spyOn(users, 'recoverPassword').and.returnValue(Promise.reject('error'));
+      it('should return an error if recover password fails', async () => {
+        spyOn(users, 'recoverPassword').and.returnValue(Promise.reject(new Error('error')));
         const req = { body: { email: 'recover@me.com' }, protocol: 'http', get: () => 'localhost' };
-        routes.post('/api/recoverpassword', req)
-        .then((response) => {
-          expect(response.error).toBe('error');
-          done();
-        })
-        .catch(catchErrors(done));
+        const next = jasmine.createSpy('next');
+
+        await routes.post('/api/recoverpassword', req, {}, next);
+        expect(next).toHaveBeenCalledWith(new Error('error'));
       });
     });
 
@@ -111,15 +109,13 @@ describe('users routes', () => {
       .catch(catchErrors(done));
     });
 
-    it('should return an error if recover password fails', (done) => {
-      spyOn(users, 'recoverPassword').and.returnValue(Promise.reject('error'));
+    it('should call next on error', async () => {
+      spyOn(users, 'recoverPassword').and.returnValue(Promise.reject(new Error('error')));
       const req = { body: { email: 'recover@me.com' }, protocol: 'http', get: () => 'localhost' };
-      routes.post('/api/recoverpassword', req)
-      .then((response) => {
-        expect(response.error).toBe('error');
-        done();
-      })
-      .catch(done.fail);
+      const next = jasmine.createSpy('next');
+
+      await routes.post('/api/recoverpassword', req, {}, next);
+      expect(next).toHaveBeenCalledWith(new Error('error'));
     });
   });
 

app/api/utils/error_handling_middleware.js

@@ -1,28 +1,10 @@
-import errorLog from 'api/log/errorLog';
+import handleError from './handleError';
 
-export default function (req, res, next) {
-  res.error = (error) => {
-    let result = error;
-    if (error instanceof Error) {
-      result = error.stack.split('\n');
-    }
+export default function (error, req, res, next) {
+  const handled = handleError(error);
 
-    if (error.code) {
-      result = error.message;
-    }
+  res.status(handled.code);
+  res.json({ error: handled.message });
 
-    let code = error.code || 500;
-
-    if (!(error.code > 0) || error.name === 'MongoError') {
-      code = 500;
-    }
-
-    res.status(code);
-    res.json({ error: result });
-
-    if (error.code === 500) {
-      errorLog.error(result);
-    }
-  };
   next();
 }

app/api/utils/handleError.js

@@ -0,0 +1,23 @@
+import errorLog from 'api/log/errorLog';
+
+export default (error, uncaught = false) => {
+  let result = error;
+
+  if (error instanceof Error) {
+    result = { code: 500, message: error.stack };
+  }
+
+  if (error.name === 'MongoError') {
+    result.code = 500;
+  }
+
+  if (uncaught) {
+    result.message = `uncaught exception or unhandled rejection, Node process finished !!\n ${result.message}`;
+  }
+
+  if (result.code === 500) {
+    errorLog.error(result.message);
+  }
+
+  return result;
+};

app/api/utils/instrumentRoutes.js

@@ -1,6 +1,6 @@
 import * as utils from 'api/utils';
 
-const executeRoute = (method, routePath, req = {}, res, app, runRoute = true) => {
+const executeRoute = (method, routePath, req = {}, res, next = () => {}, app, runRoute = true) => {
   const args = app[method].calls.allArgs().find(a => a[0] === routePath);
   if (!args) {
     throw new Error(`Route ${method.toUpperCase()} ${routePath} is not defined`);
@@ -35,7 +35,12 @@ const executeRoute = (method, routePath, req = {}, res, app, runRoute = true) =>
       return resolve();
     }
 
-    return args[args.length - 1](req, res);
+    const mockedNext = (error) => {
+      next(error);
+      resolve();
+    };
+
+    return args[args.length - 1](req, res, mockedNext);
   });
 
   if (args) {
@@ -56,14 +61,14 @@ export default (route, io) => {
   route(app, io);
   utils.validateRequest = originalValidateRequest;
 
-  const get = (routePath, req, res = {}) => executeRoute('get', routePath, req, res, app);
-  get.validation = (routePath, req, res = {}) => executeRoute('get', routePath, req, res, app, false).validation;
+  const get = (routePath, req, res = {}, next) => executeRoute('get', routePath, req, res, next, app);
+  get.validation = (routePath, req, res = {}, next) => executeRoute('get', routePath, req, res, next, app, false).validation;
 
-  const post = (routePath, req, res = {}) => executeRoute('post', routePath, req, res, app);
-  post.validation = (routePath, req, res = {}) => executeRoute('post', routePath, req, res, app, false).validation;
+  const post = (routePath, req, res = {}, next) => executeRoute('post', routePath, req, res, next, app);
+  post.validation = (routePath, req, res = {}, next) => executeRoute('post', routePath, req, res, next, app, false).validation;
 
-  const remove = (routePath, req, res = {}) => executeRoute('delete', routePath, req, res, app);
-  remove.validation = (routePath, req, res = {}) => executeRoute('delete', routePath, req, res, app, false).validation;
+  const remove = (routePath, req, res = {}, next) => executeRoute('delete', routePath, req, res, next, app);
+  remove.validation = (routePath, req, res = {}, next) => executeRoute('delete', routePath, req, res, next, app, false).validation;
 
   const instrumentedRoute = {
     get,

app/api/utils/specs/error_handling_middleware.spec.js

@@ -12,48 +12,12 @@ describe('Error handling middleware', () => {
     spyOn(errorLog, 'error'); //just to avoid annoying console output
   });
 
-  it('should call next', () => {
-    middleware(req, res, next);
-    expect(next).toHaveBeenCalled();
-  });
+  it('should respond with the error and error code as status', () => {
+    const error = { message: 'error', code: 500 };
+    middleware(error, req, res, next);
 
-  describe('when error is instanceof Error (unhandled)', () => {
-    it('should respond the error stack trace splited', () => {
-      middleware(req, res, next);
-      const error = new Error('error');
-      res.error(error);
-      expect(res.status).toHaveBeenCalledWith(500);
-      expect(res.json).toHaveBeenCalledWith({ error: error.stack.split('\n') });
-    });
-  });
-
-  describe('when error is uwazi error (handeled object with message and code)', () => {
-    it('should respond the error message with the status', () => {
-      middleware(req, res, next);
-      const error = { message: 'error', code: '345' };
-      res.error(error);
-      expect(res.status).toHaveBeenCalledWith('345');
-      expect(res.json).toHaveBeenCalledWith({ error: 'error' });
-    });
-  });
-
-  describe('when error is a MongoError', () => {
-    it('should respond the error message with the status 500', () => {
-      middleware(req, res, next);
-      const error = { name: 'MongoError', message: 'error', code: '345' };
-      res.error(error);
-      expect(res.status).toHaveBeenCalledWith(500);
-      expect(res.json).toHaveBeenCalledWith({ error: 'error' });
-    });
-  });
-
-  describe('when error code is a Mailer EAUTH error', () => {
-    it('should respond the error message with the status 500', () => {
-      middleware(req, res, next);
-      const error = { name: 'MongoError', message: 'error', code: 'EAUTH' };
-      res.error(error);
-      expect(res.status).toHaveBeenCalledWith(500);
-      expect(res.json).toHaveBeenCalledWith({ error: 'error' });
-    });
+    expect(res.status).toHaveBeenCalledWith(500);
+    expect(res.json).toHaveBeenCalledWith({ error: 'error' });
+    expect(next).toHaveBeenCalled();
   });
 });

app/api/utils/specs/handleError.spec.js

@@ -0,0 +1,54 @@
+import errorLog from 'api/log/errorLog';
+import { createError } from 'api/utils';
+
+import handleError from '../handleError';
+
+describe('handleError', () => {
+  beforeEach(() => {
+    spyOn(errorLog, 'error');
+  });
+
+  describe('when error is instance of Error', () => {
+    it('should return the error with 500 code', () => {
+      const errorInstance = new Error('error');
+      const error = handleError(errorInstance);
+
+      expect(error.code).toBe(500);
+      expect(error.message).toEqual(errorInstance.stack);
+    });
+    it('should log the error', () => {
+      const error = new Error('error');
+      handleError(error);
+
+      expect(errorLog.error).toHaveBeenCalledWith(error.stack);
+    });
+  });
+
+  describe('when error is created with createError', () => {
+    it('should return the error', () => {
+      const error = handleError(createError('test error', 400));
+      expect(error).toEqual(createError('test error', 400));
+    });
+    it('should not log the error when code is not 500', () => {
+      handleError(createError('test error', 400));
+      expect(errorLog.error).not.toHaveBeenCalled();
+
+      handleError(createError('test error'));
+      expect(errorLog.error).toHaveBeenCalledWith('test error');
+    });
+  });
+  describe('when error is a MongoError', () => {
+    it('should return the error with a 500 code', () => {
+      const error = handleError({ name: 'MongoError', message: 'error', code: '345' });
+      expect(error.code).toBe(500);
+      expect(error.message).toBe('error');
+    });
+  });
+  describe('when error is uncaught', () => {
+    it('should append the info into the message', () => {
+      const uncaught = true;
+      const error = handleError({ message: 'error' }, uncaught);
+      expect(error.message).toBe('uncaught exception or unhandled rejection, Node process finished !!\n error');
+    });
+  });
+});

server.js

@@ -17,8 +17,6 @@ var privateInstanceMiddleware = require('./app/api/auth/privateInstanceMiddlewar
 var bodyParser = require('body-parser');
 var uploadsFolder = require('./app/api/config/paths').uploadDocumentsPath;
 
-var error_handling_middleware = require('./app/api/utils/error_handling_middleware.js');
-app.use(error_handling_middleware);
 
 app.use(compression());
 var oneYear = 31557600;
@@ -45,23 +43,19 @@ var systemKeys = require('./app/api/i18n/systemKeys.js');
 var ports = require('./app/api/config/ports.js');
 const port = ports[app.get('env')];
 
-// Error logging needs to be after the last route
-var error_logging_middleware = require('./app/api/utils/error_logging_middleware.js');
-app.use(error_logging_middleware);
-
-var errorLog = require('./app/api/log/errorLog.js').createErrorLog();
+var error_handling_middleware = require('./app/api/utils/error_handling_middleware.js');
+app.use(error_handling_middleware);
 
-process.on('unhandledRejection', err => {
-  let result = err;
-  if (err instanceof Error) {
-    result = err.stack.split('\n');
-  }
+var handleError = require('./app/api/utils/handleError.js');
 
-  if (err.code) {
-    result = err.message;
-  }
+process.on('unhandledRejection', error => {
+  handleError(error, true);
+  process.exit(1);
+});
 
-  errorLog.error(result);
+process.on('uncaughtException', error => {
+  handleError(error, true);
+  process.exit(1);
 });
 
 var mongoose = require('mongoose');