From 39e91d82338e65159ac806cc5ec3fa465729f613 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 2 Jun 2017 16:27:15 -0400 Subject: [PATCH] remove address/venue/street from requested layers when creating results refactored for cleanliness and clarity --- controller/coarse_reverse.js | 77 ++++++++++++++--------- test/unit/controller/coarse_reverse.js | 86 ++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 30 deletions(-) diff --git a/controller/coarse_reverse.js b/controller/coarse_reverse.js index d86a0951..90affb5f 100644 --- a/controller/coarse_reverse.js +++ b/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(); diff --git a/test/unit/controller/coarse_reverse.js b/test/unit/controller/coarse_reverse.js index 2a086b14..e44f73f1 100644 --- a/test/unit/controller/coarse_reverse.js +++ b/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