From ca599c9974664a59e9104b49302d0d0069359081 Mon Sep 17 00:00:00 2001
From: Julian Simioni <julian@simioni.org>
Date: Wed, 31 Oct 2018 22:24:26 -0400
Subject: [PATCH] fix(errors): Only return 400 status on known parameter errors

Pelias has for a long time returned 400 as a default status whenever
anything goes wrong, as well as when a user has passed invalid
parameters.

By using a new exception class, it is now possible to differetiate
between known parameter errors, and unexpected errors that truly
represent an HTTP 500.
---
 middleware/sendJSON.js             | 94 +++++++++++-------------------
 package.json                       |  1 -
 sanitizer/PeliasParameterError.js  | 15 +++++
 sanitizer/sanitizeAll.js           |  7 +++
 test/unit/middleware/sendJSON.js   |  4 +-
 test/unit/sanitizer/sanitizeAll.js | 11 +++-
 6 files changed, 67 insertions(+), 65 deletions(-)
 create mode 100644 sanitizer/PeliasParameterError.js

diff --git a/middleware/sendJSON.js b/middleware/sendJSON.js
index 584fdf0ee..6f1e248e2 100644
--- a/middleware/sendJSON.js
+++ b/middleware/sendJSON.js
@@ -1,71 +1,45 @@
-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 isTimeoutError(error) {
+  return error instanceof es.errors.RequestTimeout;
+}
+
+function isElasticsearchError(error) {
+  const knownErrors = [ es.errors.NoConnections,
+                        es.errors.ConnectionFault ];
+
+  return knownErrors.some(function(esError) {
+    return error instanceof esError;
+  });
+}
 
 function sendJSONResponse(req, res, next) {
 
   // do nothing if no result data set
-  if (!res || !check.object(res.body) || !check.object(res.body.geocoding)) {
+  const geocoding = _.get(res, 'body.geocoding');
+
+  if (!_.isObject(geocoding)) {
     return next();
   }
 
-  // default status
-  var statusCode = 200; // 200 OK
-
-  // vary status code whenever an error was reported
-  var geocoding = res.body.geocoding;
-
-  if( check.array( geocoding.errors ) && geocoding.errors.length ){
-
-    // default status for errors is 400 Bad Request
-    statusCode = 400; // 400 Bad Request
-
-    // iterate over all reported errors
-    geocoding.errors.forEach( function( err ){
-
-      // 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
-
-          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 );
-        }
-
-      /*
-        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 );
-      }
-    });
-  }
+  const errors = geocoding.errors || [];
+
+  const errorCodes = errors.map(function(error) {
+    if (error instanceof PeliasParameterError) {
+      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);
diff --git a/package.json b/package.json
index 25104b7a7..18c62748e 100644
--- a/package.json
+++ b/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",
diff --git a/sanitizer/PeliasParameterError.js b/sanitizer/PeliasParameterError.js
new file mode 100644
index 000000000..817cfade8
--- /dev/null
+++ b/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;
diff --git a/sanitizer/sanitizeAll.js b/sanitizer/sanitizeAll.js
index 113c31160..4875e7031 100644
--- a/sanitizer/sanitizeAll.js
+++ b/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,11 @@ 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) {
+      return new PeliasParameterError(error);
+    });
+
     // if warnings occurred then set them
     // on the req object.
     if( sanity.warnings.length ){
diff --git a/test/unit/middleware/sendJSON.js b/test/unit/middleware/sendJSON.js
index d02138ff8..8e7abeb7e 100644
--- a/test/unit/middleware/sendJSON.js
+++ b/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();
       }};
diff --git a/test/unit/sanitizer/sanitizeAll.js b/test/unit/sanitizer/sanitizeAll.js
index e8502813b..c2957c275 100644
--- a/test/unit/sanitizer/sanitizeAll.js
+++ b/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,7 +38,7 @@ 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']
     };
 
@@ -77,7 +84,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']
     };