remove address/venue/street from requested layers when creating results

refactored for cleanliness and clarity

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,24 +16,47 @@ const granularities = [
   'country'
 ];
 
-function getMostGranularLayer(results) {
-  return granularities.find((granularity) => {
-    return results.hasOwnProperty(granularity);
+// remove non-coarse layers and return what's left (or all if empty)
+function getEffectiveLayers(requested_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);
   });
 }
 
 //  removing non-coarse layers could leave effective_layers empty, so it's
 //  important to check for empty layers here
-function hasResultsAtRequestedLayers(results, layers) {
-  return _.isEmpty(layers) || !_.isEmpty(_.intersection(layers, Object.keys(results)));
+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
-  // result available as the starting point
-  // 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);
+  // find the most granular layer to use as the document layer
+  const most_granular_layer = getMostGranularLayerOfResult(_.keys(results));
   const id = results[most_granular_layer][0].id;
 
   const doc = new Document('whosonfirst', most_granular_layer, id.toString());
@@ -86,8 +110,8 @@ function setup(service, should_execute) {
     }
 
     // because coarse reverse is called when non-coarse reverse didn't return
-    //  anything, don't consider non-coarse layers when filtering
-    const effective_layers = _.without(req.clean.layers, 'venue', 'address', 'street');
+    //  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'],
@@ -102,27 +126,20 @@ function setup(service, should_execute) {
         return next();
       }
 
-      // find the finest granularity requested
-      const finest_granularity_requested = granularities.findIndex((granularity) => {
-        return effective_layers.indexOf(granularity) !== -1;
-      });
+      // log how many results there were
+      logger.info(`[controller:coarse_reverse][queryType:pip][result_count:${_.size(results)}]`);
 
-      logger.info(`[controller:coarse_reverse][queryType:pip][result_count:${Object.keys(results).length}]`);
-
-      // now remove everything from the response that is more granular than the
-      // most granular layer requested.  that is, if effective_layers=['county'],
-      // remove neighbourhoods, localities, and localadmins
-      Object.keys(results).forEach((layer) => {
-        if (granularities.indexOf(layer) < finest_granularity_requested) {
-          delete results[layer];
-        }
-      });
+      // 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'],
+      // remove neighbourhoods, boroughs, localities, localadmins
+      const applicable_results = _.pick(results, getApplicableRequestedLayers(effective_layers));
 
       res.meta = {};
       res.data = [];
-      // synthesize a doc from results if there's a result at the request layer(s)
-      if (hasResultsAtRequestedLayers(results, effective_layers)) {
-        res.data.push(synthesizeDoc(results));
+
+      // if there's a result at the requested layer(s), synthesize a doc from results
+      if (hasResultsAtRequestedLayers(applicable_results, effective_layers)) {
+        res.data.push(synthesizeDoc(applicable_results));
       }
 
       return next();

test/unit/controller/coarse_reverse.js

@@ -465,6 +465,92 @@ module.exports.tests.success_conditions = (test, common) => {
 
   });
 
+  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(5);
+
+    const service = (point, do_not_track, callback) => {
+      t.equals(do_not_track, 'do_not_track value');
+      const results = {
+        neighbourhood: [
+          {
+            id: 10,
+            name: 'neighbourhood name',
+            abbr: 'neighbourhood abbr'
+          }
+        ]
+      };
+
+      callback(undefined, results);
+    };
+
+    const logger = require('pelias-mock-logger')();
+
+    const should_execute = () => { return true; };
+    const controller = proxyquire('../../../controller/coarse_reverse', {
+      'pelias-logger': logger,
+      '../helper/logging': {
+        isDNT: () => {
+          return 'do_not_track value';
+        }
+      }
+    })(service, should_execute);
+
+    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.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