From 00d033101b457af46e529d61f7b26988089a1886 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 25 Sep 2015 14:44:24 -0400 Subject: [PATCH 1/4] removed redundant is-ids-an-array check (already covered by _single_scalar_parameter sanitizer) --- sanitiser/_ids.js | 7 ------- test/unit/sanitiser/_ids.js | 14 -------------- 2 files changed, 21 deletions(-) diff --git a/sanitiser/_ids.js b/sanitiser/_ids.js index 0b4d91c7..73f9d68a 100644 --- a/sanitiser/_ids.js +++ b/sanitiser/_ids.js @@ -57,13 +57,6 @@ function sanitize( raw, clean ){ // error & warning messages var messages = { errors: [], warnings: [] }; - // 'raw.ids' can be an array if ids is specified multiple times - // see https://github.com/pelias/api/issues/272 - if (check.array( raw.ids )) { - messages.errors.push( '`ids` parameter specified multiple times.' ); - return messages; - } - if (!check.unemptyString( raw.ids )) { messages.errors.push( lengthError); return messages; diff --git a/test/unit/sanitiser/_ids.js b/test/unit/sanitiser/_ids.js index 9438f3ee..176c981a 100644 --- a/test/unit/sanitiser/_ids.js +++ b/test/unit/sanitiser/_ids.js @@ -126,20 +126,6 @@ module.exports.tests.valid_ids = function(test, common) { }); }; -module.exports.tests.array_of_ids = function(test, common) { - // see https://github.com/pelias/api/issues/272 - test('array of ids sent by queryparser', function(t) { - var raw = { ids: ['geoname:2', 'oswmay:4'] }; - var clean = {}; - - var messages = sanitize( raw, clean); - - t.deepEqual( messages.errors, ['`ids` parameter specified multiple times.'], 'error sent' ); - t.deepEqual( clean.ids, undefined, 'response is empty due to error' ); - t.end(); - }); -}; - module.exports.tests.multiple_ids = function(test, common) { test('multiple ids', function(t) { var raw = { ids: 'geonames:venue:1,osm:venue:2' }; From fef92bd34bc300b6af44765604bed4a1074cebe9 Mon Sep 17 00:00:00 2001 From: Julian Simioni Date: Fri, 25 Sep 2015 19:54:01 -0400 Subject: [PATCH 2/4] Remove the geoname _type from all layers except venue We can't distinguish between geonames of different layers due to an ambiguity in our Elasticsearch schema that we unfortunately won't be able to fix for a few weeks. So, while it's technically true that there are countries, cities, etc contained in the geonames dataset, it's still better for now to remove geonames from these layers. We have good coverage of most coarse layers from quattroshapes alone, so the impact isn't too bad. --- helper/type_mapping.js | 12 ++++++------ test/ciao/reverse/layers_multiple.coffee | 4 ++-- test/ciao/reverse/layers_single.coffee | 4 ++-- .../ciao/reverse/sources_layers_invalid_combo.coffee | 2 +- test/ciao/reverse/sources_layers_valid_combo.coffee | 2 +- test/ciao/search/layers_alias_address.coffee | 4 ++-- test/ciao/search/layers_multiple.coffee | 4 ++-- test/ciao/search/layers_single.coffee | 4 ++-- test/ciao/search/sources_layers_invalid_combo.coffee | 2 +- test/ciao/search/sources_layers_valid_combo.coffee | 2 +- test/unit/sanitiser/_layers.js | 4 ++-- 11 files changed, 22 insertions(+), 22 deletions(-) diff --git a/helper/type_mapping.js b/helper/type_mapping.js index 27fce81c..06a8ec2d 100644 --- a/helper/type_mapping.js +++ b/helper/type_mapping.js @@ -48,13 +48,13 @@ var SOURCE_TO_TYPE = { */ var LAYER_TO_TYPE = { 'venue': ['geoname','osmnode','osmway'], - 'address': ['osmaddress','openaddresses', 'geoname'], - 'country': ['admin0', 'geoname'], - 'region': ['admin1', 'geoname'], - 'county': ['admin2', 'geoname'], - 'locality': ['locality', 'geoname'], + 'address': ['osmaddress','openaddresses'], + 'country': ['admin0'], + 'region': ['admin1'], + 'county': ['admin2'], + 'locality': ['locality'], 'localadmin': ['local_admin'], - 'neighbourhood': ['neighborhood', 'geoname'] + 'neighbourhood': ['neighborhood'] }; var LAYER_ALIASES = { diff --git a/test/ciao/reverse/layers_multiple.coffee b/test/ciao/reverse/layers_multiple.coffee index 2fd0e1dc..f5f2d7dd 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","geoname","admin1"] -json.geocoding.query['type'].should.eql ["admin0","geoname","admin1"] +json.geocoding.query.types['from_layers'].should.eql ["admin0","admin1"] +json.geocoding.query['type'].should.eql ["admin0","admin1"] diff --git a/test/ciao/reverse/layers_single.coffee b/test/ciao/reverse/layers_single.coffee index e6642fb9..4b65c331 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","geoname"] -json.geocoding.query['type'].should.eql ["admin0","geoname"] +json.geocoding.query.types['from_layers'].should.eql ["admin0"] +json.geocoding.query['type'].should.eql ["admin0"] diff --git a/test/ciao/reverse/sources_layers_invalid_combo.coffee b/test/ciao/reverse/sources_layers_invalid_combo.coffee index dc74ba4a..b15fcc88 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","geoname"] +json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses"] json.geocoding.query.types['from_sources'].should.eql ["admin0","admin1","admin2","neighborhood","locality","local_admin"] 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 cc593768..a24ea221 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","geoname"] +json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses"] 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 14074bf9..9bf32a04 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","geoname"] -json.geocoding.query['type'].should.eql ["osmaddress","openaddresses","geoname"] +json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses"] +json.geocoding.query['type'].should.eql ["osmaddress","openaddresses"] diff --git a/test/ciao/search/layers_multiple.coffee b/test/ciao/search/layers_multiple.coffee index 20e67eba..5f714d4d 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","geoname","admin1"] -json.geocoding.query['type'].should.eql ["admin0","geoname","admin1"] +json.geocoding.query.types['from_layers'].should.eql ["admin0","admin1"] +json.geocoding.query['type'].should.eql ["admin0","admin1"] diff --git a/test/ciao/search/layers_single.coffee b/test/ciao/search/layers_single.coffee index 9dc79a00..227f7bc9 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","geoname"] -json.geocoding.query['type'].should.eql ["admin0","geoname"] +json.geocoding.query.types['from_layers'].should.eql ["admin0"] +json.geocoding.query['type'].should.eql ["admin0"] diff --git a/test/ciao/search/sources_layers_invalid_combo.coffee b/test/ciao/search/sources_layers_invalid_combo.coffee index 6d9702ed..fb01cdf7 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","geoname"] +json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses"] json.geocoding.query.types['from_sources'].should.eql ["admin0","admin1","admin2","neighborhood","locality","local_admin"] 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 d20cc6ca..eeb2a663 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","geoname"] +json.geocoding.query.types['from_layers'].should.eql ["osmaddress","openaddresses"] json.geocoding.query['type'].should.eql ["openaddresses"] diff --git a/test/unit/sanitiser/_layers.js b/test/unit/sanitiser/_layers.js index 3264f434..519d95d9 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','geoname']; + var address_layers = ['osmaddress','openaddresses']; 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','geoname']; + var address_layers = ['osmaddress','openaddresses']; var reg_layers = ['admin0', 'locality']; var raw = { layers: 'address,country,locality' }; From b5b3c41ba17bb72b5ce86440b66e0072632c9651 Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Tue, 29 Sep 2015 16:13:48 +0200 Subject: [PATCH 3/4] remove microtime module --- package.json | 1 - service/mget.js | 7 ++----- service/search.js | 5 +---- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/package.json b/package.json index d949fdb3..abca960e 100644 --- a/package.json +++ b/package.json @@ -47,7 +47,6 @@ "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", diff --git a/service/mget.js b/service/mget.js index 936ec8bd..a878e8cb 100644 --- a/service/mget.js +++ b/service/mget.js @@ -12,7 +12,6 @@ **/ var peliasLogger = require( 'pelias-logger' ).get( 'service/mget' ); -var microtime = require( 'microtime' ); function service( backend, query, cb ){ @@ -22,11 +21,9 @@ function service( backend, query, cb ){ docs: query } }; - - var startTime = microtime.nowDouble(); + // query new backend backend().client.mget( cmd, function( err, data ){ - peliasLogger.verbose( 'time elasticsearch query took:', microtime.nowDouble() - startTime ); // handle backend errors if( err ){ return cb( err ); } @@ -39,7 +36,7 @@ function service( backend, query, cb ){ // remove docs not actually found return doc.found; - + }).map( function( doc ){ // map metadata in to _source so we diff --git a/service/search.js b/service/search.js index 1e77f69f..c5aad5a9 100644 --- a/service/search.js +++ b/service/search.js @@ -6,14 +6,11 @@ **/ var peliasLogger = require( 'pelias-logger' ).get( 'service/search' ); -var microtime = require( 'microtime' ); function service( backend, cmd, cb ){ - - var startTime = microtime.nowDouble(); + // query new backend backend().client.search( cmd, function( err, data ){ - peliasLogger.verbose( 'time elasticsearch query took:', microtime.nowDouble() - startTime ); // handle backend errors if( err ){ return cb( err ); } From a2090241439ee6ba2ecbb548d8316209fd6cb74e Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Tue, 29 Sep 2015 19:26:08 +0200 Subject: [PATCH 4/4] remove cache control headers --- middleware/404.js | 2 +- middleware/408.js | 2 +- middleware/500.js | 2 +- middleware/headers.js | 2 +- test/ciao/404.coffee | 2 +- test/ciao/index.coffee | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/middleware/404.js b/middleware/404.js index e90ed3c3..85d81c70 100644 --- a/middleware/404.js +++ b/middleware/404.js @@ -1,7 +1,7 @@ // handle not found errors function middleware(req, res) { - res.header('Cache-Control','public,max-age=300'); // 5 minute cache + res.header('Cache-Control','public'); res.status(404).json({ error: 'not found: invalid path' }); } diff --git a/middleware/408.js b/middleware/408.js index ffbb6066..8e57eb28 100644 --- a/middleware/408.js +++ b/middleware/408.js @@ -1,7 +1,7 @@ // handle time out errors function middleware(err, req, res, next) { - res.header('Cache-Control','no-cache'); + res.header('Cache-Control','public'); var error = (err && err.message) ? err.message : err; if( res.statusCode === 408 || (error.toLowerCase().indexOf('request timeout') !== -1) ){ diff --git a/middleware/500.js b/middleware/500.js index cc3f8325..92acea60 100644 --- a/middleware/500.js +++ b/middleware/500.js @@ -3,7 +3,7 @@ var logger = require( 'pelias-logger' ).get( 'middleware-500' ); // handle application errors function middleware(err, req, res, next) { logger.error( 'Error: `%s`. Stack trace: `%s`.', err, err.stack ); - res.header('Cache-Control','no-cache'); + res.header('Cache-Control','public'); var error = (err && err.message) ? err.message : err; if( res.statusCode < 400 ){ res.status(500); } diff --git a/middleware/headers.js b/middleware/headers.js index 4b40dd21..a747be33 100644 --- a/middleware/headers.js +++ b/middleware/headers.js @@ -3,7 +3,7 @@ var pkg = require('../package'); function middleware(req, res, next){ res.header('Charset','utf8'); - res.header('Cache-Control','public,max-age=60'); + res.header('Cache-Control','public'); res.header('Server', 'Pelias/'+pkg.version); res.header('X-Powered-By', 'mapzen'); next(); diff --git a/test/ciao/404.coffee b/test/ciao/404.coffee index 7a22618d..9f6a9c3c 100644 --- a/test/ciao/404.coffee +++ b/test/ciao/404.coffee @@ -9,7 +9,7 @@ response.statusCode.should.be.equal 404 response.should.have.header 'Content-Type','application/json; charset=utf-8' #? cache-control header correctly set -response.should.have.header 'Cache-Control','public,max-age=300' +response.should.have.header 'Cache-Control','public' #? should respond in json with server info should.exist json diff --git a/test/ciao/index.coffee b/test/ciao/index.coffee index 8dd9da90..aac96fd9 100644 --- a/test/ciao/index.coffee +++ b/test/ciao/index.coffee @@ -12,7 +12,7 @@ response.should.have.header 'Content-Type','text/html; charset=utf-8' response.should.have.header 'Charset','utf8' #? cache-control header correctly set -response.should.have.header 'Cache-Control','public,max-age=60' +response.should.have.header 'Cache-Control','public' #? server header correctly set response.should.have.header 'Server'