From e90f70ce3de6d0a6320a79c2a34aee6ec7a6df8e Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Mon, 21 Sep 2015 16:19:21 -0400 Subject: [PATCH 01/39] added sanitizer that verifies that all parameters are single scalar values --- sanitiser/_single_scalar_parameters.js | 23 +++++++ sanitiser/autocomplete.js | 1 + sanitiser/place.js | 1 + sanitiser/reverse.js | 1 + sanitiser/search.js | 1 + test/unit/run.js | 1 + .../sanitiser/_single_scalar_parameters.js | 60 +++++++++++++++++++ test/unit/sanitiser/autocomplete.js | 2 +- test/unit/sanitiser/place.js | 8 +++ test/unit/sanitiser/reverse.js | 2 +- test/unit/sanitiser/search.js | 4 +- 11 files changed, 100 insertions(+), 4 deletions(-) create mode 100644 sanitiser/_single_scalar_parameters.js create mode 100644 test/unit/sanitiser/_single_scalar_parameters.js diff --git a/sanitiser/_single_scalar_parameters.js b/sanitiser/_single_scalar_parameters.js new file mode 100644 index 00000000..6bf2fd55 --- /dev/null +++ b/sanitiser/_single_scalar_parameters.js @@ -0,0 +1,23 @@ + +var _ = require('lodash'), + check = require('check-types'); + +// validate inputs +function sanitize( raw, clean ){ + // error & warning messages + var messages = { errors: [], warnings: [] }; + + Object.keys(raw).forEach(function(key) { + if (_.isArray(raw[key])) { + messages.errors.push('\'' + key + '\' parameter can only have one value'); + } else if (_.isObject(raw[key])) { + messages.errors.push('\'' + key + '\' parameter must be a scalar'); + } + + }); + + return messages; +} + +// export function +module.exports = sanitize; diff --git a/sanitiser/autocomplete.js b/sanitiser/autocomplete.js index 84e8752a..c6dc08da 100644 --- a/sanitiser/autocomplete.js +++ b/sanitiser/autocomplete.js @@ -1,5 +1,6 @@ var sanitizeAll = require('../sanitiser/sanitizeAll'), sanitizers = { + singleScalarParameters: require('../sanitiser/_single_scalar_parameters'), text: require('../sanitiser/_text'), size: require('../sanitiser/_size'), private: require('../sanitiser/_flag_bool')('private', false), diff --git a/sanitiser/place.js b/sanitiser/place.js index be68e4aa..74c80c6b 100644 --- a/sanitiser/place.js +++ b/sanitiser/place.js @@ -1,6 +1,7 @@ var sanitizeAll = require('../sanitiser/sanitizeAll'), sanitizers = { + singleScalarParameters: require('../sanitiser/_single_scalar_parameters'), ids: require('../sanitiser/_ids'), private: require('../sanitiser/_flag_bool')('private', false) }; diff --git a/sanitiser/reverse.js b/sanitiser/reverse.js index 25e8d71b..dd9eeb1d 100644 --- a/sanitiser/reverse.js +++ b/sanitiser/reverse.js @@ -1,6 +1,7 @@ var sanitizeAll = require('../sanitiser/sanitizeAll'), sanitizers = { + singleScalarParameters: require('../sanitiser/_single_scalar_parameters'), layers: require('../sanitiser/_targets')('layers', require('../query/layers')), sources: require('../sanitiser/_targets')('sources', require('../query/sources')), size: require('../sanitiser/_size'), diff --git a/sanitiser/search.js b/sanitiser/search.js index 22737180..e1b79528 100644 --- a/sanitiser/search.js +++ b/sanitiser/search.js @@ -1,6 +1,7 @@ var sanitizeAll = require('../sanitiser/sanitizeAll'), sanitizers = { + singleScalarParameters: require('../sanitiser/_single_scalar_parameters'), text: require('../sanitiser/_text'), size: require('../sanitiser/_size'), layers: require('../sanitiser/_targets')('layers', require( '../query/layers' )), diff --git a/test/unit/run.js b/test/unit/run.js index efa0c4de..50c301de 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -30,6 +30,7 @@ var tests = [ require('./middleware/distance'), require('./middleware/confidenceScoreReverse'), require('./sanitiser/_size'), + require('./sanitiser/_single_scalar_parameters'), ]; tests.map(function(t) { diff --git a/test/unit/sanitiser/_single_scalar_parameters.js b/test/unit/sanitiser/_single_scalar_parameters.js new file mode 100644 index 00000000..4b1fc9d0 --- /dev/null +++ b/test/unit/sanitiser/_single_scalar_parameters.js @@ -0,0 +1,60 @@ +var sanitize = require('../../../sanitiser/_single_scalar_parameters'); + +module.exports.tests = {}; + +module.exports.tests.single_scalar_parameters = function(test, common) { + test('all duplicate parameters should have error messages returned', function(t) { + var raw = { + arrayParameter1: ['value1', 'value2'], + scalarParameter: 'value', + arrayParameter2: ['value3'] + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + t.deepEquals(errorsAndWarnings, { + errors: [ + '\'arrayParameter1\' parameter can only have one value', + '\'arrayParameter2\' parameter can only have one value', + ], + warnings: [] + }); + t.end(); + }); + + test('object parameters should have error messages returned', function(t) { + var raw = { + objectParameter1: { key1: 'value1', key2: 'value2'}, + scalarParameter: 'value', + objectParameter2: { } + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + t.deepEquals(errorsAndWarnings, { + errors: [ + '\'objectParameter1\' parameter must be a scalar', + '\'objectParameter2\' parameter must be a scalar' + ], + warnings: [] + }); + t.end(); + }); + + test('request with all scalar parameters should return empty errors', function(t) { + var raw = { scalarParameter1: 'value1', scalarParameter2: 2, scalarParameter3: true }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + t.deepEquals(errorsAndWarnings, { errors: [], warnings: [] }); + t.end(); + }); + +}; + +module.exports.all = function (tape, common) { + function test(name, testFunction) { + return tape('SANTIZE _single_scalar_parameters ' + name, testFunction); + } + + for( var testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; diff --git a/test/unit/sanitiser/autocomplete.js b/test/unit/sanitiser/autocomplete.js index a2a0e59e..4044d7f1 100644 --- a/test/unit/sanitiser/autocomplete.js +++ b/test/unit/sanitiser/autocomplete.js @@ -4,7 +4,7 @@ module.exports.tests = {}; module.exports.tests.sanitisers = function(test, common) { test('check sanitiser list', function (t) { - var expected = ['text', 'size', 'private', 'geo_autocomplete' ]; + var expected = ['singleScalarParameters', 'text', 'size', 'private', 'geo_autocomplete' ]; t.deepEqual(Object.keys(autocomplete.sanitiser_list), expected); t.end(); }); diff --git a/test/unit/sanitiser/place.js b/test/unit/sanitiser/place.js index ca97cf54..c3cd81cf 100644 --- a/test/unit/sanitiser/place.js +++ b/test/unit/sanitiser/place.js @@ -19,6 +19,14 @@ module.exports.tests.interface = function(test, common) { }); }; +module.exports.tests.sanitisers = function(test, common) { + test('check sanitiser list', function (t) { + var expected = ['singleScalarParameters', 'ids', 'private' ]; + t.deepEqual(Object.keys(place.sanitiser_list), expected); + t.end(); + }); +}; + module.exports.tests.sanitize_private = function(test, common) { var invalid_values = [null, -1, 123, NaN, 'abc']; invalid_values.forEach(function(value) { diff --git a/test/unit/sanitiser/reverse.js b/test/unit/sanitiser/reverse.js index 759aef28..818829f9 100644 --- a/test/unit/sanitiser/reverse.js +++ b/test/unit/sanitiser/reverse.js @@ -36,7 +36,7 @@ module.exports.tests.interface = function(test, common) { module.exports.tests.sanitisers = function(test, common) { test('check sanitiser list', function (t) { - var expected = ['layers', 'sources', 'size', 'private', 'geo_reverse', 'boundary_country']; + var expected = ['singleScalarParameters', 'layers', 'sources', 'size', 'private', 'geo_reverse', 'boundary_country']; t.deepEqual(Object.keys(reverse.sanitiser_list), expected); t.end(); }); diff --git a/test/unit/sanitiser/search.js b/test/unit/sanitiser/search.js index daa2f7ea..f50a46a2 100644 --- a/test/unit/sanitiser/search.js +++ b/test/unit/sanitiser/search.js @@ -25,7 +25,7 @@ module.exports.tests.interface = function(test, common) { module.exports.tests.sanitisers = function(test, common) { test('check sanitiser list', function (t) { - var expected = ['text', 'size', 'layers', 'sources', 'private', 'geo_search', 'boundary_country' ]; + var expected = ['singleScalarParameters', 'text', 'size', 'layers', 'sources', 'private', 'geo_search', 'boundary_country' ]; t.deepEqual(Object.keys(search.sanitiser_list), expected); t.end(); }); @@ -33,7 +33,7 @@ module.exports.tests.sanitisers = function(test, common) { module.exports.tests.sanitize_invalid_text = function(test, common) { test('invalid text', function(t) { - var invalid = [ '', 100, null, undefined, new Date() ]; + var invalid = [ '', 100, null, undefined ]; invalid.forEach( function( text ){ var req = { query: { text: text } }; sanitize(req, function(){ From a21a4476d1a1be9c44a3281bfa25a76f2d574db1 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Mon, 21 Sep 2015 17:04:45 -0400 Subject: [PATCH 02/39] added ciao tests --- .../reverse/duplicate_parameter_name.coffee | 30 +++++++++++++++++++ test/ciao/reverse/non_scalar_parameter.coffee | 30 +++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 test/ciao/reverse/duplicate_parameter_name.coffee create mode 100644 test/ciao/reverse/non_scalar_parameter.coffee diff --git a/test/ciao/reverse/duplicate_parameter_name.coffee b/test/ciao/reverse/duplicate_parameter_name.coffee new file mode 100644 index 00000000..c38c4da6 --- /dev/null +++ b/test/ciao/reverse/duplicate_parameter_name.coffee @@ -0,0 +1,30 @@ + +#> set size +path: '/v1/reverse?point.lat=1&point.lon=1¶m=value1¶m=value2' + +#? 200 ok +response.statusCode.should.be.equal 200 +response.should.have.header 'charset', 'utf8' +response.should.have.header 'content-type', 'application/json; charset=utf-8' + +#? valid geocoding block +should.exist json.geocoding +should.exist json.geocoding.version +should.exist json.geocoding.attribution +should.exist json.geocoding.query +should.exist json.geocoding.engine +should.exist json.geocoding.engine.name +should.exist json.geocoding.engine.author +should.exist json.geocoding.engine.version +should.exist json.geocoding.timestamp + +#? valid geojson +json.type.should.be.equal 'FeatureCollection' +json.features.should.be.instanceof Array + +#? expected warnings +should.not.exist json.geocoding.warnings + +#? expected errors +should.exist json.geocoding.errors +json.geocoding.errors.should.eql [ '\'param\' parameter can only have one value' ] diff --git a/test/ciao/reverse/non_scalar_parameter.coffee b/test/ciao/reverse/non_scalar_parameter.coffee new file mode 100644 index 00000000..158c80b2 --- /dev/null +++ b/test/ciao/reverse/non_scalar_parameter.coffee @@ -0,0 +1,30 @@ + +#> set size +path: '/v1/reverse?point.lat=1&point.lon=1¶meter[idx]=value' + +#? 200 ok +response.statusCode.should.be.equal 200 +response.should.have.header 'charset', 'utf8' +response.should.have.header 'content-type', 'application/json; charset=utf-8' + +#? valid geocoding block +should.exist json.geocoding +should.exist json.geocoding.version +should.exist json.geocoding.attribution +should.exist json.geocoding.query +should.exist json.geocoding.engine +should.exist json.geocoding.engine.name +should.exist json.geocoding.engine.author +should.exist json.geocoding.engine.version +should.exist json.geocoding.timestamp + +#? valid geojson +json.type.should.be.equal 'FeatureCollection' +json.features.should.be.instanceof Array + +#? expected warnings +should.not.exist json.geocoding.warnings + +#? expected errors +should.exist json.geocoding.errors +json.geocoding.errors.should.eql [ '\'parameter\' parameter must be a scalar' ] From 553f9780c57c5316f7e2e15025ae13aa8e5b1d67 Mon Sep 17 00:00:00 2001 From: Diana Shkolnikov Date: Mon, 21 Sep 2015 18:30:31 -0400 Subject: [PATCH 03/39] Remove admin matching when address is not parsed --- helper/query_parser.js | 9 + middleware/distance.js | 19 +- query/defaults.js | 2 +- routes/v1.js | 4 +- sanitiser/_text.js | 5 +- .../fixture/autocomplete_linguistic_focus.js | 2 +- ...tocomplete_linguistic_focus_null_island.js | 2 +- test/unit/fixture/search_linguistic_focus.js | 2 +- .../fixture/search_linguistic_focus_bbox.js | 2 +- .../search_linguistic_focus_null_island.js | 2 +- test/unit/helper/query_parser.js | 176 +++++++----------- 11 files changed, 97 insertions(+), 128 deletions(-) diff --git a/helper/query_parser.js b/helper/query_parser.js index b60ebcd8..aad58809 100644 --- a/helper/query_parser.js +++ b/helper/query_parser.js @@ -3,6 +3,8 @@ var parser = require('addressit'); var extend = require('extend'); var layers_map = require('../query/layers'); var delim = ','; +var check = require('check-types'); +var logger = require('pelias-logger').get('api'); module.exports = {}; @@ -61,5 +63,12 @@ module.exports.get_parsed_address = function get_parsed_address(query) { } }); + // if all we found was regions, ignore it as it is not enough information to make smarter decisions + if (Object.keys(parsed_text).length === 1 && !check.undefined(parsed_text.regions)) + { + logger.info('Ignoring address parser output, regions only'); + return null; + } + return parsed_text; }; diff --git a/middleware/distance.js b/middleware/distance.js index eb1d7c21..e333fba1 100644 --- a/middleware/distance.js +++ b/middleware/distance.js @@ -2,26 +2,31 @@ var geolib = require('geolib'); var check = require('check-types'); -function setup() { +function setup(prefix) { - return computeDistances; + return function (req, res, next) { + var opts = { + prefix: prefix || 'point.' + }; + return computeDistances(req, res, next, opts); + }; } -function computeDistances(req, res, next) { +function computeDistances(req, res, next, opts) { // do nothing if no result data set if (!res || !res.data) { return next(); } - if (!(check.number(req.clean['point.lat']) && - check.number(req.clean['point.lon']))) { + if (!(check.number(req.clean[opts.prefix + 'lat']) && + check.number(req.clean[opts.prefix + 'lon']))) { return next(); } var point = { - latitude: req.clean['point.lat'], - longitude: req.clean['point.lon'] + latitude: req.clean[opts.prefix + 'lat'], + longitude: req.clean[opts.prefix + 'lon'] }; res.data.forEach(function (place) { diff --git a/query/defaults.js b/query/defaults.js index cf9422d0..c9613d66 100644 --- a/query/defaults.js +++ b/query/defaults.js @@ -31,7 +31,7 @@ module.exports = extend( false, peliasQuery.defaults, { 'focus:function': 'linear', 'focus:offset': '1km', - 'focus:scale': '50km', + 'focus:scale': '100km', 'focus:decay': 0.5, 'function_score:score_mode': 'avg', diff --git a/routes/v1.js b/routes/v1.js index 215f1752..352f7c82 100644 --- a/routes/v1.js +++ b/routes/v1.js @@ -58,6 +58,7 @@ function addRoutes(app, peliasConfig) { sanitisers.search.middleware, middleware.types, controllers.search(), + postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig), postProc.renamePlacenames(), postProc.geocodeJSON(peliasConfig, base), @@ -67,6 +68,7 @@ function addRoutes(app, peliasConfig) { sanitisers.autocomplete.middleware, middleware.types, controllers.search(null, require('../query/autocomplete')), + postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig), postProc.renamePlacenames(), postProc.geocodeJSON(peliasConfig, base), @@ -76,7 +78,7 @@ function addRoutes(app, peliasConfig) { sanitisers.reverse.middleware, middleware.types, controllers.search(undefined, reverseQuery), - postProc.distances(), + postProc.distances('point.'), // reverse confidence scoring depends on distance from origin // so it must be calculated first postProc.confidenceScoresReverse(), diff --git a/sanitiser/_text.js b/sanitiser/_text.js index 5df79225..d0cc0986 100644 --- a/sanitiser/_text.js +++ b/sanitiser/_text.js @@ -19,7 +19,10 @@ function sanitize( raw, clean ){ clean.text = raw.text; // parse text with query parser - clean.parsed_text = query_parser.get_parsed_address(clean.text); + var parsed_text = query_parser.get_parsed_address(clean.text); + if (check.assigned(parsed_text)) { + clean.parsed_text = parsed_text; + } // try to set layers from query parser results clean.types = clean.layers || {}; diff --git a/test/unit/fixture/autocomplete_linguistic_focus.js b/test/unit/fixture/autocomplete_linguistic_focus.js index 76e844e3..a1eb6d45 100644 --- a/test/unit/fixture/autocomplete_linguistic_focus.js +++ b/test/unit/fixture/autocomplete_linguistic_focus.js @@ -44,7 +44,7 @@ module.exports = { 'lon': -82.50622 }, 'offset': '1km', - 'scale': '50km', + 'scale': '100km', 'decay': 0.5 } } diff --git a/test/unit/fixture/autocomplete_linguistic_focus_null_island.js b/test/unit/fixture/autocomplete_linguistic_focus_null_island.js index ded463bf..7e105bb3 100644 --- a/test/unit/fixture/autocomplete_linguistic_focus_null_island.js +++ b/test/unit/fixture/autocomplete_linguistic_focus_null_island.js @@ -44,7 +44,7 @@ module.exports = { 'lon': 0 }, 'offset': '1km', - 'scale': '50km', + 'scale': '100km', 'decay': 0.5 } } diff --git a/test/unit/fixture/search_linguistic_focus.js b/test/unit/fixture/search_linguistic_focus.js index eac92086..b9878dec 100644 --- a/test/unit/fixture/search_linguistic_focus.js +++ b/test/unit/fixture/search_linguistic_focus.js @@ -44,7 +44,7 @@ module.exports = { 'lon': -82.50622 }, 'offset': '1km', - 'scale': '50km', + 'scale': '100km', 'decay': 0.5 } } diff --git a/test/unit/fixture/search_linguistic_focus_bbox.js b/test/unit/fixture/search_linguistic_focus_bbox.js index d7aa6544..b6e642d1 100644 --- a/test/unit/fixture/search_linguistic_focus_bbox.js +++ b/test/unit/fixture/search_linguistic_focus_bbox.js @@ -44,7 +44,7 @@ module.exports = { 'lon': -82.50622 }, 'offset': '1km', - 'scale': '50km', + 'scale': '100km', 'decay': 0.5 } } diff --git a/test/unit/fixture/search_linguistic_focus_null_island.js b/test/unit/fixture/search_linguistic_focus_null_island.js index 506dcea9..4ba2adf0 100644 --- a/test/unit/fixture/search_linguistic_focus_null_island.js +++ b/test/unit/fixture/search_linguistic_focus_null_island.js @@ -44,7 +44,7 @@ module.exports = { 'lon': 0 }, 'offset': '1km', - 'scale': '50km', + 'scale': '100km', 'decay': 0.5 } } diff --git a/test/unit/helper/query_parser.js b/test/unit/helper/query_parser.js index ad669ece..2d3b462c 100644 --- a/test/unit/helper/query_parser.js +++ b/test/unit/helper/query_parser.js @@ -13,26 +13,22 @@ module.exports.tests.interface = function(test, common) { }; module.exports.tests.split_on_comma = function(test, common) { - var queries = ['soho, new york', 'chelsea, london', '123 main, new york']; - var delim = ','; + var queries = [ + { name: 'soho', admin_parts: 'new york' }, + { name: 'chelsea', admin_parts: 'london' }, + { name: '123 main', admin_parts: 'new york' } + ]; - var testParse = function(query) { + queries.forEach(function (query) { test('naive parsing ' + query, function(t) { - var address = parser.get_parsed_address(query); - var delimIndex = query.indexOf(delim); - var name = query.substring(0, delimIndex); - var admin_parts = query.substring(delimIndex + 1).trim(); + var address = parser.get_parsed_address(query.name + ', ' + query.admin_parts); t.equal(typeof address, 'object', 'valid object'); - t.equal(address.name, name, 'name set correctly to ' + address.name); - t.equal(address.admin_parts, admin_parts, 'admin_parts set correctly to ' + address.admin_parts); + t.equal(address.name, query.name, 'name set correctly to ' + address.name); + t.equal(address.admin_parts, query.admin_parts, 'admin_parts set correctly to ' + address.admin_parts); t.end(); }); - }; - - for (var key in queries) { - testParse( queries[key] ); - } + }); }; module.exports.tests.parse_three_chars_or_less = function(test, common) { @@ -40,7 +36,8 @@ module.exports.tests.parse_three_chars_or_less = function(test, common) { var num_queries = ['1', '12', '123']; var alphanum_q = ['a1', '1a2', '12c']; - var testParse = function(query) { + var queries = chars_queries.concat(num_queries).concat(alphanum_q); + queries.forEach(function(query) { test('query length < 3 (' + query + ')', function(t) { var address = parser.get_parsed_address(query); var target_layer = layers_map.coarse; @@ -50,111 +47,64 @@ module.exports.tests.parse_three_chars_or_less = function(test, common) { t.deepEqual(layers, target_layer, 'admin_parts set correctly to ' + target_layer.join(', ')); t.end(); }); - }; - - var queries = chars_queries.concat(num_queries).concat(alphanum_q); - for (var key in queries) { - testParse( queries[key] ); - } + }); }; -module.exports.tests.parse_one_or_more_tokens = function(test, common) { - var one_token_queries = ['hyderbad', 'yugoslavia', 'somethingreallybigbutjustonetokenstill']; - var two_tokens_nonum = ['small town', 'biggg city', 'another empire']; - var two_tokens_withnum= ['123 main', 'sixty 1', '123-980 house']; - - // parse address is now always true to fix pelias/api#194 - var testParse = function(query, parse_address) { - test('query with one or more tokens (' + query + ')', function(t) { - var address = parser.get_parsed_address(query); - var target_layer = layers_map.coarse.concat(layers_map.venue); - var layers = parser.get_layers(query); - - t.equal(typeof address, 'object', 'valid object'); - - if (parse_address) { - t.deepEqual(address.regions.join(''), query, 'since query contained a number, it went through address parsing'); - } else { - t.deepEqual(layers, target_layer, 'admin_parts set correctly to ' + target_layer.join(', ')); - } - - t.end(); - }); - }; - - var queries = one_token_queries.concat(two_tokens_nonum); - for (var key in queries) { - testParse( queries[key], true ); - } - for (key in two_tokens_withnum) { - testParse( two_tokens_withnum[key], true ); - } +module.exports.tests.parse_one_token = function(test, common) { + test('query with one token', function (t) { + var address = parser.get_parsed_address('yugolsavia'); + t.equal(address, null, 'nothing address specific detected'); + t.end(); + }); + test('query with two tokens, no numbers', function (t) { + var address = parser.get_parsed_address('small town'); + t.equal(address, null, 'nothing address specific detected'); + t.end(); + }); + test('query with two tokens, number first', function (t) { + var address = parser.get_parsed_address('123 main'); + t.equal(address, null, 'nothing address specific detected'); + t.end(); + }); + test('query with two tokens, number second', function (t) { + var address = parser.get_parsed_address('main 123'); + t.equal(address, null, 'nothing address specific detected'); + t.end(); + }); + test('query with many tokens', function(t) { + var address = parser.get_parsed_address('main particle new york'); + t.equal(address, null, 'nothing address specific detected'); + t.end(); + }); }; module.exports.tests.parse_address = function(test, common) { - var addresses_nonum = [{ non_street: 'main particle', city: 'new york'}, - { non_street: 'biggg city block' }, - { non_street: 'the empire state building' } - ]; - var address_with_num = [{ number: 123, street: 'main st', city: 'new york', state: 'ny'}, - { number: 456, street: 'pine ave', city: 'san francisco', state: 'CA'}, - { number: 1980, street: 'house st', city: 'hoboken', state: 'NY'} - ]; - var address_with_zip = [{ number: 1, street: 'main st', city: 'new york', state: 'ny', zip: 10010}, - { number: 4, street: 'ape ave', city: 'san diego', state: 'CA', zip: 98970}, - { number: 19, street: 'house dr', city: 'houston', state: 'TX', zip: 79089} - ]; - - var testParse = function(query, hasNumber, hasZip) { - var testcase = 'parse query with ' + (hasNumber ? 'a house number ': 'no house number '); - testcase += 'and ' + (hasZip ? 'a zip ' : 'no zip '); - - test(testcase, function(t) { - var query_string = ''; - for (var k in query) { - query_string += ' ' + query[k]; - } - - // remove leading whitespace - query_string = query_string.substring(1); - - var address = parser.get_parsed_address(query_string); - - t.equal(typeof address, 'object', 'valid object for the address ('+query_string+')'); - - if (!hasNumber && !hasZip && query.non_street) { - t.equal(address.regions.join(''), query_string, 'expected parsing result'); - } else { - t.equal(address.regions.join(''), query.city, 'city in regions (' + query.city +')'); - } - - if ((hasNumber || hasZip) && query.street) { - t.equal(typeof address.number, 'number', 'valid house number format (' + address.number + ')'); - t.equal(address.number, query.number, 'correct house number (' + query.number + ')'); - t.equal(typeof address.street, 'string', 'valid street name format (' + address.street + ')'); - t.equal(address.street, query.street, 'correct street name (' + query.street + ')'); - } - - if (hasZip) { - t.equal(typeof address.postalcode, 'number', 'valid zip (' + address.postalcode + ')'); - t.equal(address.postalcode, query.zip, 'correct postal code (' + query.zip + ')'); - } - - t.end(); - }); - }; - - for (var key in addresses_nonum) { - testParse( addresses_nonum[key] ); - } - for (key in address_with_num) { - testParse( address_with_num[key], true ); - } - for (key in address_with_zip) { - testParse( address_with_zip[key], true, true ); - } + test('valid address, house number', function(t) { + var query_string = '123 main st new york ny'; + var address = parser.get_parsed_address(query_string); + + t.equal(typeof address, 'object', 'valid object for the address'); + t.equal(address.number, 123, 'parsed house number'); + t.equal(address.street, 'main st', 'parsed street'); + t.deepEqual(address.regions, ['new york'], 'parsed city'); + t.equal(address.state , 'NY', 'parsed state'); + t.end(); + }); + test('valid address, zipcode', function(t) { + var query_string = '123 main st new york ny 10010'; + var address = parser.get_parsed_address(query_string); + + t.equal(typeof address, 'object', 'valid object for the address'); + t.equal(address.number, 123, 'parsed house number'); + t.equal(address.street, 'main st', 'parsed street'); + t.deepEqual(address.regions, ['new york'], 'parsed city'); + t.equal(address.state , 'NY', 'parsed state'); + t.equal(address.postalcode, 10010, 'parsed zip'); + t.end(); + }); }; + module.exports.all = function (tape, common) { function test(name, testFunction) { From e16e2bbb4e368b24d5b18b138b6d433c3d244b7f Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 09:36:57 -0400 Subject: [PATCH 04/39] fixed comment --- sanitiser/_geo_reverse.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index 4b5ee959..9de74fb0 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -14,7 +14,7 @@ module.exports = function sanitize( raw, clean ){ geo_common.sanitize_point( 'point', clean, raw, LAT_LON_IS_REQUIRED ); // this hack is to allow point.lat/point.lon to be used interchanagbly - // with boundary.circle.lat/boundary.circle/lon + // with boundary.circle.lat/boundary.circle.lon if( !clean.hasOwnProperty('boundary.circle.lat') && clean.hasOwnProperty('point.lat') ){ raw['boundary.circle.lat'] = clean['point.lat']; } From 2b22d611bd8f630a6fb1dce5d02dc79ed4bb02c6 Mon Sep 17 00:00:00 2001 From: Diana Shkolnikov Date: Tue, 22 Sep 2015 12:28:26 -0400 Subject: [PATCH 05/39] Fix lost confidence score --- controller/search.js | 1 + query/text_parser.js | 7 ++++--- test/unit/controller/search.js | 31 +++++++++++++++++++++++++++++++ test/unit/service/search.js | 7 ++++++- 4 files changed, 42 insertions(+), 4 deletions(-) diff --git a/controller/search.js b/controller/search.js index 28fa3837..795ad2c6 100644 --- a/controller/search.js +++ b/controller/search.js @@ -36,6 +36,7 @@ function setup( backend, query ){ // set response data else { res.data = docs; + res.meta = meta; } next(); diff --git a/query/text_parser.js b/query/text_parser.js index 63d8b93b..408d00f9 100644 --- a/query/text_parser.js +++ b/query/text_parser.js @@ -1,4 +1,5 @@ +var logger = require('pelias-logger').get('api'); var adminFields = require('../helper/adminFields')(); /** @@ -21,9 +22,9 @@ function addParsedVariablesToQueryVariables( parsed_text, vs ){ // ? else { - console.warn( 'chaos monkey asks: what happens now?' ); - console.log( parsed_text ); - try{ throw new Error(); } catch(e){ console.error( e.stack ); } // print a stack trace + logger.warn( 'chaos monkey asks: what happens now?' ); + logger.warn( parsed_text ); + try{ throw new Error(); } catch(e){ logger.warn( e.stack ); } // print a stack trace } // ==== add parsed matches [address components] ==== diff --git a/test/unit/controller/search.js b/test/unit/controller/search.js index 9a21f4c7..2b412013 100644 --- a/test/unit/controller/search.js +++ b/test/unit/controller/search.js @@ -41,6 +41,35 @@ module.exports.tests.functional_success = function(test, common) { } }]; + var expectedMeta = { + scores: [10, 20] + }; + + var expectedData = [ + { + _id: 'myid1', + _score: 10, + _type: 'mytype1', + admin0: 'country1', + admin1: 'state1', + admin2: 'city1', + center_point: { lat: 100.1, lon: -50.5 }, + name: { default: 'test name1' }, + value: 1 + }, + { + _id: 'myid2', + _score: 20, + _type: 'mytype2', + admin0: 'country2', + admin1: 'state2', + admin2: 'city2', + center_point: { lat: 100.2, lon: -51.5 }, + name: { default: 'test name2' }, + value: 2 + } + ]; + test('functional success', function (t) { var backend = mockBackend('client/search/ok/1', function (cmd) { t.deepEqual(cmd, { @@ -66,6 +95,8 @@ module.exports.tests.functional_success = function(test, common) { var req = { clean: { a: 'b' }, errors: [], warnings: [] }; var next = function next() { t.equal(req.errors.length, 0, 'next was called without error'); + t.deepEqual(res.meta, expectedMeta, 'meta data was set'); + t.deepEqual(res.data, expectedData, 'data was set'); t.end(); }; controller(req, res, next); diff --git a/test/unit/service/search.js b/test/unit/service/search.js index a7212775..e5bf2cff 100644 --- a/test/unit/service/search.js +++ b/test/unit/service/search.js @@ -35,16 +35,21 @@ module.exports.tests.functional_success = function(test, common) { } ]; + var expectedMeta = { + scores: [10, 20] + }; + test('valid ES query', function(t) { var backend = mockBackend( 'client/search/ok/1', function( cmd ){ t.deepEqual(cmd, example_valid_es_query, 'no change to the command'); }); - setup( backend, example_valid_es_query, function(err, data) { + setup( backend, example_valid_es_query, function(err, data, meta) { t.true(Array.isArray(data), 'returns an array'); data.forEach(function(d) { t.true(typeof d === 'object', 'valid object'); }); t.deepEqual(data, expected, 'values correctly mapped'); + t.deepEqual(meta, expectedMeta, 'meta data correctly mapped'); t.end(); }); }); From 80f419846dd71473364e48cf30318273bf2af82b Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Tue, 22 Sep 2015 18:29:23 +0200 Subject: [PATCH 06/39] admin boost for sqrt(popularity), autocomplete focus not restricted to phrase matches --- package.json | 4 +- query/autocomplete.js | 3 +- query/search.js | 3 +- .../fixture/autocomplete_linguistic_focus.js | 48 +++++++++++++++++-- ...tocomplete_linguistic_focus_null_island.js | 48 +++++++++++++++++-- .../fixture/autocomplete_linguistic_only.js | 42 ++++++++++++++++ test/unit/fixture/search_boundary_country.js | 42 ++++++++++++++++ test/unit/fixture/search_full_address.js | 42 ++++++++++++++++ test/unit/fixture/search_linguistic_bbox.js | 42 ++++++++++++++++ test/unit/fixture/search_linguistic_focus.js | 42 ++++++++++++++++ .../fixture/search_linguistic_focus_bbox.js | 42 ++++++++++++++++ .../search_linguistic_focus_null_island.js | 42 ++++++++++++++++ test/unit/fixture/search_linguistic_only.js | 42 ++++++++++++++++ test/unit/fixture/search_partial_address.js | 42 ++++++++++++++++ test/unit/fixture/search_regions_address.js | 42 ++++++++++++++++ 15 files changed, 514 insertions(+), 12 deletions(-) diff --git a/package.json b/package.json index 339033f1..d949fdb3 100644 --- a/package.json +++ b/package.json @@ -44,15 +44,15 @@ "geojson-extent": "^0.3.1", "geolib": "^2.0.18", "geopipes-elasticsearch-backend": "^0.2.0", - "lodash": "^3.10.1", "iso3166-1": "^0.2.3", + "lodash": "^3.10.1", "markdown": "0.5.0", "microtime": "1.4.0", "morgan": "1.5.2", "pelias-config": "^1.0.1", "pelias-esclient": "0.0.25", "pelias-logger": "^0.0.8", - "pelias-query": "1.5.0", + "pelias-query": "2.0.0", "pelias-schema": "1.0.0", "pelias-suggester-pipeline": "2.0.2", "stats-lite": "^1.0.3", diff --git a/query/autocomplete.js b/query/autocomplete.js index 7d45aea3..62fb775e 100644 --- a/query/autocomplete.js +++ b/query/autocomplete.js @@ -13,7 +13,8 @@ query.score( peliasQuery.view.ngrams, 'must' ); // scoring boost query.score( peliasQuery.view.phrase ); -query.score( peliasQuery.view.focus ); +query.score( peliasQuery.view.focus( peliasQuery.view.ngrams ) ); +query.score( peliasQuery.view.popularity(['admin0','admin1','admin2']) ); // -------------------------------- diff --git a/query/search.js b/query/search.js index a74cf8ce..bf95b76b 100644 --- a/query/search.js +++ b/query/search.js @@ -14,7 +14,8 @@ query.score( peliasQuery.view.ngrams, 'must' ); // scoring boost query.score( peliasQuery.view.phrase ); -query.score( peliasQuery.view.focus ); +query.score( peliasQuery.view.focus( peliasQuery.view.phrase ) ); +query.score( peliasQuery.view.popularity(['admin0','admin1','admin2']) ); // address components query.score( peliasQuery.view.address('housenumber') ); diff --git a/test/unit/fixture/autocomplete_linguistic_focus.js b/test/unit/fixture/autocomplete_linguistic_focus.js index 76e844e3..2049bf0b 100644 --- a/test/unit/fixture/autocomplete_linguistic_focus.js +++ b/test/unit/fixture/autocomplete_linguistic_focus.js @@ -27,11 +27,9 @@ module.exports = { 'function_score': { 'query': { 'match': { - 'phrase.default': { - 'analyzer': 'peliasPhrase', - 'type': 'phrase', + 'name.default': { + 'analyzer': 'peliasOneEdgeGram', 'boost': 1, - 'slop': 2, 'query': 'test' } } @@ -52,6 +50,48 @@ module.exports = { '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 + }] + } }] } } diff --git a/test/unit/fixture/autocomplete_linguistic_focus_null_island.js b/test/unit/fixture/autocomplete_linguistic_focus_null_island.js index ded463bf..57d4747c 100644 --- a/test/unit/fixture/autocomplete_linguistic_focus_null_island.js +++ b/test/unit/fixture/autocomplete_linguistic_focus_null_island.js @@ -27,11 +27,9 @@ module.exports = { 'function_score': { 'query': { 'match': { - 'phrase.default': { - 'analyzer': 'peliasPhrase', - 'type': 'phrase', + 'name.default': { + 'analyzer': 'peliasOneEdgeGram', 'boost': 1, - 'slop': 2, 'query': 'test' } } @@ -52,6 +50,48 @@ module.exports = { '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 + }] + } }] } } diff --git a/test/unit/fixture/autocomplete_linguistic_only.js b/test/unit/fixture/autocomplete_linguistic_only.js index 767be651..ac059ef1 100644 --- a/test/unit/fixture/autocomplete_linguistic_only.js +++ b/test/unit/fixture/autocomplete_linguistic_only.js @@ -23,6 +23,48 @@ module.exports = { 'slop': 2 } } + }, + { + '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 + }] + } }] } } diff --git a/test/unit/fixture/search_boundary_country.js b/test/unit/fixture/search_boundary_country.js index f4a873bd..a75a8091 100644 --- a/test/unit/fixture/search_boundary_country.js +++ b/test/unit/fixture/search_boundary_country.js @@ -33,6 +33,48 @@ module.exports = { 'slop': 2 } } + }, + { + '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 + }] + } }] } } diff --git a/test/unit/fixture/search_full_address.js b/test/unit/fixture/search_full_address.js index cbad15f1..aff242b8 100644 --- a/test/unit/fixture/search_full_address.js +++ b/test/unit/fixture/search_full_address.js @@ -26,6 +26,48 @@ module.exports = { 'boost': 1 } } + }, + { + '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 + }] + } },{ 'match': { 'address.number': { diff --git a/test/unit/fixture/search_linguistic_bbox.js b/test/unit/fixture/search_linguistic_bbox.js index 69dfe4bb..754421e0 100644 --- a/test/unit/fixture/search_linguistic_bbox.js +++ b/test/unit/fixture/search_linguistic_bbox.js @@ -23,6 +23,48 @@ module.exports = { 'slop': 2 } } + }, + { + '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 + }] + } }] } }, diff --git a/test/unit/fixture/search_linguistic_focus.js b/test/unit/fixture/search_linguistic_focus.js index eac92086..072bca64 100644 --- a/test/unit/fixture/search_linguistic_focus.js +++ b/test/unit/fixture/search_linguistic_focus.js @@ -52,6 +52,48 @@ module.exports = { '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 + }] + } }] } } diff --git a/test/unit/fixture/search_linguistic_focus_bbox.js b/test/unit/fixture/search_linguistic_focus_bbox.js index d7aa6544..09262a0e 100644 --- a/test/unit/fixture/search_linguistic_focus_bbox.js +++ b/test/unit/fixture/search_linguistic_focus_bbox.js @@ -52,6 +52,48 @@ module.exports = { '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 + }] + } }] } }, diff --git a/test/unit/fixture/search_linguistic_focus_null_island.js b/test/unit/fixture/search_linguistic_focus_null_island.js index 506dcea9..832aa9f7 100644 --- a/test/unit/fixture/search_linguistic_focus_null_island.js +++ b/test/unit/fixture/search_linguistic_focus_null_island.js @@ -52,6 +52,48 @@ module.exports = { '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 + }] + } }] } } diff --git a/test/unit/fixture/search_linguistic_only.js b/test/unit/fixture/search_linguistic_only.js index 767be651..ac059ef1 100644 --- a/test/unit/fixture/search_linguistic_only.js +++ b/test/unit/fixture/search_linguistic_only.js @@ -23,6 +23,48 @@ module.exports = { 'slop': 2 } } + }, + { + '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 + }] + } }] } } diff --git a/test/unit/fixture/search_partial_address.js b/test/unit/fixture/search_partial_address.js index 5b59f70c..94210bf5 100644 --- a/test/unit/fixture/search_partial_address.js +++ b/test/unit/fixture/search_partial_address.js @@ -26,6 +26,48 @@ module.exports = { 'boost': 1 } } + }, + { + '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 + }] + } },{ 'match': { 'admin0': { diff --git a/test/unit/fixture/search_regions_address.js b/test/unit/fixture/search_regions_address.js index 5a01e27a..05d4ffe5 100644 --- a/test/unit/fixture/search_regions_address.js +++ b/test/unit/fixture/search_regions_address.js @@ -26,6 +26,48 @@ module.exports = { 'boost': 1 } } + }, + { + '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 + }] + } },{ 'match': { 'address.number': { From ebb3f33b2d0b17e272b49a676874aff67399d60c Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Fri, 18 Sep 2015 17:14:55 -0400 Subject: [PATCH 07/39] Remove unused test file --- test/unit/sanitiser/_source.js | 127 --------------------------------- 1 file changed, 127 deletions(-) delete mode 100644 test/unit/sanitiser/_source.js diff --git a/test/unit/sanitiser/_source.js b/test/unit/sanitiser/_source.js deleted file mode 100644 index 991accd5..00000000 --- a/test/unit/sanitiser/_source.js +++ /dev/null @@ -1,127 +0,0 @@ -var sanitize = require( '../../../sanitiser/_source' ); - -var success_response = { error: false }; - -module.exports.tests = {}; - -module.exports.tests.no_sources = function(test, common) { - test('source is not set', function(t) { - var req = { - query: { }, - clean: { } - }; - - var response = sanitize(req.query, req.clean); - - t.deepEqual(req.clean.types, {}, 'clean.types should be empty object'); - t.deepEqual(response.errors, [], 'no error returned'); - t.deepEqual(response.warnings, [], 'no warnings returned'); - t.end(); - }); - - test('source is empty string', function(t) { - var req = { - query: { - source: '' - }, - clean: { } - }; - - var response = sanitize(req.query, req.clean); - - t.deepEqual(req.clean.types, {}, 'clean.types should be empty object'); - t.deepEqual(response.errors, [], 'no error returned'); - t.deepEqual(response.warnings, [], 'no warnings returned'); - t.end(); - }); -}; - -module.exports.tests.valid_sources = function(test, common) { - test('geonames source', function(t) { - var req = { - query: { - source: 'geonames' - }, - clean: { } - }; - - var response = sanitize(req.query, req.clean); - - t.deepEqual(req.clean.types, { from_source: ['geoname'] }, 'clean.types should contain from_source entry with geonames'); - t.deepEqual(response.errors, [], 'no error returned'); - t.deepEqual(response.warnings, [], 'no warnings returned'); - t.end(); - }); - - test('openstreetmap source', function(t) { - var req = { - query: { - source: 'openstreetmap' - }, - clean: { } - }; - var expected_types = { - from_source: ['osmaddress', 'osmnode', 'osmway'] - }; - - var response = sanitize(req.query, req.clean); - - t.deepEqual(req.clean.types, expected_types, 'clean.types should contain from_source entry with multiple entries for openstreetmap'); - t.deepEqual(response.errors, [], 'no error returned'); - t.deepEqual(response.warnings, [], 'no warnings returned'); - t.end(); - }); - - test('multiple sources', function(t) { - var req = { - query: { - source: 'openstreetmap,openaddresses' - }, - clean: { } - }; - var expected_types = { - from_source: ['osmaddress', 'osmnode', 'osmway', 'openaddresses'] - }; - - var response = sanitize(req.query, req.clean); - - t.deepEqual(req.clean.types, expected_types, - 'clean.types should contain from_source entry with multiple entries for openstreetmap and openadresses'); - t.deepEqual(response.errors, [], 'no error returned'); - t.deepEqual(response.warnings, [], 'no warnings returned'); - t.end(); - }); -}; - -module.exports.tests.invalid_sources = function(test, common) { - test('geonames source', function(t) { - var req = { - query: { - source: 'notasource' - }, - clean: { } - }; - var expected_response = { - errors: [ - '\'notasource\' is an invalid source parameter. Valid options: geonames,openaddresses,quattroshapes,openstreetmap' - ], - warnings: [] - }; - - var response = sanitize(req.query, req.clean); - - t.deepEqual(response, expected_response, 'error with message returned'); - t.deepEqual(req.clean.types, { }, 'clean.types should remain empty'); - t.end(); - }); -}; - -module.exports.all = function (tape, common) { - function test(name, testFunction) { - return tape('SANTIZE _source ' + name, testFunction); - } - - for( var testCase in module.exports.tests ){ - module.exports.tests[testCase](test, common); - } -}; From 947797f41e84247e7ee4a1e7c1bc093937ca1a1c Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 13:28:48 -0400 Subject: [PATCH 08/39] added warning if any of boundary.circle.lat/lon/radius are supplied + tests --- sanitiser/_geo_common.js | 3 +- sanitiser/_geo_reverse.js | 11 +++++ test/unit/run.js | 1 + test/unit/sanitiser/_geo_reverse.js | 67 +++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 test/unit/sanitiser/_geo_reverse.js diff --git a/sanitiser/_geo_common.js b/sanitiser/_geo_common.js index b88e9453..4e0f8c9b 100644 --- a/sanitiser/_geo_common.js +++ b/sanitiser/_geo_common.js @@ -55,7 +55,6 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) { * @param {bool} circle_is_required */ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { - // the names we use to define the centroid var mandatoryProps = [ 'lat', 'lon' ]; @@ -86,7 +85,7 @@ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { // radius was specified without lat or lon else if( raw.hasOwnProperty( key_prefix + '.radius' ) ){ var format2 = 'missing circle param \'%s\' requires all of: \'%s\' to be present'; - throw new Error( util.format( format2, key_prefix, mandatoryProps.join('\',\'') ) ); + throw new Error( util.format( format2, key_prefix, mandatoryProps.join('\',\'') ) ); } // fields required, eg. ( totalFieldsSpecified === 0 && bbox_is_required === true ) else if( circle_is_required ){ diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index 9de74fb0..6dc134cc 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -10,11 +10,22 @@ module.exports = function sanitize( raw, clean ){ // error & warning messages var messages = { errors: [], warnings: [] }; + // helper function to determine if raw has a boundary.circle property + var hasBoundaryCircleField = function(field) { + return raw.hasOwnProperty('boundary.circle.' + field); + }; + + if (['lat', 'lon', 'radius'].some(hasBoundaryCircleField)) { + messages.warnings.push('boundary.circle is currently unsupported and being ignored'); + } + try { geo_common.sanitize_point( 'point', clean, raw, LAT_LON_IS_REQUIRED ); // this hack is to allow point.lat/point.lon to be used interchanagbly // with boundary.circle.lat/boundary.circle.lon + // + // if clean doesn't have boundary.circle.lat but clean has point.lat, set boundary.circle.lat to point.lat if( !clean.hasOwnProperty('boundary.circle.lat') && clean.hasOwnProperty('point.lat') ){ raw['boundary.circle.lat'] = clean['point.lat']; } diff --git a/test/unit/run.js b/test/unit/run.js index efa0c4de..5e9aca75 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -30,6 +30,7 @@ var tests = [ require('./middleware/distance'), require('./middleware/confidenceScoreReverse'), require('./sanitiser/_size'), + require('./sanitiser/_geo_reverse'), ]; tests.map(function(t) { diff --git a/test/unit/sanitiser/_geo_reverse.js b/test/unit/sanitiser/_geo_reverse.js new file mode 100644 index 00000000..74f349d0 --- /dev/null +++ b/test/unit/sanitiser/_geo_reverse.js @@ -0,0 +1,67 @@ +var sanitize = require('../../../sanitiser/_geo_reverse'); + +module.exports.tests = {}; + +module.exports.tests.sanitize_boundary_country = function(test, common) { + test('raw with boundary.circle.lat should add warning about ignored boundary.circle', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121', + 'boundary.circle.lat': '13.131313' + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + t.equals(clean['boundary.circle.lat'], 12.121212, 'should be set to point.lat'); + t.deepEquals(errorsAndWarnings, { + errors: [], + warnings: ['boundary.circle is currently unsupported and being ignored'] + }, 'no warnings/errors'); + t.end(); + }); + + test('raw with boundary.circle.lon should add warning about ignored boundary.circle', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121', + 'boundary.circle.lon': '31.313131' + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + t.equals(clean['boundary.circle.lon'], 21.212121, 'should be set to point.lon'); + t.deepEquals(errorsAndWarnings, { + errors: [], + warnings: ['boundary.circle is currently unsupported and being ignored'] + }, 'no warnings/errors'); + t.end(); + }); + + test('raw with boundary.circle.radius should add warning about ignored boundary.circle', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121', + 'boundary.circle.radius': '17' + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + // t.equals(clean['boundary.circle.radius'], 12.121212, 'should be set to point.lat') + t.deepEquals(errorsAndWarnings, { + errors: [], + warnings: ['boundary.circle is currently unsupported and being ignored'] + }, 'no warnings/errors'); + t.end(); + }); + +}; + +module.exports.all = function (tape, common) { + function test(name, testFunction) { + return tape('SANTIZE _geo_reverse ' + name, testFunction); + } + + for( var testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; From 9fdddd3834bbd1d14ced16d237ff7363d2880e84 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 13:56:54 -0400 Subject: [PATCH 09/39] unrolled not-not conditional to positive conditional --- sanitiser/_geo_common.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sanitiser/_geo_common.js b/sanitiser/_geo_common.js index 4e0f8c9b..00112d89 100644 --- a/sanitiser/_geo_common.js +++ b/sanitiser/_geo_common.js @@ -2,7 +2,8 @@ * helper sanitiser methods for geo parameters */ var util = require('util'), - check = require('check-types'); + check = require('check-types'), + _ = require('lodash'); /** * Parse and validate rect parameter @@ -55,6 +56,8 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) { * @param {bool} circle_is_required */ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { + // "boundary.circle", clean, raw, true + // the names we use to define the centroid var mandatoryProps = [ 'lat', 'lon' ]; @@ -144,7 +147,7 @@ function sanitize_point( key_prefix, clean, raw, point_is_required ) { */ function sanitize_coord( key, clean, param, latlon_is_required ) { var value = parseFloat( param ); - if ( !isNaN( value ) ) { + if ( _.isFinite( value ) ) { clean[key] = value; } else if (latlon_is_required) { From 0595a290524d9fbfd720bcc6f7d0703a4fbe9d46 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 14:11:54 -0400 Subject: [PATCH 10/39] renamed parameters for readability --- sanitiser/_geo_common.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/sanitiser/_geo_common.js b/sanitiser/_geo_common.js index 00112d89..c74db609 100644 --- a/sanitiser/_geo_common.js +++ b/sanitiser/_geo_common.js @@ -56,8 +56,6 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) { * @param {bool} circle_is_required */ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { - // "boundary.circle", clean, raw, true - // the names we use to define the centroid var mandatoryProps = [ 'lat', 'lon' ]; @@ -142,13 +140,13 @@ function sanitize_point( key_prefix, clean, raw, point_is_required ) { * * @param {string} key * @param {object} clean - * @param {string} param + * @param {string} rawValue * @param {bool} latlon_is_required */ -function sanitize_coord( key, clean, param, latlon_is_required ) { - var value = parseFloat( param ); - if ( _.isFinite( value ) ) { - clean[key] = value; +function sanitize_coord( key, clean, rawValue, latlon_is_required ) { + var parsedValue = parseFloat( rawValue ); + if ( _.isFinite( parsedValue ) ) { + clean[key] = parsedValue; } else if (latlon_is_required) { throw new Error( util.format( 'missing param \'%s\'', key ) ); From 0f6d08c0ecff8bc963905c7afa103df44a2dcb33 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 14:51:13 -0400 Subject: [PATCH 11/39] set boundary.circle.radius to default if caller did not supply it --- sanitiser/_geo_common.js | 2 ++ sanitiser/_geo_reverse.js | 21 +++++++------ test/unit/sanitiser/_geo_reverse.js | 47 +++++++++++++++++++++++++++++ test/unit/sanitiser/reverse.js | 2 ++ 4 files changed, 63 insertions(+), 9 deletions(-) diff --git a/sanitiser/_geo_common.js b/sanitiser/_geo_common.js index c74db609..153128bc 100644 --- a/sanitiser/_geo_common.js +++ b/sanitiser/_geo_common.js @@ -56,6 +56,8 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) { * @param {bool} circle_is_required */ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { + // "boundary.circle", clean, raw, false + // the names we use to define the centroid var mandatoryProps = [ 'lat', 'lon' ]; diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index 6dc134cc..bac20b29 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -1,5 +1,7 @@ var geo_common = require ('./_geo_common'); +var check = require('check-types'); +var defaults = require('../query/defaults'); var LAT_LON_IS_REQUIRED = true, CIRCLE_IS_REQUIRED = false, CIRCLE_MUST_BE_COMPLETE = false; @@ -20,20 +22,21 @@ module.exports = function sanitize( raw, clean ){ } try { + // first verify that point.lat/point.lon are valid geo_common.sanitize_point( 'point', clean, raw, LAT_LON_IS_REQUIRED ); - // this hack is to allow point.lat/point.lon to be used interchanagbly - // with boundary.circle.lat/boundary.circle.lon - // - // if clean doesn't have boundary.circle.lat but clean has point.lat, set boundary.circle.lat to point.lat - if( !clean.hasOwnProperty('boundary.circle.lat') && clean.hasOwnProperty('point.lat') ){ - raw['boundary.circle.lat'] = clean['point.lat']; - } - if( !clean.hasOwnProperty('boundary.circle.lon') && clean.hasOwnProperty('point.lon') ){ - raw['boundary.circle.lon'] = clean['point.lon']; + // overwrite boundary.circle.lat/lon with point.lat/lon + raw['boundary.circle.lat'] = clean['point.lat']; + raw['boundary.circle.lon'] = clean['point.lon']; + + // if no radius was passed, set the default + if (!check.assigned(raw['boundary.circle.radius'])) { + raw['boundary.circle.radius'] = defaults['boundary:circle:radius']; } + // santize the boundary.circle geo_common.sanitize_circle( 'boundary.circle', clean, raw, CIRCLE_IS_REQUIRED ); + } catch (err) { messages.errors.push( err.message ); diff --git a/test/unit/sanitiser/_geo_reverse.js b/test/unit/sanitiser/_geo_reverse.js index 74f349d0..c7fb778a 100644 --- a/test/unit/sanitiser/_geo_reverse.js +++ b/test/unit/sanitiser/_geo_reverse.js @@ -1,4 +1,5 @@ var sanitize = require('../../../sanitiser/_geo_reverse'); +var defaults = require('../../../query/defaults'); module.exports.tests = {}; @@ -54,6 +55,52 @@ module.exports.tests.sanitize_boundary_country = function(test, common) { t.end(); }); + test('boundary.circle.lat/lon should be overridden with point.lat/lon', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121', + 'boundary.circle.lat': '13.131313', + 'boundary.circle.lon': '31.313131' + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + t.equals(raw['boundary.circle.lat'], 12.121212, 'should be set to point.lat'); + t.equals(raw['boundary.circle.lon'], 21.212121, 'should be set to point.lon'); + t.equals(clean['boundary.circle.lat'], 12.121212, 'should be set to point.lat'); + t.equals(clean['boundary.circle.lon'], 21.212121, 'should be set to point.lon'); + t.end(); + }); + + test('no boundary.circle.radius supplied should be set to default', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121' + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + t.equals(raw['boundary.circle.radius'], defaults['boundary:circle:radius'], 'should be from defaults'); + t.equals(clean['boundary.circle.radius'], parseFloat(defaults['boundary:circle:radius']), 'should be same as raw'); + t.end(); + + }); + + test('explicit boundary.circle.radius should be used instead of default', function(t) { + var raw = { + 'point.lat': '12.121212', + 'point.lon': '21.212121', + 'boundary.circle.radius': '3248732857km' // this will never be the default + }; + var clean = {}; + var errorsAndWarnings = sanitize(raw, clean); + + t.equals(raw['boundary.circle.radius'], '3248732857km', 'should be parsed float'); + t.equals(clean['boundary.circle.radius'], 3248732857.0, 'should be copied from raw'); + t.end(); + + }); + }; module.exports.all = function (tape, common) { diff --git a/test/unit/sanitiser/reverse.js b/test/unit/sanitiser/reverse.js index 759aef28..85943c73 100644 --- a/test/unit/sanitiser/reverse.js +++ b/test/unit/sanitiser/reverse.js @@ -4,11 +4,13 @@ var reverse = require('../../../sanitiser/reverse'), sanitize = reverse.sanitize, middleware = reverse.middleware, + defaults = require('../../../query/defaults'), defaultError = 'missing param \'lat\'', defaultClean = { 'point.lat': 0, 'point.lon': 0, 'boundary.circle.lat': 0, 'boundary.circle.lon': 0, + 'boundary.circle.radius': parseFloat(defaults['boundary:circle:radius']), types: { }, size: 10, From 5da4e4b36bdc8dbbb8870b2de5327da38c5ec4fa Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 14:52:54 -0400 Subject: [PATCH 12/39] removed unused variable --- sanitiser/_geo_reverse.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index bac20b29..a930c66b 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -3,8 +3,7 @@ var geo_common = require ('./_geo_common'); var check = require('check-types'); var defaults = require('../query/defaults'); var LAT_LON_IS_REQUIRED = true, - CIRCLE_IS_REQUIRED = false, - CIRCLE_MUST_BE_COMPLETE = false; + CIRCLE_IS_REQUIRED = false; // validate inputs, convert types and apply defaults module.exports = function sanitize( raw, clean ){ From 56d8600218546b5ab13d399d1a7b49cfbfe63aaf Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 14:54:10 -0400 Subject: [PATCH 13/39] modified warning message for brevity --- sanitiser/_geo_reverse.js | 2 +- test/unit/sanitiser/_geo_reverse.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index a930c66b..fb77add3 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -17,7 +17,7 @@ module.exports = function sanitize( raw, clean ){ }; if (['lat', 'lon', 'radius'].some(hasBoundaryCircleField)) { - messages.warnings.push('boundary.circle is currently unsupported and being ignored'); + messages.warnings.push('boundary.circle is currently unsupported'); } try { diff --git a/test/unit/sanitiser/_geo_reverse.js b/test/unit/sanitiser/_geo_reverse.js index c7fb778a..e03e7ddd 100644 --- a/test/unit/sanitiser/_geo_reverse.js +++ b/test/unit/sanitiser/_geo_reverse.js @@ -16,7 +16,7 @@ module.exports.tests.sanitize_boundary_country = function(test, common) { t.equals(clean['boundary.circle.lat'], 12.121212, 'should be set to point.lat'); t.deepEquals(errorsAndWarnings, { errors: [], - warnings: ['boundary.circle is currently unsupported and being ignored'] + warnings: ['boundary.circle is currently unsupported'] }, 'no warnings/errors'); t.end(); }); @@ -33,7 +33,7 @@ module.exports.tests.sanitize_boundary_country = function(test, common) { t.equals(clean['boundary.circle.lon'], 21.212121, 'should be set to point.lon'); t.deepEquals(errorsAndWarnings, { errors: [], - warnings: ['boundary.circle is currently unsupported and being ignored'] + warnings: ['boundary.circle is currently unsupported'] }, 'no warnings/errors'); t.end(); }); @@ -50,7 +50,7 @@ module.exports.tests.sanitize_boundary_country = function(test, common) { // t.equals(clean['boundary.circle.radius'], 12.121212, 'should be set to point.lat') t.deepEquals(errorsAndWarnings, { errors: [], - warnings: ['boundary.circle is currently unsupported and being ignored'] + warnings: ['boundary.circle is currently unsupported'] }, 'no warnings/errors'); t.end(); }); From dc623d5af7450332ef1ffc8510180b38a81c62de Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Thu, 17 Sep 2015 17:47:37 -0400 Subject: [PATCH 14/39] Send 3 part gid (source, layer, id) in GeoJSON responses --- helper/geojsonify.js | 11 +++++++++++ test/unit/helper/geojsonify.js | 3 +++ 2 files changed, 14 insertions(+) diff --git a/helper/geojsonify.js b/helper/geojsonify.js index 9ce77ad4..63b3cd88 100644 --- a/helper/geojsonify.js +++ b/helper/geojsonify.js @@ -177,6 +177,16 @@ function copyProperties( source, props, dst ) { }); } +/** + * Create a gid from a document + * @TODO modify all importers to create separate source and layer fields to remove mapping + * + * @param {object} src + */ +function makeGid(src) { + return lookupSource(src) + ':' + lookupLayer(src) + ':' + src._id; +} + /** * Determine and set place id, type, and source * @@ -185,6 +195,7 @@ function copyProperties( source, props, dst ) { */ function addMetaData(src, dst) { dst.id = src._id; + dst.gid = makeGid(src); dst.layer = lookupLayer(src); dst.source = lookupSource(src); } diff --git a/test/unit/helper/geojsonify.js b/test/unit/helper/geojsonify.js index 9ed4ed0a..a66421de 100644 --- a/test/unit/helper/geojsonify.js +++ b/test/unit/helper/geojsonify.js @@ -141,6 +141,7 @@ module.exports.tests.search = function(test, common) { }, 'properties': { 'id': 'id1', + 'gid': 'type1:type1:id1', 'layer': 'type1', 'source': 'type1', 'label': '\'Round Midnight Jazz and Blues Bar, test3, Angel', @@ -169,6 +170,7 @@ module.exports.tests.search = function(test, common) { }, 'properties': { 'id': 'id2', + 'gid': 'type2:type2:id2', 'layer': 'type2', 'source': 'type2', 'label': 'Blues Cafe, test3, Smithfield', @@ -194,6 +196,7 @@ module.exports.tests.search = function(test, common) { }, 'properties': { 'id': '34633854', + 'gid': 'osm:venue:34633854', 'layer': 'venue', 'source': 'osm', 'label': 'Empire State Building, Manhattan, NY', From 4df0f98b142a401caaf6c22784bb487bd8629b55 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Fri, 18 Sep 2015 16:24:00 -0400 Subject: [PATCH 15/39] Add type list, and raw mappings to/from source/layer and type --- helper/type_mapping.js | 78 ++++++++++++++++++++++++++++++++ test/unit/helper/type_mapping.js | 46 +++++++++++++++++++ test/unit/run.js | 1 + 3 files changed, 125 insertions(+) create mode 100644 helper/type_mapping.js create mode 100644 test/unit/helper/type_mapping.js diff --git a/helper/type_mapping.js b/helper/type_mapping.js new file mode 100644 index 00000000..0ac2a30d --- /dev/null +++ b/helper/type_mapping.js @@ -0,0 +1,78 @@ +var ALL_TYPES = [ + 'geoname', + 'osmnode', + 'osmway', + 'admin0', + 'admin1', + 'admin2', + 'neighborhood', + 'locality', + 'local_admin', + 'osmaddress', + 'openaddresses' +]; + +var TYPE_TO_SOURCE = { + 'geoname': 'gn', + 'osmnode': 'osm', + 'osmway': 'osm', + 'admin0': 'qs', + 'admin1': 'qs', + 'admin2': 'qs', + 'neighborhood': 'qs', + 'locality': 'qs', + 'local_admin': 'qs', + 'osmaddress': 'osm', + 'openaddresses': 'oa' +}; + +/* + * This doesn't include alias layers such as coarse + */ +var TYPE_TO_LAYER = { + 'geoname': 'venue', + 'osmnode': 'venue', + 'osmway': 'venue', + 'admin0': 'country', + 'admin1': 'region', + 'admin2': 'county', + 'neighborhood': 'neighbourhood', + 'locality': 'locality', + 'local_admin': 'localadmin', + 'osmaddress': 'address', + 'openaddresses': 'address' +}; + +var SOURCE_TO_TYPE = { + 'gn' : ['geoname'], + 'geonames' : ['geoname'], + 'oa' : ['openaddresses'], + 'openaddresses' : ['openaddresses'], + 'qs' : ['admin0', 'admin1', 'admin2', 'neighborhood', 'locality', 'local_admin'], + 'quattroshapes' : ['admin0', 'admin1', 'admin2', 'neighborhood', 'locality', 'local_admin'], + 'osm' : ['osmaddress', 'osmnode', 'osmway'], + 'openstreetmap' : ['osmaddress', 'osmnode', 'osmway'] +}; + +/** + * This includeds alias layers + */ +var LAYER_TO_TYPE = { + 'venue': ['geoname','osmnode','osmway'], + 'address': ['osmaddress','openaddresses'], + 'country': ['admin0'], + 'region': ['admin1'], + 'county': ['admin2'], + 'locality': ['locality'], + 'localadmin': ['local_admin'], + 'neighbourhood': ['neighborhood'], + 'coarse': ['admin0','admin1','admin2','neighborhood','locality','local_admin'], +}; + +module.exports = { + types_list: ALL_TYPES, + type_to_source: TYPE_TO_SOURCE, + type_to_layer: TYPE_TO_LAYER, + source_to_type: SOURCE_TO_TYPE, + layer_to_type: LAYER_TO_TYPE +}; diff --git a/test/unit/helper/type_mapping.js b/test/unit/helper/type_mapping.js new file mode 100644 index 00000000..1d1c945e --- /dev/null +++ b/test/unit/helper/type_mapping.js @@ -0,0 +1,46 @@ +var check = require('check-types'); +var type_mapping = require('../../../helper/type_mapping'); + +module.exports.tests = {}; + +module.exports.tests.interfaces = function(test, common) { + test('types_list', function(t) { + t.ok(check.array(type_mapping.types_list), 'is array'); + t.ok(check.hasLength(type_mapping.types_list, 11), 'has correct number of elements'); + t.end(); + }); + + test('type to source mapping', function(t) { + t.ok(check.object(type_mapping.type_to_source), 'is object'); + t.ok(check.hasLength(Object.keys(type_mapping.type_to_source), 11), 'has correct number of elements'); + t.end(); + }); + + test('type to layer mapping', function(t) { + t.ok(check.object(type_mapping.type_to_layer), 'is object'); + t.ok(check.hasLength(Object.keys(type_mapping.type_to_layer), 11), 'has correct number of elements'); + t.end(); + }); + + test('source to type mapping', function(t) { + t.ok(check.object(type_mapping.source_to_type), 'is object'); + t.ok(check.hasLength(Object.keys(type_mapping.source_to_type), 8), 'has correct number of elements'); + t.end(); + }); + + test('layer to type mapping', function(t) { + t.ok(check.object(type_mapping.layer_to_type), 'is object'); + t.ok(check.hasLength(Object.keys(type_mapping.layer_to_type), 9), 'has correct number of elements'); + t.end(); + }); +}; + +module.exports.all = function (tape, common) { + function test(name, testFunction) { + return tape('type_mapping: ' + name, testFunction); + } + + for( var testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; diff --git a/test/unit/run.js b/test/unit/run.js index 50c301de..6b24259f 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -26,6 +26,7 @@ var tests = [ require('./helper/geojsonify'), require('./helper/outputSchema'), require('./helper/types'), + require('./helper/type_mapping'), require('./sanitiser/_geo_common'), require('./middleware/distance'), require('./middleware/confidenceScoreReverse'), From 68c9661c7092018becea0c83da569b995d80453c Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Fri, 18 Sep 2015 16:40:05 -0400 Subject: [PATCH 16/39] Remove all mappings other than helper/type_mapping --- helper/geojsonify.js | 20 ++++---------------- helper/query_parser.js | 4 ++-- helper/types.js | 6 +++--- query/layers.js | 15 --------------- query/sources.js | 14 -------------- query/types.js | 16 ---------------- sanitiser/_ids.js | 3 ++- sanitiser/reverse.js | 5 +++-- sanitiser/search.js | 5 +++-- test/unit/helper/query_parser.js | 5 +++-- test/unit/query/types.js | 23 ----------------------- test/unit/run.js | 1 - test/unit/sanitiser/_ids.js | 4 ++-- test/unit/sanitiser/_layers.js | 3 ++- test/unit/sanitiser/_sources.js | 3 ++- 15 files changed, 26 insertions(+), 101 deletions(-) delete mode 100644 query/layers.js delete mode 100644 query/sources.js delete mode 100644 query/types.js delete mode 100644 test/unit/query/types.js diff --git a/helper/geojsonify.js b/helper/geojsonify.js index 63b3cd88..d1010962 100644 --- a/helper/geojsonify.js +++ b/helper/geojsonify.js @@ -2,7 +2,8 @@ var GeoJSON = require('geojson'), extent = require('geojson-extent'), outputGenerator = require('./outputGenerator'), - logger = require('pelias-logger').get('api'); + logger = require('pelias-logger').get('api'), + type_mapping = require('./type_mapping'); // Properties to be copied var DETAILS_PROPS = [ @@ -22,22 +23,9 @@ var DETAILS_PROPS = [ ]; -var SOURCES = { - 'geoname': 'gn', - 'osmnode': 'osm', - 'osmway': 'osm', - 'admin0': 'qs', - 'admin1': 'qs', - 'admin2': 'qs', - 'neighborhood': 'qs', - 'locality': 'qs', - 'local_admin': 'qs', - 'osmaddress': 'osm', - 'openaddresses': 'oa' -}; - function lookupSource(src) { - return SOURCES.hasOwnProperty(src._type) ? SOURCES[src._type] : src._type; + var sources = type_mapping.type_to_source; + return sources.hasOwnProperty(src._type) ? sources[src._type] : src._type; } function lookupLayer(src) { diff --git a/helper/query_parser.js b/helper/query_parser.js index b60ebcd8..cee589b0 100644 --- a/helper/query_parser.js +++ b/helper/query_parser.js @@ -1,7 +1,7 @@ var parser = require('addressit'); var extend = require('extend'); -var layers_map = require('../query/layers'); +var type_mapping = require('../helper/type_mapping'); var delim = ','; module.exports = {}; @@ -9,7 +9,7 @@ module.exports = {}; module.exports.get_layers = function get_layers(query) { if (query.length <= 3 ) { // no address parsing required - return layers_map.coarse; + return type_mapping.layer_to_type.coarse; } }; diff --git a/helper/types.js b/helper/types.js index 249c27a0..c057345f 100644 --- a/helper/types.js +++ b/helper/types.js @@ -1,4 +1,4 @@ -var valid_types = require( '../query/types' ); +var type_mapping = require( '../helper/type_mapping' ); /** * Calculate the set-style intersection of two arrays @@ -24,7 +24,7 @@ module.exports = function calculate_types(clean_types) { * perform a set intersection of their specified types */ if (clean_types.from_layers || clean_types.from_sources) { - var types = valid_types; + var types = type_mapping.types_list; if (clean_types.from_layers) { types = intersection(types, clean_types.from_layers); @@ -46,4 +46,4 @@ module.exports = function calculate_types(clean_types) { } throw new Error('no types specified'); -}; \ No newline at end of file +}; diff --git a/query/layers.js b/query/layers.js deleted file mode 100644 index 7f16d1fb..00000000 --- a/query/layers.js +++ /dev/null @@ -1,15 +0,0 @@ -/* - * Mapping from data layers to type values - */ - -module.exports = { - 'venue': ['geoname','osmnode','osmway'], - 'address': ['osmaddress','openaddresses'], - 'country': ['admin0'], - 'region': ['admin1'], - 'county': ['admin2'], - 'locality': ['locality'], - 'localadmin': ['local_admin'], - 'neighbourhood': ['neighborhood'], - 'coarse': ['admin0','admin1','admin2','neighborhood','locality','local_admin'], -}; diff --git a/query/sources.js b/query/sources.js deleted file mode 100644 index 3715be86..00000000 --- a/query/sources.js +++ /dev/null @@ -1,14 +0,0 @@ -/* - * Mapping from data sources to type values - */ - -module.exports = { - 'gn' : ['geoname'], - 'geonames' : ['geoname'], - 'oa' : ['openaddresses'], - 'openaddresses' : ['openaddresses'], - 'qs' : ['admin0', 'admin1', 'admin2', 'neighborhood', 'locality', 'local_admin'], - 'quattroshapes' : ['admin0', 'admin1', 'admin2', 'neighborhood', 'locality', 'local_admin'], - 'osm' : ['osmaddress', 'osmnode', 'osmway'], - 'openstreetmap' : ['osmaddress', 'osmnode', 'osmway'] -}; diff --git a/query/types.js b/query/types.js deleted file mode 100644 index 5f8cfab7..00000000 --- a/query/types.js +++ /dev/null @@ -1,16 +0,0 @@ - -// querable types - -module.exports = [ - 'geoname', - 'osmnode', - 'osmway', - 'admin0', - 'admin1', - 'admin2', - 'neighborhood', - 'locality', - 'local_admin', - 'osmaddress', - 'openaddresses' -]; diff --git a/sanitiser/_ids.js b/sanitiser/_ids.js index 0194da02..da509b0d 100644 --- a/sanitiser/_ids.js +++ b/sanitiser/_ids.js @@ -1,6 +1,7 @@ var _ = require('lodash'), check = require('check-types'), - types = require('../query/types'); + type_mapping = require('../helper/type_mapping'), + types = type_mapping.types_list; var ID_DELIM = ':'; diff --git a/sanitiser/reverse.js b/sanitiser/reverse.js index dd9eeb1d..11937bbc 100644 --- a/sanitiser/reverse.js +++ b/sanitiser/reverse.js @@ -1,9 +1,10 @@ +var type_mapping = require('../helper/type_mapping'); var sanitizeAll = require('../sanitiser/sanitizeAll'), sanitizers = { singleScalarParameters: require('../sanitiser/_single_scalar_parameters'), - layers: require('../sanitiser/_targets')('layers', require('../query/layers')), - sources: require('../sanitiser/_targets')('sources', require('../query/sources')), + layers: require('../sanitiser/_targets')('layers', type_mapping.layer_to_type), + sources: require('../sanitiser/_targets')('sources', type_mapping.source_to_type), size: require('../sanitiser/_size'), private: require('../sanitiser/_flag_bool')('private', false), geo_reverse: require('../sanitiser/_geo_reverse'), diff --git a/sanitiser/search.js b/sanitiser/search.js index e1b79528..dd46f419 100644 --- a/sanitiser/search.js +++ b/sanitiser/search.js @@ -1,11 +1,12 @@ +var type_mapping = require('../helper/type_mapping'); var sanitizeAll = require('../sanitiser/sanitizeAll'), sanitizers = { singleScalarParameters: require('../sanitiser/_single_scalar_parameters'), text: require('../sanitiser/_text'), size: require('../sanitiser/_size'), - layers: require('../sanitiser/_targets')('layers', require( '../query/layers' )), - sources: require('../sanitiser/_targets')('sources', require( '../query/sources' )), + layers: require('../sanitiser/_targets')('layers', type_mapping.layer_to_type), + sources: require('../sanitiser/_targets')('sources', type_mapping.source_to_type), private: require('../sanitiser/_flag_bool')('private', false), geo_search: require('../sanitiser/_geo_search'), boundary_country: require('../sanitiser/_boundary_country'), diff --git a/test/unit/helper/query_parser.js b/test/unit/helper/query_parser.js index ad669ece..c1e3fd52 100644 --- a/test/unit/helper/query_parser.js +++ b/test/unit/helper/query_parser.js @@ -1,6 +1,7 @@ - var parser = require('../../../helper/query_parser'); -var layers_map = require('../../../query/layers'); + +var type_mapping = require('../../../helper/type_mapping'); +var layers_map = type_mapping.layer_to_type; module.exports.tests = {}; diff --git a/test/unit/query/types.js b/test/unit/query/types.js deleted file mode 100644 index eee830a6..00000000 --- a/test/unit/query/types.js +++ /dev/null @@ -1,23 +0,0 @@ - -var types = require('../../../query/types'); - -module.exports.tests = {}; - -module.exports.tests.interface = function(test, common) { - test('valid interface', function(t) { - t.true(Array.isArray(types), 'valid array'); - t.equal(types.length, 11, 'valid array'); - t.end(); - }); -}; - -module.exports.all = function (tape, common) { - - function test(name, testFunction) { - return tape('types ' + name, testFunction); - } - - for( var testCase in module.exports.tests ){ - module.exports.tests[testCase](test, common); - } -}; diff --git a/test/unit/run.js b/test/unit/run.js index 6b24259f..a95764e5 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -17,7 +17,6 @@ var tests = [ require('./sanitiser/_layers'), require('./sanitiser/reverse'), require('./sanitiser/place'), - require('./query/types'), require('./query/search'), require('./query/autocomplete'), require('./query/reverse'), diff --git a/test/unit/sanitiser/_ids.js b/test/unit/sanitiser/_ids.js index 02e25f18..26445018 100644 --- a/test/unit/sanitiser/_ids.js +++ b/test/unit/sanitiser/_ids.js @@ -1,7 +1,7 @@ var sanitize = require('../../../sanitiser/_ids'); var delimiter = ':'; -var types = require('../../../query/types'); +var type_mapping = require('../../../helper/type_mapping'); var inputs = { valid: [ 'geoname:1', 'osmnode:2', 'admin0:53', 'osmway:44', 'geoname:5' ], invalid: [ ':', '', '::', 'geoname:', ':234', 'gibberish:23' ] @@ -13,7 +13,7 @@ var formatError = function(input) { var lengthError = 'invalid param \'ids\': length must be >0'; var defaultMissingTypeError = function(input) { var type = input.split(delimiter)[0]; - return type + ' is invalid. It must be one of these values - [' + types.join(', ') + ']'; + return type + ' is invalid. It must be one of these values - [' + type_mapping.types_list.join(', ') + ']'; }; module.exports.tests = {}; diff --git a/test/unit/sanitiser/_layers.js b/test/unit/sanitiser/_layers.js index 12d2cfd3..50117c08 100644 --- a/test/unit/sanitiser/_layers.js +++ b/test/unit/sanitiser/_layers.js @@ -1,5 +1,6 @@ +var type_mapping = require('../../../helper/type_mapping'); -var sanitize = require('../../../sanitiser/_targets')('layers', require('../../../query/layers')); +var sanitize = require('../../../sanitiser/_targets')('layers', type_mapping.layer_to_type); module.exports.tests = {}; diff --git a/test/unit/sanitiser/_sources.js b/test/unit/sanitiser/_sources.js index 05507a36..d2ef405f 100644 --- a/test/unit/sanitiser/_sources.js +++ b/test/unit/sanitiser/_sources.js @@ -1,4 +1,5 @@ -var sanitize = require( '../../../sanitiser/_targets' )('sources', require('../../../query/sources')); +var type_mapping = require('../../../helper/type_mapping'); +var sanitize = require( '../../../sanitiser/_targets' )('sources', type_mapping.source_to_type); var success_messages = { error: false }; From 8b1037d7c88e5ec51b5157a85a8cc5b48996ed04 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Mon, 21 Sep 2015 14:59:39 -0400 Subject: [PATCH 17/39] Separate real layers and alias layers into separate objects --- helper/query_parser.js | 2 +- helper/type_mapping.js | 16 ++++++++++++---- sanitiser/reverse.js | 2 +- sanitiser/search.js | 2 +- test/unit/helper/query_parser.js | 2 +- test/unit/helper/type_mapping.js | 8 +++++++- test/unit/sanitiser/_layers.js | 2 +- 7 files changed, 24 insertions(+), 10 deletions(-) diff --git a/helper/query_parser.js b/helper/query_parser.js index cee589b0..4af009aa 100644 --- a/helper/query_parser.js +++ b/helper/query_parser.js @@ -9,7 +9,7 @@ module.exports = {}; module.exports.get_layers = function get_layers(query) { if (query.length <= 3 ) { // no address parsing required - return type_mapping.layer_to_type.coarse; + return type_mapping.layer_with_aliases_to_type.coarse; } }; diff --git a/helper/type_mapping.js b/helper/type_mapping.js index 0ac2a30d..d2b0f758 100644 --- a/helper/type_mapping.js +++ b/helper/type_mapping.js @@ -1,3 +1,5 @@ +var extend = require('extend'); + var ALL_TYPES = [ 'geoname', 'osmnode', @@ -55,7 +57,7 @@ var SOURCE_TO_TYPE = { }; /** - * This includeds alias layers + * This does not included alias layers, those are built separately */ var LAYER_TO_TYPE = { 'venue': ['geoname','osmnode','osmway'], @@ -65,14 +67,20 @@ var LAYER_TO_TYPE = { 'county': ['admin2'], 'locality': ['locality'], 'localadmin': ['local_admin'], - 'neighbourhood': ['neighborhood'], - 'coarse': ['admin0','admin1','admin2','neighborhood','locality','local_admin'], + 'neighbourhood': ['neighborhood'] +}; + +var LAYER_ALIASES = { + 'coarse': ['admin0','admin1','admin2','neighborhood','locality','local_admin'] }; +var LAYER_WITH_ALIASES_TO_TYPE = extend({}, LAYER_ALIASES, LAYER_TO_TYPE); + module.exports = { types_list: ALL_TYPES, type_to_source: TYPE_TO_SOURCE, type_to_layer: TYPE_TO_LAYER, source_to_type: SOURCE_TO_TYPE, - layer_to_type: LAYER_TO_TYPE + layer_to_type: LAYER_TO_TYPE, + layer_with_aliases_to_type: LAYER_WITH_ALIASES_TO_TYPE }; diff --git a/sanitiser/reverse.js b/sanitiser/reverse.js index 11937bbc..154fd3d9 100644 --- a/sanitiser/reverse.js +++ b/sanitiser/reverse.js @@ -3,7 +3,7 @@ var type_mapping = require('../helper/type_mapping'); var sanitizeAll = require('../sanitiser/sanitizeAll'), sanitizers = { singleScalarParameters: require('../sanitiser/_single_scalar_parameters'), - layers: require('../sanitiser/_targets')('layers', type_mapping.layer_to_type), + layers: require('../sanitiser/_targets')('layers', type_mapping.layer_with_aliases_to_type), sources: require('../sanitiser/_targets')('sources', type_mapping.source_to_type), size: require('../sanitiser/_size'), private: require('../sanitiser/_flag_bool')('private', false), diff --git a/sanitiser/search.js b/sanitiser/search.js index dd46f419..cc596b26 100644 --- a/sanitiser/search.js +++ b/sanitiser/search.js @@ -5,7 +5,7 @@ var sanitizeAll = require('../sanitiser/sanitizeAll'), singleScalarParameters: require('../sanitiser/_single_scalar_parameters'), text: require('../sanitiser/_text'), size: require('../sanitiser/_size'), - layers: require('../sanitiser/_targets')('layers', type_mapping.layer_to_type), + layers: require('../sanitiser/_targets')('layers', type_mapping.layer_with_aliases_to_type), sources: require('../sanitiser/_targets')('sources', type_mapping.source_to_type), private: require('../sanitiser/_flag_bool')('private', false), geo_search: require('../sanitiser/_geo_search'), diff --git a/test/unit/helper/query_parser.js b/test/unit/helper/query_parser.js index c1e3fd52..02c96531 100644 --- a/test/unit/helper/query_parser.js +++ b/test/unit/helper/query_parser.js @@ -1,7 +1,7 @@ var parser = require('../../../helper/query_parser'); var type_mapping = require('../../../helper/type_mapping'); -var layers_map = type_mapping.layer_to_type; +var layers_map = type_mapping.layer_with_aliases_to_type; module.exports.tests = {}; diff --git a/test/unit/helper/type_mapping.js b/test/unit/helper/type_mapping.js index 1d1c945e..fa50c191 100644 --- a/test/unit/helper/type_mapping.js +++ b/test/unit/helper/type_mapping.js @@ -30,7 +30,13 @@ module.exports.tests.interfaces = function(test, common) { test('layer to type mapping', function(t) { t.ok(check.object(type_mapping.layer_to_type), 'is object'); - t.ok(check.hasLength(Object.keys(type_mapping.layer_to_type), 9), 'has correct number of elements'); + t.equal(Object.keys(type_mapping.layer_to_type).length, 8, 'has correct number of elements'); + t.end(); + }); + + test('layer to type mapping (with aliases)', function(t) { + t.ok(check.object(type_mapping.layer_with_aliases_to_type), 'is object'); + t.ok(check.hasLength(Object.keys(type_mapping.layer_with_aliases_to_type), 9), 'has correct number of elements'); t.end(); }); }; diff --git a/test/unit/sanitiser/_layers.js b/test/unit/sanitiser/_layers.js index 50117c08..519d95d9 100644 --- a/test/unit/sanitiser/_layers.js +++ b/test/unit/sanitiser/_layers.js @@ -1,6 +1,6 @@ var type_mapping = require('../../../helper/type_mapping'); -var sanitize = require('../../../sanitiser/_targets')('layers', type_mapping.layer_to_type); +var sanitize = require('../../../sanitiser/_targets')('layers', type_mapping.layer_with_aliases_to_type); module.exports.tests = {}; From 882aad4916592749075924efc47277219aa97e29 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Mon, 21 Sep 2015 17:32:00 -0400 Subject: [PATCH 18/39] Extract code to sanitize single id to separate function --- sanitiser/_ids.js | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/sanitiser/_ids.js b/sanitiser/_ids.js index da509b0d..b45f0d1c 100644 --- a/sanitiser/_ids.js +++ b/sanitiser/_ids.js @@ -15,6 +15,27 @@ var formatError = function(input) { return 'id `' + input + 'is invalid: must be of the format type:id for ex: \'geoname:4163334\''; }; +function sanitizeId(rawId, messages) { + var param_index = rawId.indexOf(ID_DELIM); + var type = rawId.substring(0, param_index ); + var id = rawId.substring(param_index + 1); + + // check id format + if(!check.contains(rawId, ID_DELIM) || !check.unemptyString( id ) || !check.unemptyString( type )) { + messages.errors.push( formatError(rawId) ); + } + // type text must be one of the types + else if( !_.contains( types, type ) ){ + messages.errors.push( type + ' is invalid. It must be one of these values - [' + types.join(', ') + ']' ); + } + else { + return { + id: id, + type: type + }; + } +} + function sanitize( raw, clean ){ // error & warning messages var messages = { errors: [], warnings: [] }; @@ -43,25 +64,8 @@ function sanitize( raw, clean ){ } // cycle through raw ids and set those which are valid - var validIds = rawIds.map( function( rawId ){ - var param_index = rawId.indexOf(ID_DELIM); - var type = rawId.substring(0, param_index ); - var id = rawId.substring(param_index + 1); - - // check id format - if(!check.contains(rawId, ID_DELIM) || !check.unemptyString( id ) || !check.unemptyString( type )) { - messages.errors.push( formatError(rawId) ); - } - // type text must be one of the types - else if( !_.contains( types, type ) ){ - messages.errors.push( type + ' is invalid. It must be one of these values - [' + types.join(', ') + ']' ); - } - else { - return { - id: id, - type: type - }; - } + var validIds = rawIds.map(function(rawId) { + return sanitizeId(rawId, messages); }); if (validIds.every(check.object)) { From fa44effdac7621d3e9f78910cee5bb1ae57be01f Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Mon, 21 Sep 2015 17:57:32 -0400 Subject: [PATCH 19/39] Add geoname type to most layers The Geonames dataset includes lots of different kinds of places, so add them to the mapping. --- helper/type_mapping.js | 12 ++++++------ test/unit/sanitiser/_layers.js | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/helper/type_mapping.js b/helper/type_mapping.js index d2b0f758..2f058543 100644 --- a/helper/type_mapping.js +++ b/helper/type_mapping.js @@ -61,13 +61,13 @@ var SOURCE_TO_TYPE = { */ var LAYER_TO_TYPE = { 'venue': ['geoname','osmnode','osmway'], - 'address': ['osmaddress','openaddresses'], - 'country': ['admin0'], - 'region': ['admin1'], - 'county': ['admin2'], - 'locality': ['locality'], + 'address': ['osmaddress','openaddresses', 'geoname'], + 'country': ['admin0', 'geoname'], + 'region': ['admin1', 'geoname'], + 'county': ['admin2', 'geoname'], + 'locality': ['locality', 'geoname'], 'localadmin': ['local_admin'], - 'neighbourhood': ['neighborhood'] + 'neighbourhood': ['neighborhood', 'geoname'] }; var LAYER_ALIASES = { diff --git a/test/unit/sanitiser/_layers.js b/test/unit/sanitiser/_layers.js index 519d95d9..3264f434 100644 --- a/test/unit/sanitiser/_layers.js +++ b/test/unit/sanitiser/_layers.js @@ -43,7 +43,7 @@ module.exports.tests.sanitize_layers = function(test, common) { t.end(); }); test('address (alias) layer', function(t) { - var address_layers = ['osmaddress','openaddresses']; + var address_layers = ['osmaddress','openaddresses','geoname']; var raw = { layers: 'address' }; var clean = {}; @@ -76,7 +76,7 @@ module.exports.tests.sanitize_layers = function(test, common) { t.end(); }); test('address alias layer plus regular layers', function(t) { - var address_layers = ['osmaddress','openaddresses']; + var address_layers = ['osmaddress','openaddresses','geoname']; var reg_layers = ['admin0', 'locality']; var raw = { layers: 'address,country,locality' }; From b2f3b54f662c442dadd7e0dded656599301b3d7e Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Mon, 21 Sep 2015 18:10:08 -0400 Subject: [PATCH 20/39] Leave only geonames specific mapping in gejsonify helper --- helper/geojsonify.js | 50 ++++++++++++++++++-------------------------- 1 file changed, 20 insertions(+), 30 deletions(-) diff --git a/helper/geojsonify.js b/helper/geojsonify.js index d1010962..9822ad02 100644 --- a/helper/geojsonify.js +++ b/helper/geojsonify.js @@ -3,7 +3,8 @@ var GeoJSON = require('geojson'), extent = require('geojson-extent'), outputGenerator = require('./outputGenerator'), logger = require('pelias-logger').get('api'), - type_mapping = require('./type_mapping'); + type_mapping = require('./type_mapping'), + _ = require('lodash'); // Properties to be copied var DETAILS_PROPS = [ @@ -28,36 +29,25 @@ function lookupSource(src) { return sources.hasOwnProperty(src._type) ? sources[src._type] : src._type; } +/* + * Use the type to layer mapping, except for Geonames, where having a full + * Elasticsearch document source allows a more specific layer name to be chosen + */ function lookupLayer(src) { - switch(src._type) { - case 'osmnode': - case 'osmway': - return 'venue'; - case 'admin0': - return 'country'; - case 'admin1': - return 'region'; - case 'admin2': - return 'county'; - case 'neighborhood': - return 'neighbourhood'; - case 'locality': - return 'locality'; - case 'local_admin': - return 'localadmin'; - case 'osmaddress': - case 'openaddresses': - return 'address'; - case 'geoname': - if (src.category && src.category.indexOf('admin') !== -1) { - if (src.category.indexOf('admin:city') !== -1) { return 'locality'; } - if (src.category.indexOf('admin:admin1') !== -1) { return 'region'; } - if (src.category.indexOf('admin:admin2') !== -1) { return 'county'; } - return 'neighbourhood'; // this could also be 'local_admin' - } - - if (src.name) { return 'venue'; } - if (src.address) { return 'address'; } + if (src._type === 'geoname') { + if (src.category && src.category.indexOf('admin') !== -1) { + if (src.category.indexOf('admin:city') !== -1) { return 'locality'; } + if (src.category.indexOf('admin:admin1') !== -1) { return 'region'; } + if (src.category.indexOf('admin:admin2') !== -1) { return 'county'; } + return 'neighbourhood'; // this could also be 'local_admin' + } + + if (src.name) { return 'venue'; } + if (src.address) { return 'address'; } + } + + if (_.contains(type_mapping.types_list, src._type)) { + return type_mapping.type_to_layer[src._type]; } logger.warn('[geojsonify]: could not map _type ', src._type); From aabca1569cc7c00a4ef00aa646532e26d63d92f1 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Mon, 21 Sep 2015 18:16:46 -0400 Subject: [PATCH 21/39] Fix old reference to /doc endpoint --- test/unit/controller/place.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/controller/place.js b/test/unit/controller/place.js index 1fde7942..755ac1b9 100644 --- a/test/unit/controller/place.js +++ b/test/unit/controller/place.js @@ -85,7 +85,7 @@ module.exports.tests.functional_failure = function(test, common) { module.exports.all = function (tape, common) { function test(name, testFunction) { - return tape('GET /doc ' + name, testFunction); + return tape('GET /place ' + name, testFunction); } for( var testCase in module.exports.tests ){ From 2505e92a6211fd5ceffcb75a9c4b06c52a562c39 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Mon, 21 Sep 2015 18:22:24 -0400 Subject: [PATCH 22/39] Expect an array of types from _ids sanitiser This doesn't have any effect by itself but allows for the 3-part gid sanitiser to possibly return multiple types (i.e. in the case of osm:venue:1000) --- controller/place.js | 2 +- sanitiser/_ids.js | 2 +- test/unit/controller/place.js | 8 ++++---- test/unit/mock/backend.js | 2 +- test/unit/sanitiser/_ids.js | 8 ++++---- test/unit/sanitiser/place.js | 2 +- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/controller/place.js b/controller/place.js index 8622f234..42e7c626 100644 --- a/controller/place.js +++ b/controller/place.js @@ -16,7 +16,7 @@ function setup( backend ){ var query = req.clean.ids.map( function(id) { return { _index: 'pelias', - _type: id.type, + type: id.types, _id: id.id }; }); diff --git a/sanitiser/_ids.js b/sanitiser/_ids.js index b45f0d1c..46effa41 100644 --- a/sanitiser/_ids.js +++ b/sanitiser/_ids.js @@ -31,7 +31,7 @@ function sanitizeId(rawId, messages) { else { return { id: id, - type: type + types: [type] }; } } diff --git a/test/unit/controller/place.js b/test/unit/controller/place.js index 755ac1b9..20455e3a 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 = { @@ -57,7 +57,7 @@ module.exports.tests.functional_success = function(test, common) { t.deepEqual(json.features, expected, 'values correctly mapped'); } }; - var req = { clean: { ids: [ {'id' : 123, 'type': 'a' } ] }, errors: [], warnings: [] }; + var req = { clean: { ids: [ {'id' : 123, types: [ 'a' ] } ] }, errors: [], warnings: [] }; var next = function next() { t.equal(req.errors.length, 0, 'next was called without error'); t.end(); @@ -70,10 +70,10 @@ 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, 'type': 'b' } ] }, errors: [], warnings: [] }; + var req = { clean: { ids: [ {'id' : 123, types: [ 'b' ] } ] }, errors: [], warnings: [] }; var next = function( message ){ t.equal(req.errors[0],'a backend error occurred','error passed to errorHandler'); t.end(); diff --git a/test/unit/mock/backend.js b/test/unit/mock/backend.js index 201ab7b5..7d0eb8d7 100644 --- a/test/unit/mock/backend.js +++ b/test/unit/mock/backend.js @@ -94,4 +94,4 @@ function searchEnvelope( options ){ return { hits: { total: options.length, hits: options } }; } -module.exports = setup; \ No newline at end of file +module.exports = setup; diff --git a/test/unit/sanitiser/_ids.js b/test/unit/sanitiser/_ids.js index 26445018..3be9183b 100644 --- a/test/unit/sanitiser/_ids.js +++ b/test/unit/sanitiser/_ids.js @@ -90,7 +90,7 @@ module.exports.tests.valid_ids = function(test, common) { test('ids: valid input', function(t) { inputs.valid.forEach( function( input ){ var input_parts = input.split(delimiter); - var expected_clean = { ids: [ { id: input_parts[1], type: input_parts[0] } ]}; + var expected_clean = { ids: [ { id: input_parts[1], types: [ input_parts[0] ]} ]}; var raw = { ids: input }; var clean = {}; @@ -111,7 +111,7 @@ module.exports.tests.valid_ids = function(test, common) { // construct the expected id and type for each valid input inputs.valid.forEach( function( input ){ var input_parts = input.split(delimiter); - expected_clean.ids.push({ id: input_parts[1], type: input_parts[0] }); + expected_clean.ids.push({ id: input_parts[1], types: [ input_parts[0] ]}); }); var messages = sanitize( raw, clean ); @@ -138,7 +138,7 @@ module.exports.tests.array_of_ids = function(test, common) { module.exports.tests.multiple_ids = function(test, common) { test('duplicate ids', function(t) { - var expected_clean = { ids: [ { id: '1', type: 'geoname' }, { id: '2', type: 'osmnode' } ] }; + var expected_clean = { ids: [ { id: '1', types: [ 'geoname' ] }, { id: '2', types: [ 'osmnode' ] } ] }; var raw = { ids: 'geoname:1,osmnode:2' }; var clean = {}; @@ -153,7 +153,7 @@ module.exports.tests.multiple_ids = function(test, common) { module.exports.tests.de_dupe = function(test, common) { test('duplicate ids', function(t) { - var expected_clean = { ids: [ { id: '1', type: 'geoname' }, { id: '2', type: 'osmnode' } ]}; + var expected_clean = { ids: [ { id: '1', types: [ 'geoname' ] }, { id: '2', types: [ 'osmnode' ] } ] }; var raw = { ids: 'geoname:1,osmnode:2,geoname:1' }; var clean = {}; diff --git a/test/unit/sanitiser/place.js b/test/unit/sanitiser/place.js index c3cd81cf..26042c5e 100644 --- a/test/unit/sanitiser/place.js +++ b/test/unit/sanitiser/place.js @@ -1,7 +1,7 @@ var place = require('../../../sanitiser/place'), sanitize = place.sanitize, middleware = place.middleware, - defaultClean = { ids: [ { id: '123', type: 'geoname' } ], private: false }; + defaultClean = { ids: [ { id: '123', types: [ 'geoname' ] } ], private: false }; // these are the default values you would expect when no input params are specified. module.exports.tests = {}; From da1314eeff13bd86c37c8f4679391d2c6e02f70e Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Mon, 21 Sep 2015 17:34:20 -0400 Subject: [PATCH 23/39] Add mapping function to get type from source and layer --- helper/type_mapping.js | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/helper/type_mapping.js b/helper/type_mapping.js index 2f058543..a00979b7 100644 --- a/helper/type_mapping.js +++ b/helper/type_mapping.js @@ -76,11 +76,25 @@ var LAYER_ALIASES = { var LAYER_WITH_ALIASES_TO_TYPE = extend({}, LAYER_ALIASES, LAYER_TO_TYPE); +/** + * Calculate the set-style intersection of two arrays + */ +var intersection = function intersection(set1, set2) { + return set2.filter(function(value) { + return set1.indexOf(value) !== -1; + }); +}; + +var sourceAndLayerToType = function sourceAndLayerToType(source, layer) { + return intersection(SOURCE_TO_TYPE[source], LAYER_WITH_ALIASES_TO_TYPE[layer]); +}; + module.exports = { types_list: ALL_TYPES, type_to_source: TYPE_TO_SOURCE, type_to_layer: TYPE_TO_LAYER, source_to_type: SOURCE_TO_TYPE, layer_to_type: LAYER_TO_TYPE, - layer_with_aliases_to_type: LAYER_WITH_ALIASES_TO_TYPE + layer_with_aliases_to_type: LAYER_WITH_ALIASES_TO_TYPE, + source_and_layer_to_type: sourceAndLayerToType }; From d11d1854292f5ef75742c6a8f6085b2d829dba73 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Mon, 21 Sep 2015 18:30:24 -0400 Subject: [PATCH 24/39] Fix missing space in error message --- sanitiser/_ids.js | 2 +- test/unit/sanitiser/_ids.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sanitiser/_ids.js b/sanitiser/_ids.js index 46effa41..d7648960 100644 --- a/sanitiser/_ids.js +++ b/sanitiser/_ids.js @@ -12,7 +12,7 @@ var ID_DELIM = ':'; var lengthError = 'invalid param \'ids\': length must be >0'; var formatError = function(input) { - return 'id `' + input + 'is invalid: must be of the format type:id for ex: \'geoname:4163334\''; + return 'id `' + input + ' is invalid: must be of the format type:id for ex: \'geoname:4163334\''; }; function sanitizeId(rawId, messages) { diff --git a/test/unit/sanitiser/_ids.js b/test/unit/sanitiser/_ids.js index 3be9183b..d7c65d33 100644 --- a/test/unit/sanitiser/_ids.js +++ b/test/unit/sanitiser/_ids.js @@ -8,7 +8,7 @@ var inputs = { }; var formatError = function(input) { - return 'id `' + input + 'is invalid: must be of the format type:id for ex: \'geoname:4163334\''; + return 'id `' + input + ' is invalid: must be of the format type:id for ex: \'geoname:4163334\''; }; var lengthError = 'invalid param \'ids\': length must be >0'; var defaultMissingTypeError = function(input) { From d4fff265748ba1f6945f02c908ef45d1a11fb8d7 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Mon, 21 Sep 2015 18:56:52 -0400 Subject: [PATCH 25/39] Derive type, source, and layer list instead of hardcoding --- helper/geojsonify.js | 2 +- helper/type_mapping.js | 25 ++++++++++--------------- helper/types.js | 2 +- test/unit/helper/type_mapping.js | 6 +++--- 4 files changed, 15 insertions(+), 20 deletions(-) diff --git a/helper/geojsonify.js b/helper/geojsonify.js index 9822ad02..94b41b84 100644 --- a/helper/geojsonify.js +++ b/helper/geojsonify.js @@ -46,7 +46,7 @@ function lookupLayer(src) { if (src.address) { return 'address'; } } - if (_.contains(type_mapping.types_list, src._type)) { + if (_.contains(type_mapping.types, src._type)) { return type_mapping.type_to_layer[src._type]; } diff --git a/helper/type_mapping.js b/helper/type_mapping.js index a00979b7..ab7bbe56 100644 --- a/helper/type_mapping.js +++ b/helper/type_mapping.js @@ -1,19 +1,5 @@ var extend = require('extend'); -var ALL_TYPES = [ - 'geoname', - 'osmnode', - 'osmway', - 'admin0', - 'admin1', - 'admin2', - 'neighborhood', - 'locality', - 'local_admin', - 'osmaddress', - 'openaddresses' -]; - var TYPE_TO_SOURCE = { 'geoname': 'gn', 'osmnode': 'osm', @@ -76,6 +62,13 @@ var LAYER_ALIASES = { var LAYER_WITH_ALIASES_TO_TYPE = extend({}, LAYER_ALIASES, LAYER_TO_TYPE); +/* + * derive the list of types, sources, and layers from above mappings + */ +var TYPES = Object.keys(TYPE_TO_SOURCE); +var SOURCES = Object.keys(SOURCE_TO_TYPE); +var LAYERS = Object.keys(LAYER_TO_TYPE); + /** * Calculate the set-style intersection of two arrays */ @@ -90,7 +83,9 @@ var sourceAndLayerToType = function sourceAndLayerToType(source, layer) { }; module.exports = { - types_list: ALL_TYPES, + types: TYPES, + sources: SOURCES, + layers: LAYERS, type_to_source: TYPE_TO_SOURCE, type_to_layer: TYPE_TO_LAYER, source_to_type: SOURCE_TO_TYPE, diff --git a/helper/types.js b/helper/types.js index c057345f..aa3337e8 100644 --- a/helper/types.js +++ b/helper/types.js @@ -24,7 +24,7 @@ module.exports = function calculate_types(clean_types) { * perform a set intersection of their specified types */ if (clean_types.from_layers || clean_types.from_sources) { - var types = type_mapping.types_list; + var types = type_mapping.types; if (clean_types.from_layers) { types = intersection(types, clean_types.from_layers); diff --git a/test/unit/helper/type_mapping.js b/test/unit/helper/type_mapping.js index fa50c191..144fadfb 100644 --- a/test/unit/helper/type_mapping.js +++ b/test/unit/helper/type_mapping.js @@ -4,9 +4,9 @@ var type_mapping = require('../../../helper/type_mapping'); module.exports.tests = {}; module.exports.tests.interfaces = function(test, common) { - test('types_list', function(t) { - t.ok(check.array(type_mapping.types_list), 'is array'); - t.ok(check.hasLength(type_mapping.types_list, 11), 'has correct number of elements'); + test('types list', function(t) { + t.ok(check.array(type_mapping.types), 'is array'); + t.ok(check.hasLength(type_mapping.types, 11), 'has correct number of elements'); t.end(); }); From 2811569bcd10957c8c239e04d168190a2b22152e Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Mon, 21 Sep 2015 18:59:11 -0400 Subject: [PATCH 26/39] Switch to source:layer:id ids format in /place --- sanitiser/_ids.js | 55 ++++++++----- test/ciao/place/basic_place.coffee | 4 +- test/ciao/reverse/layers_invalid.coffee | 4 +- .../reverse/layers_mix_invalid_valid.coffee | 4 +- test/ciao/reverse/layers_multiple.coffee | 4 +- test/ciao/reverse/layers_single.coffee | 4 +- .../sources_layers_invalid_combo.coffee | 4 +- .../reverse/sources_layers_valid_combo.coffee | 4 +- test/ciao/search/layers_alias_address.coffee | 4 +- test/ciao/search/layers_invalid.coffee | 4 +- .../search/layers_mix_invalid_valid.coffee | 4 +- test/ciao/search/layers_multiple.coffee | 4 +- test/ciao/search/layers_single.coffee | 4 +- .../sources_layers_invalid_combo.coffee | 4 +- .../search/sources_layers_valid_combo.coffee | 4 +- test/unit/sanitiser/_ids.js | 78 ++++++++++--------- test/unit/sanitiser/place.js | 10 +-- 17 files changed, 110 insertions(+), 89 deletions(-) diff --git a/sanitiser/_ids.js b/sanitiser/_ids.js index d7648960..0b4d91c7 100644 --- a/sanitiser/_ids.js +++ b/sanitiser/_ids.js @@ -1,39 +1,56 @@ var _ = require('lodash'), check = require('check-types'), - type_mapping = require('../helper/type_mapping'), - types = type_mapping.types_list; + type_mapping = require('../helper/type_mapping'); var ID_DELIM = ':'; -// validate inputs, convert types and apply defaults -// id generally looks like 'geoname:4163334' (type:id) -// so, both type and id are required fields. +// validate inputs, convert types and apply defaults id generally looks like +// 'geonames:venue:4163334' (source:layer:id) so, all three are required var lengthError = 'invalid param \'ids\': length must be >0'; var formatError = function(input) { - return 'id `' + input + ' is invalid: must be of the format type:id for ex: \'geoname:4163334\''; + return 'id `' + input + ' is invalid: must be of the format source:layer:id for ex: \'geonames:venue:4163334\''; +}; + +var targetError = function(target, target_list) { + return target + ' is invalid. It must be one of these values - [' + target_list.join(', ') + ']'; }; function sanitizeId(rawId, messages) { - var param_index = rawId.indexOf(ID_DELIM); - var type = rawId.substring(0, param_index ); - var id = rawId.substring(param_index + 1); + var parts = rawId.split(ID_DELIM); + + if ( parts.length < 3 ) { + messages.errors.push( formatError(rawId) ); + return; + } - // check id format - if(!check.contains(rawId, ID_DELIM) || !check.unemptyString( id ) || !check.unemptyString( type )) { + var source = parts[0]; + var layer = parts[1]; + var id = parts.slice(2).join(ID_DELIM); + + // check if any parts of the gid are empty + if (_.contains([source, layer, id], '')) { messages.errors.push( formatError(rawId) ); + return; } - // type text must be one of the types - else if( !_.contains( types, type ) ){ - messages.errors.push( type + ' is invalid. It must be one of these values - [' + types.join(', ') + ']' ); + + if (!_.contains(type_mapping.sources, source)) { + messages.errors.push( targetError(source, type_mapping.sources) ); + return; } - else { - return { - id: id, - types: [type] - }; + + if (!_.contains(type_mapping.layers, layer)) { + messages.errors.push( targetError(layer, type_mapping.layers) ); + return; } + + var types = type_mapping.source_and_layer_to_type(source, layer); + + return { + id: id, + types: types + }; } function sanitize( raw, clean ){ diff --git a/test/ciao/place/basic_place.coffee b/test/ciao/place/basic_place.coffee index 39cfda18..b4a18711 100644 --- a/test/ciao/place/basic_place.coffee +++ b/test/ciao/place/basic_place.coffee @@ -1,6 +1,6 @@ #> basic place -path: '/v1/place?ids=geoname:1' +path: '/v1/place?ids=geonames:venue:1' #? 200 ok response.statusCode.should.be.equal 200 @@ -29,5 +29,5 @@ should.not.exist json.geocoding.errors should.not.exist json.geocoding.warnings #? inputs -json.geocoding.query['ids'].should.eql [{ id: '1', type: 'geoname' }] +json.geocoding.query['ids'].should.eql [{ id: '1', types: [ 'geoname' ] }] should.not.exist json.geocoding.query['size'] diff --git a/test/ciao/reverse/layers_invalid.coffee b/test/ciao/reverse/layers_invalid.coffee index 0da5790e..c4d858fc 100644 --- a/test/ciao/reverse/layers_invalid.coffee +++ b/test/ciao/reverse/layers_invalid.coffee @@ -24,7 +24,7 @@ json.features.should.be.instanceof Array #? expected errors should.exist json.geocoding.errors -json.geocoding.errors.should.eql [ '\'notlayer\' is an invalid layers parameter. Valid options: venue,address,country,region,county,locality,localadmin,neighbourhood,coarse' ] +json.geocoding.errors.should.eql [ '\'notlayer\' is an invalid layers parameter. Valid options: coarse,venue,address,country,region,county,locality,localadmin,neighbourhood' ] #? expected warnings should.not.exist json.geocoding.warnings @@ -32,4 +32,4 @@ should.not.exist json.geocoding.warnings #? inputs json.geocoding.query['size'].should.eql 10 should.not.exist json.geocoding.query['types'] -should.not.exist json.geocoding.query['type'] \ No newline at end of file +should.not.exist json.geocoding.query['type'] diff --git a/test/ciao/reverse/layers_mix_invalid_valid.coffee b/test/ciao/reverse/layers_mix_invalid_valid.coffee index daa7bc2d..71a95fa4 100644 --- a/test/ciao/reverse/layers_mix_invalid_valid.coffee +++ b/test/ciao/reverse/layers_mix_invalid_valid.coffee @@ -24,7 +24,7 @@ json.features.should.be.instanceof Array #? expected errors should.exist json.geocoding.errors -json.geocoding.errors.should.eql [ '\'notlayer\' is an invalid layers parameter. Valid options: venue,address,country,region,county,locality,localadmin,neighbourhood,coarse' ] +json.geocoding.errors.should.eql [ '\'notlayer\' is an invalid layers parameter. Valid options: coarse,venue,address,country,region,county,locality,localadmin,neighbourhood' ] #? expected warnings should.not.exist json.geocoding.warnings @@ -32,4 +32,4 @@ should.not.exist json.geocoding.warnings #? inputs json.geocoding.query['size'].should.eql 10 should.not.exist json.geocoding.query['types'] -should.not.exist json.geocoding.query['type'] \ No newline at end of file +should.not.exist json.geocoding.query['type'] diff --git a/test/ciao/reverse/layers_multiple.coffee b/test/ciao/reverse/layers_multiple.coffee index efa27e83..2fd0e1dc 100644 --- a/test/ciao/reverse/layers_multiple.coffee +++ b/test/ciao/reverse/layers_multiple.coffee @@ -30,5 +30,5 @@ should.not.exist json.geocoding.warnings #? inputs json.geocoding.query['size'].should.eql 10 -json.geocoding.query.types['from_layers'].should.eql ["admin0","admin1"] -json.geocoding.query['type'].should.eql ["admin0","admin1"] \ No newline at end of file +json.geocoding.query.types['from_layers'].should.eql ["admin0","geoname","admin1"] +json.geocoding.query['type'].should.eql ["admin0","geoname","admin1"] diff --git a/test/ciao/reverse/layers_single.coffee b/test/ciao/reverse/layers_single.coffee index 641edad1..e6642fb9 100644 --- a/test/ciao/reverse/layers_single.coffee +++ b/test/ciao/reverse/layers_single.coffee @@ -30,5 +30,5 @@ should.not.exist json.geocoding.warnings #? inputs json.geocoding.query['size'].should.eql 10 -json.geocoding.query.types['from_layers'].should.eql ["admin0"] -json.geocoding.query['type'].should.eql ["admin0"] \ No newline at end of file +json.geocoding.query.types['from_layers'].should.eql ["admin0","geoname"] +json.geocoding.query['type'].should.eql ["admin0","geoname"] diff --git a/test/ciao/reverse/sources_layers_invalid_combo.coffee b/test/ciao/reverse/sources_layers_invalid_combo.coffee index 06d18c37..dc74ba4a 100644 --- a/test/ciao/reverse/sources_layers_invalid_combo.coffee +++ b/test/ciao/reverse/sources_layers_invalid_combo.coffee @@ -31,6 +31,6 @@ should.not.exist json.geocoding.warnings #? inputs json.geocoding.query['size'].should.eql 10 -json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses"] +json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses","geoname"] json.geocoding.query.types['from_sources'].should.eql ["admin0","admin1","admin2","neighborhood","locality","local_admin"] -should.not.exist json.geocoding.query['type'] \ No newline at end of file +should.not.exist json.geocoding.query['type'] diff --git a/test/ciao/reverse/sources_layers_valid_combo.coffee b/test/ciao/reverse/sources_layers_valid_combo.coffee index 286d4916..cc593768 100644 --- a/test/ciao/reverse/sources_layers_valid_combo.coffee +++ b/test/ciao/reverse/sources_layers_valid_combo.coffee @@ -30,5 +30,5 @@ should.not.exist json.geocoding.warnings #? inputs json.geocoding.query['size'].should.eql 10 -json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses"] -json.geocoding.query['type'].should.eql ["openaddresses"] \ No newline at end of file +json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses","geoname"] +json.geocoding.query['type'].should.eql ["openaddresses"] diff --git a/test/ciao/search/layers_alias_address.coffee b/test/ciao/search/layers_alias_address.coffee index c7c58472..14074bf9 100644 --- a/test/ciao/search/layers_alias_address.coffee +++ b/test/ciao/search/layers_alias_address.coffee @@ -31,5 +31,5 @@ should.not.exist json.geocoding.warnings #? inputs json.geocoding.query['text'].should.eql 'a' json.geocoding.query['size'].should.eql 10 -json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses"] -json.geocoding.query['type'].should.eql ["osmaddress","openaddresses"] \ No newline at end of file +json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses","geoname"] +json.geocoding.query['type'].should.eql ["osmaddress","openaddresses","geoname"] diff --git a/test/ciao/search/layers_invalid.coffee b/test/ciao/search/layers_invalid.coffee index babe7ca6..3a7c9d38 100644 --- a/test/ciao/search/layers_invalid.coffee +++ b/test/ciao/search/layers_invalid.coffee @@ -24,7 +24,7 @@ json.features.should.be.instanceof Array #? expected errors should.exist json.geocoding.errors -json.geocoding.errors.should.eql [ '\'notlayer\' is an invalid layers parameter. Valid options: venue,address,country,region,county,locality,localadmin,neighbourhood,coarse' ] +json.geocoding.errors.should.eql [ '\'notlayer\' is an invalid layers parameter. Valid options: coarse,venue,address,country,region,county,locality,localadmin,neighbourhood' ] #? expected warnings should.not.exist json.geocoding.warnings @@ -33,4 +33,4 @@ should.not.exist json.geocoding.warnings json.geocoding.query['text'].should.eql 'a' json.geocoding.query['size'].should.eql 10 should.not.exist json.geocoding.query['types'] -should.not.exist json.geocoding.query['type'] \ No newline at end of file +should.not.exist json.geocoding.query['type'] diff --git a/test/ciao/search/layers_mix_invalid_valid.coffee b/test/ciao/search/layers_mix_invalid_valid.coffee index a496bdb7..c1a53645 100644 --- a/test/ciao/search/layers_mix_invalid_valid.coffee +++ b/test/ciao/search/layers_mix_invalid_valid.coffee @@ -24,7 +24,7 @@ json.features.should.be.instanceof Array #? expected errors should.exist json.geocoding.errors -json.geocoding.errors.should.eql [ '\'notlayer\' is an invalid layers parameter. Valid options: venue,address,country,region,county,locality,localadmin,neighbourhood,coarse' ] +json.geocoding.errors.should.eql [ '\'notlayer\' is an invalid layers parameter. Valid options: coarse,venue,address,country,region,county,locality,localadmin,neighbourhood' ] #? expected warnings should.not.exist json.geocoding.warnings @@ -33,4 +33,4 @@ should.not.exist json.geocoding.warnings json.geocoding.query['text'].should.eql 'a' json.geocoding.query['size'].should.eql 10 should.not.exist json.geocoding.query['types'] -should.not.exist json.geocoding.query['type'] \ No newline at end of file +should.not.exist json.geocoding.query['type'] diff --git a/test/ciao/search/layers_multiple.coffee b/test/ciao/search/layers_multiple.coffee index db83fed7..20e67eba 100644 --- a/test/ciao/search/layers_multiple.coffee +++ b/test/ciao/search/layers_multiple.coffee @@ -31,5 +31,5 @@ should.not.exist json.geocoding.warnings #? inputs json.geocoding.query['text'].should.eql 'a' json.geocoding.query['size'].should.eql 10 -json.geocoding.query.types['from_layers'].should.eql ["admin0","admin1"] -json.geocoding.query['type'].should.eql ["admin0","admin1"] \ No newline at end of file +json.geocoding.query.types['from_layers'].should.eql ["admin0","geoname","admin1"] +json.geocoding.query['type'].should.eql ["admin0","geoname","admin1"] diff --git a/test/ciao/search/layers_single.coffee b/test/ciao/search/layers_single.coffee index b0d130eb..9dc79a00 100644 --- a/test/ciao/search/layers_single.coffee +++ b/test/ciao/search/layers_single.coffee @@ -31,5 +31,5 @@ should.not.exist json.geocoding.warnings #? inputs json.geocoding.query['text'].should.eql 'a' json.geocoding.query['size'].should.eql 10 -json.geocoding.query.types['from_layers'].should.eql ["admin0"] -json.geocoding.query['type'].should.eql ["admin0"] \ No newline at end of file +json.geocoding.query.types['from_layers'].should.eql ["admin0","geoname"] +json.geocoding.query['type'].should.eql ["admin0","geoname"] diff --git a/test/ciao/search/sources_layers_invalid_combo.coffee b/test/ciao/search/sources_layers_invalid_combo.coffee index ce388de7..6d9702ed 100644 --- a/test/ciao/search/sources_layers_invalid_combo.coffee +++ b/test/ciao/search/sources_layers_invalid_combo.coffee @@ -32,6 +32,6 @@ should.not.exist json.geocoding.warnings #? inputs json.geocoding.query['text'].should.eql 'a' json.geocoding.query['size'].should.eql 10 -json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses"] +json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses","geoname"] json.geocoding.query.types['from_sources'].should.eql ["admin0","admin1","admin2","neighborhood","locality","local_admin"] -should.not.exist json.geocoding.query['type'] \ No newline at end of file +should.not.exist json.geocoding.query['type'] diff --git a/test/ciao/search/sources_layers_valid_combo.coffee b/test/ciao/search/sources_layers_valid_combo.coffee index 6e11c9c7..d20cc6ca 100644 --- a/test/ciao/search/sources_layers_valid_combo.coffee +++ b/test/ciao/search/sources_layers_valid_combo.coffee @@ -31,5 +31,5 @@ should.not.exist json.geocoding.warnings #? inputs json.geocoding.query['text'].should.eql 'a' json.geocoding.query['size'].should.eql 10 -json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses"] -json.geocoding.query['type'].should.eql ["openaddresses"] \ No newline at end of file +json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses","geoname"] +json.geocoding.query['type'].should.eql ["openaddresses"] diff --git a/test/unit/sanitiser/_ids.js b/test/unit/sanitiser/_ids.js index d7c65d33..9438f3ee 100644 --- a/test/unit/sanitiser/_ids.js +++ b/test/unit/sanitiser/_ids.js @@ -8,13 +8,9 @@ var inputs = { }; var formatError = function(input) { - return 'id `' + input + ' is invalid: must be of the format type:id for ex: \'geoname:4163334\''; + return 'id `' + input + ' is invalid: must be of the format source:layer:id for ex: \'geonames:venue:4163334\''; }; var lengthError = 'invalid param \'ids\': length must be >0'; -var defaultMissingTypeError = function(input) { - var type = input.split(delimiter)[0]; - return type + ' is invalid. It must be one of these values - [' + type_mapping.types_list.join(', ') + ']'; - }; module.exports.tests = {}; @@ -74,50 +70,58 @@ module.exports.tests.invalid_ids = function(test, common) { t.end(); }); - test('invalid id: type name invalid', function(t) { - var raw = { ids: 'gibberish:23' }; + test('invalid id: source name invalid', function(t) { + var raw = { ids: 'invalidsource:venue:23' }; var clean = {}; + var expected_error = 'invalidsource is invalid. It must be one of these values - [' + type_mapping.sources.join(', ') + ']'; var messages = sanitize(raw, clean); - t.equal(messages.errors[0], defaultMissingTypeError('gibberish:23'), 'format error returned'); + t.equal(messages.errors[0], expected_error, 'format error returned'); + t.equal(clean.ids, undefined, 'ids unset in clean object'); + t.end(); + }); + + test('invalid id: old style 2 part id', function(t) { + var raw = { ids: 'geonames:23' }; + var clean = {}; + + var messages = sanitize(raw, clean); + + t.equal(messages.errors[0], formatError('geonames:23'), 'format error returned'); t.equal(clean.ids, undefined, 'ids unset in clean object'); t.end(); }); }; module.exports.tests.valid_ids = function(test, common) { - test('ids: valid input', function(t) { - inputs.valid.forEach( function( input ){ - var input_parts = input.split(delimiter); - var expected_clean = { ids: [ { id: input_parts[1], types: [ input_parts[0] ]} ]}; - var raw = { ids: input }; - var clean = {}; - - var messages = sanitize( raw, clean ); - - t.deepEqual( messages.errors, [], 'no error (' + input + ')' ); - t.deepEqual( clean, expected_clean, 'clean set correctly (' + input + ')'); - }); + test('ids: valid input (openaddresses)', function(t) { + var raw = { ids: 'openaddresses:address:20' }; + var clean = {}; + var expected_ids = [{ + id: '20', + types: [ 'openaddresses' ] + }]; + + var messages = sanitize( raw, clean ); + + t.deepEqual( messages.errors, [], ' no errors'); + t.deepEqual( clean.ids, expected_ids, 'single type value returned'); t.end(); }); - test('ids: valid input with multiple values' , function(t) { - var raw = { ids: inputs.valid.join(',') }; + test('ids: valid input (osm)', function(t) { + var raw = { ids: 'osm:venue:500' }; var clean = {}; - var expected_clean={ - ids: [], - }; - // construct the expected id and type for each valid input - inputs.valid.forEach( function( input ){ - var input_parts = input.split(delimiter); - expected_clean.ids.push({ id: input_parts[1], types: [ input_parts[0] ]}); - }); + var expected_ids = [{ + id: '500', + types: [ 'osmnode', 'osmway' ] + }]; var messages = sanitize( raw, clean ); - t.deepEqual( messages.errors, [], 'no errors' ); - t.deepEqual( clean, expected_clean, 'clean set correctly' ); + t.deepEqual( messages.errors, [], ' no errors'); + t.deepEqual( clean.ids, expected_ids, 'osm could be two types, but that\'s ok'); t.end(); }); }; @@ -137,10 +141,10 @@ module.exports.tests.array_of_ids = function(test, common) { }; module.exports.tests.multiple_ids = function(test, common) { - test('duplicate ids', function(t) { - var expected_clean = { ids: [ { id: '1', types: [ 'geoname' ] }, { id: '2', types: [ 'osmnode' ] } ] }; - var raw = { ids: 'geoname:1,osmnode:2' }; + test('multiple ids', function(t) { + var raw = { ids: 'geonames:venue:1,osm:venue:2' }; var clean = {}; + var expected_clean = { ids: [ { id: '1', types: [ 'geoname' ] }, { id: '2', types: [ 'osmnode', 'osmway' ] } ] }; var messages = sanitize( raw, clean); @@ -153,8 +157,8 @@ module.exports.tests.multiple_ids = function(test, common) { module.exports.tests.de_dupe = function(test, common) { test('duplicate ids', function(t) { - var expected_clean = { ids: [ { id: '1', types: [ 'geoname' ] }, { id: '2', types: [ 'osmnode' ] } ] }; - var raw = { ids: 'geoname:1,osmnode:2,geoname:1' }; + var expected_clean = { ids: [ { id: '1', types: [ 'geoname' ] }, { id: '2', types: [ 'osmnode', 'osmway' ] } ] }; + var raw = { ids: 'geonames:venue:1,osm:venue:2,geonames:venue:1' }; var clean = {}; var messages = sanitize( raw, clean ); diff --git a/test/unit/sanitiser/place.js b/test/unit/sanitiser/place.js index 26042c5e..f7cda1e9 100644 --- a/test/unit/sanitiser/place.js +++ b/test/unit/sanitiser/place.js @@ -31,7 +31,7 @@ module.exports.tests.sanitize_private = function(test, common) { var invalid_values = [null, -1, 123, NaN, 'abc']; invalid_values.forEach(function(value) { test('invalid private param ' + value, function(t) { - var req = { query: { ids:'geoname:123', 'private': value } }; + var req = { query: { ids:'geonames:venue:123', 'private': value } }; sanitize(req, function(){ t.deepEqual( req.errors, [], 'no errors' ); t.deepEqual( req.warnings, [], 'no warnings' ); @@ -44,7 +44,7 @@ module.exports.tests.sanitize_private = function(test, common) { var valid_values = ['true', true, 1]; valid_values.forEach(function(value) { test('valid private param ' + value, function(t) { - var req = { query: { ids:'geoname:123', 'private': value } }; + var req = { query: { ids:'geonames:venue:123', 'private': value } }; sanitize(req, function(){ t.deepEqual( req.errors, [], 'no errors' ); t.deepEqual( req.warnings, [], 'no warnings' ); @@ -57,7 +57,7 @@ module.exports.tests.sanitize_private = function(test, common) { var valid_false_values = ['false', false, 0]; valid_false_values.forEach(function(value) { test('test setting false explicitly ' + value, function(t) { - var req = { query: { ids:'geoname:123', 'private': value } }; + var req = { query: { ids:'geonames:venue:123', 'private': value } }; sanitize(req, function(){ t.deepEqual( req.errors, [], 'no errors' ); t.deepEqual( req.warnings, [], 'no warnings' ); @@ -68,7 +68,7 @@ module.exports.tests.sanitize_private = function(test, common) { }); test('test default behavior', function(t) { - var req = { query: { ids:'geoname:123' } }; + var req = { query: { ids:'geonames:venue:123' } }; sanitize(req, function(){ t.deepEqual( req.errors, [], 'no errors' ); t.deepEqual( req.warnings, [], 'no warnings' ); @@ -91,7 +91,7 @@ module.exports.tests.invalid_params = function(test, common) { module.exports.tests.middleware_success = function(test, common) { test('middleware success', function(t) { - var req = { query: { ids: 'geoname:123' }}; + var req = { query: { ids: 'geonames:venue:123' }}; var next = function(){ t.deepEqual( req.errors, [], 'no errors' ); t.deepEqual( req.warnings, [], 'no warnings' ); From 7b9d61c5ecddc0438742e83186b43b75c55354a2 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Tue, 22 Sep 2015 13:51:46 -0400 Subject: [PATCH 27/39] Use lodash intersection method --- helper/type_mapping.js | 14 +++----------- helper/types.js | 14 +++----------- 2 files changed, 6 insertions(+), 22 deletions(-) diff --git a/helper/type_mapping.js b/helper/type_mapping.js index ab7bbe56..27fce81c 100644 --- a/helper/type_mapping.js +++ b/helper/type_mapping.js @@ -1,4 +1,5 @@ -var extend = require('extend'); +var extend = require('extend'), + _ = require('lodash'); var TYPE_TO_SOURCE = { 'geoname': 'gn', @@ -69,17 +70,8 @@ var TYPES = Object.keys(TYPE_TO_SOURCE); var SOURCES = Object.keys(SOURCE_TO_TYPE); var LAYERS = Object.keys(LAYER_TO_TYPE); -/** - * Calculate the set-style intersection of two arrays - */ -var intersection = function intersection(set1, set2) { - return set2.filter(function(value) { - return set1.indexOf(value) !== -1; - }); -}; - var sourceAndLayerToType = function sourceAndLayerToType(source, layer) { - return intersection(SOURCE_TO_TYPE[source], LAYER_WITH_ALIASES_TO_TYPE[layer]); + return _.intersection(SOURCE_TO_TYPE[source], LAYER_WITH_ALIASES_TO_TYPE[layer]); }; module.exports = { diff --git a/helper/types.js b/helper/types.js index aa3337e8..610d1aba 100644 --- a/helper/types.js +++ b/helper/types.js @@ -1,13 +1,5 @@ var type_mapping = require( '../helper/type_mapping' ); - -/** - * Calculate the set-style intersection of two arrays - */ -var intersection = function intersection(set1, set2) { - return set2.filter(function(value) { - return set1.indexOf(value) !== -1; - }); -}; +var _ = require('lodash'); /** * Combine all types and determine the unique subset @@ -27,11 +19,11 @@ module.exports = function calculate_types(clean_types) { var types = type_mapping.types; if (clean_types.from_layers) { - types = intersection(types, clean_types.from_layers); + types = _.intersection(types, clean_types.from_layers); } if (clean_types.from_sources) { - types = intersection(types, clean_types.from_sources); + types = _.intersection(types, clean_types.from_sources); } return types; From e157749de33d9d7fb3f37dc53c476f2ed7f02f4c Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Tue, 22 Sep 2015 13:57:53 -0400 Subject: [PATCH 28/39] Use _.contains instead of .indexOf --- helper/geojsonify.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/helper/geojsonify.js b/helper/geojsonify.js index 94b41b84..ca499613 100644 --- a/helper/geojsonify.js +++ b/helper/geojsonify.js @@ -35,10 +35,10 @@ function lookupSource(src) { */ function lookupLayer(src) { if (src._type === 'geoname') { - if (src.category && src.category.indexOf('admin') !== -1) { - if (src.category.indexOf('admin:city') !== -1) { return 'locality'; } - if (src.category.indexOf('admin:admin1') !== -1) { return 'region'; } - if (src.category.indexOf('admin:admin2') !== -1) { return 'county'; } + if (_.contains(src.category, 'admin')) { + if (_.contains(src.category, 'admin:city')) { return 'locality'; } + if (_.contains(src.category, 'admin:admin1')) { return 'region'; } + if (_.contains(src.category, 'admin:admin2')) { return 'county'; } return 'neighbourhood'; // this could also be 'local_admin' } From a2cc31f142cb0b197d3c08db9b0274b296097439 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Tue, 22 Sep 2015 14:12:12 -0400 Subject: [PATCH 29/39] Leave a note for future us A small essay on Elasticsearch queries and fields and types. --- controller/place.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/controller/place.js b/controller/place.js index 42e7c626..101430c5 100644 --- a/controller/place.js +++ b/controller/place.js @@ -16,6 +16,15 @@ function setup( backend ){ var query = req.clean.ids.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. It's important to use `type` here instead of + * `_type`, as the former actually queries against the type, and thus + * can accept multiple match values. `_type`, on the other hand, + * simply changes the actual URL of the query sent to Elasticsearch to + * contain a type, which obviously can only take a single type. + */ type: id.types, _id: id.id }; From 2ba206f1220fa92b8eacb6c68b768202139000a8 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 15:18:08 -0400 Subject: [PATCH 30/39] removed debug --- sanitiser/_geo_common.js | 1 - 1 file changed, 1 deletion(-) diff --git a/sanitiser/_geo_common.js b/sanitiser/_geo_common.js index 153128bc..bc5a4bcc 100644 --- a/sanitiser/_geo_common.js +++ b/sanitiser/_geo_common.js @@ -56,7 +56,6 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) { * @param {bool} circle_is_required */ function sanitize_circle( key_prefix, clean, raw, circle_is_required ) { - // "boundary.circle", clean, raw, false // the names we use to define the centroid var mandatoryProps = [ 'lat', 'lon' ]; From 1c066b65e73c6f4a11722ae889f174f0b3b27291 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 22 Sep 2015 15:47:17 -0400 Subject: [PATCH 31/39] switched undefined check to positive lodash call --- sanitiser/_geo_reverse.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sanitiser/_geo_reverse.js b/sanitiser/_geo_reverse.js index fb77add3..c15af48b 100644 --- a/sanitiser/_geo_reverse.js +++ b/sanitiser/_geo_reverse.js @@ -1,6 +1,6 @@ var geo_common = require ('./_geo_common'); -var check = require('check-types'); +var _ = require('lodash'); var defaults = require('../query/defaults'); var LAT_LON_IS_REQUIRED = true, CIRCLE_IS_REQUIRED = false; @@ -29,7 +29,7 @@ module.exports = function sanitize( raw, clean ){ raw['boundary.circle.lon'] = clean['point.lon']; // if no radius was passed, set the default - if (!check.assigned(raw['boundary.circle.radius'])) { + if ( _.isUndefined( raw['boundary.circle.radius'] ) ) { raw['boundary.circle.radius'] = defaults['boundary:circle:radius']; } From 62315a1775ba655cf70222445aecf78b6ea42d03 Mon Sep 17 00:00:00 2001 From: Diana Shkolnikov Date: Tue, 22 Sep 2015 16:15:46 -0400 Subject: [PATCH 32/39] Revert defaults.json change --- query/defaults.js | 2 +- test/unit/fixture/autocomplete_linguistic_focus.js | 2 +- test/unit/fixture/autocomplete_linguistic_focus_null_island.js | 2 +- test/unit/fixture/search_linguistic_focus.js | 2 +- test/unit/fixture/search_linguistic_focus_bbox.js | 2 +- test/unit/fixture/search_linguistic_focus_null_island.js | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/query/defaults.js b/query/defaults.js index c9613d66..cf9422d0 100644 --- a/query/defaults.js +++ b/query/defaults.js @@ -31,7 +31,7 @@ module.exports = extend( false, peliasQuery.defaults, { 'focus:function': 'linear', 'focus:offset': '1km', - 'focus:scale': '100km', + 'focus:scale': '50km', 'focus:decay': 0.5, 'function_score:score_mode': 'avg', diff --git a/test/unit/fixture/autocomplete_linguistic_focus.js b/test/unit/fixture/autocomplete_linguistic_focus.js index a1eb6d45..76e844e3 100644 --- a/test/unit/fixture/autocomplete_linguistic_focus.js +++ b/test/unit/fixture/autocomplete_linguistic_focus.js @@ -44,7 +44,7 @@ module.exports = { 'lon': -82.50622 }, 'offset': '1km', - 'scale': '100km', + 'scale': '50km', 'decay': 0.5 } } diff --git a/test/unit/fixture/autocomplete_linguistic_focus_null_island.js b/test/unit/fixture/autocomplete_linguistic_focus_null_island.js index 7e105bb3..ded463bf 100644 --- a/test/unit/fixture/autocomplete_linguistic_focus_null_island.js +++ b/test/unit/fixture/autocomplete_linguistic_focus_null_island.js @@ -44,7 +44,7 @@ module.exports = { 'lon': 0 }, 'offset': '1km', - 'scale': '100km', + 'scale': '50km', 'decay': 0.5 } } diff --git a/test/unit/fixture/search_linguistic_focus.js b/test/unit/fixture/search_linguistic_focus.js index b9878dec..eac92086 100644 --- a/test/unit/fixture/search_linguistic_focus.js +++ b/test/unit/fixture/search_linguistic_focus.js @@ -44,7 +44,7 @@ module.exports = { 'lon': -82.50622 }, 'offset': '1km', - 'scale': '100km', + 'scale': '50km', 'decay': 0.5 } } diff --git a/test/unit/fixture/search_linguistic_focus_bbox.js b/test/unit/fixture/search_linguistic_focus_bbox.js index b6e642d1..d7aa6544 100644 --- a/test/unit/fixture/search_linguistic_focus_bbox.js +++ b/test/unit/fixture/search_linguistic_focus_bbox.js @@ -44,7 +44,7 @@ module.exports = { 'lon': -82.50622 }, 'offset': '1km', - 'scale': '100km', + 'scale': '50km', 'decay': 0.5 } } diff --git a/test/unit/fixture/search_linguistic_focus_null_island.js b/test/unit/fixture/search_linguistic_focus_null_island.js index 4ba2adf0..506dcea9 100644 --- a/test/unit/fixture/search_linguistic_focus_null_island.js +++ b/test/unit/fixture/search_linguistic_focus_null_island.js @@ -44,7 +44,7 @@ module.exports = { 'lon': 0 }, 'offset': '1km', - 'scale': '100km', + 'scale': '50km', 'decay': 0.5 } } From ce32802368f6e7fce05302279279c1c9d3a3fbad Mon Sep 17 00:00:00 2001 From: Diana Shkolnikov Date: Tue, 22 Sep 2015 19:15:05 -0400 Subject: [PATCH 33/39] Hotfix for confidence exception --- middleware/confidenceScore.js | 37 ++++++----- test/unit/middleware/confidenceScore.js | 82 +++++++++++++++++++++++++ test/unit/run.js | 1 + 3 files changed, 106 insertions(+), 14 deletions(-) create mode 100644 test/unit/middleware/confidenceScore.js diff --git a/middleware/confidenceScore.js b/middleware/confidenceScore.js index 04b87b7d..d04e0ab9 100644 --- a/middleware/confidenceScore.js +++ b/middleware/confidenceScore.js @@ -13,17 +13,21 @@ var stats = require('stats-lite'); var logger = require('pelias-logger').get('api'); +var check = require('check-types'); var RELATIVE_SCORES = true; function setup(peliasConfig) { - RELATIVE_SCORES = peliasConfig.hasOwnProperty('relativeScores') ? peliasConfig.relativeScores : true; + if (check.assigned(peliasConfig)) { + RELATIVE_SCORES = peliasConfig.hasOwnProperty('relativeScores') ? peliasConfig.relativeScores : true; + } return computeScores; } function computeScores(req, res, next) { // do nothing if no result data set - if (!res || !res.data || !res.meta) { + if (check.undefined(req.clean) || check.undefined(res) || + check.undefined(res.data) || check.undefined(res.meta)) { return next(); } @@ -71,6 +75,7 @@ function computeConfidenceScore(req, mean, stdev, hit) { // TODO: look at categories and location hit.confidence /= checkCount; + hit.confidence = Number((hit.confidence).toFixed(3)); logger.debug('[confidence]:', hit.confidence, hit.name.default); @@ -78,16 +83,16 @@ function computeConfidenceScore(req, mean, stdev, hit) { } function checkForDealBreakers(req, hit) { - if (!req.clean.parsed_text) { + if (check.undefined(req.clean.parsed_text)) { return false; } - if (req.clean.parsed_text.state && req.clean.parsed_text.state !== hit.admin1_abbr) { + if (check.assigned(req.clean.parsed_text.state) && req.clean.parsed_text.state !== hit.admin1_abbr) { logger.debug('[confidence][deal-breaker]: state !== admin1_abbr'); return true; } - if (req.clean.parsed_text.postalcode && req.clean.parsed_text.postalcode !== hit.zip) { + if (check.assigned(req.clean.parsed_text.postalcode) && req.clean.parsed_text.postalcode !== hit.zip) { logger.debug('[confidence][deal-breaker]: postalcode !== zip'); return true; } @@ -117,7 +122,8 @@ function checkDistanceFromMean(score, mean, stdev) { */ function checkName(text, parsed_text, hit) { // parsed_text name should take precedence if available since it's the cleaner name property - if (parsed_text && parsed_text.name && hit.name.default.toLowerCase() === parsed_text.name.toLowerCase()) { + if (check.assigned(parsed_text) && check.assigned(parsed_text.name) && + hit.name.default.toLowerCase() === parsed_text.name.toLowerCase()) { return 1; } @@ -139,7 +145,9 @@ function checkName(text, parsed_text, hit) { * @returns {number} */ function checkQueryType(text, hit) { - if (!!text.number && (!hit.address || (hit.address && !hit.address.number))) { + if (check.assigned(text) && check.assigned(text.number) && + (check.undefined(hit.address) || + (check.assigned(hit.address) && check.undefined(hit.address.number)))) { return 0; } return 1; @@ -156,22 +164,23 @@ function checkQueryType(text, hit) { function propMatch(textProp, hitProp, expectEnriched) { // both missing, but expect to have enriched value in result => BAD - if (!textProp && !hitProp && expectEnriched) { return 0; } + if (check.undefined(textProp) && check.undefined(hitProp) && check.assigned(expectEnriched)) { return 0; } // both missing, and no enrichment expected => GOOD - if (!textProp && !hitProp) { return 1; } + if (check.undefined(textProp) && check.undefined(hitProp)) { return 1; } // text has it, result doesn't => BAD - if (textProp && !hitProp) { return 0; } + if (check.assigned(textProp) && check.undefined(hitProp)) { return 0; } // text missing, result has it, and enrichment is expected => GOOD - if (!textProp && hitProp && expectEnriched) { return 1; } + if (check.undefined(textProp) && check.assigned(hitProp) && check.assigned(expectEnriched)) { return 1; } // text missing, result has it, enrichment not desired => 50/50 - if (!textProp && hitProp) { return 0.5; } + if (check.undefined(textProp) && check.assigned(hitProp)) { return 0.5; } // both present, values match => GREAT - if (textProp && hitProp && textProp.toString().toLowerCase() === hitProp.toString().toLowerCase()) { return 1; } + if (check.assigned(textProp) && check.assigned(hitProp) && + textProp.toString().toLowerCase() === hitProp.toString().toLowerCase()) { return 1; } // ¯\_(ツ)_/¯ return 0.7; @@ -200,7 +209,7 @@ function checkAddress(text, hit) { var checkCount = 5; var res = 0; - if (text && text.number && text.street) { + if (check.assigned(text) && check.assigned(text.number) && check.assigned(text.street)) { res += propMatch(text.number, (hit.address ? hit.address.number : null), false); res += propMatch(text.street, (hit.address ? hit.address.street : null), false); res += propMatch(text.postalcode, (hit.address ? hit.address.zip: null), true); diff --git a/test/unit/middleware/confidenceScore.js b/test/unit/middleware/confidenceScore.js new file mode 100644 index 00000000..c32a37d4 --- /dev/null +++ b/test/unit/middleware/confidenceScore.js @@ -0,0 +1,82 @@ +var confidenceScore = require('../../../middleware/confidenceScore')(); + +module.exports.tests = {}; + +module.exports.tests.confidenceScore = function(test, common) { + + test('empty res and req should not throw exception', function(t) { + try { + confidenceScore({}, {}, function() {}); + t.pass('no exception'); + } + catch (e) { + t.fail('an exception should not have been thrown'); + } + finally { + t.end(); + } + + }); + + test('res.results without parsed_text should not throw exception', function(t) { + var req = {}; + var res = { + data: [{ + name: 'foo' + }], + meta: [10] + }; + + try { + confidenceScore(req, res, function() {}); + t.pass('no exception'); + } + catch (e) { + t.fail('an exception should not have been thrown'); + console.log(e.stack); + } + finally { + t.end(); + } + + }); + + + test('res.results without parsed_text should not throw exception', function(t) { + var req = { + clean: { text: 'test name1' } + }; + var res = { + data: [{ + _score: 10, + found: true, + value: 1, + center_point: { lat: 100.1, lon: -50.5 }, + name: { default: 'test name1' }, + admin0: 'country1', admin1: 'state1', admin2: 'city1' + }, { + value: 2, + center_point: { lat: 100.2, lon: -51.5 }, + name: { default: 'test name2' }, + admin0: 'country2', admin1: 'state2', admin2: 'city2', + _score: 20 + }], + meta: {scores: [10]} + }; + + confidenceScore(req, res, function() {}); + t.equal(res.data[0].confidence, 0.6, 'score was set'); + t.end(); + }); + +}; + +module.exports.all = function (tape, common) { + function test(name, testFunction) { + return tape('[middleware] confidenceScore: ' + name, testFunction); + } + + for( var testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; diff --git a/test/unit/run.js b/test/unit/run.js index 977c1c58..74202992 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -29,6 +29,7 @@ var tests = [ require('./sanitiser/_geo_common'), require('./middleware/distance'), require('./middleware/confidenceScoreReverse'), + require('./middleware/confidenceScore'), require('./sanitiser/_size'), require('./sanitiser/_single_scalar_parameters'), require('./sanitiser/_geo_reverse'), From 2b9c3d79aa636400249cd43127f21670acd49d6d Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Wed, 23 Sep 2015 18:14:59 +0200 Subject: [PATCH 34/39] rename outputGenerator -> labelGenerator --- helper/geojsonify.js | 4 ++-- helper/{outputGenerator.js => labelGenerator.js} | 2 +- helper/{outputSchema.json => labelSchema.json} | 0 test/unit/helper/{outputSchema.js => labelSchema.js} | 2 +- test/unit/run.js | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) rename helper/{outputGenerator.js => labelGenerator.js} (94%) rename helper/{outputSchema.json => labelSchema.json} (100%) rename test/unit/helper/{outputSchema.js => labelSchema.js} (96%) diff --git a/helper/geojsonify.js b/helper/geojsonify.js index ca499613..d01b6f36 100644 --- a/helper/geojsonify.js +++ b/helper/geojsonify.js @@ -1,7 +1,7 @@ var GeoJSON = require('geojson'), extent = require('geojson-extent'), - outputGenerator = require('./outputGenerator'), + labelGenerator = require('./labelGenerator'), logger = require('pelias-logger').get('api'), type_mapping = require('./type_mapping'), _ = require('lodash'); @@ -116,7 +116,7 @@ function addDetails(src, dst) { * @param {object} dst */ function addLabel(src, dst) { - dst.label = outputGenerator(src); + dst.label = labelGenerator(src); } /** diff --git a/helper/outputGenerator.js b/helper/labelGenerator.js similarity index 94% rename from helper/outputGenerator.js rename to helper/labelGenerator.js index d8b47f0d..3269fcf2 100644 --- a/helper/outputGenerator.js +++ b/helper/labelGenerator.js @@ -1,5 +1,5 @@ -var schemas = require('./outputSchema.json'); +var schemas = require('./labelSchema.json'); module.exports = function( record ){ diff --git a/helper/outputSchema.json b/helper/labelSchema.json similarity index 100% rename from helper/outputSchema.json rename to helper/labelSchema.json diff --git a/test/unit/helper/outputSchema.js b/test/unit/helper/labelSchema.js similarity index 96% rename from test/unit/helper/outputSchema.js rename to test/unit/helper/labelSchema.js index cefd08da..f85c43aa 100644 --- a/test/unit/helper/outputSchema.js +++ b/test/unit/helper/labelSchema.js @@ -1,5 +1,5 @@ -var schemas = require('../../../helper/outputSchema.json'); +var schemas = require('../../../helper/labelSchema.json'); var alpha3 = require('../mock/alpha3.json'); module.exports.tests = {}; diff --git a/test/unit/run.js b/test/unit/run.js index 74202992..1f42b5b2 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -23,7 +23,7 @@ var tests = [ require('./query/defaults'), require('./helper/query_parser'), require('./helper/geojsonify'), - require('./helper/outputSchema'), + require('./helper/labelSchema'), require('./helper/types'), require('./helper/type_mapping'), require('./sanitiser/_geo_common'), From 3a54a8b32fc4b2df3c15b1d2ad3e571b14c10a56 Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Wed, 23 Sep 2015 19:00:33 +0200 Subject: [PATCH 35/39] fix label issues --- helper/labelGenerator.js | 22 ++- helper/labelSchema.json | 2 +- test/unit/helper/labelGenerator.js | 235 +++++++++++++++++++++++++++++ test/unit/run.js | 1 + 4 files changed, 247 insertions(+), 13 deletions(-) create mode 100644 test/unit/helper/labelGenerator.js diff --git a/helper/labelGenerator.js b/helper/labelGenerator.js index 3269fcf2..1938232d 100644 --- a/helper/labelGenerator.js +++ b/helper/labelGenerator.js @@ -1,9 +1,11 @@ -var schemas = require('./labelSchema.json'); +var _ = require('lodash'), + check = require('check-types'), + schemas = require('./labelSchema.json'); module.exports = function( record ){ - var adminParts = []; + var labelParts = [ record.name.default ]; var schema = schemas.default; @@ -13,9 +15,9 @@ module.exports = function( record ){ var buildOutput = function(parts, schemaArr, record) { for (var i=0; i Date: Wed, 23 Sep 2015 19:23:45 +0200 Subject: [PATCH 36/39] flip lon values for boundary.rect --- query/search.js | 4 ++-- test/unit/fixture/search_linguistic_bbox.js | 4 ++-- test/unit/fixture/search_linguistic_focus_bbox.js | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/query/search.js b/query/search.js index bf95b76b..a3787535 100644 --- a/query/search.js +++ b/query/search.js @@ -85,9 +85,9 @@ function generateQuery( clean ){ check.number(clean['boundary.rect.min_lon']) && check.number(clean['boundary.rect.max_lon']) ){ vs.set({ - 'boundary:rect:top': clean['boundary.rect.min_lat'], + 'boundary:rect:top': clean['boundary.rect.max_lat'], 'boundary:rect:right': clean['boundary.rect.max_lon'], - 'boundary:rect:bottom': clean['boundary.rect.max_lat'], + 'boundary:rect:bottom': clean['boundary.rect.min_lat'], 'boundary:rect:left': clean['boundary.rect.min_lon'] }); } diff --git a/test/unit/fixture/search_linguistic_bbox.js b/test/unit/fixture/search_linguistic_bbox.js index 754421e0..6c98b010 100644 --- a/test/unit/fixture/search_linguistic_bbox.js +++ b/test/unit/fixture/search_linguistic_bbox.js @@ -73,9 +73,9 @@ module.exports = { 'must': [{ 'geo_bounding_box': { 'center_point': { - 'top': 47.47, + 'top': 11.51, 'right': -61.84, - 'bottom': 11.51, + 'bottom': 47.47, 'left': -103.16 }, '_cache': true, diff --git a/test/unit/fixture/search_linguistic_focus_bbox.js b/test/unit/fixture/search_linguistic_focus_bbox.js index 09262a0e..e6a04824 100644 --- a/test/unit/fixture/search_linguistic_focus_bbox.js +++ b/test/unit/fixture/search_linguistic_focus_bbox.js @@ -102,9 +102,9 @@ module.exports = { 'must': [{ 'geo_bounding_box': { 'center_point': { - 'top': 47.47, + 'top': 11.51, 'right': -61.84, - 'bottom': 11.51, + 'bottom': 47.47, 'left': -103.16 }, '_cache': true, From b23b52682eb55b3c00f8cc82d93a7256bee9d957 Mon Sep 17 00:00:00 2001 From: Riordan Date: Wed, 23 Sep 2015 14:08:00 -0400 Subject: [PATCH 37/39] :house: :copyright: :free: OpenAddresses added to attribution under CC0 --- public/attribution.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/public/attribution.md b/public/attribution.md index db9c392e..2302c093 100644 --- a/public/attribution.md +++ b/public/attribution.md @@ -2,6 +2,6 @@ * Geocoding by [Pelias](https://mapzen.com/pelias) from [Mapzen](https://mapzen.com) * Data from * [OpenStreetMap](http://www.openstreetmap.org/copyright) © OpenStreetMap contributors under [ODbL](http://opendatacommons.org/licenses/odbl/) + * [OpenAddresses](http://openaddresses.io) under a [Creative Commons Zero](https://github.com/openaddresses/openaddresses/blob/master/sources/LICENSE) public domain designation * [Quattroshapes](https://github.com/foursquare/quattroshapes/blob/master/LICENSE.md) under [CC-BY-2.0](https://creativecommons.org/licenses/by/2.0/) * [GeoNames](http://www.geonames.org/) under [CC-BY-3.0](https://creativecommons.org/licenses/by/2.0/) - * and other sources From 84dfa0f45ed54ee6cdfeb087906d9f27f04cad4a Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Thu, 24 Sep 2015 17:05:47 +0200 Subject: [PATCH 38/39] add custom singapore schema, it is unusual as it is both a city and a country --- helper/labelSchema.json | 4 ++++ test/unit/helper/labelGenerator.js | 15 +++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/helper/labelSchema.json b/helper/labelSchema.json index 86f01e00..49c96017 100644 --- a/helper/labelSchema.json +++ b/helper/labelSchema.json @@ -7,6 +7,10 @@ "local": ["neighbourhood", "county", "localadmin", "locality", "region"], "regional": ["county","country","region"] }, + "SGP": { + "local": ["neighbourhood", "region", "county", "localadmin", "locality"], + "regional": ["county","country","region"] + }, "default": { "local": ["localadmin", "locality", "neighbourhood", "county"], "regional": ["region_a", "region", "country"] diff --git a/test/unit/helper/labelGenerator.js b/test/unit/helper/labelGenerator.js index 87ba4814..c70f0e50 100644 --- a/test/unit/helper/labelGenerator.js +++ b/test/unit/helper/labelGenerator.js @@ -223,6 +223,21 @@ module.exports.tests.new_zealand = function(test, common) { }); }; +// SGP venue +module.exports.tests.singapore_mcdonalds = function(test, common) { + test('singapore_mcdonalds', function(t) { + var doc = { + 'name': { 'default': 'McDonald\'s' }, + 'country_a': 'SGP', + 'country': 'Singapore', + 'region': 'Central Singapore', + 'locality': 'Singapore' + }; + t.equal(generator(doc),'McDonald\'s, Central Singapore, Singapore'); + t.end(); + }); +}; + module.exports.all = function (tape, common) { function test(name, testFunction) { From 0918d3031f49ef0bde4fe063f3b02128ed347c68 Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Fri, 25 Sep 2015 10:43:38 +0200 Subject: [PATCH 39/39] remove duplicate test --- test/unit/helper/labelGenerator.js | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/test/unit/helper/labelGenerator.js b/test/unit/helper/labelGenerator.js index c70f0e50..e1bf0da3 100644 --- a/test/unit/helper/labelGenerator.js +++ b/test/unit/helper/labelGenerator.js @@ -49,28 +49,6 @@ module.exports.tests.nyc_office = function(test, common) { }); }; -// USA venue -module.exports.tests.nyc_office = function(test, common) { - test('30 West 26th Street', function(t) { - var doc = { - 'name': { 'default': '30 West 26th Street' }, - 'housenumber': '30', - 'street': 'West 26th Street', - 'postalcode': '10010', - 'country_a': 'USA', - 'country': 'United States', - 'region': 'New York', - 'region_a': 'NY', - 'county': 'New York County', - 'localadmin': 'Manhattan', - 'locality': 'New York', - 'neighbourhood': 'Flatiron District' - }; - t.equal(generator(doc),'30 West 26th Street, Manhattan, NY'); - t.end(); - }); -}; - // AUS state module.exports.tests.new_south_wales = function(test, common) { test('new south wales', function(t) {