From 976f252e3f01de65ea8f83e79551ae2e448010cf Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 2 Jun 2017 16:27:15 -0400 Subject: [PATCH 1/4] remove address/venue/street from requested layers when creating results refactored for cleanliness and clarity --- test/unit/controller/coarse_reverse.js | 86 ++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/test/unit/controller/coarse_reverse.js b/test/unit/controller/coarse_reverse.js index bb170a37..d99f2fc1 100644 --- a/test/unit/controller/coarse_reverse.js +++ b/test/unit/controller/coarse_reverse.js @@ -533,6 +533,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 From 1c9c68e5e32846e1bf1214b6a6ef731b6e50590a Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 2 Jun 2017 17:08:20 -0400 Subject: [PATCH 2/4] switched to t.plan for easier testing with next() --- test/unit/controller/coarse_reverse.js | 87 +------------------------- 1 file changed, 1 insertion(+), 86 deletions(-) diff --git a/test/unit/controller/coarse_reverse.js b/test/unit/controller/coarse_reverse.js index d99f2fc1..5fbe854c 100644 --- a/test/unit/controller/coarse_reverse.js +++ b/test/unit/controller/coarse_reverse.js @@ -449,6 +449,7 @@ module.exports.tests.success_conditions = (test, common) => { }; t.deepEquals(res, expected); + t.notOk(logger.hasErrorMessages()); t.end(); @@ -533,92 +534,6 @@ 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 From 10b1d282010053e4ddece83e6351aa0f9a148a3e Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Sat, 3 Jun 2017 15:07:58 -0400 Subject: [PATCH 3/4] only query ES for non-coarse layers on non-coarse reverse requests --- query/reverse.js | 4 +- query/reverse_defaults.js | 1 + test/unit/fixture/reverse_null_island.js | 5 +++ test/unit/fixture/reverse_standard.js | 5 +++ .../fixture/reverse_with_boundary_country.js | 5 +++ .../fixture/reverse_with_layer_filtering.js | 2 +- ..._with_layer_filtering_non_coarse_subset.js | 41 +++++++++++++++++++ .../fixture/reverse_with_source_filtering.js | 5 +++ test/unit/query/reverse.js | 33 ++++++++++++--- 9 files changed, 94 insertions(+), 7 deletions(-) create mode 100644 test/unit/fixture/reverse_with_layer_filtering_non_coarse_subset.js diff --git a/query/reverse.js b/query/reverse.js index 72a3c3a6..fbe1251d 100644 --- a/query/reverse.js +++ b/query/reverse.js @@ -3,6 +3,7 @@ const peliasQuery = require('pelias-query'); const defaults = require('./reverse_defaults'); const check = require('check-types'); +const _ = require('lodash'); const logger = require('pelias-logger').get('api'); //------------------------------ @@ -44,7 +45,8 @@ function generateQuery( clean ){ // layers if( check.array(clean.layers) && clean.layers.length ) { - vs.var( 'layers', clean.layers); + // only include non-coarse layers + vs.var( 'layers', _.intersection(clean.layers, ['address', 'street', 'venue'])); logStr += '[param:layers] '; } diff --git a/query/reverse_defaults.js b/query/reverse_defaults.js index 3b468a05..d8b72292 100644 --- a/query/reverse_defaults.js +++ b/query/reverse_defaults.js @@ -6,6 +6,7 @@ module.exports = _.merge({}, peliasQuery.defaults, { 'size': 1, 'track_scores': true, + 'layers': ['venue', 'address', 'street'], 'centroid:field': 'center_point', diff --git a/test/unit/fixture/reverse_null_island.js b/test/unit/fixture/reverse_null_island.js index df6972ca..ff9ef745 100644 --- a/test/unit/fixture/reverse_null_island.js +++ b/test/unit/fixture/reverse_null_island.js @@ -13,6 +13,11 @@ module.exports = { 'lon': 0 } } + }, + { + 'terms': { + 'layer': ['venue', 'address', 'street'] + } }] } }, diff --git a/test/unit/fixture/reverse_standard.js b/test/unit/fixture/reverse_standard.js index b70fe958..21bc08d3 100644 --- a/test/unit/fixture/reverse_standard.js +++ b/test/unit/fixture/reverse_standard.js @@ -13,6 +13,11 @@ module.exports = { 'lon': -82.50622 } } + }, + { + 'terms': { + 'layer': ['venue', 'address', 'street'] + } }] } }, diff --git a/test/unit/fixture/reverse_with_boundary_country.js b/test/unit/fixture/reverse_with_boundary_country.js index c21cb44b..e50ed14d 100644 --- a/test/unit/fixture/reverse_with_boundary_country.js +++ b/test/unit/fixture/reverse_with_boundary_country.js @@ -23,6 +23,11 @@ module.exports = { 'lon': -82.50622 } } + }, + { + 'terms': { + 'layer': ['venue', 'address', 'street'] + } }] } }, diff --git a/test/unit/fixture/reverse_with_layer_filtering.js b/test/unit/fixture/reverse_with_layer_filtering.js index ee7ab7b0..dd9571ad 100644 --- a/test/unit/fixture/reverse_with_layer_filtering.js +++ b/test/unit/fixture/reverse_with_layer_filtering.js @@ -17,7 +17,7 @@ module.exports = { }, { 'terms': { - 'layer': ['country'] + 'layer': ['venue', 'address', 'street'] } } ] diff --git a/test/unit/fixture/reverse_with_layer_filtering_non_coarse_subset.js b/test/unit/fixture/reverse_with_layer_filtering_non_coarse_subset.js new file mode 100644 index 00000000..046a34e5 --- /dev/null +++ b/test/unit/fixture/reverse_with_layer_filtering_non_coarse_subset.js @@ -0,0 +1,41 @@ +var vs = require('../../../query/reverse_defaults'); + +module.exports = { + 'query': { + 'bool': { + 'filter': [ + { + 'geo_distance': { + 'distance': '500km', + 'distance_type': 'plane', + 'optimize_bbox': 'indexed', + 'center_point': { + 'lat': 29.49136, + 'lon': -82.50622 + } + } + }, + { + 'terms': { + 'layer': ['venue', 'street'] + } + } + ] + } + }, + 'sort': [ + '_score', + { + '_geo_distance': { + 'center_point': { + 'lat': 29.49136, + 'lon': -82.50622 + }, + 'order': 'asc', + 'distance_type': 'plane' + } + } + ], + 'size': vs.size, + 'track_scores': true +}; diff --git a/test/unit/fixture/reverse_with_source_filtering.js b/test/unit/fixture/reverse_with_source_filtering.js index 21e04662..3eb32bb4 100644 --- a/test/unit/fixture/reverse_with_source_filtering.js +++ b/test/unit/fixture/reverse_with_source_filtering.js @@ -19,6 +19,11 @@ module.exports = { 'terms': { 'source': ['test'] } + }, + { + 'terms': { + 'layer': ['venue', 'address', 'street'] + } } ] } diff --git a/test/unit/query/reverse.js b/test/unit/query/reverse.js index 24ada46a..a638fc53 100644 --- a/test/unit/query/reverse.js +++ b/test/unit/query/reverse.js @@ -132,23 +132,46 @@ module.exports.tests.query = function(test, common) { t.end(); }); - test('valid layers filter', function(t) { - var query = generate({ + test('valid layers filter', (t) => { + const query = generate({ 'point.lat': 29.49136, 'point.lon': -82.50622, 'boundary.circle.lat': 29.49136, 'boundary.circle.lon': -82.50622, 'boundary.circle.radius': 500, - 'layers': ['country'] + // only venue, address, and street layers should be retained + 'layers': ['neighbourhood', 'venue', 'locality', 'address', 'region', 'street', 'country'] }); - var compiled = JSON.parse( JSON.stringify( query ) ); - var expected = require('../fixture/reverse_with_layer_filtering'); + const compiled = JSON.parse( JSON.stringify( query ) ); + const expected = require('../fixture/reverse_with_layer_filtering'); + + t.deepEqual(compiled.type, 'reverse', 'query type set'); + t.deepEqual(compiled.body, expected, 'valid reverse query with source filtering'); + t.end(); + + }); + + test('valid layers filter - subset of non-coarse layers', (t) => { + const query = generate({ + 'point.lat': 29.49136, + 'point.lon': -82.50622, + 'boundary.circle.lat': 29.49136, + 'boundary.circle.lon': -82.50622, + 'boundary.circle.radius': 500, + // only venue, address, and street layers should be retained + 'layers': ['neighbourhood', 'venue', 'street', 'locality'] + }); + + const compiled = JSON.parse( JSON.stringify( query ) ); + const expected = require('../fixture/reverse_with_layer_filtering_non_coarse_subset'); t.deepEqual(compiled.type, 'reverse', 'query type set'); t.deepEqual(compiled.body, expected, 'valid reverse query with source filtering'); t.end(); + }); + }; module.exports.all = function (tape, common) { From 0f55ff2546fb96d50fccd16719fa8834f079f543 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Mon, 5 Jun 2017 17:09:55 -0400 Subject: [PATCH 4/4] removed superseded initial coarse reverse call --- routes/v1.js | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/routes/v1.js b/routes/v1.js index 80c3d9b5..9fb8f8d2 100644 --- a/routes/v1.js +++ b/routes/v1.js @@ -97,14 +97,9 @@ 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( - 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 coarse_reverse_should_execute = all( + isPipServiceEnabled, not(hasRequestErrors), not(hasResponseData) ); const placeholderShouldExecute = all( @@ -114,7 +109,7 @@ function addRoutes(app, peliasConfig) { // execute under the following conditions: // - there are no errors or data // - request is not coarse OR pip service is disabled - const original_reverse_should_execute = all( + const non_coarse_reverse_should_execute = all( not(hasResponseDataOrRequestErrors), any( not(isCoarseReverse), @@ -202,9 +197,8 @@ function addRoutes(app, peliasConfig) { sanitizers.reverse.middleware, middleware.requestLanguage, middleware.calcSize(), + controllers.search(peliasConfig.api, esclient, queries.reverse, non_coarse_reverse_should_execute), 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