Merge pull request #1231 from pelias/fix-error-codes

Fix error code handling

middleware/sendJSON.js

@@ -1,72 +1,50 @@
-var check = require('check-types'),
-    es = require('elasticsearch'),
-    logger = require( 'pelias-logger' ).get( 'api' ),
-    exceptions = require('elasticsearch-exceptions/lib/exceptions/SupportedExceptions');
+const _ = require('lodash');
+const es = require('elasticsearch');
+const logger = require( 'pelias-logger' ).get( 'api' );
+const PeliasParameterError = require('../sanitizer/PeliasParameterError');
 
-// create a list of regular expressions to match against.
-// note: list created when the server starts up; for performance reasons.
-var exceptionRegexList = exceptions.map( function( exceptionName ){
-  return new RegExp( '^' + exceptionName );
-});
-
-function sendJSONResponse(req, res, next) {
-
-  // do nothing if no result data set
-  if (!res || !check.object(res.body) || !check.object(res.body.geocoding)) {
-    return next();
-  }
-
-  // default status
-  var statusCode = 200; // 200 OK
-
-  // vary status code whenever an error was reported
-  var geocoding = res.body.geocoding;
+function isParameterError(error) {
+  return error instanceof PeliasParameterError;
+}
 
-  if( check.array( geocoding.errors ) && geocoding.errors.length ){
+function isTimeoutError(error) {
+  return error instanceof es.errors.RequestTimeout;
+}
 
-    // default status for errors is 400 Bad Request
-    statusCode = 400; // 400 Bad Request
+function isElasticsearchError(error) {
+  const knownErrors = [ es.errors.NoConnections,
+                        es.errors.ConnectionFault ];
 
-    // iterate over all reported errors
-    geocoding.errors.forEach( function( err ){
+  return knownErrors.some(function(esError) {
+    return error instanceof esError;
+  });
+}
 
-      // custom status codes for instances of the Error() object.
-      if( err instanceof Error ){
-        /*
-          elasticsearch errors
-          see: https://github.com/elastic/elasticsearch-js/blob/master/src/lib/errors.js
+function sendJSONResponse(req, res, next) {
 
-          408 Request Timeout
-          500 Internal Server Error
-          502 Bad Gateway
-        */
-        if( err instanceof es.errors.RequestTimeout ){ statusCode = Math.max( statusCode, 408 ); }
-        else if( err instanceof es.errors.NoConnections ){ statusCode = Math.max( statusCode, 502 ); }
-        else if( err instanceof es.errors.ConnectionFault ){ statusCode = Math.max( statusCode, 502 ); }
-        else {
-          logger.warn( 'unknown geocoding error object:', err.constructor.name, err );
-          statusCode = Math.max( statusCode, 500 );
-        }
+  // do nothing if no result data set
+  const geocoding = _.get(res, 'body.geocoding');
 
-      /*
-        some elasticsearch errors are only returned as strings (not instances of Error).
-        in this case we (unfortunately) need to match the exception at position 0 inside the string.
-      */
-      } else if( check.string( err ) ){
-        for( var i=0; i<exceptionRegexList.length; i++ ){
-          // check error string against a list of known elasticsearch exceptions
-          if( err.match( exceptionRegexList[i] ) ){
-            statusCode = Math.max( statusCode, 500 );
-            break; // break on first match
-          }
-        }
-        logger.warn( 'unknown geocoding error string:', err );
-      } else {
-        logger.warn( 'unknown geocoding error type:', typeof err, err );
-      }
-    });
+  if (!_.isPlainObject(geocoding)) {
+    return next();
   }
 
+  const errors = geocoding.errors || [];
+
+  const errorCodes = errors.map(function(error) {
+    if (isParameterError(error)) {
+      return 400;
+    } else if (isTimeoutError(error)) {
+      return 408;
+    } else if (isElasticsearchError(error)) {
+      return 502;
+    } else {
+      return 500;
+    }
+  });
+
+  const statusCode = Math.max(200, ...errorCodes);
+
   // respond
   return res.status(statusCode).json(res.body);
 }

package.json

@@ -41,7 +41,6 @@
     "async": "^2.0.0",
     "check-types": "^7.0.0",
     "elasticsearch": "^15.0.0",
-    "elasticsearch-exceptions": "0.0.4",
     "express": "^4.8.8",
     "geojson": "^0.5.0",
     "geolib": "^2.0.18",

sanitizer/PeliasParameterError.js

@@ -0,0 +1,15 @@
+class PeliasParameterError extends Error {
+  constructor(message = '') {
+    super(message);
+  }
+
+  toString() {
+    return this.message;
+  }
+
+  toJSON() {
+    return this.message;
+  }
+}
+
+module.exports = PeliasParameterError;

sanitizer/sanitizeAll.js

@@ -1,3 +1,5 @@
+const PeliasParameterError = require('./PeliasParameterError');
+
 function sanitize( req, sanitizers ){
   // init an object to store clean (sanitized) input parameters if not initialized
   req.clean = req.clean || {};
@@ -19,6 +21,19 @@ function sanitize( req, sanitizers ){
       req.errors = req.errors.concat( sanity.errors );
     }
 
+    // all errors must be returned as PeliasParameterError object to trigger HTTP 400 errors
+    req.errors = req.errors.map(function(error) {
+      // replace any existing Error objects with the right class
+      // preserve the message and stack trace
+      if (error instanceof Error) {
+        const new_error = new PeliasParameterError(error.message);
+        new_error.stack = error.stack;
+        return new_error;
+      } else {
+        return new PeliasParameterError(error);
+      }
+    });
+
     // if warnings occurred then set them
     // on the req object.
     if( sanity.warnings.length ){

test/unit/middleware/sendJSON.js

@@ -70,7 +70,7 @@ module.exports.tests.default_error_status = function(test, common) {
 
     res.status = function( code ){
       return { json: function( body ){
-        t.equal( code, 400, '400 Bad Request' );
+        t.equal( code, 500, '500 Internal Server Error is default error' );
         t.deepEqual( body, res.body, 'body set' );
         t.end();
       }};
@@ -214,7 +214,7 @@ module.exports.tests.unknown_exception = function(test, common) {
 
     res.status = function( code ){
       return { json: function( body ){
-        t.equal( code, 400, '400 Bad Request' );
+        t.equal( code, 500, '500 error returned' );
         t.deepEqual( body, res.body, 'body set' );
         t.end();
       }};

test/unit/sanitizer/sanitizeAll.js

@@ -1,7 +1,14 @@
 var sanitizeAll = require('../../../sanitizer/sanitizeAll');
+const PeliasParameterError = require('../../../sanitizer/PeliasParameterError');
 
 module.exports.tests = {};
 
+function makeErrors(errors) {
+  return errors.map(function(error) {
+    return new PeliasParameterError(error);
+  });
+}
+
 module.exports.tests.all = function(test, common) {
   test('req.clean/errors/warnings should be initialized when they are not', function(t) {
     var req = {};
@@ -31,12 +38,16 @@ module.exports.tests.all = function(test, common) {
         a: 'first sanitizer',
         b: 'second sanitizer'
       },
-      errors: ['error 1', 'error 2', 'error 3'],
+      errors: makeErrors(['error 1', 'error 2', 'error 3']),
       warnings: ['warning 1', 'warning 2', 'warning 3']
     };
 
+
     sanitizeAll.runAllChecks(req, sanitizers);
-    t.deepEquals(req, expected_req);
+    t.deepEquals(req, expected_req, 'error messages are as expected');
+    t.ok(req.errors[0] instanceof PeliasParameterError, 'error has correct class');
+    t.ok(req.errors[1] instanceof PeliasParameterError, 'error has correct class');
+    t.ok(req.errors[2] instanceof PeliasParameterError, 'error has correct class');
     t.end();
 
   });
@@ -77,7 +88,7 @@ module.exports.tests.all = function(test, common) {
         a: 'first sanitizer',
         b: 'second sanitizer'
       },
-      errors: ['pre-existing error', 'error 1', 'error 2', 'error 3'],
+      errors: makeErrors(['pre-existing error', 'error 1', 'error 2', 'error 3']),
       warnings: ['pre-existing warning', 'warning 1', 'warning 2', 'warning 3']
     };
 
@@ -86,6 +97,35 @@ module.exports.tests.all = function(test, common) {
     t.end();
   });
 
+  test('Error objects should be converted to correct type', function(t) {
+    var req = {};
+    var sanitizers = {
+      'first': {
+        sanitize: function(){
+          req.clean.a = 'first sanitizer';
+          return {
+            errors: [new Error('error 1'), 'error 2'],
+            warnings: []
+          };
+        }
+      }
+    };
+
+    var expected_req = {
+      clean: {
+        a: 'first sanitizer'
+      },
+      errors: makeErrors(['error 1', 'error 2']),
+      warnings: []
+    };
+
+    sanitizeAll.runAllChecks(req, sanitizers);
+    t.deepEquals(req, expected_req);
+    t.end();
+
+  });
+
+
   test('req.query should be passed to individual sanitizers when available', function(t) {
     var req = {
       query: {