diff --git a/controller/coarse_reverse.js b/controller/coarse_reverse.js index 07a370d7..bb974940 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,60 +16,79 @@ 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) { + // 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) { - return _.intersection(layers, Object.keys(results)).length > 0; +// removing non-coarse layers could leave effective_layers empty, so it's +// 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 - // 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()); 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')) { - const parsedBoundingBox = results[most_granular_layer][0].bounding_box.split(',').map(parseFloat); + // set bounding box if available + 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], - lon: parsedBoundingBox[0] + lat: parsed_bounding_box[3], + lon: parsed_bounding_box[0] }, lowerRight: { - lat: parsedBoundingBox[1], - lon: parsedBoundingBox[2] + lat: parsed_bounding_box[1], + 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 - const finest_granularity_requested = granularities.findIndex((granularity) => { - return req.clean.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 req.clean.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, req.clean.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/controller/predicates/is_coarse_reverse.js b/controller/predicates/is_coarse_reverse.js index c8a292dd..e02f90c5 100644 --- a/controller/predicates/is_coarse_reverse.js +++ b/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)); }; diff --git a/routes/v1.js b/routes/v1.js index da4936be..80c3d9b5 100644 --- a/routes/v1.js +++ b/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 diff --git a/test/unit/controller/coarse_reverse.js b/test/unit/controller/coarse_reverse.js index c37c76e0..bb170a37 100644 --- a/test/unit/controller/coarse_reverse.js +++ b/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, should_execute); + const controller = setup(service, _.constant(false)); 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(); });