From b84652ba2029ce422f19b6415558ab9af1635b70 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Tue, 22 Sep 2015 14:50:45 -0400 Subject: [PATCH 01/12] Refactor sanitize_coord parameter handling This should help reduce duplication when passing values in, as the key won't have to be specified twice. Also, the parameters are in the same order as the other sanitize_* methods. --- sanitiser/_geo_common.js | 16 ++++++++-------- test/unit/sanitiser/_geo_common.js | 8 ++++---- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/sanitiser/_geo_common.js b/sanitiser/_geo_common.js index 0ca1184e..3287bc31 100644 --- a/sanitiser/_geo_common.js +++ b/sanitiser/_geo_common.js @@ -36,7 +36,7 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) { // check each property individually. now that it is known a bbox is present, // all properties must exist, so pass the true flag for coord_is_required properties.forEach(function(prop) { - sanitize_coord(prop, clean, raw[prop], true); + sanitize_coord(prop, clean, raw, true); }); } @@ -52,7 +52,7 @@ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { // sanitize both a point and a radius if radius is present // otherwise just sanittize the point if( check.assigned( raw[ key_prefix + '.radius' ] ) ){ - sanitize_coord( key_prefix + '.radius', clean, raw[ key_prefix + '.radius' ], true ); + sanitize_coord( key_prefix + '.radius', clean, raw, true ); sanitize_point( key_prefix, clean, raw, true); } else { sanitize_point( key_prefix, clean, raw, circle_is_required); @@ -89,20 +89,20 @@ function sanitize_point( key_prefix, clean, raw, point_is_required ) { // check each property individually. now that it is known a bbox is present, // all properties must exist, so pass the true flag for coord_is_required properties.forEach(function(prop) { - sanitize_coord(prop, clean, raw[prop], true); + sanitize_coord(prop, clean, raw, true); }); } /** * Validate lat,lon values * - * @param {string} key - * @param {object} clean - * @param {string} rawValue + * @param {string} key - which key to validate + * @param {object} clean - cleaned parameters object + * @param {object} raw - the raw request object * @param {bool} latlon_is_required */ -function sanitize_coord( key, clean, rawValue, latlon_is_required ) { - var parsedValue = parseFloat( rawValue ); +function sanitize_coord( key, clean, raw, latlon_is_required ) { + var parsedValue = parseFloat( raw[key] ); if ( _.isFinite( parsedValue ) ) { clean[key] = parsedValue; } diff --git a/test/unit/sanitiser/_geo_common.js b/test/unit/sanitiser/_geo_common.js index 47ae29fe..4da77d69 100644 --- a/test/unit/sanitiser/_geo_common.js +++ b/test/unit/sanitiser/_geo_common.js @@ -20,7 +20,7 @@ module.exports.tests.coord = function(test, common) { }; var mandatory = false; - sanitize.sanitize_coord( 'foo', clean, params.foo, mandatory ); + sanitize.sanitize_coord( 'foo', clean, params, mandatory ); t.equal(clean.foo, params.foo); t.end(); }); @@ -32,7 +32,7 @@ module.exports.tests.coord = function(test, common) { }; var mandatory = false; - sanitize.sanitize_coord( 'foo', clean, params.foo, mandatory ); + sanitize.sanitize_coord( 'foo', clean, params, mandatory ); t.equal(clean.foo, undefined, 'not set'); t.end(); }); @@ -43,7 +43,7 @@ module.exports.tests.coord = function(test, common) { var mandatory = false; t.doesNotThrow( function(){ - sanitize.sanitize_coord( 'foo', clean, params.foo, mandatory ); + sanitize.sanitize_coord( 'foo', clean, params, mandatory ); }); t.end(); }); @@ -54,7 +54,7 @@ module.exports.tests.coord = function(test, common) { var mandatory = true; t.throws( function(){ - sanitize.sanitize_coord( 'foo', clean, params.foo, mandatory ); + sanitize.sanitize_coord( 'foo', clean, params, mandatory ); }); t.end(); }); From db8f98a51f4d96b73fd06e5b9417f757315e4f54 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Tue, 22 Sep 2015 16:43:53 -0400 Subject: [PATCH 02/12] Remove completed @todo --- test/unit/sanitiser/search.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/sanitiser/search.js b/test/unit/sanitiser/search.js index 59bc87f3..27b1d172 100644 --- a/test/unit/sanitiser/search.js +++ b/test/unit/sanitiser/search.js @@ -5,7 +5,6 @@ var extend = require('extend'), middleware = search.middleware, defaultError = 'invalid param \'text\': text length, must be >0'; // these are the default values you would expect when no input params are specified. -// @todo: why is this different from $defaultClean? var emptyClean = { private: false, size: 10, types: {} }; module.exports.tests = {}; From 29a78db4039e0ee58cc52f3afc3447b9a17b7e53 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Tue, 22 Sep 2015 16:53:46 -0400 Subject: [PATCH 03/12] Add null island bounding box test --- test/unit/sanitiser/_geo_common.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/unit/sanitiser/_geo_common.js b/test/unit/sanitiser/_geo_common.js index 4da77d69..f8a27a8b 100644 --- a/test/unit/sanitiser/_geo_common.js +++ b/test/unit/sanitiser/_geo_common.js @@ -172,6 +172,23 @@ module.exports.tests.rect = function(test, common) { t.end(); }); + test('valid rect quad, null island', function (t) { + var clean = {}; + var params = { + 'boundary.rect.min_lat': 0, + 'boundary.rect.max_lat': 0, + 'boundary.rect.min_lon': 0, + 'boundary.rect.max_lon': 0 + }; + var mandatory = false; + + sanitize.sanitize_rect( 'boundary.rect', clean, params, mandatory ); + t.equal(clean['boundary.rect.min_lat'], params['boundary.rect.min_lat'], 'min_lat approved'); + t.equal(clean['boundary.rect.max_lat'], params['boundary.rect.max_lat'], 'min_lat approved'); + t.equal(clean['boundary.rect.min_lon'], params['boundary.rect.min_lon'], 'min_lat approved'); + t.equal(clean['boundary.rect.max_lon'], params['boundary.rect.max_lon'], 'min_lat approved'); + t.end(); + }); test('invalid rect - partially specified', function (t) { var clean = {}; var params = { From 36925ca874f507386ef9078078f4db588d06b2f4 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Tue, 22 Sep 2015 16:54:11 -0400 Subject: [PATCH 04/12] Remove bbox sanitizer functionality tests from search sanitizer tests Full functionality testing for the bbox sanitizer should be in the sanitizer/_geo_common tests. In the search sanitizer tests, only testing the inclusion of the correct sanitizers, and the interactions between them, really seems to make sense. --- test/unit/sanitiser/search.js | 81 +++++++---------------------------- 1 file changed, 16 insertions(+), 65 deletions(-) diff --git a/test/unit/sanitiser/search.js b/test/unit/sanitiser/search.js index 27b1d172..2b171406 100644 --- a/test/unit/sanitiser/search.js +++ b/test/unit/sanitiser/search.js @@ -185,74 +185,25 @@ module.exports.tests.sanitize_optional_geo = function(test, common) { }; module.exports.tests.sanitize_bounding_rect = function(test, common) { - - // convernience function to avoid refactoring the succict geojson bbox - // fixtures in to the more verbose bounding.rect format. - var mapGeoJsonBboxFormatToBoundingRectFormat = function( bbox ){ - var split = bbox.split(','); - return { - 'boundary.rect.min_lon': split[0], - 'boundary.rect.max_lat': split[1], - 'boundary.rect.max_lon': split[2], - 'boundary.rect.min_lat': split[3] + test('valid bounding rect', function(t) { + var req = { + query: { + text: 'test', + 'boundary.rect.min_lat': -40.659, + 'boundary.rect.max_lat': -41.614, + 'boundary.rect.min_lon': 174.612, + 'boundary.rect.max_lon': 176.333 + } }; - }; - - var bboxes = { - invalid: [ - '91;-181,-91,181', // invalid - semicolon between coordinates - 'these,are,not,numbers', - '0,0,0,notANumber', - ',,,', - '91, -181, -91', // invalid - missing a coordinate - '123,12', // invalid - missing coordinates - '' // invalid - empty param - ].map(mapGeoJsonBboxFormatToBoundingRectFormat), - - valid: [ - '-179,90,34,-80', // valid top_right lon/lat, bottom_left lon/lat - '0,0,0,0', - '10,20,30,40', - '-40,-20,10,30', - '-40,-20,10,30', - '-1200,20,1000,20', - '-1400,90,1400,-90', - // wrapped latitude coordinates - '-181,90,34,-180', - '-170,91,-181,45', - '-181,91,181,-91', - '91, -181,-91,11', - '91, -11,-91,181' - ].map(mapGeoJsonBboxFormatToBoundingRectFormat) - }; - test('invalid bounding rect', function(t) { - bboxes.invalid.forEach( function( bbox ){ - var req = { query: { text: 'test' } }; - extend( req.query, bbox ); - sanitize(req, function(){ - t.equal(req.clean['boundary.rect.min_lon'], undefined); - t.equal(req.clean['boundary.rect.max_lat'], undefined); - t.equal(req.clean['boundary.rect.max_lon'], undefined); - t.equal(req.clean['boundary.rect.min_lat'], undefined); - t.equal(req.errors.length, 1, 'bounding error'); - }); - }); - t.end(); - }); - test('valid bounding rect', function(t) { - bboxes.valid.forEach( function( bbox ){ - var req = { query: { text: 'test' } }; - extend( req.query, bbox ); - sanitize(req, function(){ - t.equal(req.errors[0], undefined, 'no error'); - t.equal(req.clean['boundary.rect.min_lon'], parseFloat(bbox['boundary.rect.min_lon'])); - t.equal(req.clean['boundary.rect.max_lat'], parseFloat(bbox['boundary.rect.max_lat'])); - t.equal(req.clean['boundary.rect.max_lon'], parseFloat(bbox['boundary.rect.max_lon'])); - t.equal(req.clean['boundary.rect.min_lat'], parseFloat(bbox['boundary.rect.min_lat'])); - }); + sanitize(req, function(){ + t.equal(req.errors[0], undefined, 'no error'); + t.equal(req.clean['boundary.rect.min_lon'], parseFloat(req.query['boundary.rect.min_lon'])); + t.equal(req.clean['boundary.rect.max_lat'], parseFloat(req.query['boundary.rect.max_lat'])); + t.equal(req.clean['boundary.rect.max_lon'], parseFloat(req.query['boundary.rect.max_lon'])); + t.equal(req.clean['boundary.rect.min_lat'], parseFloat(req.query['boundary.rect.min_lat'])); + t.end(); }); - t.end(); }); }; From 9dcc296a7021fe97f3a038c500c1dc1a40626941 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Wed, 23 Sep 2015 15:19:52 -0400 Subject: [PATCH 05/12] Add focus.viewport rect sanitizer to search --- sanitiser/_geo_search.js | 2 ++ test/unit/sanitiser/search.js | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/sanitiser/_geo_search.js b/sanitiser/_geo_search.js index badeec52..0e5ec896 100644 --- a/sanitiser/_geo_search.js +++ b/sanitiser/_geo_search.js @@ -2,6 +2,7 @@ var geo_common = require ('./_geo_common'); var LAT_LON_IS_REQUIRED = false; var RECT_IS_REQUIRED = false; var CIRCLE_IS_REQUIRED = false; +var VIEWPORT_IS_REQUIRED = false; // validate inputs, convert types and apply defaults module.exports = function sanitize( raw, clean ){ @@ -13,6 +14,7 @@ module.exports = function sanitize( raw, clean ){ geo_common.sanitize_point( 'focus.point', clean, raw, LAT_LON_IS_REQUIRED ); geo_common.sanitize_rect( 'boundary.rect', clean, raw, RECT_IS_REQUIRED ); geo_common.sanitize_circle( 'boundary.circle', clean, raw, CIRCLE_IS_REQUIRED ); + geo_common.sanitize_rect( 'focus.viewport', clean, raw, VIEWPORT_IS_REQUIRED ); } catch (err) { messages.errors.push( err.message ); diff --git a/test/unit/sanitiser/search.js b/test/unit/sanitiser/search.js index 2b171406..eed05ee6 100644 --- a/test/unit/sanitiser/search.js +++ b/test/unit/sanitiser/search.js @@ -207,6 +207,28 @@ module.exports.tests.sanitize_bounding_rect = function(test, common) { }); }; +module.exports.tests.sanitize_viewport = function(test, common) { + test('valid viewport', function(t) { + var req = { + query: { + text: 'test', + 'focus.viewport.min_lat': '37', + 'focus.viewport.max_lat': '38', + 'focus.viewport.min_lon': '-123', + 'focus.viewport.max_lon': '-122' + } + }; + sanitize(req, function() { + t.equal(req.errors[0], undefined, 'no error'); + t.equal(req.clean['focus.viewport.min_lat'], parseFloat(req.query['focus.viewport.min_lat']), 'correct min_lat in clean'); + t.equal(req.clean['focus.viewport.max_lat'], parseFloat(req.query['focus.viewport.max_lat']), 'correct max_lat in clean'); + t.equal(req.clean['focus.viewport.min_lon'], parseFloat(req.query['focus.viewport.min_lon']), 'correct min_lon in clean'); + t.equal(req.clean['focus.viewport.max_lon'], parseFloat(req.query['focus.viewport.max_lon']), 'correct max_lon in clean'); + t.end(); + }); + }); +}; + module.exports.tests.sanitize_size = function(test, common) { test('invalid size value', function(t) { var req = { query: { size: 'a', text: 'test', lat: 0, lon: 0 } }; From 06b44e5355851bc0accdd13b65184034bd4c9463 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Wed, 23 Sep 2015 15:43:22 -0400 Subject: [PATCH 06/12] Disallow specifying both focus.point and focus.viewport --- sanitiser/_geo_search.js | 12 +++++++++ test/unit/sanitiser/search.js | 47 +++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/sanitiser/_geo_search.js b/sanitiser/_geo_search.js index 0e5ec896..8aef9b7f 100644 --- a/sanitiser/_geo_search.js +++ b/sanitiser/_geo_search.js @@ -1,3 +1,4 @@ +var check = require('check-types'); var geo_common = require ('./_geo_common'); var LAT_LON_IS_REQUIRED = false; var RECT_IS_REQUIRED = false; @@ -10,6 +11,17 @@ module.exports = function sanitize( raw, clean ){ // error & warning messages var messages = { errors: [], warnings: [] }; + // disallow specifying both focus.point and focus.viewport + if ( ( raw['focus.viewport.min_lat'] || + raw['focus.viewport.max_lat'] || + raw['focus.viewport.min_lon'] || + raw['focus.viewport.max_lon'] ) && + ( raw['focus.point.lat'] || + raw['focus.point.lon'] ) ) { + messages.errors.push( 'focus.point and focus.viewport can\'t both be set' ); + return messages; + } + try { geo_common.sanitize_point( 'focus.point', clean, raw, LAT_LON_IS_REQUIRED ); geo_common.sanitize_rect( 'boundary.rect', clean, raw, RECT_IS_REQUIRED ); diff --git a/test/unit/sanitiser/search.js b/test/unit/sanitiser/search.js index eed05ee6..4f4e2880 100644 --- a/test/unit/sanitiser/search.js +++ b/test/unit/sanitiser/search.js @@ -227,6 +227,53 @@ module.exports.tests.sanitize_viewport = function(test, common) { t.end(); }); }); + + test('error returned if focus.point and focus.viewpoint specified', function(t) { + var req = { + query: { + text: 'test', + 'focus.point.lat': '10', + 'focus.point.lon': '15', + 'focus.viewport.min_lat': '37', + 'focus.viewport.max_lat': '38', + 'focus.viewport.min_lon': '-123', + 'focus.viewport.max_lon': '-122' + } + }; + + sanitize(req, function() { + t.equal(req.errors[0], 'focus.point and focus.viewport can\'t both be set', 'no error'); + t.notOk(req.clean.hasOwnProperty('focus.viewport.min_lat'), 'clean should be empty'); + t.notOk(req.clean.hasOwnProperty('focus.viewport.max_lat'), 'clean should be empty'); + t.notOk(req.clean.hasOwnProperty('focus.viewport.min_lon'), 'clean should be empty'); + t.notOk(req.clean.hasOwnProperty('focus.viewport.max_lon'), 'clean should be empty'); + t.notOk(req.clean.hasOwnProperty('focus.point.lat'), 'clean should be empty'); + t.notOk(req.clean.hasOwnProperty('focus.point.lon'), 'clean should be empty'); + t.end(); + }); + }); + + test('error returned if focus.point and focus.viewpoint partially specified', function(t) { + var req = { + query: { + text: 'test', + 'focus.point.lat': '10', + 'focus.viewport.min_lat': '37', + 'focus.viewport.max_lon': '-122' + } + }; + + sanitize(req, function() { + t.equal(req.errors[0], 'focus.point and focus.viewport can\'t both be set', 'no error'); + t.notOk(req.clean.hasOwnProperty('focus.viewport.min_lat'), 'clean should be empty'); + t.notOk(req.clean.hasOwnProperty('focus.viewport.max_lat'), 'clean should be empty'); + t.notOk(req.clean.hasOwnProperty('focus.viewport.min_lon'), 'clean should be empty'); + t.notOk(req.clean.hasOwnProperty('focus.viewport.max_lon'), 'clean should be empty'); + t.notOk(req.clean.hasOwnProperty('focus.point.lat'), 'clean should be empty'); + t.notOk(req.clean.hasOwnProperty('focus.point.lon'), 'clean should be empty'); + t.end(); + }); + }); }; module.exports.tests.sanitize_size = function(test, common) { From 57fb96047151ccce1b0283e924a20d49d90f043c Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Wed, 23 Sep 2015 15:55:53 -0400 Subject: [PATCH 07/12] Set centerpoint of viewport in search query This simply reuses the focus:point:{lat|lon} variables, but sets them using the centerpoint of the viewport. Eventually we should calculate a radius and use that here. --- query/search.js | 14 ++- .../fixture/search_linguistic_viewport.js | 105 ++++++++++++++++++ test/unit/query/search.js | 18 +++ 3 files changed, 131 insertions(+), 6 deletions(-) create mode 100644 test/unit/fixture/search_linguistic_viewport.js diff --git a/query/search.js b/query/search.js index a3787535..57fab047 100644 --- a/query/search.js +++ b/query/search.js @@ -69,13 +69,15 @@ function generateQuery( clean ){ } // focus viewport - // @todo: change these to the correct request variable names - // @todo: calculate the centroid from the viewport box - if( clean.focus && clean.focus.viewport ){ - var vp = clean.focus.viewport; + if( check.number(clean['focus.viewport.min_lat']) && + check.number(clean['focus.viewport.max_lat']) && + check.number(clean['focus.viewport.min_lon']) && + check.number(clean['focus.viewport.max_lon']) ) { + // calculate the centroid from the viewport box + // simply set focus:point:lat/lon, until we improve this with a radius vs.set({ - 'focus:point:lat': vp.min_lat + ( vp.max_lat - vp.min_lat ) / 2, - 'focus:point:lon': vp.min_lon + ( vp.max_lon - vp.min_lon ) / 2 + 'focus:point:lat': clean['focus.viewport.min_lat'] + ( clean['focus.viewport.max_lat'] - clean['focus.viewport.min_lat'] ) / 2, + 'focus:point:lon': clean['focus.viewport.min_lon'] + ( clean['focus.viewport.max_lon'] - clean['focus.viewport.min_lon'] ) / 2 }); } diff --git a/test/unit/fixture/search_linguistic_viewport.js b/test/unit/fixture/search_linguistic_viewport.js new file mode 100644 index 00000000..7c101db0 --- /dev/null +++ b/test/unit/fixture/search_linguistic_viewport.js @@ -0,0 +1,105 @@ + +module.exports = { + 'query': { + 'filtered': { + 'query': { + 'bool': { + 'must': [{ + 'match': { + 'name.default': { + 'query': 'test', + 'boost': 1, + 'analyzer': 'peliasOneEdgeGram' + } + } + }], + 'should': [{ + 'match': { + 'phrase.default': { + 'query': 'test', + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2 + } + } + }, { + 'function_score': { + 'query': { + 'match': { + 'phrase.default': { + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2, + 'query': 'test' + } + } + }, + 'functions': [{ + 'linear': { + 'center_point': { + 'origin': { + 'lat': 29.49136, + 'lon': -82.50622 + }, + 'offset': '1km', + 'scale': '50km', + 'decay': 0.5 + } + } + }], + 'score_mode': 'avg', + 'boost_mode': 'replace' + } + }, + { + 'function_score': { + 'query': { + 'filtered': { + 'filter': { + 'exists': { + 'field': 'popularity' + } + } + } + }, + 'max_boost': 2, + 'score_mode': 'first', + 'boost_mode': 'replace', + 'filter': { + 'or': [ + { + 'type': { + 'value': 'admin0' + } + }, + { + 'type': { + 'value': 'admin1' + } + }, + { + 'type': { + 'value': 'admin2' + } + } + ] + }, + 'functions': [{ + 'field_value_factor': { + 'modifier': 'sqrt', + 'field': 'popularity' + }, + 'weight': 1 + }] + } + }] + } + } + } + }, + 'sort': [ '_sort' ], + 'size': 10, + 'track_scores': true +}; diff --git a/test/unit/query/search.js b/test/unit/query/search.js index e236eaa7..cb2a6d25 100644 --- a/test/unit/query/search.js +++ b/test/unit/query/search.js @@ -79,6 +79,24 @@ module.exports.tests.query = function(test, common) { t.end(); }); + test('search search + viewport', function(t) { + var query = generate({ + text: 'test', size: 10, + 'focus.viewport.min_lat': 28.49136, + 'focus.viewport.max_lat': 30.49136, + 'focus.viewport.min_lon': -87.50622, + 'focus.viewport.max_lon': -77.50622, + layers: ['test'] + }); + + var compiled = JSON.parse( JSON.stringify( query ) ); + var expected = require('../fixture/search_linguistic_viewport'); + expected.sort = sort; + + t.deepEqual(compiled, expected, 'valid search query'); + t.end(); + }); + test('search search + focus on null island', function(t) { var query = generate({ text: 'test', size: 10, From dcadc7832ea9c8c8a133bb752379318edf136f9d Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 29 Sep 2015 14:53:05 -0400 Subject: [PATCH 08/12] add focus:scale to /search requests with just viewport.*, calculated from bounding box diagonal, minimum of 1 --- query/search.js | 23 +++- .../fixture/search_linguistic_viewport.js | 2 +- ...search_linguistic_viewport_min_diagonal.js | 105 ++++++++++++++++++ test/unit/query/search.js | 18 +++ 4 files changed, 144 insertions(+), 4 deletions(-) create mode 100644 test/unit/fixture/search_linguistic_viewport_min_diagonal.js diff --git a/query/search.js b/query/search.js index 57fab047..147e2dfa 100644 --- a/query/search.js +++ b/query/search.js @@ -1,7 +1,8 @@ var peliasQuery = require('pelias-query'), defaults = require('./defaults'), textParser = require('./text_parser'), - check = require('check-types'); + check = require('check-types'), + geolib = require('geolib'); //------------------------------ // general-purpose search query @@ -74,10 +75,10 @@ function generateQuery( clean ){ check.number(clean['focus.viewport.min_lon']) && check.number(clean['focus.viewport.max_lon']) ) { // calculate the centroid from the viewport box - // simply set focus:point:lat/lon, until we improve this with a radius vs.set({ 'focus:point:lat': clean['focus.viewport.min_lat'] + ( clean['focus.viewport.max_lat'] - clean['focus.viewport.min_lat'] ) / 2, - 'focus:point:lon': clean['focus.viewport.min_lon'] + ( clean['focus.viewport.max_lon'] - clean['focus.viewport.min_lon'] ) / 2 + 'focus:point:lon': clean['focus.viewport.min_lon'] + ( clean['focus.viewport.max_lon'] - clean['focus.viewport.min_lon'] ) / 2, + 'focus:scale': calculateDiagonalDistance(clean) + 'km' }); } @@ -125,4 +126,20 @@ function generateQuery( clean ){ return query.render( vs ); } +// return diagonal distance in km, with min=1 +function calculateDiagonalDistance(clean) { + var diagonalDistance = geolib.getDistance( + { latitude: clean['focus.viewport.min_lat'], longitude: clean['focus.viewport.min_lon'] }, + { latitude: clean['focus.viewport.max_lat'], longitude: clean['focus.viewport.max_lon'] }, + 1000 + ) / 1000; + + if (diagonalDistance === 0) { + return 1; + } + + return diagonalDistance; + +} + module.exports = generateQuery; diff --git a/test/unit/fixture/search_linguistic_viewport.js b/test/unit/fixture/search_linguistic_viewport.js index 7c101db0..f0e3132a 100644 --- a/test/unit/fixture/search_linguistic_viewport.js +++ b/test/unit/fixture/search_linguistic_viewport.js @@ -44,7 +44,7 @@ module.exports = { 'lon': -82.50622 }, 'offset': '1km', - 'scale': '50km', + 'scale': '994km', 'decay': 0.5 } } diff --git a/test/unit/fixture/search_linguistic_viewport_min_diagonal.js b/test/unit/fixture/search_linguistic_viewport_min_diagonal.js new file mode 100644 index 00000000..36b8a0ac --- /dev/null +++ b/test/unit/fixture/search_linguistic_viewport_min_diagonal.js @@ -0,0 +1,105 @@ + +module.exports = { + 'query': { + 'filtered': { + 'query': { + 'bool': { + 'must': [{ + 'match': { + 'name.default': { + 'query': 'test', + 'boost': 1, + 'analyzer': 'peliasOneEdgeGram' + } + } + }], + 'should': [{ + 'match': { + 'phrase.default': { + 'query': 'test', + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2 + } + } + }, { + 'function_score': { + 'query': { + 'match': { + 'phrase.default': { + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2, + 'query': 'test' + } + } + }, + 'functions': [{ + 'linear': { + 'center_point': { + 'origin': { + 'lat': 28.49136, + 'lon': -87.50623 + }, + 'offset': '1km', + 'scale': '1km', + 'decay': 0.5 + } + } + }], + 'score_mode': 'avg', + 'boost_mode': 'replace' + } + }, + { + 'function_score': { + 'query': { + 'filtered': { + 'filter': { + 'exists': { + 'field': 'popularity' + } + } + } + }, + 'max_boost': 2, + 'score_mode': 'first', + 'boost_mode': 'replace', + 'filter': { + 'or': [ + { + 'type': { + 'value': 'admin0' + } + }, + { + 'type': { + 'value': 'admin1' + } + }, + { + 'type': { + 'value': 'admin2' + } + } + ] + }, + 'functions': [{ + 'field_value_factor': { + 'modifier': 'sqrt', + 'field': 'popularity' + }, + 'weight': 1 + }] + } + }] + } + } + } + }, + 'sort': [ '_sort' ], + 'size': 10, + 'track_scores': true +}; diff --git a/test/unit/query/search.js b/test/unit/query/search.js index cb2a6d25..d543c7c7 100644 --- a/test/unit/query/search.js +++ b/test/unit/query/search.js @@ -97,6 +97,24 @@ module.exports.tests.query = function(test, common) { t.end(); }); + test('search with viewport diagonal < 1km should set scale to 1km', function(t) { + var query = generate({ + text: 'test', size: 10, + 'focus.viewport.min_lat': 28.49135, + 'focus.viewport.max_lat': 28.49137, + 'focus.viewport.min_lon': -87.50622, + 'focus.viewport.max_lon': -87.50624, + layers: ['test'] + }); + + var compiled = JSON.parse( JSON.stringify( query ) ); + var expected = require('../fixture/search_linguistic_viewport_min_diagonal'); + expected.sort = sort; + + t.deepEqual(compiled, expected, 'valid search query'); + t.end(); + }); + test('search search + focus on null island', function(t) { var query = generate({ text: 'test', size: 10, From a26bf0b9a8593613faae04161eee56f6570e26ea Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 29 Sep 2015 15:08:28 -0400 Subject: [PATCH 09/12] switched to Math.max to avoid the explicit conditional --- query/search.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/query/search.js b/query/search.js index 147e2dfa..9e4d62f2 100644 --- a/query/search.js +++ b/query/search.js @@ -134,11 +134,7 @@ function calculateDiagonalDistance(clean) { 1000 ) / 1000; - if (diagonalDistance === 0) { - return 1; - } - - return diagonalDistance; + return Math.max(diagonalDistance, 1); } From 687c26555fdab7cc65738c82f483a6149b57065e Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Thu, 8 Oct 2015 17:08:09 +0200 Subject: [PATCH 10/12] installing pelias/scripts no longer required --- README.md | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/README.md b/README.md index 725443f2..652f289c 100644 --- a/README.md +++ b/README.md @@ -9,16 +9,6 @@ See our [API Documentation](https://github.com/pelias/api/blob/master/public/api ## Install Dependencies -The API uses [elasticsearch scripts](https://github.com/pelias/scripts) for additional scoring/sorting logic. You -**must** install them, as documented [here](https://github.com/pelias/scripts#pelias-scripts). Failure to do so will -result in the following error: - -``` -ElasticsearchIllegalArgumentException[Unable to find on disk script admin_boost] -``` - -Once you are done with installing the scripts, Run the following - ```bash npm install ``` From 0e8f4253c05ec15204236a2b192d5a414864d2b6 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Tue, 29 Sep 2015 19:08:23 -0400 Subject: [PATCH 11/12] Return multiple results in place when osm node and way share an id It turns out the _type parameter to the Elasticsearch [multiget](https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-multi-get.html) API does not allow an array of possible values. We were depending on its ability to search multiple types to allow searching for OSM nodes and ways. But, since this doesn't work we essentially have to do it ourselves. There is also the problem that OSM nodes and ways share an ID space. So a gid such as `osm:venue:5` could in theory correspond to 2 records. It seems like the only nice thing to do in that case is return both results. This PR "unrolls" such queries. For example, in the case of `osm:venue:5`, the sanitisers will return the following array of objects to be turned into multiget queries: ``` [{ id: 5, types: ["osmway", "osmnode"] }] ``` Before, this would turn into a multiget query with only one entry, like this: ``` { "docs": [ { "_index": "pelias", "_type": [ " osmnode", "osmway" ], "_id": 5 } ] } ``` now it would look like this: { "docs": [ { "_index": "pelias", "_type": "osmnode", "_id": 5 }, { "_index": "pelias", "_type": "osmnode", "_id": 5 } ] } TLDR you might get back more records from /place than the number of ids you specified, but at least you will definitely get back what you are looking for. --- controller/place.js | 33 +++++++++++++++++++++++++++------ test/unit/controller/place.js | 4 ++-- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/controller/place.js b/controller/place.js index 2a4462cc..c4e78978 100644 --- a/controller/place.js +++ b/controller/place.js @@ -13,14 +13,35 @@ function setup( backend ){ return next(); } - var query = req.clean.ids.map( function(id) { + /* req.clean.ids contains an array of objects with id and types properties. + * types is an array of one or more types, since it can't always be known which single + * type a gid might belong to (osmnode and osmway both have source osm and layer venue). + * + * However, the mget Elasticsearch query only accepts a single type at a + * time. + * + * So, first create a new array that, has an entry + * with each type and id combination. This requires creating a new array with more entries + * than req.clean.ids in the case where entries have multiple types. + */ + + var recordsToReturn = req.clean.ids.reduce(function (acc, ids_element) { + ids_element.types.forEach(function(type) { + acc.push({ + id: ids_element.id, + type: type + }); + }); + return acc; + }, []); + + /* + * Next, map the list of records to an Elasticsearch mget query + */ + var query = recordsToReturn.map( function(id) { return { _index: 'pelias', - /* - * some gids aren't resolvable to a single type (ex: osmnode and osmway - * both have source osm and layer venue), so expect an array of - * possible values. */ - _type: id.types, + _type: id.type, _id: id.id }; }); diff --git a/test/unit/controller/place.js b/test/unit/controller/place.js index 1d38f5d5..e8a8c333 100644 --- a/test/unit/controller/place.js +++ b/test/unit/controller/place.js @@ -41,7 +41,7 @@ module.exports.tests.functional_success = function(test, common) { test('functional success', function(t) { var backend = mockBackend( 'client/mget/ok/1', function( cmd ){ - t.deepEqual(cmd, { body: { docs: [ { _id: 123, _index: 'pelias', _type: [ 'a' ] } ] } }, 'correct backend command'); + t.deepEqual(cmd, { body: { docs: [ { _id: 123, _index: 'pelias', _type: 'a' } ] } }, 'correct backend command'); }); var controller = setup( backend ); var res = { @@ -70,7 +70,7 @@ module.exports.tests.functional_success = function(test, common) { module.exports.tests.functional_failure = function(test, common) { test('functional failure', function(t) { var backend = mockBackend( 'client/mget/fail/1', function( cmd ){ - t.deepEqual(cmd, { body: { docs: [ { _id: 123, _index: 'pelias', _type: [ 'b' ] } ] } }, 'correct backend command'); + t.deepEqual(cmd, { body: { docs: [ { _id: 123, _index: 'pelias', _type: 'b' } ] } }, 'correct backend command'); }); var controller = setup( backend ); var req = { clean: { ids: [ {'id' : 123, types: [ 'b' ] } ] }, errors: [], warnings: [] }; From d732021d5e589af2a41a14f5a6b0009e9e52d40d Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 13 Oct 2015 15:54:22 -0400 Subject: [PATCH 12/12] fixed tests --- .../fixture/search_linguistic_viewport.js | 193 ++++++++++-------- ...search_linguistic_viewport_min_diagonal.js | 193 ++++++++++-------- test/unit/query/search.js | 2 - 3 files changed, 224 insertions(+), 164 deletions(-) diff --git a/test/unit/fixture/search_linguistic_viewport.js b/test/unit/fixture/search_linguistic_viewport.js index f0e3132a..46cdc3e9 100644 --- a/test/unit/fixture/search_linguistic_viewport.js +++ b/test/unit/fixture/search_linguistic_viewport.js @@ -1,105 +1,136 @@ - module.exports = { 'query': { 'filtered': { 'query': { 'bool': { - 'must': [{ - 'match': { - 'name.default': { - 'query': 'test', - 'boost': 1, - 'analyzer': 'peliasOneEdgeGram' - } - } - }], - 'should': [{ - 'match': { - 'phrase.default': { - 'query': 'test', - 'analyzer': 'peliasPhrase', - 'type': 'phrase', - 'boost': 1, - 'slop': 2 + 'must': [ + { + 'match': { + 'name.default': { + 'analyzer': 'peliasOneEdgeGram', + 'boost': 1, + 'query': 'test' + } } } - }, { - 'function_score': { - 'query': { - 'match': { - 'phrase.default': { - 'analyzer': 'peliasPhrase', - 'type': 'phrase', - 'boost': 1, - 'slop': 2, - 'query': 'test' - } + ], + 'should': [ + { + 'match': { + 'phrase.default': { + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2, + 'query': 'test' } - }, - 'functions': [{ - 'linear': { - 'center_point': { - 'origin': { - 'lat': 29.49136, - 'lon': -82.50622 - }, - 'offset': '1km', - 'scale': '994km', - 'decay': 0.5 - } - } - }], - 'score_mode': 'avg', - 'boost_mode': 'replace' - } - }, - { - 'function_score': { - 'query': { - 'filtered': { - 'filter': { - 'exists': { - 'field': 'popularity' + } + }, + { + 'function_score': { + 'query': { + 'match': { + 'phrase.default': { + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2, + 'query': 'test' } } - } - }, - 'max_boost': 2, - 'score_mode': 'first', - 'boost_mode': 'replace', - 'filter': { - 'or': [ + }, + 'functions': [ { - 'type': { - 'value': 'admin0' + 'weight': 2, + 'linear': { + 'center_point': { + 'origin': { + 'lat': 29.49136, + 'lon': -82.50622 + }, + 'offset': '1km', + 'scale': '994km', + 'decay': 0.5 + } } - }, - { - 'type': { - 'value': 'admin1' + } + ], + 'score_mode': 'avg', + 'boost_mode': 'replace' + } + }, + { + 'function_score': { + 'query': { + 'match': { + 'phrase.default': { + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2, + 'query': 'test' } - }, + } + }, + 'max_boost': 20, + 'functions': [ { - 'type': { - 'value': 'admin2' + 'field_value_factor': { + 'modifier': 'log1p', + 'field': 'popularity' + }, + 'weight': 1 + } + ], + 'score_mode': 'first', + 'boost_mode': 'replace', + 'filter': { + 'exists': { + 'field': 'popularity' + } + } + } + }, + { + 'function_score': { + 'query': { + 'match': { + 'phrase.default': { + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2, + 'query': 'test' } } - ] - }, - 'functions': [{ - 'field_value_factor': { - 'modifier': 'sqrt', - 'field': 'popularity' }, - 'weight': 1 - }] + 'max_boost': 20, + 'functions': [ + { + 'field_value_factor': { + 'modifier': 'log1p', + 'field': 'population' + }, + 'weight': 2 + } + ], + 'score_mode': 'first', + 'boost_mode': 'replace', + 'filter': { + 'exists': { + 'field': 'population' + } + } + } } - }] + ] } } } }, - 'sort': [ '_sort' ], 'size': 10, - 'track_scores': true + 'track_scores': true, + 'sort': [ + '_score' + ] }; diff --git a/test/unit/fixture/search_linguistic_viewport_min_diagonal.js b/test/unit/fixture/search_linguistic_viewport_min_diagonal.js index 36b8a0ac..c5306919 100644 --- a/test/unit/fixture/search_linguistic_viewport_min_diagonal.js +++ b/test/unit/fixture/search_linguistic_viewport_min_diagonal.js @@ -1,105 +1,136 @@ - module.exports = { 'query': { 'filtered': { 'query': { 'bool': { - 'must': [{ - 'match': { - 'name.default': { - 'query': 'test', - 'boost': 1, - 'analyzer': 'peliasOneEdgeGram' - } - } - }], - 'should': [{ - 'match': { - 'phrase.default': { - 'query': 'test', - 'analyzer': 'peliasPhrase', - 'type': 'phrase', - 'boost': 1, - 'slop': 2 + 'must': [ + { + 'match': { + 'name.default': { + 'analyzer': 'peliasOneEdgeGram', + 'boost': 1, + 'query': 'test' + } } } - }, { - 'function_score': { - 'query': { - 'match': { - 'phrase.default': { - 'analyzer': 'peliasPhrase', - 'type': 'phrase', - 'boost': 1, - 'slop': 2, - 'query': 'test' - } + ], + 'should': [ + { + 'match': { + 'phrase.default': { + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2, + 'query': 'test' } - }, - 'functions': [{ - 'linear': { - 'center_point': { - 'origin': { - 'lat': 28.49136, - 'lon': -87.50623 - }, - 'offset': '1km', - 'scale': '1km', - 'decay': 0.5 - } - } - }], - 'score_mode': 'avg', - 'boost_mode': 'replace' - } - }, - { - 'function_score': { - 'query': { - 'filtered': { - 'filter': { - 'exists': { - 'field': 'popularity' + } + }, + { + 'function_score': { + 'query': { + 'match': { + 'phrase.default': { + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2, + 'query': 'test' } } - } - }, - 'max_boost': 2, - 'score_mode': 'first', - 'boost_mode': 'replace', - 'filter': { - 'or': [ + }, + 'functions': [ { - 'type': { - 'value': 'admin0' + 'weight': 2, + 'linear': { + 'center_point': { + 'origin': { + 'lat': 28.49136, + 'lon': -87.50623 + }, + 'offset': '1km', + 'scale': '1km', + 'decay': 0.5 + } } - }, - { - 'type': { - 'value': 'admin1' + } + ], + 'score_mode': 'avg', + 'boost_mode': 'replace' + } + }, + { + 'function_score': { + 'query': { + 'match': { + 'phrase.default': { + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2, + 'query': 'test' } - }, + } + }, + 'max_boost': 20, + 'functions': [ { - 'type': { - 'value': 'admin2' + 'field_value_factor': { + 'modifier': 'log1p', + 'field': 'popularity' + }, + 'weight': 1 + } + ], + 'score_mode': 'first', + 'boost_mode': 'replace', + 'filter': { + 'exists': { + 'field': 'popularity' + } + } + } + }, + { + 'function_score': { + 'query': { + 'match': { + 'phrase.default': { + 'analyzer': 'peliasPhrase', + 'type': 'phrase', + 'boost': 1, + 'slop': 2, + 'query': 'test' } } - ] - }, - 'functions': [{ - 'field_value_factor': { - 'modifier': 'sqrt', - 'field': 'popularity' }, - 'weight': 1 - }] + 'max_boost': 20, + 'functions': [ + { + 'field_value_factor': { + 'modifier': 'log1p', + 'field': 'population' + }, + 'weight': 2 + } + ], + 'score_mode': 'first', + 'boost_mode': 'replace', + 'filter': { + 'exists': { + 'field': 'population' + } + } + } } - }] + ] } } } }, - 'sort': [ '_sort' ], 'size': 10, - 'track_scores': true + 'track_scores': true, + 'sort': [ + '_score' + ] }; diff --git a/test/unit/query/search.js b/test/unit/query/search.js index c7f12ec4..3a11e054 100644 --- a/test/unit/query/search.js +++ b/test/unit/query/search.js @@ -85,7 +85,6 @@ module.exports.tests.query = function(test, common) { var compiled = JSON.parse( JSON.stringify( query ) ); var expected = require('../fixture/search_linguistic_viewport'); - expected.sort = sort; t.deepEqual(compiled, expected, 'valid search query'); t.end(); @@ -103,7 +102,6 @@ module.exports.tests.query = function(test, common) { var compiled = JSON.parse( JSON.stringify( query ) ); var expected = require('../fixture/search_linguistic_viewport_min_diagonal'); - expected.sort = sort; t.deepEqual(compiled, expected, 'valid search query'); t.end();