From 58701f01690eedba2e07613f297cbeb30ac3cf39 Mon Sep 17 00:00:00 2001
From: Stephen Hess <stephen.k.hess@gmail.com>
Date: Mon, 6 Mar 2017 17:05:49 -0500
Subject: [PATCH] return do-nothing PiP lookup function if config lacks
 `pipService`

- added URI-formatted `api.pipService` config support in schema
- added predicate that determines whether pipService is enabled/disabled
- reworked should-execute conditions for non-coarse reverse
---
 .../predicates/is_pip_service_enabled.js      |  9 +++
 routes/v1.js                                  | 25 ++++--
 schema.js                                     |  3 +-
 service/pointinpolygon.js                     | 57 +++++++------
 .../predicates/is_pip_service_enabled.js      | 42 ++++++++++
 test/unit/run.js                              |  1 +
 test/unit/schema.js                           | 80 +++++++++++++++++++
 test/unit/service/pointinpolygon.js           | 31 ++++++-
 8 files changed, 218 insertions(+), 30 deletions(-)
 create mode 100644 controller/predicates/is_pip_service_enabled.js
 create mode 100644 test/unit/controller/predicates/is_pip_service_enabled.js

diff --git a/controller/predicates/is_pip_service_enabled.js b/controller/predicates/is_pip_service_enabled.js
new file mode 100644
index 00000000..0324da1d
--- /dev/null
+++ b/controller/predicates/is_pip_service_enabled.js
@@ -0,0 +1,9 @@
+const _ = require('lodash');
+
+module.exports = (uri) => {
+  // this predicate relies upon the fact that the schema has already validated
+  //  that api.pipService is a URI-formatted string
+  return (request, response) => {
+    return uri !== undefined;
+  };
+};
diff --git a/routes/v1.js b/routes/v1.js
index ecb19a3c..46f44893 100644
--- a/routes/v1.js
+++ b/routes/v1.js
@@ -60,9 +60,6 @@ var postProc = {
   assignLabels: require('../middleware/assignLabels')
 };
 
-// make configurable to return do-nothing service if PIP URL not set
-const pipService = require('../service/pointinpolygon')('http://localhost:3102');
-
 // predicates that drive whether controller/search runs
 const hasData = require('../controller/predicates/has_data');
 const hasErrors = require('../controller/predicates/has_errors');
@@ -80,6 +77,9 @@ const hasDataOrErrors = any(hasData, hasErrors);
 function addRoutes(app, peliasConfig) {
   const esclient = elasticsearch.Client(peliasConfig.esclient);
 
+  const isPipServiceEnabled = require('../controller/predicates/is_pip_service_enabled')(peliasConfig.api.pipService);
+  const pipService = require('../service/pointinpolygon')(peliasConfig.api.pipService);
+
   var base = '/v1/';
 
   /** ------------------------- routers ------------------------- **/
@@ -151,8 +151,23 @@ function addRoutes(app, peliasConfig) {
     reverse: createRouter([
       sanitizers.reverse.middleware,
       middleware.calcSize(),
-      controllers.coarse_reverse(pipService, all(isCoarseReverse, not(hasErrors))),
-      controllers.search(peliasConfig.api, esclient, queries.reverse, not(any(hasDataOrErrors, isCoarseReverse))),
+      controllers.coarse_reverse(pipService,
+        all(
+          not(hasErrors), isPipServiceEnabled, isCoarseReverse
+        )
+      ),
+      controllers.search(peliasConfig.api, esclient, queries.reverse,
+        // execute under the following conditions:
+        // - there are no errors or data
+        // - request is not coarse OR pip service is disabled
+        all(
+          not(hasDataOrErrors),
+          any(
+            not(isCoarseReverse),
+            not(isPipServiceEnabled)
+          )
+        )
+      ),
       postProc.distances('point.'),
       // reverse confidence scoring depends on distance from origin
       //  so it must be calculated first
diff --git a/schema.js b/schema.js
index 61e6c258..78a9141f 100644
--- a/schema.js
+++ b/schema.js
@@ -25,7 +25,8 @@ module.exports = Joi.object().keys({
     requestRetries: Joi.number().integer().min(0),
     localization: Joi.object().keys({
       flipNumberAndStreetCountries: Joi.array().items(Joi.string().regex(/^[A-Z]{3}$/))
-    }).unknown(false)
+    }).unknown(false),
+    pipService: Joi.string().uri({ scheme: /https?/ })
 
   }).requiredKeys('version', 'indexName', 'host').unknown(true),
   esclient: Joi.object().keys({
diff --git a/service/pointinpolygon.js b/service/pointinpolygon.js
index aaece195..21b6fdcf 100644
--- a/service/pointinpolygon.js
+++ b/service/pointinpolygon.js
@@ -1,33 +1,44 @@
 const logger = require( 'pelias-logger' ).get( 'pointinpolygon' );
 const request = require('request');
+const _ = require('lodash');
 
 module.exports = (url) => {
-  function service( centroid, callback ){
-    const requestUrl = `${url}/${centroid.lon}/${centroid.lat}`;
-
-    request.get(requestUrl, (err, response, body) => {
-      if (err) {
-        logger.error(JSON.stringify(err));
-        callback(err);
-      }
-      else if (response.statusCode === 200) {
-        try {
-          const parsed = JSON.parse(body);
-          callback(err, parsed);
+  if (!_.isEmpty(url)) {
+    logger.info(`using point-in-polygon service at ${url}`);
+
+    return function pointinpolygon( centroid, callback ) {
+      const requestUrl = `${url}/${centroid.lon}/${centroid.lat}`;
+
+      request.get(requestUrl, (err, response, body) => {
+        if (err) {
+          logger.error(JSON.stringify(err));
+          callback(err);
         }
-        catch (err) {
-          logger.error(`${requestUrl}: could not parse response body: ${body}`);
-          callback(`${requestUrl} returned status 200 but with non-JSON response: ${body}`);
+        else if (response.statusCode === 200) {
+          try {
+            const parsed = JSON.parse(body);
+            callback(err, parsed);
+          }
+          catch (err) {
+            logger.error(`${requestUrl}: could not parse response body: ${body}`);
+            callback(`${requestUrl} returned status 200 but with non-JSON response: ${body}`);
+          }
         }
-      }
-      else {
-        logger.error(`${requestUrl} returned status ${response.statusCode}: ${body}`);
-        callback(`${requestUrl} returned status ${response.statusCode}: ${body}`);
-      }
-    });
+        else {
+          logger.error(`${requestUrl} returned status ${response.statusCode}: ${body}`);
+          callback(`${requestUrl} returned status ${response.statusCode}: ${body}`);
+        }
+      });
 
-  }
+    };
 
-  return service;
+  } else {
+    logger.warn('point-in-polygon service disabled');
+
+    return (centroid, callback) => {
+      callback(`point-in-polygon service disabled, unable to resolve ${JSON.stringify(centroid)}`);
+    };
+
+  }
 
 };
diff --git a/test/unit/controller/predicates/is_pip_service_enabled.js b/test/unit/controller/predicates/is_pip_service_enabled.js
new file mode 100644
index 00000000..1d3cd63a
--- /dev/null
+++ b/test/unit/controller/predicates/is_pip_service_enabled.js
@@ -0,0 +1,42 @@
+'use strict';
+
+const _ = require('lodash');
+const is_pip_service_enabled = require('../../../../controller/predicates/is_pip_service_enabled');
+
+module.exports.tests = {};
+
+module.exports.tests.interface = (test, common) => {
+  test('valid interface', (t) => {
+    t.equal(typeof is_pip_service_enabled, 'function', 'is_pip_service_enabled  is a function');
+    t.equal(typeof is_pip_service_enabled(), 'function', 'is_pip_service_enabled() is a function');
+    t.end();
+  });
+};
+
+module.exports.tests.true_conditions = (test, common) => {
+  test('string uri should return true', (t) => {
+    t.ok(is_pip_service_enabled('pip uri')());
+    t.end();
+
+  });
+
+};
+
+module.exports.tests.false_conditions = (test, common) => {
+  test('undefined uri should return false', (t) => {
+    t.notOk(is_pip_service_enabled()());
+    t.end();
+
+  });
+
+};
+
+module.exports.all = (tape, common) => {
+  function test(name, testFunction) {
+    return tape(`GET /is_pip_service_enabled ${name}`, testFunction);
+  }
+
+  for( const testCase in module.exports.tests ){
+    module.exports.tests[testCase](test, common);
+  }
+};
diff --git a/test/unit/run.js b/test/unit/run.js
index a20d2d3a..b746d979 100644
--- a/test/unit/run.js
+++ b/test/unit/run.js
@@ -18,6 +18,7 @@ var tests = [
   require('./controller/predicates/has_data'),
   require('./controller/predicates/has_errors'),
   require('./controller/predicates/is_coarse_reverse'),
+  require('./controller/predicates/is_pip_service_enabled'),
   require('./helper/diffPlaces'),
   require('./helper/geojsonify'),
   require('./helper/logging'),
diff --git a/test/unit/schema.js b/test/unit/schema.js
index e1f9cfd2..6ed11add 100644
--- a/test/unit/schema.js
+++ b/test/unit/schema.js
@@ -367,6 +367,86 @@ module.exports.tests.api_validation = (test, common) => {
 
   });
 
+  test('non-string api.pipService should throw error', (t) => {
+    [null, 17, {}, [], true].forEach((value) => {
+      var config = {
+        api: {
+          version: 'version value',
+          indexName: 'index name value',
+          host: 'host value',
+          pipService: value
+        },
+        esclient: {}
+      };
+
+      t.throws(validate.bind(null, config), /"pipService" must be a string/);
+
+    });
+
+    t.end();
+
+  });
+
+  test('non-URI-formatted api.pipService should throw error', (t) => {
+    ['this is not a URI'].forEach((value) => {
+      var config = {
+        api: {
+          version: 'version value',
+          indexName: 'index name value',
+          host: 'host value',
+          pipService: value
+        },
+        esclient: {}
+      };
+
+      t.throws(validate.bind(null, config), /"pipService" must be a valid uri/);
+
+    });
+
+    t.end();
+
+  });
+
+  test('non-http/https api.pipService should throw error', (t) => {
+    ['ftp', 'git', 'unknown'].forEach((scheme) => {
+      var config = {
+        api: {
+          version: 'version value',
+          indexName: 'index name value',
+          host: 'host value',
+          pipService: `${scheme}://localhost`
+        },
+        esclient: {}
+      };
+
+      t.throws(validate.bind(null, config), /"pipService" must be a valid uri/);
+
+    });
+
+    t.end();
+
+  });
+
+  test('http/https api.pipService should not throw error', (t) => {
+    ['http', 'https'].forEach((scheme) => {
+      var config = {
+        api: {
+          version: 'version value',
+          indexName: 'index name value',
+          host: 'host value',
+          pipService: `${scheme}://localhost`
+        },
+        esclient: {}
+      };
+
+      t.doesNotThrow(validate.bind(null, config), `${scheme} should be allowed`);
+
+    });
+
+    t.end();
+
+  });
+
 };
 
 module.exports.tests.esclient_validation = (test, common) => {
diff --git a/test/unit/service/pointinpolygon.js b/test/unit/service/pointinpolygon.js
index a85fcd96..ac08b3f7 100644
--- a/test/unit/service/pointinpolygon.js
+++ b/test/unit/service/pointinpolygon.js
@@ -17,6 +17,26 @@ module.exports.tests.interface = (test, common) => {
   });
 };
 
+module.exports.tests.do_nothing_service = (test, common) => {
+  test('undefined PiP uri should return service that logs fact that PiP service is not available', (t) => {
+    const logger = require('pelias-mock-logger')();
+
+    const service = proxyquire('../../../service/pointinpolygon', {
+      'pelias-logger': logger
+    })();
+
+    service({ lat: 12.121212, lon: 21.212121 }, (err) => {
+      t.deepEquals(logger.getWarnMessages(), [
+        'point-in-polygon service disabled'
+      ]);
+      t.equals(err, 'point-in-polygon service disabled, unable to resolve {"lat":12.121212,"lon":21.212121}');
+      t.end();
+    });
+
+  });
+
+};
+
 module.exports.tests.success = (test, common) => {
   test('lat and lon should be passed to server', (t) => {
     const pipServer = require('express')();
@@ -28,12 +48,21 @@ module.exports.tests.success = (test, common) => {
     });
 
     const server = pipServer.listen();
+    const port = server.address().port;
 
-    const service = setup(`http://localhost:${server.address().port}`);
+    const logger = require('pelias-mock-logger')();
+
+    const service = proxyquire('../../../service/pointinpolygon', {
+      'pelias-logger': logger
+    })(`http://localhost:${port}`);
 
     service({ lat: 12.121212, lon: 21.212121}, (err, results) => {
       t.notOk(err);
       t.deepEquals(results, { field: 'value' });
+
+      t.ok(logger.isInfoMessage(`using point-in-polygon service at http://localhost:${port}`));
+      t.notOk(logger.hasErrorMessages());
+      
       t.end();
 
       server.close();