From 2811569bcd10957c8c239e04d168190a2b22152e Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Mon, 21 Sep 2015 18:59:11 -0400 Subject: [PATCH] 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' );