Merge pull request #1573 from pelias/treat-timeouts-as-502

Return 502 response code for service timeouts instead of 400

Author: orangejulius

middleware/sendJSON.js

@@ -2,13 +2,15 @@ const _ = require('lodash');
 const es = require('elasticsearch');
 const logger = require( 'pelias-logger' ).get( 'api' );
 const PeliasParameterError = require('../sanitizer/PeliasParameterError');
+const PeliasTimeoutError = require('../sanitizer/PeliasTimeoutError');
 
 function isParameterError(error) {
   return error instanceof PeliasParameterError;
 }
 
 function isTimeoutError(error) {
-  return error instanceof es.errors.RequestTimeout;
+  return error instanceof PeliasTimeoutError ||
+         error instanceof es.errors.RequestTimeout;
 }
 
 function isElasticsearchError(error) {

sanitizer/PeliasParameterError.js

@@ -3,9 +3,7 @@ class PeliasParameterError extends Error {
     super(message);
   }
 
-  // syntax had to be changed due to jshint bug
-  // https://github.com/jshint/jshint/issues/3358
-  ['toString']() {
+  toString() {
     return this.message;
   }
 

sanitizer/PeliasTimeoutError.js

@@ -0,0 +1,16 @@
+// Error subclass for timeouts contacting Pelias services
+class PeliasTimeoutError extends Error {
+  constructor(message = '') {
+    super(message);
+  }
+
+  toString() {
+    return this.message;
+  }
+
+  toJSON() {
+    return this.message;
+  }
+}
+
+module.exports = PeliasTimeoutError;

sanitizer/sanitizeAll.js

@@ -1,4 +1,28 @@
 const PeliasParameterError = require('./PeliasParameterError');
+const PeliasTimeoutError = require('../sanitizer/PeliasTimeoutError');
+
+function getCorrectErrorType(message) {
+  if (message.includes( 'Timeout')) {
+    return new PeliasTimeoutError(message);
+  }
+
+  // most errors are parameter errors
+  return new PeliasParameterError(message);
+}
+
+// Some Pelias code returns Error objects,
+// some returns strings. We want to standardize
+// on Error objects here
+function ensureInstanceOfError(error) {
+  if (error instanceof Error) {
+    // preserve the message and stack trace of existing Error objecs
+    const newError = getCorrectErrorType(error.message);
+    newError.stack = error.stack;
+    return newError;
+  }
+
+  return getCorrectErrorType(error);
+}
 
 function sanitize( req, sanitizers ){
   // init an object to store clean (sanitized) input parameters if not initialized
@@ -21,25 +45,15 @@ 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 ){
       req.warnings = req.warnings.concat( sanity.warnings );
     }
   }
+
+  // all errors must be mapped to a correct Error type to trigger the right HTTP response codes
+  req.errors = req.errors.map(ensureInstanceOfError);
 }
 
 // Adds to goodParameters every acceptable parameter passed through API call

test/unit/middleware/sendJSON.js

@@ -1,5 +1,6 @@
-var es = require('elasticsearch'),
-    middleware = require('../../../middleware/sendJSON');
+const es = require('elasticsearch');
+const middleware = require('../../../middleware/sendJSON');
+const PeliasTimeoutError = require('../../../sanitizer/PeliasTimeoutError');
 
 module.exports.tests = {};
 
@@ -206,6 +207,24 @@ module.exports.tests.search_phase_execution_exception = function(test, common) {
   });
 };
 
+module.exports.tests.service_timeout_exception = function(test, common) {
+  test('service timeout exception', function(t) {
+    var res = { body: { geocoding: {
+      errors: [ new PeliasTimeoutError('Timeout: could not reach Placeholder service') ]
+    }}};
+
+    res.status = function( code ){
+      return { json: function( body ){
+        t.equal( code, 502, 'Bad Gateway' );
+        t.deepEqual( body, res.body, 'body set' );
+        t.end();
+      }};
+    };
+
+    middleware(null, res);
+  });
+};
+
 module.exports.tests.unknown_exception = function(test, common) {
   test('unknown exception', function(t) {
     var res = { body: { geocoding: {

test/unit/sanitizer/sanitizeAll.js

@@ -1,5 +1,6 @@
 var sanitizeAll = require('../../../sanitizer/sanitizeAll');
 const PeliasParameterError = require('../../../sanitizer/PeliasParameterError');
+const PeliasTimeoutError = require('../../../sanitizer/PeliasTimeoutError');
 
 module.exports.tests = {};
 
@@ -97,7 +98,7 @@ module.exports.tests.all = function(test, common) {
     t.end();
   });
 
-  test('Error objects should be converted to correct type', function(t) {
+  test('Normal error objects should be converted to PeliasParameterError type', function(t) {
     var req = {};
     var sanitizers = {
       'first': {
@@ -125,6 +126,33 @@ module.exports.tests.all = function(test, common) {
 
   });
 
+  test('Timeout error should be converted to PeliasTimeoutError type', function(t) {
+    var req = {};
+    const error_message = 'Timeout: could not reach Placeholder service';
+    var sanitizers = {
+      'first': {
+        sanitize: function(){
+          req.clean.a = 'first sanitizer';
+          return {
+            errors: [new Error(error_message)],
+            warnings: []
+          };
+        }
+      }
+    };
+
+    var expected_req = {
+      clean: {
+        a: 'first sanitizer'
+      },
+      errors: [ new PeliasTimeoutError(error_message) ],
+      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 = {