added better error handling

controller/placeholder.js

@@ -57,10 +57,19 @@ function setup(placeholderService, should_execute) {
     }
 
     placeholderService.search(req.clean.text, req.clean.lang.iso6393, logging.isDNT(req), (err, results) => {
-      res.meta = {};
-      res.data = _.flatten(results.map((result) => {
-        return synthesizeDocs(result);
-      }));
+      if (err) {
+        if (_.isObject(err) && err.message) {
+          req.errors.push( err.message );
+        } else {
+          req.errors.push( err );
+        }
+
+      } else {
+        res.meta = {};
+        res.data = _.flatten(results.map((result) => {
+          return synthesizeDocs(result);
+        }));
+      }
 
       return next();
     });

service/placeholder.js

@@ -19,11 +19,15 @@ module.exports = function setup(url) {
   logger.info(`using placeholder service at ${url}`);
   return {
     search: (text, lang, do_not_track, callback) => {
-      const requestUrl = `${url}/search?text=${text}&lang=${lang}`;
+      const requestUrl = `${url}/search`;
 
       const options = {
         method: 'GET',
-        url: requestUrl
+        url: requestUrl,
+        qs: {
+          text: text,
+          lang: lang
+        }
       };
 
       if (do_not_track) {
@@ -40,22 +44,35 @@ module.exports = function setup(url) {
             try {
               const parsed = JSON.parse(data);
               return callback(null, parsed);
+              
             }
             catch (err) {
-              logger.error(`${encodeURI(requestUrl)} could not parse response: ${data}`);
-              return callback(`${encodeURI(requestUrl)} could not parse response: ${data}`);
+              if (do_not_track) {
+                logger.error(`${requestUrl} could not parse response: ${data}`);
+                return callback(`${requestUrl} could not parse response: ${data}`);
+              } else {
+                logger.error(`${response.request.href} could not parse response: ${data}`);
+                return callback(`${response.request.href} could not parse response: ${data}`);
+              }
+
             }
           }
           else {
             // otherwise there was a non-200 status so handle generically
-            logger.error(`${encodeURI(requestUrl)} returned status ${response.statusCode}: ${data}`);
-            return callback(`${encodeURI(requestUrl)} returned status ${response.statusCode}: ${data}`);
+            if (do_not_track) {
+              logger.error(`${requestUrl} returned status ${response.statusCode}: ${data}`);
+              return callback(`${requestUrl} returned status ${response.statusCode}: ${data}`);
+            } else {
+              logger.error(`${response.request.href} returned status ${response.statusCode}: ${data}`);
+              return callback(`${response.request.href} returned status ${response.statusCode}: ${data}`);
+            }
+
           }
         }));
 
       })
       .on('error', (err) => {
-        logger.error(JSON.stringify(err));
+        logger.error(`${requestUrl}: ${JSON.stringify(err)}`);
         callback(err);
       });
 

test/unit/controller/placeholder.js

@@ -449,6 +449,102 @@ module.exports.tests.do_not_track = function(test, common) {
 
 };
 
+module.exports.tests.error_conditions = (test, common) => {
+  test('service return error string should add to req.errors', (t) => {
+    const placeholderService = {
+      search: (text, language, do_not_track, callback) => {
+        callback('placeholder service error', []);
+      }
+    };
+
+    const should_execute = (req, res) => {
+      return true;
+    };
+
+    const controller = placeholder(placeholderService, should_execute);
+
+    const req = {
+      clean: {
+        text: 'query value',
+        lang: {
+          iso6393: 'language value'
+        }
+      },
+      errors: []
+    };
+    const res = {};
+
+    controller(req, res, () => {
+      t.deepEquals(req.errors, ['placeholder service error']);
+      t.end();
+    });
+
+  });
+
+  test('service return error object should add message to req.errors', (t) => {
+    const placeholderService = {
+      search: (text, language, do_not_track, callback) => {
+        callback(Error('placeholder service error'), []);
+      }
+    };
+
+    const should_execute = (req, res) => {
+      return true;
+    };
+
+    const controller = placeholder(placeholderService, should_execute);
+
+    const req = {
+      clean: {
+        text: 'query value',
+        lang: {
+          iso6393: 'language value'
+        }
+      },
+      errors: []
+    };
+    const res = {};
+
+    controller(req, res, () => {
+      t.deepEquals(req.errors, ['placeholder service error']);
+      t.end();
+    });
+
+  });
+
+  test('service return error object should add stringified error to req.errors', (t) => {
+    const placeholderService = {
+      search: (text, language, do_not_track, callback) => {
+        callback({ error_key: 'error_value' }, []);
+      }
+    };
+
+    const should_execute = (req, res) => {
+      return true;
+    };
+
+    const controller = placeholder(placeholderService, should_execute);
+
+    const req = {
+      clean: {
+        text: 'query value',
+        lang: {
+          iso6393: 'language value'
+        }
+      },
+      errors: []
+    };
+    const res = {};
+
+    controller(req, res, () => {
+      t.deepEquals(req.errors, [{ error_key: 'error_value' }]);
+      t.end();
+    });
+
+  });
+
+};
+
 module.exports.all = function (tape, common) {
 
   function test(name, testFunction) {

test/unit/service/placeholder.js

@@ -64,6 +64,31 @@ module.exports.tests.failure_conditions = (test, common) => {
 
   });
 
+  test('server returning error should log it sanitized and return no results', (t) => {
+    const server = express().listen();
+    const port = server.address().port;
+
+    // immediately close the server so to ensure an error response
+    server.close();
+
+    const logger = require('pelias-mock-logger')();
+
+    const service = proxyquire('../../../service/placeholder', {
+      'pelias-logger': logger
+    })(`http://localhost:${port}`);
+
+    service.search('search text', 'search lang', false, (err, results) => {
+      t.equals(err.code, 'ECONNREFUSED');
+      t.notOk(results);
+      t.ok(logger.isErrorMessage(/ECONNREFUSED/), 'there should be a connection refused error message');
+      t.end();
+
+      server.close();
+
+    });
+
+  });
+
   test('server returning non-200 response should log error and return no results', (t) => {
     const placeholderServer = express();
     placeholderServer.get('/search', (req, res, next) => {
@@ -92,6 +117,34 @@ module.exports.tests.failure_conditions = (test, common) => {
 
   });
 
+  test('server returning non-200 response should log sanitized error when do_not_track and return no results', (t) => {
+    const placeholderServer = express();
+    placeholderServer.get('/search', (req, res, next) => {
+      res.status(400).send('a bad request was made');
+    });
+
+    const server = placeholderServer.listen();
+    const port = server.address().port;
+
+    const logger = require('pelias-mock-logger')();
+
+    const service = proxyquire('../../../service/placeholder', {
+      'pelias-logger': logger
+    })(`http://localhost:${port}`);
+
+    service.search('search text', 'search lang', true, (err, results) => {
+      t.equals(err, `http://localhost:${port}/search returned status 400: a bad request was made`);
+      t.notOk(results);
+      t.ok(logger.isErrorMessage(`http://localhost:${port}/search ` +
+        `returned status 400: a bad request was made`));
+      t.end();
+
+      server.close();
+
+    });
+
+  });
+
   test('server returning 404 statusCode should be treated the same as other statusCodes', (t) => {
     // at one point placeholder treated 0 results as a 404 instead of just an unparseable input
     const placeholderServer = express();
@@ -150,6 +203,35 @@ module.exports.tests.failure_conditions = (test, common) => {
 
   });
 
+  test('server returning 200 statusCode but with non-JSON response should log sanitized error and return undefined', (t) => {
+    const placeholderServer = express();
+    placeholderServer.get('/search', (req, res, next) => {
+      res.status(200).send('this is not parseable as JSON');
+    });
+
+    const server = placeholderServer.listen();
+    const port = server.address().port;
+
+    const logger = require('pelias-mock-logger')();
+
+    const service = proxyquire('../../../service/placeholder', {
+      'pelias-logger': logger
+    })(`http://localhost:${port}`);
+
+    service.search('search text', 'search lang', true, (err, results) => {
+      t.equals(err, `http://localhost:${port}/search ` +
+        `could not parse response: this is not parseable as JSON`);
+      t.notOk(results, 'should return undefined');
+      t.ok(logger.isErrorMessage(`http://localhost:${port}/search ` +
+        `could not parse response: this is not parseable as JSON`));
+      t.end();
+
+      server.close();
+
+    });
+
+  });
+
 };
 
 module.exports.tests.success_conditions = (test, common) => {