Merge pull request #899 from pelias/add-coarse-reverse-fallback

Add coarse reverse fallback

controller/coarse_reverse.js

@@ -2,7 +2,8 @@ const logger = require('pelias-logger').get('coarse_reverse');
 const _ = require('lodash');
 const Document = require('pelias-model').Document;
 
-const granularities = [
+// do not change order, other functionality depends on most-to-least granular order
+const coarse_granularities = [
   'neighbourhood',
   'borough',
   'locality',
@@ -15,60 +16,79 @@ const granularities = [
   'country'
 ];
 
-function getMostGranularLayer(results) {
+// remove non-coarse layers and return what's left (or all if empty)
-  return granularities.find((granularity) => {
+function getEffectiveLayers(requested_layers) {
-    return results.hasOwnProperty(granularity);
+  // remove non-coarse layers
+  const non_coarse_layers_removed = _.without(requested_layers, 'venue', 'address', 'street');
+
+  // if resulting array is empty, use all coarse granularities
+  if (_.isEmpty(non_coarse_layers_removed)) {
+    return coarse_granularities;
+  }
+
+  // otherwise use requested layers with non-coarse layers removed
+  return non_coarse_layers_removed;
+
+}
+
+// drop from coarse_granularities until there's one that was requested
+// this depends on coarse_granularities being ordered
+function getApplicableRequestedLayers(requested_layers) {
+  return _.dropWhile(coarse_granularities, (coarse_granularity) => {
+    return !_.includes(requested_layers, coarse_granularity);
   });
 }
 
-function hasResultsAtRequestedLayers(results, layers) {
+//  removing non-coarse layers could leave effective_layers empty, so it's
-  return _.intersection(layers, Object.keys(results)).length > 0;
+//  important to check for empty layers here
+function hasResultsAtRequestedLayers(results, requested_layers) {
+  return !_.isEmpty(_.intersection(_.keys(results), requested_layers));
 }
 
+// get the most granular layer from the results by taking the head of the intersection
+// of coarse_granularities (which are ordered) and the result layers
+// ['neighbourhood', 'borough', 'locality'] - ['locality', 'borough'] = 'borough'
+// this depends on coarse_granularities being ordered
+function getMostGranularLayerOfResult(result_layers) {
+  return _.head(_.intersection(coarse_granularities, result_layers));
+}
+
+// create a model.Document from what's left, using the most granular
+// result available as the starting point
 function synthesizeDoc(results) {
-  // now create a model.Document from what's level, using the most granular
+  // find the most granular layer to use as the document layer
-  // result available as the starting point
+  const most_granular_layer = getMostGranularLayerOfResult(_.keys(results));
-  // the immediately above cannot be re-used since county may be the most
-  // granular layer requested but the results may start at region (no county found)
-  const most_granular_layer = getMostGranularLayer(results);
   const id = results[most_granular_layer][0].id;
 
   const doc = new Document('whosonfirst', most_granular_layer, id.toString());
   doc.setName('default', results[most_granular_layer][0].name);
 
-  if (results[most_granular_layer][0].hasOwnProperty('centroid')) {
+  // assign the administrative hierarchy
+  _.keys(results).forEach((layer) => {
+    doc.addParent(layer, results[layer][0].name, results[layer][0].id.toString(), results[layer][0].abbr);
+  });
+
+  // set centroid if available
+  if (_.has(results[most_granular_layer][0], 'centroid')) {
     doc.setCentroid( results[most_granular_layer][0].centroid );
   }
 
-  if (results[most_granular_layer][0].hasOwnProperty('bounding_box')) {
+  // set bounding box if available
-    const parsedBoundingBox = results[most_granular_layer][0].bounding_box.split(',').map(parseFloat);
+  if (_.has(results[most_granular_layer][0], 'bounding_box')) {
+    const parsed_bounding_box = results[most_granular_layer][0].bounding_box.split(',').map(parseFloat);
     doc.setBoundingBox({
       upperLeft: {
-        lat: parsedBoundingBox[3],
+        lat: parsed_bounding_box[3],
-        lon: parsedBoundingBox[0]
+        lon: parsed_bounding_box[0]
       },
       lowerRight: {
-        lat: parsedBoundingBox[1],
+        lat: parsed_bounding_box[1],
-        lon: parsedBoundingBox[2]
+        lon: parsed_bounding_box[2]
       }
     });
 
   }
 
-  if (_.has(results, 'country[0].abbr')) {
-    doc.setAlpha3(results.country[0].abbr);
-  }
-
-  // assign the administrative hierarchy
-  Object.keys(results).forEach((layer) => {
-    if (results[layer][0].hasOwnProperty('abbr')) {
-      doc.addParent(layer, results[layer][0].name, results[layer][0].id.toString(), results[layer][0].abbr);
-    } else {
-      doc.addParent(layer, results[layer][0].name, results[layer][0].id.toString());
-    }
-
-  });
-
   const esDoc = doc.toESDocument();
   esDoc.data._id = esDoc._id;
   esDoc.data._type = esDoc._type;
@@ -83,6 +103,15 @@ function setup(service, should_execute) {
       return next();
     }
 
+    // because coarse reverse is called when non-coarse reverse didn't return
+    //  anything, treat requested layers as if it didn't contain non-coarse layers
+    const effective_layers = getEffectiveLayers(req.clean.layers);
+
+    const centroid = {
+      lat: req.clean['point.lat'],
+      lon: req.clean['point.lon']
+    };
+
     service(req, (err, results) => {
       // if there's an error, log it and bail
       if (err) {
@@ -91,27 +120,20 @@ function setup(service, should_execute) {
         return next();
       }
 
-      // find the finest granularity requested
+      // log how many results there were
-      const finest_granularity_requested = granularities.findIndex((granularity) => {
+      logger.info(`[controller:coarse_reverse][queryType:pip][result_count:${_.size(results)}]`);
-        return req.clean.layers.indexOf(granularity) !== -1;
-      });
 
-      logger.info(`[controller:coarse_reverse][queryType:pip][result_count:${Object.keys(results).length}]`);
+      // now keep everything from the response that is equal to or less granular
-
+      // than the most granular layer requested.  that is, if effective_layers=['county'],
-      // now remove everything from the response that is more granular than the
+      // remove neighbourhoods, boroughs, localities, localadmins
-      // most granular layer requested.  that is, if req.clean.layers=['county'],
+      const applicable_results = _.pick(results, getApplicableRequestedLayers(effective_layers));
-      // remove neighbourhoods, localities, and localadmins
-      Object.keys(results).forEach((layer) => {
-        if (granularities.indexOf(layer) < finest_granularity_requested) {
-          delete results[layer];
-        }
-      });
 
       res.meta = {};
       res.data = [];
-      // synthesize a doc from results if there's a result at the request layer(s)
+
-      if (hasResultsAtRequestedLayers(results, req.clean.layers)) {
+      // if there's a result at the requested layer(s), synthesize a doc from results
-        res.data.push(synthesizeDoc(results));
+      if (hasResultsAtRequestedLayers(applicable_results, effective_layers)) {
+        res.data.push(synthesizeDoc(applicable_results));
       }
 
       return next();

controller/predicates/is_coarse_reverse.js

@@ -5,5 +5,5 @@ const non_coarse_layers = ['address', 'street', 'venue'];
 module.exports = (req, res) => {
   // returns true if layers is undefined, empty, or contains 'address', 'street', or 'venue'
   return !_.isEmpty(req.clean.layers) &&
-          _.intersection(req.clean.layers, non_coarse_layers).length === 0;
+          _.isEmpty(_.intersection(req.clean.layers, non_coarse_layers));
 };

routes/v1.js

@@ -97,8 +97,14 @@ function addRoutes(app, peliasConfig) {
   const placeholderService = serviceWrapper(placeholderConfiguration);
   const isPlaceholderServiceEnabled = _.constant(placeholderConfiguration.isEnabled());
 
+  // use coarse reverse when requested layers are all coarse
   const coarse_reverse_should_execute = all(
-    not(hasRequestErrors), isPipServiceEnabled, isCoarseReverse
+    isPipServiceEnabled, isCoarseReverse, not(hasRequestErrors)
+  );
+
+  // fallback to coarse reverse when regular reverse didn't return anything
+  const coarse_reverse_fallback_should_execute = all(
+    isPipServiceEnabled, not(isCoarseReverse), not(hasRequestErrors), not(hasResponseData)
   );
 
   const placeholderShouldExecute = all(
@@ -198,6 +204,7 @@ function addRoutes(app, peliasConfig) {
       middleware.calcSize(),
       controllers.coarse_reverse(pipService, coarse_reverse_should_execute),
       controllers.search(peliasConfig.api, esclient, queries.reverse, original_reverse_should_execute),
+      controllers.coarse_reverse(pipService, coarse_reverse_fallback_should_execute),
       postProc.distances('point.'),
       // reverse confidence scoring depends on distance from origin
       //  so it must be calculated first

test/unit/controller/coarse_reverse.js

@@ -2,6 +2,7 @@
 
 const setup = require('../../../controller/coarse_reverse');
 const proxyquire =  require('proxyquire').noCallThru();
+const _  = require('lodash');
 
 module.exports.tests = {};
 
@@ -15,12 +16,13 @@ module.exports.tests.interface = (test, common) => {
 
 module.exports.tests.early_exit_conditions = (test, common) => {
   test('should_execute returning false should not call service', (t) => {
+    t.plan(2);
+
     const service = () => {
       throw Error('service should not have been called');
     };
 
-    const should_execute = () => { return false; };
+    const controller = setup(service, _.constant(false));
-    const controller = setup(service, should_execute);
 
     const req = {
       clean: {
@@ -30,14 +32,12 @@ module.exports.tests.early_exit_conditions = (test, common) => {
     };
 
     // verify that next was called
-    let next_was_called = false;
     const next = () => {
-      next_was_called = true;
+      t.pass('next() was called');
     };
 
     // passing res=undefined verifies that it wasn't interacted with
     t.doesNotThrow(controller.bind(null, req, undefined, next));
-    t.ok(next_was_called);
     t.end();
 
   });
@@ -46,6 +46,8 @@ module.exports.tests.early_exit_conditions = (test, common) => {
 
 module.exports.tests.error_conditions = (test, common) => {
   test('service error should log and call next', (t) => {
+    t.plan(3);
+
     const service = (req, callback) => {
       t.deepEquals(req, { clean: { layers: ['locality'] } } );
       callback('this is an error');
@@ -53,10 +55,9 @@ module.exports.tests.error_conditions = (test, common) => {
 
     const logger = require('pelias-mock-logger')();
 
-    const should_execute = () => { return true; };
     const controller = proxyquire('../../../controller/coarse_reverse', {
       'pelias-logger': logger
-    })(service, should_execute);
+    })(service, _.constant(true));
 
     const req = {
       clean: {
@@ -65,16 +66,14 @@ module.exports.tests.error_conditions = (test, common) => {
     };
 
     // verify that next was called
-    let next_was_called = false;
     const next = () => {
-      next_was_called = true;
+      t.pass('next() was called');
     };
 
     // passing res=undefined verifies that it wasn't interacted with
     controller(req, undefined, next);
 
     t.ok(logger.isErrorMessage('this is an error'));
-    t.ok(next_was_called);
     t.end();
 
   });
@@ -83,6 +82,8 @@ module.exports.tests.error_conditions = (test, common) => {
 
 module.exports.tests.success_conditions = (test, common) => {
   test('service returning results should use first entry for each layer', (t) => {
+    t.plan(4);
+
     const service = (req, callback) => {
       t.deepEquals(req, { clean: { layers: ['neighbourhood'] } } );
 
@@ -143,10 +144,9 @@ module.exports.tests.success_conditions = (test, common) => {
 
     const logger = require('pelias-mock-logger')();
 
-    const should_execute = () => { return true; };
     const controller = proxyquire('../../../controller/coarse_reverse', {
       'pelias-logger': logger
-    })(service, should_execute);
+    })(service, _.constant(true));
 
     const req = {
       clean: {
@@ -157,9 +157,8 @@ module.exports.tests.success_conditions = (test, common) => {
     const res = { };
 
     // verify that next was called
-    let next_was_called = false;
     const next = () => {
-      next_was_called = true;
+      t.pass('next() was called');
     };
 
     controller(req, res, next);
@@ -211,7 +210,6 @@ module.exports.tests.success_conditions = (test, common) => {
             country_id: ['100'],
             country_a: ['xyz']
           },
-          alpha3: 'XYZ',
           center_point: {
             lat: 12.121212,
             lon: 21.212121
@@ -222,14 +220,14 @@ module.exports.tests.success_conditions = (test, common) => {
     };
 
     t.deepEquals(res, expected);
-
     t.notOk(logger.hasErrorMessages());
-    t.ok(next_was_called);
     t.end();
 
   });
 
   test('layers missing from results should be ignored', (t) => {
+    t.plan(4);
+
     const service = (req, callback) => {
       t.deepEquals(req, { clean: { layers: ['neighbourhood'] } } );
 
@@ -238,7 +236,6 @@ module.exports.tests.success_conditions = (test, common) => {
           {
             id: 10,
             name: 'neighbourhood name',
-            abbr: 'neighbourhood abbr',
             centroid: {
               lat: 12.121212,
               lon: 21.212121
@@ -253,10 +250,9 @@ module.exports.tests.success_conditions = (test, common) => {
 
     const logger = require('pelias-mock-logger')();
 
-    const should_execute = () => { return true; };
     const controller = proxyquire('../../../controller/coarse_reverse', {
       'pelias-logger': logger
-    })(service, should_execute);
+    })(service, _.constant(true));
 
     const req = {
       clean: {
@@ -267,9 +263,8 @@ module.exports.tests.success_conditions = (test, common) => {
     const res = { };
 
     // verify that next was called
-    let next_was_called = false;
     const next = () => {
-      next_was_called = true;
+      t.pass('next() was called');
     };
 
     controller(req, res, next);
@@ -292,7 +287,7 @@ module.exports.tests.success_conditions = (test, common) => {
           parent: {
             neighbourhood: ['neighbourhood name'],
             neighbourhood_id: ['10'],
-            neighbourhood_a: ['neighbourhood abbr']
+            neighbourhood_a: [null]
           },
           center_point: {
             lat: 12.121212,
@@ -304,14 +299,14 @@ module.exports.tests.success_conditions = (test, common) => {
     };
 
     t.deepEquals(res, expected);
-
     t.notOk(logger.hasErrorMessages());
-    t.ok(next_was_called);
     t.end();
 
   });
 
   test('most granular layer missing centroid should not set', (t) => {
+    t.plan(4);
+
     const service = (req, callback) => {
       t.deepEquals(req, { clean: { layers: ['neighbourhood'] } } );
 
@@ -331,10 +326,9 @@ module.exports.tests.success_conditions = (test, common) => {
 
     const logger = require('pelias-mock-logger')();
 
-    const should_execute = () => { return true; };
     const controller = proxyquire('../../../controller/coarse_reverse', {
       'pelias-logger': logger
-    })(service, should_execute);
+    })(service, _.constant(true));
 
     const req = {
       clean: {
@@ -345,9 +339,8 @@ module.exports.tests.success_conditions = (test, common) => {
     const res = { };
 
     // verify that next was called
-    let next_was_called = false;
     const next = () => {
-      next_was_called = true;
+      t.pass('next() was called');
     };
 
     controller(req, res, next);
@@ -378,14 +371,14 @@ module.exports.tests.success_conditions = (test, common) => {
     };
 
     t.deepEquals(res, expected);
-
     t.notOk(logger.hasErrorMessages());
-    t.ok(next_was_called);
     t.end();
 
   });
 
   test('most granular layer missing bounding_box should not set', (t) => {
+    t.plan(4);
+
     const service = (req, callback) => {
       t.deepEquals(req, { clean: { layers: ['neighbourhood'] } } );
 
@@ -408,10 +401,9 @@ module.exports.tests.success_conditions = (test, common) => {
 
     const logger = require('pelias-mock-logger')();
 
-    const should_execute = () => { return true; };
     const controller = proxyquire('../../../controller/coarse_reverse', {
       'pelias-logger': logger
-    })(service, should_execute);
+    })(service, _.constant(true));
 
     const req = {
       clean: {
@@ -422,9 +414,8 @@ module.exports.tests.success_conditions = (test, common) => {
     const res = { };
 
     // verify that next was called
-    let next_was_called = false;
     const next = () => {
-      next_was_called = true;
+      t.pass('next() was called');
     };
 
     controller(req, res, next);
@@ -458,9 +449,254 @@ module.exports.tests.success_conditions = (test, common) => {
     };
 
     t.deepEquals(res, expected);
+    t.notOk(logger.hasErrorMessages());
+    t.end();
+
+  });
+
+  test('no requested layers should use everything', (t) => {
+    // this test is used to test coarse reverse fallback for when non-coarse reverse
+    //  was requested but no non-coarse results were found
+
+    // by plan'ing the number of tests, we can verify that next() was called w/o
+    //  additional bookkeeping
+    t.plan(4);
+
+    const service = (req, callback) => {
+      const results = {
+        neighbourhood: [
+          {
+            id: 10,
+            name: 'neighbourhood name',
+            abbr: 'neighbourhood abbr'
+          }
+        ]
+      };
+
+      callback(undefined, results);
+    };
+
+    const logger = require('pelias-mock-logger')();
+
+    const controller = proxyquire('../../../controller/coarse_reverse', {
+      'pelias-logger': logger
+    })(service, _.constant(true));
 
+    const req = {
+      clean: {
+        layers: [],
+        point: {
+          lat: 12.121212,
+          lon: 21.212121
+        }
+      }
+    };
+
+    const res = { };
+
+    // verify that next was called
+    const next = () => {
+      t.pass('next() should have been called');
+    };
+
+    controller(req, res, next);
+
+    const expected = {
+      meta: {},
+      data: [
+        {
+          _id: '10',
+          _type: 'neighbourhood',
+          layer: 'neighbourhood',
+          source: 'whosonfirst',
+          source_id: '10',
+          name: {
+            'default': 'neighbourhood name'
+          },
+          phrase: {
+            'default': 'neighbourhood name'
+          },
+          parent: {
+            neighbourhood: ['neighbourhood name'],
+            neighbourhood_id: ['10'],
+            neighbourhood_a: ['neighbourhood abbr']
+          }
+        }
+      ]
+    };
+
+    t.deepEquals(req.clean.layers, [], 'req.clean.layers should be unmodified');
+    t.deepEquals(res, expected);
     t.notOk(logger.hasErrorMessages());
-    t.ok(next_was_called);
+
+    t.end();
+
+  });
+
+  test('layers specifying only venue, address, or street should not exclude coarse results', (t) => {
+    // this test is used to test coarse reverse fallback for when non-coarse reverse
+    //  was requested but no non-coarse results were found
+    const non_coarse_layers = ['venue', 'address', 'street'];
+    const tests_per_non_coarse_layer = 4;
+
+    // by plan'ing the number of tests, we can verify that next() was called w/o
+    //  additional bookkeeping
+    t.plan(non_coarse_layers.length * tests_per_non_coarse_layer);
+
+    non_coarse_layers.forEach((non_coarse_layer) => {
+      const service = (req, callback) => {
+        const results = {
+          neighbourhood: [
+            {
+              id: 10,
+              name: 'neighbourhood name',
+              abbr: 'neighbourhood abbr'
+            }
+          ]
+        };
+
+        callback(undefined, results);
+      };
+
+      const logger = require('pelias-mock-logger')();
+
+      const controller = proxyquire('../../../controller/coarse_reverse', {
+        'pelias-logger': logger
+      })(service, _.constant(true));
+
+      const req = {
+        clean: {
+          layers: [non_coarse_layer],
+          point: {
+            lat: 12.121212,
+            lon: 21.212121
+          }
+        }
+      };
+
+      const res = { };
+
+      // verify that next was called
+      const next = () => {
+        t.pass('next() should have been called');
+      };
+
+      controller(req, res, next);
+
+      const expected = {
+        meta: {},
+        data: [
+          {
+            _id: '10',
+            _type: 'neighbourhood',
+            layer: 'neighbourhood',
+            source: 'whosonfirst',
+            source_id: '10',
+            name: {
+              'default': 'neighbourhood name'
+            },
+            phrase: {
+              'default': 'neighbourhood name'
+            },
+            parent: {
+              neighbourhood: ['neighbourhood name'],
+              neighbourhood_id: ['10'],
+              neighbourhood_a: ['neighbourhood abbr']
+            }
+          }
+        ]
+      };
+
+      t.deepEquals(req.clean.layers, [non_coarse_layer], 'req.clean.layers should be unmodified');
+      t.deepEquals(res, expected);
+      t.notOk(logger.hasErrorMessages());
+
+    });
+
+    t.end();
+
+  });
+
+  test('layers specifying venue, address, or street AND coarse layer should not exclude coarse results', (t) => {
+    // this test is used to test coarse reverse fallback for when non-coarse reverse
+    //  was requested but no non-coarse results were found
+    const non_coarse_layers = ['venue', 'address', 'street'];
+    const tests_per_non_coarse_layer = 4;
+
+    // by plan'ing the number of tests, we can verify that next() was called w/o
+    //  additional bookkeeping
+    t.plan(non_coarse_layers.length * tests_per_non_coarse_layer);
+
+    non_coarse_layers.forEach((non_coarse_layer) => {
+      const service = (req, callback) => {
+        const results = {
+          neighbourhood: [
+            {
+              id: 10,
+              name: 'neighbourhood name',
+              abbr: 'neighbourhood abbr'
+            }
+          ]
+        };
+
+        callback(undefined, results);
+      };
+
+      const logger = require('pelias-mock-logger')();
+
+      const controller = proxyquire('../../../controller/coarse_reverse', {
+        'pelias-logger': logger
+      })(service, _.constant(true));
+
+      const req = {
+        clean: {
+          layers: [non_coarse_layer, 'neighbourhood'],
+          point: {
+            lat: 12.121212,
+            lon: 21.212121
+          }
+        }
+      };
+
+      const res = { };
+
+      // verify that next was called
+      const next = () => {
+        t.pass('next() should have been called');
+      };
+
+      controller(req, res, next);
+
+      const expected = {
+        meta: {},
+        data: [
+          {
+            _id: '10',
+            _type: 'neighbourhood',
+            layer: 'neighbourhood',
+            source: 'whosonfirst',
+            source_id: '10',
+            name: {
+              'default': 'neighbourhood name'
+            },
+            phrase: {
+              'default': 'neighbourhood name'
+            },
+            parent: {
+              neighbourhood: ['neighbourhood name'],
+              neighbourhood_id: ['10'],
+              neighbourhood_a: ['neighbourhood abbr']
+            }
+          }
+        ]
+      };
+
+      t.deepEquals(req.clean.layers, [non_coarse_layer, 'neighbourhood'], 'req.clean.layers should be unmodified');
+      t.deepEquals(res, expected);
+      t.notOk(logger.hasErrorMessages());
+
+    });
+
     t.end();
 
   });
@@ -469,6 +705,8 @@ module.exports.tests.success_conditions = (test, common) => {
 
 module.exports.tests.failure_conditions = (test, common) => {
   test('service returning 0 results at the requested layer should return nothing', (t) => {
+    t.plan(4);
+
     const service = (req, callback) => {
       t.deepEquals(req, { clean: { layers: ['neighbourhood'] } } );
 
@@ -517,10 +755,9 @@ module.exports.tests.failure_conditions = (test, common) => {
 
     const logger = require('pelias-mock-logger')();
 
-    const should_execute = () => { return true; };
     const controller = proxyquire('../../../controller/coarse_reverse', {
       'pelias-logger': logger
-    })(service, should_execute);
+    })(service, _.constant(true));
 
     const req = {
       clean: {
@@ -531,9 +768,8 @@ module.exports.tests.failure_conditions = (test, common) => {
     const res = { };
 
     // verify that next was called
-    let next_was_called = false;
     const next = () => {
-      next_was_called = true;
+      t.pass('next() was called');
     };
 
     controller(req, res, next);
@@ -544,9 +780,7 @@ module.exports.tests.failure_conditions = (test, common) => {
     };
 
     t.deepEquals(res, expected);
-
     t.notOk(logger.hasErrorMessages());
-    t.ok(next_was_called);
     t.end();
 
   });