From a4b4e286d30e7cb1903ec8d14cae34b9595bbb8c Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Mon, 9 Jan 2017 22:56:51 -0500 Subject: [PATCH 01/43] replaced map of singletons in backend with single singleton --- service/mget.js | 2 +- service/search.js | 2 +- src/backend.js | 17 ++++------------- test/unit/mock/backend.js | 2 +- 4 files changed, 7 insertions(+), 16 deletions(-) diff --git a/service/mget.js b/service/mget.js index c709f28e..681582e3 100644 --- a/service/mget.js +++ b/service/mget.js @@ -23,7 +23,7 @@ function service( backend, query, cb ){ }; // query new backend - backend().client.mget( cmd, function( err, data ){ + backend.client.mget( cmd, function( err, data ){ // log total ms elasticsearch reported the query took to execute if( data && data.took ){ diff --git a/service/search.js b/service/search.js index 8e12b69e..1776fb8c 100644 --- a/service/search.js +++ b/service/search.js @@ -10,7 +10,7 @@ var logger = require( 'pelias-logger' ).get( 'api' ); function service( backend, cmd, cb ){ // query new backend - backend().client.search( cmd, function( err, data ){ + backend.client.search( cmd, function( err, data ){ // log total ms elasticsearch reported the query took to execute if( data && data.took ){ diff --git a/src/backend.js b/src/backend.js index c983dbba..e99ca5d2 100644 --- a/src/backend.js +++ b/src/backend.js @@ -1,14 +1,5 @@ -var config = require( 'pelias-config' ).generate().esclient; -var Backend = require('geopipes-elasticsearch-backend'), - client = require('elasticsearch').Client(config), - backends = {}; +const config = require( 'pelias-config' ).generate().esclient; +const Backend = require('geopipes-elasticsearch-backend'); +const client = require('elasticsearch').Client(config); -function getBackend( index, type ){ - var key = ( index + ':' + type ); - if( !backends[key] ){ - backends[key] = new Backend( client, index, type ); - } - return backends[key]; -} - -module.exports = getBackend; +module.exports = new Backend(client); diff --git a/test/unit/mock/backend.js b/test/unit/mock/backend.js index 739ed2cb..9292fe33 100644 --- a/test/unit/mock/backend.js +++ b/test/unit/mock/backend.js @@ -89,7 +89,7 @@ function setup( key, cmdCb ){ } }; } - return backend; + return backend(); } function mgetEnvelope( options ){ From e35ba281375a76bd4bef2116a07e400d2cbe0754 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 10 Jan 2017 00:00:06 -0500 Subject: [PATCH 02/43] add proxyquire to tests for pelias-logger to swallow some error logging --- service/mget.js | 2 +- service/search.js | 2 +- test/unit/middleware/parseBBox.js | 16 +++++++++ test/unit/run.js | 60 +++++++++++++++---------------- test/unit/service/mget.js | 39 ++++++++++++++++---- test/unit/service/search.js | 33 ++++++++++++++--- 6 files changed, 109 insertions(+), 43 deletions(-) diff --git a/service/mget.js b/service/mget.js index 681582e3..99576176 100644 --- a/service/mget.js +++ b/service/mget.js @@ -32,7 +32,7 @@ function service( backend, query, cb ){ // handle backend errors if( err ){ - logger.error( 'backend error', err ); + logger.error( `backend error ${err}`); return cb( err ); } diff --git a/service/search.js b/service/search.js index 1776fb8c..82e13dc1 100644 --- a/service/search.js +++ b/service/search.js @@ -19,7 +19,7 @@ function service( backend, cmd, cb ){ // handle backend errors if( err ){ - logger.error( 'backend error', err ); + logger.error( `backend error ${err}` ); return cb( err ); } diff --git a/test/unit/middleware/parseBBox.js b/test/unit/middleware/parseBBox.js index 4e2cd802..78186805 100644 --- a/test/unit/middleware/parseBBox.js +++ b/test/unit/middleware/parseBBox.js @@ -1,5 +1,7 @@ var parseBBox = require('../../../middleware/parseBBox')(); +const proxyquire = require('proxyquire').noCallThru(); + module.exports.tests = {}; module.exports.tests.computeDistance = function(test, common) { @@ -46,6 +48,20 @@ module.exports.tests.computeDistance = function(test, common) { ] }; + const parseBBox = proxyquire('../../../middleware/parseBBox', { + 'pelias-logger': { + get: () => { + return { + error: (msg1, msg2) => { + t.equals(msg1, 'Invalid bounding_box json string:'); + t.deepEquals(msg2, { bounding_box: 'garbage json' }); + } + }; + + } + } + })(); + parseBBox({}, res, function () { t.deepEquals(res, expected, 'correct bounding_box'); t.end(); diff --git a/test/unit/run.js b/test/unit/run.js index 017492b5..31e64f1e 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -11,8 +11,8 @@ var common = { var tests = [ require('./app'), require('./controller/index'), - require('./controller/place'), - require('./controller/search'), + // require('./controller/place'), + // require('./controller/search'), require('./helper/diffPlaces'), require('./helper/geojsonify'), require('./helper/logging'), @@ -41,36 +41,36 @@ var tests = [ require('./query/search_original'), require('./query/structured_geocoding'), require('./query/text_parser'), - require('./sanitizer/_boundary_country'), - require('./sanitizer/_flag_bool'), - require('./sanitizer/_geo_common'), - require('./sanitizer/_geo_reverse'), - require('./sanitizer/_groups'), - require('./sanitizer/_ids'), - require('./sanitizer/_iso2_to_iso3'), - require('./sanitizer/_layers'), - require('./sanitizer/_city_name_standardizer'), - require('./sanitizer/_single_scalar_parameters'), - require('./sanitizer/_size'), - require('./sanitizer/_sources'), - require('./sanitizer/_sources_and_layers'), - require('./sanitizer/_synthesize_analysis'), - require('./sanitizer/_text'), - require('./sanitizer/_text_addressit'), - require('./sanitizer/_tokenizer'), - require('./sanitizer/_deprecate_quattroshapes'), - require('./sanitizer/_categories'), - require('./sanitizer/nearby'), + // require('./sanitizer/_boundary_country'), + // require('./sanitizer/_flag_bool'), + // require('./sanitizer/_geo_common'), + // require('./sanitizer/_geo_reverse'), + // require('./sanitizer/_groups'), + // require('./sanitizer/_ids'), + // require('./sanitizer/_iso2_to_iso3'), + // require('./sanitizer/_layers'), + // require('./sanitizer/_city_name_standardizer'), + // require('./sanitizer/_single_scalar_parameters'), + // require('./sanitizer/_size'), + // require('./sanitizer/_sources'), + // require('./sanitizer/_sources_and_layers'), + // require('./sanitizer/_synthesize_analysis'), + // require('./sanitizer/_text'), + // require('./sanitizer/_text_addressit'), + // require('./sanitizer/_tokenizer'), + // require('./sanitizer/_deprecate_quattroshapes'), + // require('./sanitizer/_categories'), + // require('./sanitizer/nearby'), require('./src/backend'), require('./src/configValidation'), - require('./sanitizer/autocomplete'), - require('./sanitizer/structured_geocoding'), - require('./sanitizer/place'), - require('./sanitizer/reverse'), - require('./sanitizer/sanitizeAll'), - require('./sanitizer/search'), - require('./sanitizer/search_fallback'), - require('./sanitizer/wrap'), + // require('./sanitizer/autocomplete'), + // require('./sanitizer/structured_geocoding'), + // require('./sanitizer/place'), + // require('./sanitizer/reverse'), + // require('./sanitizer/sanitizeAll'), + // require('./sanitizer/search'), + // require('./sanitizer/search_fallback'), + // require('./sanitizer/wrap'), require('./service/mget'), require('./service/search') ]; diff --git a/test/unit/service/mget.js b/test/unit/service/mget.js index e2723e6e..aea0a62d 100644 --- a/test/unit/service/mget.js +++ b/test/unit/service/mget.js @@ -1,17 +1,28 @@ -var setup = require('../../../service/mget'), +var service = require('../../../service/mget'), mockBackend = require('../mock/backend'); +const proxyquire = require('proxyquire').noCallThru(); + module.exports.tests = {}; module.exports.tests.interface = function(test, common) { test('valid interface', function(t) { - t.equal(typeof setup, 'function', 'setup is a function'); + var service = proxyquire('../../../service/mget', { + 'pelias-logger': { + get: (section) => { + t.equal(section, 'api'); + } + } + + }); + + t.equal(typeof service, 'function', 'service is a function'); t.end(); }); }; -// functionally test service +// functionally test service module.exports.tests.functional_success = function(test, common) { var expected = [ @@ -21,7 +32,7 @@ module.exports.tests.functional_success = function(test, common) { center_point: { lat: 100.1, lon: -50.5 }, name: { default: 'test name1' }, parent: { country: ['country1'], region: ['state1'], county: ['city1'] } - }, + }, { _id: 'myid2', _type: 'mytype2', value: 2, @@ -35,7 +46,7 @@ module.exports.tests.functional_success = function(test, common) { var backend = mockBackend( 'client/mget/ok/1', function( cmd ){ t.deepEqual(cmd, { body: { docs: [ { _id: 123, _index: 'pelias', _type: 'a' } ] } }, 'correct backend command'); }); - setup( backend, [ { _id: 123, _index: 'pelias', _type: 'a' } ], function(err, data) { + service( backend, [ { _id: 123, _index: 'pelias', _type: 'a' } ], function(err, data) { t.true(Array.isArray(data), 'returns an array'); data.forEach(function(d) { t.true(typeof d === 'object', 'valid object'); @@ -62,7 +73,21 @@ module.exports.tests.functional_failure = function(test, common) { t.notDeepEqual(cmd, { body: { docs: [ { _id: 123, _index: 'pelias', _type: 'a' } ] } }, 'incorrect backend command'); }); invalid_queries.forEach(function(query) { - setup( backend, [ query ], function(err, data) { + // mock out pelias-logger so we can assert what's being logged + var service = proxyquire('../../../service/mget', { + 'pelias-logger': { + get: () => { + return { + error: (msg) => { + t.equal(msg, 'backend error a backend error occurred'); + } + }; + } + } + + }); + + service( backend, [ query ], function(err, data) { t.equal(err, 'a backend error occurred','error passed to errorHandler'); t.equal(data, undefined, 'data is undefined'); }); @@ -81,4 +106,4 @@ module.exports.all = function (tape, common) { for( var testCase in module.exports.tests ){ module.exports.tests[testCase](test, common); } -}; \ No newline at end of file +}; diff --git a/test/unit/service/search.js b/test/unit/service/search.js index cf72b75e..7167ed6c 100644 --- a/test/unit/service/search.js +++ b/test/unit/service/search.js @@ -1,14 +1,25 @@ -var setup = require('../../../service/search'), +var service = require('../../../service/search'), mockBackend = require('../mock/backend'); +const proxyquire = require('proxyquire').noCallThru(); + var example_valid_es_query = { body: { a: 'b' }, index: 'pelias' }; module.exports.tests = {}; module.exports.tests.interface = function(test, common) { test('valid interface', function(t) { - t.equal(typeof setup, 'function', 'setup is a function'); + var service = proxyquire('../../../service/mget', { + 'pelias-logger': { + get: (section) => { + t.equal(section, 'api'); + } + } + + }); + + t.equal(typeof service, 'function', 'service is a function'); t.end(); }); }; @@ -45,7 +56,7 @@ module.exports.tests.functional_success = function(test, common) { 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, meta) { + service( 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'); @@ -71,7 +82,21 @@ module.exports.tests.functional_failure = function(test, common) { t.notDeepEqual(cmd, example_valid_es_query, 'incorrect backend command'); }); invalid_queries.forEach(function(query) { - setup( backend, [ query ], function(err, data) { + // mock out pelias-logger so we can assert what's being logged + var service = proxyquire('../../../service/search', { + 'pelias-logger': { + get: () => { + return { + error: (msg) => { + t.equal(msg, 'backend error a backend error occurred'); + } + }; + } + } + + }); + + service( backend, [ query ], function(err, data) { t.equal(err, 'a backend error occurred','error passed to errorHandler'); t.equal(data, undefined, 'data is undefined'); }); From de2ff8cad75fd180804dc802cb0508fdea3ee390 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 10 Jan 2017 00:05:52 -0500 Subject: [PATCH 03/43] removed debug --- controller/place.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/controller/place.js b/controller/place.js index ebf9df18..7d223201 100644 --- a/controller/place.js +++ b/controller/place.js @@ -25,8 +25,6 @@ function setup( config, backend ){ logger.debug( '[ES req]', query ); service.mget( backend, query, function( err, docs ) { - console.log('err:' + err); - // error handler if( err ){ req.errors.push( err ); From 0bd983e2cd247e9a45a74b4a828ec7a8dfdd9241 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 13 Jan 2017 11:30:50 -0500 Subject: [PATCH 04/43] removed geopipes-elasticsearch-backend dependency --- package.json | 1 - src/backend.js | 5 +++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 08549887..0bf5d5c6 100644 --- a/package.json +++ b/package.json @@ -46,7 +46,6 @@ "geojson": "^0.4.0", "geojson-extent": "^0.3.1", "geolib": "^2.0.18", - "geopipes-elasticsearch-backend": "^0.2.0", "iso3166-1": "^0.2.3", "joi": "^10.1.0", "lodash": "^4.5.0", diff --git a/src/backend.js b/src/backend.js index e99ca5d2..44059d5b 100644 --- a/src/backend.js +++ b/src/backend.js @@ -1,5 +1,6 @@ const config = require( 'pelias-config' ).generate().esclient; -const Backend = require('geopipes-elasticsearch-backend'); const client = require('elasticsearch').Client(config); -module.exports = new Backend(client); +module.exports = { + client: client +}; From 511fbe41b4acafb5e17ba4e376b243394e2f01d6 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 13 Jan 2017 11:49:12 -0500 Subject: [PATCH 05/43] removed unneeded require --- routes/v1.js | 1 - 1 file changed, 1 deletion(-) diff --git a/routes/v1.js b/routes/v1.js index 89b86613..4f550721 100644 --- a/routes/v1.js +++ b/routes/v1.js @@ -1,4 +1,3 @@ -var express = require('express'); var Router = require('express').Router; var reverseQuery = require('../query/reverse'); From 5be3765ecede1a8690a85cfb7a4790b497c05a92 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 13 Jan 2017 12:00:44 -0500 Subject: [PATCH 06/43] removed debug --- sanitizer/search_fallback.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sanitizer/search_fallback.js b/sanitizer/search_fallback.js index d483d4e3..2c5d0c2a 100644 --- a/sanitizer/search_fallback.js +++ b/sanitizer/search_fallback.js @@ -19,7 +19,7 @@ module.exports.middleware = function( req, res, next ){ // log the query that caused a fallback since libpostal+new-queries didn't return anything if (req.path === '/v1/search') { var queryText = logging.isDNT(req) ? '[text removed]' : req.clean.text; - logger.info(queryText); + // logger.info(queryText); } sanitize( req, function( err, clean ){ From 948c6a9de56f26419b359955cd1809d169234335 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 13 Jan 2017 12:06:37 -0500 Subject: [PATCH 07/43] enable all unit tests --- test/unit/run.js | 56 ++++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/test/unit/run.js b/test/unit/run.js index 31e64f1e..817f26dd 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -41,36 +41,36 @@ var tests = [ require('./query/search_original'), require('./query/structured_geocoding'), require('./query/text_parser'), - // require('./sanitizer/_boundary_country'), - // require('./sanitizer/_flag_bool'), - // require('./sanitizer/_geo_common'), - // require('./sanitizer/_geo_reverse'), - // require('./sanitizer/_groups'), - // require('./sanitizer/_ids'), - // require('./sanitizer/_iso2_to_iso3'), - // require('./sanitizer/_layers'), - // require('./sanitizer/_city_name_standardizer'), - // require('./sanitizer/_single_scalar_parameters'), - // require('./sanitizer/_size'), - // require('./sanitizer/_sources'), - // require('./sanitizer/_sources_and_layers'), - // require('./sanitizer/_synthesize_analysis'), - // require('./sanitizer/_text'), - // require('./sanitizer/_text_addressit'), - // require('./sanitizer/_tokenizer'), - // require('./sanitizer/_deprecate_quattroshapes'), - // require('./sanitizer/_categories'), - // require('./sanitizer/nearby'), + require('./sanitizer/_boundary_country'), + require('./sanitizer/_flag_bool'), + require('./sanitizer/_geo_common'), + require('./sanitizer/_geo_reverse'), + require('./sanitizer/_groups'), + require('./sanitizer/_ids'), + require('./sanitizer/_iso2_to_iso3'), + require('./sanitizer/_layers'), + require('./sanitizer/_city_name_standardizer'), + require('./sanitizer/_single_scalar_parameters'), + require('./sanitizer/_size'), + require('./sanitizer/_sources'), + require('./sanitizer/_sources_and_layers'), + require('./sanitizer/_synthesize_analysis'), + require('./sanitizer/_text'), + require('./sanitizer/_text_addressit'), + require('./sanitizer/_tokenizer'), + require('./sanitizer/_deprecate_quattroshapes'), + require('./sanitizer/_categories'), + require('./sanitizer/nearby'), require('./src/backend'), require('./src/configValidation'), - // require('./sanitizer/autocomplete'), - // require('./sanitizer/structured_geocoding'), - // require('./sanitizer/place'), - // require('./sanitizer/reverse'), - // require('./sanitizer/sanitizeAll'), - // require('./sanitizer/search'), - // require('./sanitizer/search_fallback'), - // require('./sanitizer/wrap'), + require('./sanitizer/autocomplete'), + require('./sanitizer/structured_geocoding'), + require('./sanitizer/place'), + require('./sanitizer/reverse'), + require('./sanitizer/sanitizeAll'), + require('./sanitizer/search'), + require('./sanitizer/search_fallback'), + require('./sanitizer/wrap'), require('./service/mget'), require('./service/search') ]; From 00234265e6c5b11a25caaa7e435ed1a9ef2d26f8 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 13 Jan 2017 12:07:06 -0500 Subject: [PATCH 08/43] always inject backend instead of require'ing inside --- controller/place.js | 4 ---- controller/search.js | 5 ----- routes/v1.js | 15 ++++++++------- 3 files changed, 8 insertions(+), 16 deletions(-) diff --git a/controller/place.js b/controller/place.js index 7d223201..77562f70 100644 --- a/controller/place.js +++ b/controller/place.js @@ -2,10 +2,6 @@ var service = { mget: require('../service/mget') }; var logger = require('pelias-logger').get('api'); function setup( config, backend ){ - - // allow overriding of dependencies - backend = backend || require('../src/backend'); - function controller( req, res, next ){ // do not run controller when a request diff --git a/controller/search.js b/controller/search.js index f4aa3ab5..b81f9c4b 100644 --- a/controller/search.js +++ b/controller/search.js @@ -5,11 +5,6 @@ var logger = require('pelias-logger').get('api'); var logging = require( '../helper/logging' ); function setup( config, backend, query ){ - - // allow overriding of dependencies - backend = backend || require('../src/backend'); - query = query || require('../query/search'); - function controller( req, res, next ){ // do not run controller when a request // validation error has occurred. diff --git a/routes/v1.js b/routes/v1.js index 4f550721..bb38498f 100644 --- a/routes/v1.js +++ b/routes/v1.js @@ -1,5 +1,6 @@ var Router = require('express').Router; var reverseQuery = require('../query/reverse'); +var backend = require('../src/backend'); /** ----------------------- sanitizers ----------------------- **/ var sanitizers = { @@ -77,9 +78,9 @@ function addRoutes(app, peliasConfig) { // 2nd parameter is `backend` which gets initialized internally // 3rd parameter is which query module to use, use fallback/geodisambiguation // first, then use original search strategy if first query didn't return anything - controllers.search(peliasConfig, undefined, queries.libpostal), + controllers.search(peliasConfig, backend, queries.libpostal), sanitizers.search_fallback.middleware, - controllers.search(peliasConfig, undefined, queries.fallback_to_old_prod), + controllers.search(peliasConfig, backend, queries.fallback_to_old_prod), postProc.trimByGranularity(), postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig), @@ -97,7 +98,7 @@ function addRoutes(app, peliasConfig) { structured: createRouter([ sanitizers.structured_geocoding.middleware, middleware.calcSize(), - controllers.search(peliasConfig, undefined, queries.structured_geocoding), + controllers.search(peliasConfig, backend, queries.structured_geocoding), postProc.trimByGranularityStructured(), postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig), @@ -114,7 +115,7 @@ function addRoutes(app, peliasConfig) { ]), autocomplete: createRouter([ sanitizers.autocomplete.middleware, - controllers.search(peliasConfig, null, require('../query/autocomplete')), + controllers.search(peliasConfig, backend, require('../query/autocomplete')), postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig), postProc.dedupe(), @@ -130,7 +131,7 @@ function addRoutes(app, peliasConfig) { reverse: createRouter([ sanitizers.reverse.middleware, middleware.calcSize(), - controllers.search(peliasConfig, undefined, reverseQuery), + controllers.search(peliasConfig, backend, reverseQuery), postProc.distances('point.'), // reverse confidence scoring depends on distance from origin // so it must be calculated first @@ -148,7 +149,7 @@ function addRoutes(app, peliasConfig) { nearby: createRouter([ sanitizers.nearby.middleware, middleware.calcSize(), - controllers.search(peliasConfig, undefined, reverseQuery), + controllers.search(peliasConfig, backend, reverseQuery), postProc.distances('point.'), // reverse confidence scoring depends on distance from origin // so it must be calculated first @@ -165,7 +166,7 @@ function addRoutes(app, peliasConfig) { ]), place: createRouter([ sanitizers.place.middleware, - controllers.place(peliasConfig), + controllers.place(peliasConfig, backend), postProc.accuracy(), postProc.localNamingConventions(), postProc.renamePlacenames(), From 53283eb1780bbf812f863d3ba1b85809725cb034 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 13 Jan 2017 12:09:00 -0500 Subject: [PATCH 09/43] actually remove debug this time --- sanitizer/search_fallback.js | 1 - 1 file changed, 1 deletion(-) diff --git a/sanitizer/search_fallback.js b/sanitizer/search_fallback.js index 2c5d0c2a..27f2bc9d 100644 --- a/sanitizer/search_fallback.js +++ b/sanitizer/search_fallback.js @@ -19,7 +19,6 @@ module.exports.middleware = function( req, res, next ){ // log the query that caused a fallback since libpostal+new-queries didn't return anything if (req.path === '/v1/search') { var queryText = logging.isDNT(req) ? '[text removed]' : req.clean.text; - // logger.info(queryText); } sanitize( req, function( err, clean ){ From 8c249c26f48841aa24bc84b9c5b885c6eb2c01d2 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 13 Jan 2017 12:24:30 -0500 Subject: [PATCH 10/43] initialize esclient in routes/v1, eliminating need for src/backend --- controller/place.js | 4 ++-- controller/search.js | 8 +++---- routes/v1.js | 19 ++++++++-------- service/mget.js | 12 +++++----- service/search.js | 10 ++++----- src/backend.js | 6 ----- test/unit/controller/place.js | 2 +- test/unit/controller/search.js | 2 +- test/unit/mock/backend.js | 28 +++++++++++------------- test/unit/run.js | 5 ++--- test/unit/service/mget.js | 4 ++-- test/unit/service/search.js | 4 ++-- test/unit/src/backend.js | 40 ---------------------------------- 13 files changed, 48 insertions(+), 96 deletions(-) delete mode 100644 src/backend.js delete mode 100644 test/unit/src/backend.js diff --git a/controller/place.js b/controller/place.js index 77562f70..28523665 100644 --- a/controller/place.js +++ b/controller/place.js @@ -1,7 +1,7 @@ var service = { mget: require('../service/mget') }; var logger = require('pelias-logger').get('api'); -function setup( config, backend ){ +function setup( config, esclient ){ function controller( req, res, next ){ // do not run controller when a request @@ -20,7 +20,7 @@ function setup( config, backend ){ logger.debug( '[ES req]', query ); - service.mget( backend, query, function( err, docs ) { + service.mget( esclient, query, function( err, docs ) { // error handler if( err ){ req.errors.push( err ); diff --git a/controller/search.js b/controller/search.js index b81f9c4b..8c845938 100644 --- a/controller/search.js +++ b/controller/search.js @@ -4,7 +4,7 @@ var service = { search: require('../service/search') }; var logger = require('pelias-logger').get('api'); var logging = require( '../helper/logging' ); -function setup( config, backend, query ){ +function setup( config, esclient, query ){ function controller( req, res, next ){ // do not run controller when a request // validation error has occurred. @@ -33,7 +33,7 @@ function setup( config, backend, query ){ return next(); } - // backend command + // elasticsearch command var cmd = { index: config.indexName, searchType: 'dfs_query_then_fetch', @@ -42,8 +42,8 @@ function setup( config, backend, query ){ logger.debug( '[ES req]', cmd ); - // query backend - service.search( backend, cmd, function( err, docs, meta ){ + // query elasticsearch + service.search( esclient, cmd, function( err, docs, meta ){ // error handler if( err ){ diff --git a/routes/v1.js b/routes/v1.js index bb38498f..e7e4d941 100644 --- a/routes/v1.js +++ b/routes/v1.js @@ -1,6 +1,8 @@ var Router = require('express').Router; var reverseQuery = require('../query/reverse'); -var backend = require('../src/backend'); + +const config = require( 'pelias-config' ).generate(); +const esclient = require('elasticsearch').Client(config.esclient); /** ----------------------- sanitizers ----------------------- **/ var sanitizers = { @@ -75,12 +77,11 @@ function addRoutes(app, peliasConfig) { search: createRouter([ sanitizers.search.middleware, middleware.calcSize(), - // 2nd parameter is `backend` which gets initialized internally // 3rd parameter is which query module to use, use fallback/geodisambiguation // first, then use original search strategy if first query didn't return anything - controllers.search(peliasConfig, backend, queries.libpostal), + controllers.search(peliasConfig, esclient, queries.libpostal), sanitizers.search_fallback.middleware, - controllers.search(peliasConfig, backend, queries.fallback_to_old_prod), + controllers.search(peliasConfig, esclient, queries.fallback_to_old_prod), postProc.trimByGranularity(), postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig), @@ -98,7 +99,7 @@ function addRoutes(app, peliasConfig) { structured: createRouter([ sanitizers.structured_geocoding.middleware, middleware.calcSize(), - controllers.search(peliasConfig, backend, queries.structured_geocoding), + controllers.search(peliasConfig, esclient, queries.structured_geocoding), postProc.trimByGranularityStructured(), postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig), @@ -115,7 +116,7 @@ function addRoutes(app, peliasConfig) { ]), autocomplete: createRouter([ sanitizers.autocomplete.middleware, - controllers.search(peliasConfig, backend, require('../query/autocomplete')), + controllers.search(peliasConfig, esclient, require('../query/autocomplete')), postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig), postProc.dedupe(), @@ -131,7 +132,7 @@ function addRoutes(app, peliasConfig) { reverse: createRouter([ sanitizers.reverse.middleware, middleware.calcSize(), - controllers.search(peliasConfig, backend, reverseQuery), + controllers.search(peliasConfig, esclient, reverseQuery), postProc.distances('point.'), // reverse confidence scoring depends on distance from origin // so it must be calculated first @@ -149,7 +150,7 @@ function addRoutes(app, peliasConfig) { nearby: createRouter([ sanitizers.nearby.middleware, middleware.calcSize(), - controllers.search(peliasConfig, backend, reverseQuery), + controllers.search(peliasConfig, esclient, reverseQuery), postProc.distances('point.'), // reverse confidence scoring depends on distance from origin // so it must be calculated first @@ -166,7 +167,7 @@ function addRoutes(app, peliasConfig) { ]), place: createRouter([ sanitizers.place.middleware, - controllers.place(peliasConfig, backend), + controllers.place(peliasConfig, esclient), postProc.accuracy(), postProc.localNamingConventions(), postProc.renamePlacenames(), diff --git a/service/mget.js b/service/mget.js index 99576176..b68a5bed 100644 --- a/service/mget.js +++ b/service/mget.js @@ -13,26 +13,26 @@ var logger = require( 'pelias-logger' ).get( 'api' ); -function service( backend, query, cb ){ +function service( esclient, query, cb ){ - // backend command + // elasticsearch command var cmd = { body: { docs: query } }; - // query new backend - backend.client.mget( cmd, function( err, data ){ + // query elasticsearch + esclient.mget( cmd, function( err, data ){ // log total ms elasticsearch reported the query took to execute if( data && data.took ){ logger.verbose( 'time elasticsearch reported:', data.took / 1000 ); } - // handle backend errors + // handle elasticsearch errors if( err ){ - logger.error( `backend error ${err}`); + logger.error( `elasticsearch error ${err}`); return cb( err ); } diff --git a/service/search.js b/service/search.js index 82e13dc1..9453aaa2 100644 --- a/service/search.js +++ b/service/search.js @@ -7,19 +7,19 @@ var logger = require( 'pelias-logger' ).get( 'api' ); -function service( backend, cmd, cb ){ +function service( esclient, cmd, cb ){ - // query new backend - backend.client.search( cmd, function( err, data ){ + // query elasticsearch + esclient.search( cmd, function( err, data ){ // log total ms elasticsearch reported the query took to execute if( data && data.took ){ logger.verbose( 'time elasticsearch reported:', data.took / 1000 ); } - // handle backend errors + // handle elasticsearch errors if( err ){ - logger.error( `backend error ${err}` ); + logger.error( `elasticsearch error ${err}` ); return cb( err ); } diff --git a/src/backend.js b/src/backend.js deleted file mode 100644 index 44059d5b..00000000 --- a/src/backend.js +++ /dev/null @@ -1,6 +0,0 @@ -const config = require( 'pelias-config' ).generate().esclient; -const client = require('elasticsearch').Client(config); - -module.exports = { - client: client -}; diff --git a/test/unit/controller/place.js b/test/unit/controller/place.js index 0376eec6..216b99f3 100644 --- a/test/unit/controller/place.js +++ b/test/unit/controller/place.js @@ -110,7 +110,7 @@ module.exports.tests.functional_failure = function(test, common) { var controller = setup( fakeDefaultConfig, backend ); var req = { clean: { ids: [ {'id' : 123, layers: [ 'b' ] } ] }, errors: [], warnings: [] }; var next = function( message ){ - t.equal(req.errors[0],'a backend error occurred','error passed to errorHandler'); + t.equal(req.errors[0],'an elasticsearch error occurred','error passed to errorHandler'); t.end(); }; controller(req, undefined, next ); diff --git a/test/unit/controller/search.js b/test/unit/controller/search.js index 86f07b97..2b87990d 100644 --- a/test/unit/controller/search.js +++ b/test/unit/controller/search.js @@ -151,7 +151,7 @@ module.exports.tests.functional_failure = function(test, common) { var controller = setup( fakeDefaultConfig, backend, mockQuery() ); var req = { clean: { a: 'b' }, errors: [], warnings: [] }; var next = function(){ - t.equal(req.errors[0],'a backend error occurred'); + t.equal(req.errors[0],'an elasticsearch error occurred'); t.end(); }; controller(req, undefined, next ); diff --git a/test/unit/mock/backend.js b/test/unit/mock/backend.js index 9292fe33..104f84dc 100644 --- a/test/unit/mock/backend.js +++ b/test/unit/mock/backend.js @@ -4,7 +4,7 @@ responses['client/suggest/ok/1'] = function( cmd, cb ){ return cb( undefined, suggestEnvelope([ { score: 1, text: 'mocktype:mockid1' } ], [ { score: 2, text: 'mocktype:mockid2' } ]) ); }; responses['client/suggest/fail/1'] = function( cmd, cb ){ - return cb( 'a backend error occurred' ); + return cb( 'an elasticsearch error occurred' ); }; responses['client/search/ok/1'] = function( cmd, cb ){ return cb( undefined, searchEnvelope([{ @@ -32,7 +32,7 @@ responses['client/search/ok/1'] = function( cmd, cb ){ }])); }; responses['client/search/fail/1'] = function( cmd, cb ){ - return cb( 'a backend error occurred' ); + return cb( 'an elasticsearch error occurred' ); }; responses['client/search/timeout/1'] = function( cmd, cb) { @@ -73,19 +73,17 @@ responses['client/mget/fail/1'] = responses['client/search/fail/1']; function setup( key, cmdCb ){ function backend( a, b ){ return { - client: { - mget: function( cmd, cb ){ - if( 'function' === typeof cmdCb ){ cmdCb( cmd ); } - return responses[key.indexOf('mget') === -1 ? 'client/mget/ok/1' : key].apply( this, arguments ); - }, - suggest: function( cmd, cb ){ - if( 'function' === typeof cmdCb ){ cmdCb( cmd ); } - return responses[key].apply( this, arguments ); - }, - search: function( cmd, cb ){ - if( 'function' === typeof cmdCb ){ cmdCb( cmd ); } - return responses[key].apply( this, arguments ); - } + mget: function( cmd, cb ){ + if( 'function' === typeof cmdCb ){ cmdCb( cmd ); } + return responses[key.indexOf('mget') === -1 ? 'client/mget/ok/1' : key].apply( this, arguments ); + }, + suggest: function( cmd, cb ){ + if( 'function' === typeof cmdCb ){ cmdCb( cmd ); } + return responses[key].apply( this, arguments ); + }, + search: function( cmd, cb ){ + if( 'function' === typeof cmdCb ){ cmdCb( cmd ); } + return responses[key].apply( this, arguments ); } }; } diff --git a/test/unit/run.js b/test/unit/run.js index 817f26dd..cc89a699 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -11,8 +11,8 @@ var common = { var tests = [ require('./app'), require('./controller/index'), - // require('./controller/place'), - // require('./controller/search'), + require('./controller/place'), + require('./controller/search'), require('./helper/diffPlaces'), require('./helper/geojsonify'), require('./helper/logging'), @@ -61,7 +61,6 @@ var tests = [ require('./sanitizer/_deprecate_quattroshapes'), require('./sanitizer/_categories'), require('./sanitizer/nearby'), - require('./src/backend'), require('./src/configValidation'), require('./sanitizer/autocomplete'), require('./sanitizer/structured_geocoding'), diff --git a/test/unit/service/mget.js b/test/unit/service/mget.js index aea0a62d..6d347111 100644 --- a/test/unit/service/mget.js +++ b/test/unit/service/mget.js @@ -79,7 +79,7 @@ module.exports.tests.functional_failure = function(test, common) { get: () => { return { error: (msg) => { - t.equal(msg, 'backend error a backend error occurred'); + t.equal(msg, 'elasticsearch error an elasticsearch error occurred'); } }; } @@ -88,7 +88,7 @@ module.exports.tests.functional_failure = function(test, common) { }); service( backend, [ query ], function(err, data) { - t.equal(err, 'a backend error occurred','error passed to errorHandler'); + t.equal(err, 'an elasticsearch error occurred','error passed to errorHandler'); t.equal(data, undefined, 'data is undefined'); }); }); diff --git a/test/unit/service/search.js b/test/unit/service/search.js index 7167ed6c..146b9744 100644 --- a/test/unit/service/search.js +++ b/test/unit/service/search.js @@ -88,7 +88,7 @@ module.exports.tests.functional_failure = function(test, common) { get: () => { return { error: (msg) => { - t.equal(msg, 'backend error a backend error occurred'); + t.equal(msg, 'elasticsearch error an elasticsearch error occurred'); } }; } @@ -97,7 +97,7 @@ module.exports.tests.functional_failure = function(test, common) { }); service( backend, [ query ], function(err, data) { - t.equal(err, 'a backend error occurred','error passed to errorHandler'); + t.equal(err, 'an elasticsearch error occurred','error passed to errorHandler'); t.equal(data, undefined, 'data is undefined'); }); }); diff --git a/test/unit/src/backend.js b/test/unit/src/backend.js deleted file mode 100644 index 1b750396..00000000 --- a/test/unit/src/backend.js +++ /dev/null @@ -1,40 +0,0 @@ -var proxyquire = require('proxyquire'); - -var stubConfig = { - generate: function generate() { - return { - esclient: { - hosts: [ - 'http://notLocalhost:9200', - 'http://anotherHost:9200' - ] - } - }; - } -}; - - -module.exports.tests = {}; - -module.exports.tests.config_properly_passed = function(test, common) { - test('Elasticsearch config is properly passed to elasticsearch module', function(t) { - var stubElasticsearchClient = { - Client: function(config) { - t.deepEquals(config.hosts, [ 'http://notLocalhost:9200', 'http://anotherHost:9200' ], 'hosts set correctly' ); - t.end(); - } - }; - - proxyquire('../../../src/backend', { 'pelias-config': stubConfig, 'elasticsearch': stubElasticsearchClient } ); - }); -}; - -module.exports.all = function (tape, common) { - function test(name, testFunction) { - return tape('SANTIZE src/backend ' + name, testFunction); - } - - for( var testCase in module.exports.tests ){ - module.exports.tests[testCase](test, common); - } -}; From 330e7570d7feeed3c8dfdae31062744ec917f2e8 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 13 Jan 2017 12:30:31 -0500 Subject: [PATCH 11/43] added reverse and autocomplete to queries object --- routes/v1.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/routes/v1.js b/routes/v1.js index e7e4d941..933519ef 100644 --- a/routes/v1.js +++ b/routes/v1.js @@ -1,5 +1,4 @@ var Router = require('express').Router; -var reverseQuery = require('../query/reverse'); const config = require( 'pelias-config' ).generate(); const esclient = require('elasticsearch').Client(config.esclient); @@ -32,7 +31,9 @@ var controllers = { var queries = { libpostal: require('../query/search'), fallback_to_old_prod: require('../query/search_original'), - structured_geocoding: require('../query/structured_geocoding') + structured_geocoding: require('../query/structured_geocoding'), + reverse: require('../query/reverse'), + autocomplete: require('../query/autocomplete') }; /** ----------------------- controllers ----------------------- **/ @@ -116,7 +117,7 @@ function addRoutes(app, peliasConfig) { ]), autocomplete: createRouter([ sanitizers.autocomplete.middleware, - controllers.search(peliasConfig, esclient, require('../query/autocomplete')), + controllers.search(peliasConfig, esclient, queries.autocomplete), postProc.distances('focus.point.'), postProc.confidenceScores(peliasConfig), postProc.dedupe(), @@ -132,7 +133,7 @@ function addRoutes(app, peliasConfig) { reverse: createRouter([ sanitizers.reverse.middleware, middleware.calcSize(), - controllers.search(peliasConfig, esclient, reverseQuery), + controllers.search(peliasConfig, esclient, queries.reverse), postProc.distances('point.'), // reverse confidence scoring depends on distance from origin // so it must be calculated first @@ -150,7 +151,7 @@ function addRoutes(app, peliasConfig) { nearby: createRouter([ sanitizers.nearby.middleware, middleware.calcSize(), - controllers.search(peliasConfig, esclient, reverseQuery), + controllers.search(peliasConfig, esclient, queries.reverse), postProc.distances('point.'), // reverse confidence scoring depends on distance from origin // so it must be calculated first From d5d08e8f4539db2a902e3a977e19c665191cda98 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Fri, 13 Jan 2017 14:00:10 -0500 Subject: [PATCH 12/43] moved esclient instantiation to function scope --- routes/v1.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/routes/v1.js b/routes/v1.js index 933519ef..97892a58 100644 --- a/routes/v1.js +++ b/routes/v1.js @@ -1,8 +1,5 @@ var Router = require('express').Router; -const config = require( 'pelias-config' ).generate(); -const esclient = require('elasticsearch').Client(config.esclient); - /** ----------------------- sanitizers ----------------------- **/ var sanitizers = { autocomplete: require('../sanitizer/autocomplete'), @@ -63,6 +60,7 @@ var postProc = { * @param {object} peliasConfig */ function addRoutes(app, peliasConfig) { + const esclient = require('elasticsearch').Client(peliasConfig.esclient); var base = '/v1/'; From 4ed4b8b35799983de63860f5e0457a079beff86f Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Wed, 18 Jan 2017 12:56:42 -0500 Subject: [PATCH 13/43] added retry support for ES requests that timeout --- app.js | 2 +- controller/search.js | 77 +++++++++++++++++++++++++++++++------------- package.json | 2 ++ routes/v1.js | 36 ++++++++++----------- 4 files changed, 76 insertions(+), 41 deletions(-) diff --git a/app.js b/app.js index 2dabb3c9..40c85964 100644 --- a/app.js +++ b/app.js @@ -23,7 +23,7 @@ var legacy = require('./routes/legacy'); legacy.addRoutes(app, peliasConfig.api); var v1 = require('./routes/v1'); -v1.addRoutes(app, peliasConfig.api); +v1.addRoutes(app, peliasConfig); /** ----------------------- error middleware ----------------------- **/ diff --git a/controller/search.js b/controller/search.js index 8c845938..eb324f10 100644 --- a/controller/search.js +++ b/controller/search.js @@ -1,8 +1,11 @@ +'use strict'; + var _ = require('lodash'); var service = { search: require('../service/search') }; var logger = require('pelias-logger').get('api'); var logging = require( '../helper/logging' ); +const retry = require('retry'); function setup( config, esclient, query ){ function controller( req, res, next ){ @@ -33,6 +36,23 @@ function setup( config, esclient, query ){ return next(); } + logger.debug( '[ES req]', cmd ); + + // options for retry + // default number of retries to 3 (seems reasonable) + // factor of 1 means that each retry attempt will esclient requestTimeout + const operationOptions = { + retries: _.get(esclient, 'transport.maxRetries', 3), + factor: 1, + minTimeout: _.get(esclient, 'transport.requestTimeout') + }; + + // prepend the config timeout since the total number of attempts is maxRetries+1 + const timeouts = [operationOptions.minTimeout].concat(retry.timeouts(operationOptions)); + + // setup a new operation + const operation = retry.operation(operationOptions); + // elasticsearch command var cmd = { index: config.indexName, @@ -40,31 +60,44 @@ function setup( config, esclient, query ){ body: renderedQuery.body }; - logger.debug( '[ES req]', cmd ); + operation.attempt((currentAttempt) => { + // query elasticsearch + service.search( esclient, cmd, function( err, docs, meta ){ + // returns true if the operation should be attempted again + // (handles bookkeeping of maxRetries) + if (operation.retry(err)) { + logger.info('request timed out, retrying'); + return; + } - // query elasticsearch - service.search( esclient, cmd, function( err, docs, meta ){ + // error handler + if( err ){ + if (_.isObject(err) && err.message) { + req.errors.push( err.message ); + } else { + req.errors.push( err ); + } + } + // set response data + else { + // log that a retry was successful + // most requests succeed on first attempt so this declutters log files + if (currentAttempt > 1) { + logger.info(`succeeded on retry ${currentAttempt-1}`); + } + + res.data = docs; + res.meta = meta || {}; + // store the query_type for subsequent middleware + res.meta.query_type = renderedQuery.type; - // error handler - if( err ){ - if (_.isObject(err) && err.message) { - req.errors.push( err.message ); - } else { - req.errors.push( err ); + logger.info(`[controller:search] [queryType:${renderedQuery.type}] [es_result_count:` + + (res.data && res.data.length ? res.data.length : 0)); } - } - // set response data - else { - res.data = docs; - res.meta = meta || {}; - // store the query_type for subsequent middleware - res.meta.query_type = renderedQuery.type; - - logger.info(`[controller:search] [queryType:${renderedQuery.type}] [es_result_count:` + - (res.data && res.data.length ? res.data.length : 0)); - } - logger.debug('[ES response]', docs); - next(); + logger.debug('[ES response]', docs); + next(); + }); + }); } diff --git a/package.json b/package.json index 0bf5d5c6..435c7e68 100644 --- a/package.json +++ b/package.json @@ -58,6 +58,8 @@ "pelias-model": "4.4.0", "pelias-query": "8.11.0", "pelias-text-analyzer": "1.7.0", + "performance-now": "^2.0.0", + "retry": "^0.10.1", "stats-lite": "2.0.3", "through2": "^2.0.3" }, diff --git a/routes/v1.js b/routes/v1.js index 97892a58..97bf7849 100644 --- a/routes/v1.js +++ b/routes/v1.js @@ -68,22 +68,22 @@ function addRoutes(app, peliasConfig) { var routers = { index: createRouter([ - controllers.mdToHTML(peliasConfig, './public/apiDoc.md') + controllers.mdToHTML(peliasConfig.api, './public/apiDoc.md') ]), attribution: createRouter([ - controllers.mdToHTML(peliasConfig, './public/attribution.md') + controllers.mdToHTML(peliasConfig.api, './public/attribution.md') ]), search: createRouter([ sanitizers.search.middleware, middleware.calcSize(), // 3rd parameter is which query module to use, use fallback/geodisambiguation // first, then use original search strategy if first query didn't return anything - controllers.search(peliasConfig, esclient, queries.libpostal), + controllers.search(peliasConfig.api, esclient, queries.libpostal), sanitizers.search_fallback.middleware, - controllers.search(peliasConfig, esclient, queries.fallback_to_old_prod), + controllers.search(peliasConfig.api, esclient, queries.fallback_to_old_prod), postProc.trimByGranularity(), postProc.distances('focus.point.'), - postProc.confidenceScores(peliasConfig), + postProc.confidenceScores(peliasConfig.api), postProc.confidenceScoresFallback(), postProc.dedupe(), postProc.accuracy(), @@ -92,16 +92,16 @@ function addRoutes(app, peliasConfig) { postProc.parseBoundingBox(), postProc.normalizeParentIds(), postProc.assignLabels(), - postProc.geocodeJSON(peliasConfig, base), + postProc.geocodeJSON(peliasConfig.api, base), postProc.sendJSON ]), structured: createRouter([ sanitizers.structured_geocoding.middleware, middleware.calcSize(), - controllers.search(peliasConfig, esclient, queries.structured_geocoding), + controllers.search(peliasConfig.api, esclient, queries.structured_geocoding), postProc.trimByGranularityStructured(), postProc.distances('focus.point.'), - postProc.confidenceScores(peliasConfig), + postProc.confidenceScores(peliasConfig.api), postProc.confidenceScoresFallback(), postProc.dedupe(), postProc.accuracy(), @@ -110,14 +110,14 @@ function addRoutes(app, peliasConfig) { postProc.parseBoundingBox(), postProc.normalizeParentIds(), postProc.assignLabels(), - postProc.geocodeJSON(peliasConfig, base), + postProc.geocodeJSON(peliasConfig.api, base), postProc.sendJSON ]), autocomplete: createRouter([ sanitizers.autocomplete.middleware, - controllers.search(peliasConfig, esclient, queries.autocomplete), + controllers.search(peliasConfig.api, esclient, queries.autocomplete), postProc.distances('focus.point.'), - postProc.confidenceScores(peliasConfig), + postProc.confidenceScores(peliasConfig.api), postProc.dedupe(), postProc.accuracy(), postProc.localNamingConventions(), @@ -125,13 +125,13 @@ function addRoutes(app, peliasConfig) { postProc.parseBoundingBox(), postProc.normalizeParentIds(), postProc.assignLabels(), - postProc.geocodeJSON(peliasConfig, base), + postProc.geocodeJSON(peliasConfig.api, base), postProc.sendJSON ]), reverse: createRouter([ sanitizers.reverse.middleware, middleware.calcSize(), - controllers.search(peliasConfig, esclient, queries.reverse), + controllers.search(peliasConfig.api, esclient, queries.reverse), postProc.distances('point.'), // reverse confidence scoring depends on distance from origin // so it must be calculated first @@ -143,13 +143,13 @@ function addRoutes(app, peliasConfig) { postProc.parseBoundingBox(), postProc.normalizeParentIds(), postProc.assignLabels(), - postProc.geocodeJSON(peliasConfig, base), + postProc.geocodeJSON(peliasConfig.api, base), postProc.sendJSON ]), nearby: createRouter([ sanitizers.nearby.middleware, middleware.calcSize(), - controllers.search(peliasConfig, esclient, queries.reverse), + controllers.search(peliasConfig.api, esclient, queries.reverse), postProc.distances('point.'), // reverse confidence scoring depends on distance from origin // so it must be calculated first @@ -161,19 +161,19 @@ function addRoutes(app, peliasConfig) { postProc.parseBoundingBox(), postProc.normalizeParentIds(), postProc.assignLabels(), - postProc.geocodeJSON(peliasConfig, base), + postProc.geocodeJSON(peliasConfig.api, base), postProc.sendJSON ]), place: createRouter([ sanitizers.place.middleware, - controllers.place(peliasConfig, esclient), + controllers.place(peliasConfig.api, esclient), postProc.accuracy(), postProc.localNamingConventions(), postProc.renamePlacenames(), postProc.parseBoundingBox(), postProc.normalizeParentIds(), postProc.assignLabels(), - postProc.geocodeJSON(peliasConfig, base), + postProc.geocodeJSON(peliasConfig.api, base), postProc.sendJSON ]), status: createRouter([ From fe0457cb8bf3f16f4c76be8724c48f5c1e21f7ea Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Wed, 18 Jan 2017 13:52:55 -0500 Subject: [PATCH 14/43] removed maxRetries default --- controller/search.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controller/search.js b/controller/search.js index eb324f10..d8d5c458 100644 --- a/controller/search.js +++ b/controller/search.js @@ -39,10 +39,10 @@ function setup( config, esclient, query ){ logger.debug( '[ES req]', cmd ); // options for retry - // default number of retries to 3 (seems reasonable) + // maxRetries is from the ES client which defaults to 3 // factor of 1 means that each retry attempt will esclient requestTimeout const operationOptions = { - retries: _.get(esclient, 'transport.maxRetries', 3), + retries: _.get(esclient, 'transport.maxRetries'), factor: 1, minTimeout: _.get(esclient, 'transport.requestTimeout') }; From 055bbeacbaba1964051ccce8c90c0a9c47ee12bf Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Wed, 18 Jan 2017 13:59:32 -0500 Subject: [PATCH 15/43] removed performance-now dependency --- package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/package.json b/package.json index 435c7e68..b4543725 100644 --- a/package.json +++ b/package.json @@ -58,7 +58,6 @@ "pelias-model": "4.4.0", "pelias-query": "8.11.0", "pelias-text-analyzer": "1.7.0", - "performance-now": "^2.0.0", "retry": "^0.10.1", "stats-lite": "2.0.3", "through2": "^2.0.3" From 40e6b1b3b766b61113b980e228871d6813a50584 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 11:32:06 -0500 Subject: [PATCH 16/43] renamed variable for clarity --- controller/place.js | 4 ++-- controller/search.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/controller/place.js b/controller/place.js index 28523665..309702cd 100644 --- a/controller/place.js +++ b/controller/place.js @@ -1,7 +1,7 @@ var service = { mget: require('../service/mget') }; var logger = require('pelias-logger').get('api'); -function setup( config, esclient ){ +function setup( apiConfig, esclient ){ function controller( req, res, next ){ // do not run controller when a request @@ -12,7 +12,7 @@ function setup( config, esclient ){ var query = req.clean.ids.map( function(id) { return { - _index: config.indexName, + _index: apiConfig.indexName, _type: id.layers, _id: id.id }; diff --git a/controller/search.js b/controller/search.js index d8d5c458..3fa3da2f 100644 --- a/controller/search.js +++ b/controller/search.js @@ -7,7 +7,7 @@ var logger = require('pelias-logger').get('api'); var logging = require( '../helper/logging' ); const retry = require('retry'); -function setup( config, esclient, query ){ +function setup( apiConfig, esclient, query ){ function controller( req, res, next ){ // do not run controller when a request // validation error has occurred. @@ -55,7 +55,7 @@ function setup( config, esclient, query ){ // elasticsearch command var cmd = { - index: config.indexName, + index: apiConfig.indexName, searchType: 'dfs_query_then_fetch', body: renderedQuery.body }; From b2ebdd9f4bba68b36967065a14b4ad801752c1d0 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 11:36:46 -0500 Subject: [PATCH 17/43] moved require to top --- routes/v1.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/routes/v1.js b/routes/v1.js index 97bf7849..3b8a22dc 100644 --- a/routes/v1.js +++ b/routes/v1.js @@ -1,4 +1,5 @@ var Router = require('express').Router; +var elasticsearch = require('elasticsearch'); /** ----------------------- sanitizers ----------------------- **/ var sanitizers = { @@ -60,7 +61,7 @@ var postProc = { * @param {object} peliasConfig */ function addRoutes(app, peliasConfig) { - const esclient = require('elasticsearch').Client(peliasConfig.esclient); + const esclient = elasticsearch.Client(peliasConfig.esclient); var base = '/v1/'; From f1e5edb4cfd9852cd0ee895b09109bc8c3beb72c Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 12:10:12 -0500 Subject: [PATCH 18/43] added back logging statement for fallback queries added unit tests to cover conditionals, simplified conditional --- sanitizer/search_fallback.js | 6 ++- test/unit/sanitizer/search_fallback.js | 64 ++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/sanitizer/search_fallback.js b/sanitizer/search_fallback.js index 27f2bc9d..9bdb0c1e 100644 --- a/sanitizer/search_fallback.js +++ b/sanitizer/search_fallback.js @@ -6,19 +6,21 @@ var sanitizeAll = require('../sanitizer/sanitizeAll'), var sanitize = function(req, cb) { sanitizeAll(req, sanitizers, cb); }; var logger = require('pelias-logger').get('api'); var logging = require( '../helper/logging' ); +var _ = require('lodash'); // middleware module.exports.middleware = function( req, res, next ){ // if res.data already has results then don't call the _text_autocomplete sanitizer // this has been put into place for when the libpostal integration way of querying // ES doesn't return anything and we want to fallback to the old logic - if (res && res.hasOwnProperty('data') && res.data.length > 0) { + if (_.get(res, 'data', []).length > 0) { return next(); } // log the query that caused a fallback since libpostal+new-queries didn't return anything if (req.path === '/v1/search') { - var queryText = logging.isDNT(req) ? '[text removed]' : req.clean.text; + const queryText = logging.isDNT(req) ? '[text removed]' : req.clean.text; + logger.info(`fallback queryText: ${queryText}`); } sanitize( req, function( err, clean ){ diff --git a/test/unit/sanitizer/search_fallback.js b/test/unit/sanitizer/search_fallback.js index 82bad1b0..69e35fcd 100644 --- a/test/unit/sanitizer/search_fallback.js +++ b/test/unit/sanitizer/search_fallback.js @@ -107,6 +107,70 @@ module.exports.tests.sanitize = function(test, common) { }); + test('req.clean.text should be logged when isDNT=false', (t) => { + const infoLog = []; + + const search = proxyquire('../../../sanitizer/search_fallback', { + 'pelias-logger': { + get: () => { + return { + info: (msg) => { + infoLog.push(msg); + } + }; + } + }, + '../helper/logging': { + isDNT: () => { return false; } + } + }); + + const req = { + path: '/v1/search', + clean: { + text: 'this is the query text' + } + }; + + search.middleware(req, undefined, () => { + t.deepEquals(infoLog, [`fallback queryText: ${req.clean.text}`]); + t.end(); + }); + + }); + + test('req.clean.text should not be logged when isDNT=true', (t) => { + const infoLog = []; + + const search = proxyquire('../../../sanitizer/search_fallback', { + 'pelias-logger': { + get: () => { + return { + info: (msg) => { + infoLog.push(msg); + } + }; + } + }, + '../helper/logging': { + isDNT: () => { return true; } + } + }); + + const req = { + path: '/v1/search', + clean: { + text: 'this is the query text' + } + }; + + search.middleware(req, undefined, () => { + t.deepEquals(infoLog, ['fallback queryText: [text removed]']); + t.end(); + }); + + }); + }; module.exports.all = function (tape, common) { From e83c12cd0e382848bad6c0977495252ba9eb051a Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 13:56:40 -0500 Subject: [PATCH 19/43] added test for early bail when errors are present simplified conditionals using lodash `.get` --- controller/search.js | 4 ++-- test/unit/controller/search.js | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/controller/search.js b/controller/search.js index 3fa3da2f..f3ed67ad 100644 --- a/controller/search.js +++ b/controller/search.js @@ -11,14 +11,14 @@ function setup( apiConfig, esclient, query ){ function controller( req, res, next ){ // do not run controller when a request // validation error has occurred. - if( req.errors && req.errors.length ){ + if (_.get(req, 'errors', []).length > 0) { return next(); } // do not run controller if there are already results // this was added during libpostal integration. if the libpostal parse/query // doesn't return anything then fallback to old search-engine-y behavior - if (res && res.hasOwnProperty('data') && res.data.length > 0) { + if (_.get(res, 'data', []).length > 0) { return next(); } diff --git a/test/unit/controller/search.js b/test/unit/controller/search.js index 2b87990d..41d32bcd 100644 --- a/test/unit/controller/search.js +++ b/test/unit/controller/search.js @@ -173,6 +173,29 @@ module.exports.tests.timeout = function(test, common) { }); }; +module.exports.tests.existing_errors = function(test, common) { + test('req with errors should not call backend', function(t) { + var esclient = function() { + throw new Error('esclient should not have been called'); + }; + var controller = setup( fakeDefaultConfig, esclient, mockQuery() ); + + // the existence of `errors` means that a sanitizer detected an error, + // so don't call the esclient + var req = { + errors: ['error'] + }; + var res = { }; + + t.doesNotThrow(() => { + controller(req, res, () => {}); + }); + t.end(); + + }); + +}; + module.exports.tests.existing_results = function(test, common) { test('res with existing data should not call backend', function(t) { var backend = function() { From 67bba11001a7e0c251cc09a5c58b4274a80bc2b7 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 14:19:55 -0500 Subject: [PATCH 20/43] removed object indirection in favor of function for easier proxyquire --- controller/search.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controller/search.js b/controller/search.js index f3ed67ad..b7d8bc6d 100644 --- a/controller/search.js +++ b/controller/search.js @@ -2,7 +2,7 @@ var _ = require('lodash'); -var service = { search: require('../service/search') }; +var searchService = require('../service/search'); var logger = require('pelias-logger').get('api'); var logging = require( '../helper/logging' ); const retry = require('retry'); @@ -62,7 +62,7 @@ function setup( apiConfig, esclient, query ){ operation.attempt((currentAttempt) => { // query elasticsearch - service.search( esclient, cmd, function( err, docs, meta ){ + searchService( esclient, cmd, function( err, docs, meta ){ // returns true if the operation should be attempted again // (handles bookkeeping of maxRetries) if (operation.retry(err)) { From 0121d3db2adb39eba18d24a8e118ae6f90ce7217 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 14:29:47 -0500 Subject: [PATCH 21/43] get # of retries from API config instead of ES client --- controller/search.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controller/search.js b/controller/search.js index b7d8bc6d..4e85f25f 100644 --- a/controller/search.js +++ b/controller/search.js @@ -39,10 +39,10 @@ function setup( apiConfig, esclient, query ){ logger.debug( '[ES req]', cmd ); // options for retry - // maxRetries is from the ES client which defaults to 3 + // maxRetries is from the API config with default of 3 // factor of 1 means that each retry attempt will esclient requestTimeout const operationOptions = { - retries: _.get(esclient, 'transport.maxRetries'), + retries: _.get(apiConfig, 'requestRetries', 3), factor: 1, minTimeout: _.get(esclient, 'transport.requestTimeout') }; From 995b6109e3fbeaca6c7ad3223b195b068b237d02 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 14:31:02 -0500 Subject: [PATCH 22/43] removed used variable --- controller/search.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/controller/search.js b/controller/search.js index 4e85f25f..52b0281a 100644 --- a/controller/search.js +++ b/controller/search.js @@ -47,9 +47,6 @@ function setup( apiConfig, esclient, query ){ minTimeout: _.get(esclient, 'transport.requestTimeout') }; - // prepend the config timeout since the total number of attempts is maxRetries+1 - const timeouts = [operationOptions.minTimeout].concat(retry.timeouts(operationOptions)); - // setup a new operation const operation = retry.operation(operationOptions); From bf62a1844b4ba81b8184704b2af5caf7de1cf238 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 16:02:10 -0500 Subject: [PATCH 23/43] added tests for verifying request retry behavior limited retry behavior to errors with status 408 (request timeout). this also reduces reliance on unit/mock/backend and unit/mock/query. --- controller/search.js | 7 +- test/unit/controller/search.js | 410 ++++++++++++++++++++++----------- test/unit/mock/backend.js | 8 - 3 files changed, 284 insertions(+), 141 deletions(-) diff --git a/controller/search.js b/controller/search.js index 52b0281a..d1343350 100644 --- a/controller/search.js +++ b/controller/search.js @@ -8,6 +8,10 @@ var logging = require( '../helper/logging' ); const retry = require('retry'); function setup( apiConfig, esclient, query ){ + function isRequestTimeout(err) { + return _.get(err, 'status') === 408; + } + function controller( req, res, next ){ // do not run controller when a request // validation error has occurred. @@ -62,7 +66,8 @@ function setup( apiConfig, esclient, query ){ searchService( esclient, cmd, function( err, docs, meta ){ // returns true if the operation should be attempted again // (handles bookkeeping of maxRetries) - if (operation.retry(err)) { + // only consider for status 408 (request timeout) + if (isRequestTimeout(err) && operation.retry(err)) { logger.info('request timed out, retrying'); return; } diff --git a/test/unit/controller/search.js b/test/unit/controller/search.js index 41d32bcd..8652cbb9 100644 --- a/test/unit/controller/search.js +++ b/test/unit/controller/search.js @@ -1,3 +1,5 @@ +'use strict'; + var setup = require('../../../controller/search'), mockBackend = require('../mock/backend'), mockQuery = require('../mock/query'); @@ -19,158 +21,302 @@ var fakeDefaultConfig = { }; // functionally test controller (backend success) -module.exports.tests.functional_success = function(test, common) { - - // expected geojson features for 'client/suggest/ok/1' fixture - var expected = [{ - type: 'Feature', - geometry: { - type: 'Point', - coordinates: [-50.5, 100.1] - }, - properties: { - id: 'myid1', - layer: 'mytype1', - text: 'test name1, city1, state1' - } - }, { - type: 'Feature', - geometry: { - type: 'Point', - coordinates: [-51.5, 100.2] - }, - properties: { - id: 'myid2', - layer: 'mytype2', - text: 'test name2, city2, state2' - } - }]; - - var expectedMeta = { - scores: [10, 20], - query_type: 'mock' - }; - - var expectedData = [ - { - _id: 'myid1', - _score: 10, - _type: 'mytype1', - _matched_queries: ['query 1', 'query 2'], - parent: { - country: ['country1'], - region: ['state1'], - county: ['city1'] - }, - center_point: { lat: 100.1, lon: -50.5 }, - name: { default: 'test name1' }, - value: 1 - }, - { - _id: 'myid2', - _score: 20, - _type: 'mytype2', - _matched_queries: ['query 3'], - parent: { - country: ['country2'], - region: ['state2'], - county: ['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, { - body: {a: 'b'}, - index: 'pelias', - searchType: 'dfs_query_then_fetch' - }, 'correct backend command'); - }); - var controller = setup(fakeDefaultConfig, backend, mockQuery()); - var res = { - status: function (code) { - t.equal(code, 200, 'status set'); - return res; +// module.exports.tests.functional_success = function(test, common) { +// +// // expected geojson features for 'client/suggest/ok/1' fixture +// var expected = [{ +// type: 'Feature', +// geometry: { +// type: 'Point', +// coordinates: [-50.5, 100.1] +// }, +// properties: { +// id: 'myid1', +// layer: 'mytype1', +// text: 'test name1, city1, state1' +// } +// }, { +// type: 'Feature', +// geometry: { +// type: 'Point', +// coordinates: [-51.5, 100.2] +// }, +// properties: { +// id: 'myid2', +// layer: 'mytype2', +// text: 'test name2, city2, state2' +// } +// }]; +// +// var expectedMeta = { +// scores: [10, 20], +// query_type: 'mock' +// }; +// +// var expectedData = [ +// { +// _id: 'myid1', +// _score: 10, +// _type: 'mytype1', +// _matched_queries: ['query 1', 'query 2'], +// parent: { +// country: ['country1'], +// region: ['state1'], +// county: ['city1'] +// }, +// center_point: { lat: 100.1, lon: -50.5 }, +// name: { default: 'test name1' }, +// value: 1 +// }, +// { +// _id: 'myid2', +// _score: 20, +// _type: 'mytype2', +// _matched_queries: ['query 3'], +// parent: { +// country: ['country2'], +// region: ['state2'], +// county: ['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, { +// body: {a: 'b'}, +// index: 'pelias', +// searchType: 'dfs_query_then_fetch' +// }, 'correct backend command'); +// }); +// var controller = setup(fakeDefaultConfig, backend, mockQuery()); +// var res = { +// status: function (code) { +// t.equal(code, 200, 'status set'); +// return res; +// }, +// json: function (json) { +// t.equal(typeof json, 'object', 'returns json'); +// t.equal(typeof json.date, 'number', 'date set'); +// t.equal(json.type, 'FeatureCollection', 'valid geojson'); +// t.true(Array.isArray(json.features), 'features is array'); +// t.deepEqual(json.features, expected, 'values correctly mapped'); +// } +// }; +// 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); +// }); +// +// test('functional success with alternate index name', function(t) { +// var fakeCustomizedConfig = { +// indexName: 'alternateindexname' +// }; +// +// var backend = mockBackend('client/search/ok/1', function (cmd) { +// t.deepEqual(cmd, { +// body: {a: 'b'}, +// index: 'alternateindexname', +// searchType: 'dfs_query_then_fetch' +// }, 'correct backend command'); +// }); +// var controller = setup(fakeCustomizedConfig, backend, mockQuery()); +// var res = { +// status: function (code) { +// t.equal(code, 200, 'status set'); +// return res; +// } +// }; +// var req = { clean: { a: 'b' }, errors: [], warnings: [] }; +// var next = function next() { +// t.equal(req.errors.length, 0, 'next was called without error'); +// t.end(); +// }; +// controller(req, res, next); +// }); +// }; +// +// // functionally test controller (backend failure) +// module.exports.tests.functional_failure = function(test, common) { +// test('functional failure', function(t) { +// var backend = mockBackend( 'client/search/fail/1', function( cmd ){ +// t.deepEqual(cmd, { body: { a: 'b' }, index: 'pelias', searchType: 'dfs_query_then_fetch' }, 'correct backend command'); +// }); +// var controller = setup( fakeDefaultConfig, backend, mockQuery() ); +// var req = { clean: { a: 'b' }, errors: [], warnings: [] }; +// var next = function(){ +// t.equal(req.errors[0],'an elasticsearch error occurred'); +// t.end(); +// }; +// controller(req, undefined, next ); +// }); +// }; + +module.exports.tests.timeout = function(test, common) { + test('default # of request timeout retries should be 3', (t) => { + const config = { + indexName: 'indexName value' + }; + const esclient = 'this is the esclient'; + const query = () => { + return { body: 'this is the query body' }; + }; + + let searchServiceCallCount = 0; + + const timeoutError = { + status: 408, + displayName: 'RequestTimeout', + message: 'Request Timeout after 17ms' + }; + + // request timeout messages willl be written here + const infoMesssages = []; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/search', { + '../service/search': (esclient, cmd, callback) => { + t.equal(esclient, 'this is the esclient'); + t.deepEqual(cmd, { + index: 'indexName value', + searchType: 'dfs_query_then_fetch', + body: 'this is the query body' + }); + + // not that the searchService got called + searchServiceCallCount++; + + callback(timeoutError); }, - json: function (json) { - t.equal(typeof json, 'object', 'returns json'); - t.equal(typeof json.date, 'number', 'date set'); - t.equal(json.type, 'FeatureCollection', 'valid geojson'); - t.true(Array.isArray(json.features), 'features is array'); - t.deepEqual(json.features, expected, 'values correctly mapped'); + 'pelias-logger': { + get: (service) => { + t.equal(service, 'api'); + return { + info: (msg) => { + infoMesssages.push(msg); + }, + debug: () => {} + }; + } } - }; - 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'); + })(config, esclient, query); + + const req = { clean: { }, errors: [], warnings: [] }; + const res = {}; + + var next = function() { + t.equal(searchServiceCallCount, 3+1); + t.deepEqual( + infoMesssages.filter((msg)=> { return msg === 'request timed out, retrying'; } ).length, + 3, + 'there should be 3 request timed out info messages' + ); + t.deepEqual(req, { + clean: {}, + errors: [timeoutError.message], + warnings: [] + }); + t.deepEqual(res, {}); t.end(); }; + controller(req, res, next); + }); - test('functional success with alternate index name', function(t) { - var fakeCustomizedConfig = { - indexName: 'alternateindexname' + test('explicit apiConfig.requestRetries should retry that many times', (t) => { + const config = { + indexName: 'indexName value', + requestRetries: 17 + }; + const esclient = 'this is the esclient'; + const query = () => { + return { }; }; - var backend = mockBackend('client/search/ok/1', function (cmd) { - t.deepEqual(cmd, { - body: {a: 'b'}, - index: 'alternateindexname', - searchType: 'dfs_query_then_fetch' - }, 'correct backend command'); - }); - var controller = setup(fakeCustomizedConfig, backend, mockQuery()); - var res = { - status: function (code) { - t.equal(code, 200, 'status set'); - return res; - } + let searchServiceCallCount = 0; + + const timeoutError = { + status: 408, + displayName: 'RequestTimeout', + message: 'Request Timeout after 17ms' }; - var req = { clean: { a: 'b' }, errors: [], warnings: [] }; - var next = function next() { - t.equal(req.errors.length, 0, 'next was called without error'); + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/search', { + '../service/search': (esclient, cmd, callback) => { + // not that the searchService got called + searchServiceCallCount++; + + callback(timeoutError); + } + })(config, esclient, query); + + const req = { clean: { }, errors: [], warnings: [] }; + const res = {}; + + var next = function() { + t.equal(searchServiceCallCount, 17+1); t.end(); }; + controller(req, res, next); + }); -}; -// functionally test controller (backend failure) -module.exports.tests.functional_failure = function(test, common) { - test('functional failure', function(t) { - var backend = mockBackend( 'client/search/fail/1', function( cmd ){ - t.deepEqual(cmd, { body: { a: 'b' }, index: 'pelias', searchType: 'dfs_query_then_fetch' }, 'correct backend command'); - }); - var controller = setup( fakeDefaultConfig, backend, mockQuery() ); - var req = { clean: { a: 'b' }, errors: [], warnings: [] }; - var next = function(){ - t.equal(req.errors[0],'an elasticsearch error occurred'); - t.end(); + test('only status code 408 should be considered a retryable request', (t) => { + const config = { + indexName: 'indexName value', + requestRetries: 17 + }; + const esclient = 'this is the esclient'; + const query = () => { + return { }; }; - controller(req, undefined, next ); - }); -}; -module.exports.tests.timeout = function(test, common) { - test('timeout', function(t) { - var backend = mockBackend( 'client/search/timeout/1', function( cmd ){ - t.deepEqual(cmd, { body: { a: 'b' }, index: 'pelias', searchType: 'dfs_query_then_fetch' }, 'correct backend command'); - }); - var controller = setup( fakeDefaultConfig, backend, mockQuery() ); - var req = { clean: { a: 'b' }, errors: [], warnings: [] }; - var next = function(){ - t.equal(req.errors[0],'Request Timeout after 5000ms'); + let searchServiceCallCount = 0; + + const nonTimeoutError = { + status: 500, + displayName: 'InternalServerError', + message: 'an internal server error occurred' + }; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/search', { + '../service/search': (esclient, cmd, callback) => { + // not that the searchService got called + searchServiceCallCount++; + + callback(nonTimeoutError); + } + })(config, esclient, query); + + const req = { clean: { }, errors: [], warnings: [] }; + const res = {}; + + var next = function() { + t.equal(searchServiceCallCount, 1); + t.deepEqual(req, { + clean: {}, + errors: [nonTimeoutError.message], + warnings: [] + }); t.end(); }; - controller(req, undefined, next ); + + controller(req, res, next); + }); + }; module.exports.tests.existing_errors = function(test, common) { diff --git a/test/unit/mock/backend.js b/test/unit/mock/backend.js index 104f84dc..53df354f 100644 --- a/test/unit/mock/backend.js +++ b/test/unit/mock/backend.js @@ -35,14 +35,6 @@ responses['client/search/fail/1'] = function( cmd, cb ){ return cb( 'an elasticsearch error occurred' ); }; -responses['client/search/timeout/1'] = function( cmd, cb) { - // timeout errors are objects - return cb({ - status: 408, - message: 'Request Timeout after 5000ms' - }); -}; - responses['client/mget/ok/1'] = function( cmd, cb ){ return cb( undefined, mgetEnvelope([{ _id: 'myid1', From fbe0de386ea46de026ccc20f683c1d00fae59f84 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 16:12:48 -0500 Subject: [PATCH 24/43] added ending ']' --- controller/search.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/search.js b/controller/search.js index d1343350..40cba485 100644 --- a/controller/search.js +++ b/controller/search.js @@ -94,7 +94,7 @@ function setup( apiConfig, esclient, query ){ res.meta.query_type = renderedQuery.type; logger.info(`[controller:search] [queryType:${renderedQuery.type}] [es_result_count:` + - (res.data && res.data.length ? res.data.length : 0)); + (res.data && res.data.length ? res.data.length : 0) + ']'); } logger.debug('[ES response]', docs); next(); From c7b83e96d57c9aa973fc0b2219c43ff4be5527d4 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 16:13:19 -0500 Subject: [PATCH 25/43] added tests for case where error is a string, not an object --- test/unit/controller/search.js | 41 ++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/unit/controller/search.js b/test/unit/controller/search.js index 8652cbb9..717589b1 100644 --- a/test/unit/controller/search.js +++ b/test/unit/controller/search.js @@ -317,6 +317,47 @@ module.exports.tests.timeout = function(test, common) { }); + test('string error should not retry and be logged as-is', (t) => { + const config = { + indexName: 'indexName value', + requestRetries: 17 + }; + const esclient = 'this is the esclient'; + const query = () => { + return { }; + }; + + let searchServiceCallCount = 0; + + const stringTypeError = 'this is an error string'; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/search', { + '../service/search': (esclient, cmd, callback) => { + // not that the searchService got called + searchServiceCallCount++; + + callback(stringTypeError); + } + })(config, esclient, query); + + const req = { clean: { }, errors: [], warnings: [] }; + const res = {}; + + var next = function() { + t.equal(searchServiceCallCount, 1); + t.deepEqual(req, { + clean: {}, + errors: [stringTypeError], + warnings: [] + }); + t.end(); + }; + + controller(req, res, next); + + }); + }; module.exports.tests.existing_errors = function(test, common) { From 96f9d12ff5abe58ea9f222ce3f707fa58753fd1d Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 17:04:51 -0500 Subject: [PATCH 26/43] rewrote tests to proxyquire service/search to reduce complexity added a few more tests for coverage, removed unused things from unit/mock/backend --- controller/search.js | 12 +- test/unit/controller/search.js | 438 +++++++++++++++++++++------------ test/unit/mock/backend.js | 3 - 3 files changed, 293 insertions(+), 160 deletions(-) diff --git a/controller/search.js b/controller/search.js index 40cba485..08f2731a 100644 --- a/controller/search.js +++ b/controller/search.js @@ -1,10 +1,10 @@ 'use strict'; -var _ = require('lodash'); +const _ = require('lodash'); -var searchService = require('../service/search'); -var logger = require('pelias-logger').get('api'); -var logging = require( '../helper/logging' ); +const searchService = require('../service/search'); +const logger = require('pelias-logger').get('api'); +const logging = require( '../helper/logging' ); const retry = require('retry'); function setup( apiConfig, esclient, query ){ @@ -68,7 +68,7 @@ function setup( apiConfig, esclient, query ){ // (handles bookkeeping of maxRetries) // only consider for status 408 (request timeout) if (isRequestTimeout(err) && operation.retry(err)) { - logger.info('request timed out, retrying'); + logger.info(`request timed out on attempt ${currentAttempt}, retrying`); return; } @@ -94,7 +94,7 @@ function setup( apiConfig, esclient, query ){ res.meta.query_type = renderedQuery.type; logger.info(`[controller:search] [queryType:${renderedQuery.type}] [es_result_count:` + - (res.data && res.data.length ? res.data.length : 0) + ']'); + _.get(res, 'data', []).length + ']'); } logger.debug('[ES response]', docs); next(); diff --git a/test/unit/controller/search.js b/test/unit/controller/search.js index 717589b1..39928938 100644 --- a/test/unit/controller/search.js +++ b/test/unit/controller/search.js @@ -15,150 +15,286 @@ module.exports.tests.interface = function(test, common) { }); }; -// reminder: this is only the api subsection of the full config -var fakeDefaultConfig = { - indexName: 'pelias' -}; +module.exports.tests.success = function(test, common) { + test('successful request to search service should set data and meta', (t) => { + const config = { + indexName: 'indexName value' + }; + const esclient = 'this is the esclient'; + const query = () => { + return { + body: 'this is the query body', + type: 'this is the query type' + }; + }; + + // request timeout messages willl be written here + const infoMesssages = []; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/search', { + '../service/search': (esclient, cmd, callback) => { + t.equal(esclient, 'this is the esclient'); + t.deepEqual(cmd, { + index: 'indexName value', + searchType: 'dfs_query_then_fetch', + body: 'this is the query body' + }); + + const docs = [{}, {}]; + const meta = { key: 'value' }; + + callback(undefined, docs, meta); + }, + 'pelias-logger': { + get: (service) => { + t.equal(service, 'api'); + return { + info: (msg) => { + infoMesssages.push(msg); + }, + debug: () => {} + }; + } + } + })(config, esclient, query); + + const req = { clean: { }, errors: [], warnings: [] }; + const res = {}; + + var next = function() { + t.deepEqual(req, { + clean: {}, + errors: [], + warnings: [] + }); + t.deepEquals(res.data, [{}, {}]); + t.deepEquals(res.meta, { key: 'value', query_type: 'this is the query type' }); + + t.ok(infoMesssages.find((msg) => { + return msg === '[controller:search] [queryType:this is the query type] [es_result_count:2]'; + })); + t.end(); + }; + + controller(req, res, next); + + }); + + test('undefined meta should set empty object into res', (t) => { + const config = { + indexName: 'indexName value' + }; + const esclient = 'this is the esclient'; + const query = () => { + return { + body: 'this is the query body', + type: 'this is the query type' + }; + }; + + // request timeout messages willl be written here + const infoMesssages = []; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/search', { + '../service/search': (esclient, cmd, callback) => { + t.equal(esclient, 'this is the esclient'); + t.deepEqual(cmd, { + index: 'indexName value', + searchType: 'dfs_query_then_fetch', + body: 'this is the query body' + }); + + const docs = [{}, {}]; + + callback(undefined, docs, undefined); + }, + 'pelias-logger': { + get: (service) => { + t.equal(service, 'api'); + return { + info: (msg) => { + infoMesssages.push(msg); + }, + debug: () => {} + }; + } + } + })(config, esclient, query); + + const req = { clean: { }, errors: [], warnings: [] }; + const res = {}; + + var next = function() { + t.deepEqual(req, { + clean: {}, + errors: [], + warnings: [] + }); + t.deepEquals(res.data, [{}, {}]); + t.deepEquals(res.meta, { query_type: 'this is the query type' }); + + t.ok(infoMesssages.find((msg) => { + return msg === '[controller:search] [queryType:this is the query type] [es_result_count:2]'; + })); + t.end(); + }; + + controller(req, res, next); + + }); + + test('undefined docs should log 0 results', (t) => { + const config = { + indexName: 'indexName value' + }; + const esclient = 'this is the esclient'; + const query = () => { + return { + body: 'this is the query body', + type: 'this is the query type' + }; + }; + + // request timeout messages willl be written here + const infoMesssages = []; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/search', { + '../service/search': (esclient, cmd, callback) => { + t.equal(esclient, 'this is the esclient'); + t.deepEqual(cmd, { + index: 'indexName value', + searchType: 'dfs_query_then_fetch', + body: 'this is the query body' + }); + + const meta = { key: 'value' }; + + callback(undefined, undefined, meta); + }, + 'pelias-logger': { + get: (service) => { + t.equal(service, 'api'); + return { + info: (msg) => { + infoMesssages.push(msg); + }, + debug: () => {} + }; + } + } + })(config, esclient, query); + + const req = { clean: { }, errors: [], warnings: [] }; + const res = {}; + + var next = function() { + t.deepEqual(req, { + clean: {}, + errors: [], + warnings: [] + }); + t.equals(res.data, undefined); + t.deepEquals(res.meta, { key: 'value', query_type: 'this is the query type' }); + + t.ok(infoMesssages.find((msg) => { + return msg === '[controller:search] [queryType:this is the query type] [es_result_count:0]'; + })); + t.end(); + }; + + controller(req, res, next); + + }); + + test('successful request on retry to search service should log info message', (t) => { + const config = { + indexName: 'indexName value' + }; + const esclient = 'this is the esclient'; + const query = () => { + return { + body: 'this is the query body', + type: 'this is the query type' + }; + }; + + let searchServiceCallCount = 0; + + const timeoutError = { + status: 408, + displayName: 'RequestTimeout', + message: 'Request Timeout after 17ms' + }; + + // request timeout messages willl be written here + const infoMesssages = []; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/search', { + '../service/search': (esclient, cmd, callback) => { + t.equal(esclient, 'this is the esclient'); + t.deepEqual(cmd, { + index: 'indexName value', + searchType: 'dfs_query_then_fetch', + body: 'this is the query body' + }); -// functionally test controller (backend success) -// module.exports.tests.functional_success = function(test, common) { -// -// // expected geojson features for 'client/suggest/ok/1' fixture -// var expected = [{ -// type: 'Feature', -// geometry: { -// type: 'Point', -// coordinates: [-50.5, 100.1] -// }, -// properties: { -// id: 'myid1', -// layer: 'mytype1', -// text: 'test name1, city1, state1' -// } -// }, { -// type: 'Feature', -// geometry: { -// type: 'Point', -// coordinates: [-51.5, 100.2] -// }, -// properties: { -// id: 'myid2', -// layer: 'mytype2', -// text: 'test name2, city2, state2' -// } -// }]; -// -// var expectedMeta = { -// scores: [10, 20], -// query_type: 'mock' -// }; -// -// var expectedData = [ -// { -// _id: 'myid1', -// _score: 10, -// _type: 'mytype1', -// _matched_queries: ['query 1', 'query 2'], -// parent: { -// country: ['country1'], -// region: ['state1'], -// county: ['city1'] -// }, -// center_point: { lat: 100.1, lon: -50.5 }, -// name: { default: 'test name1' }, -// value: 1 -// }, -// { -// _id: 'myid2', -// _score: 20, -// _type: 'mytype2', -// _matched_queries: ['query 3'], -// parent: { -// country: ['country2'], -// region: ['state2'], -// county: ['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, { -// body: {a: 'b'}, -// index: 'pelias', -// searchType: 'dfs_query_then_fetch' -// }, 'correct backend command'); -// }); -// var controller = setup(fakeDefaultConfig, backend, mockQuery()); -// var res = { -// status: function (code) { -// t.equal(code, 200, 'status set'); -// return res; -// }, -// json: function (json) { -// t.equal(typeof json, 'object', 'returns json'); -// t.equal(typeof json.date, 'number', 'date set'); -// t.equal(json.type, 'FeatureCollection', 'valid geojson'); -// t.true(Array.isArray(json.features), 'features is array'); -// t.deepEqual(json.features, expected, 'values correctly mapped'); -// } -// }; -// 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); -// }); -// -// test('functional success with alternate index name', function(t) { -// var fakeCustomizedConfig = { -// indexName: 'alternateindexname' -// }; -// -// var backend = mockBackend('client/search/ok/1', function (cmd) { -// t.deepEqual(cmd, { -// body: {a: 'b'}, -// index: 'alternateindexname', -// searchType: 'dfs_query_then_fetch' -// }, 'correct backend command'); -// }); -// var controller = setup(fakeCustomizedConfig, backend, mockQuery()); -// var res = { -// status: function (code) { -// t.equal(code, 200, 'status set'); -// return res; -// } -// }; -// var req = { clean: { a: 'b' }, errors: [], warnings: [] }; -// var next = function next() { -// t.equal(req.errors.length, 0, 'next was called without error'); -// t.end(); -// }; -// controller(req, res, next); -// }); -// }; -// -// // functionally test controller (backend failure) -// module.exports.tests.functional_failure = function(test, common) { -// test('functional failure', function(t) { -// var backend = mockBackend( 'client/search/fail/1', function( cmd ){ -// t.deepEqual(cmd, { body: { a: 'b' }, index: 'pelias', searchType: 'dfs_query_then_fetch' }, 'correct backend command'); -// }); -// var controller = setup( fakeDefaultConfig, backend, mockQuery() ); -// var req = { clean: { a: 'b' }, errors: [], warnings: [] }; -// var next = function(){ -// t.equal(req.errors[0],'an elasticsearch error occurred'); -// t.end(); -// }; -// controller(req, undefined, next ); -// }); -// }; + if (searchServiceCallCount < 2) { + // note that the searchService got called + searchServiceCallCount++; + callback(timeoutError); + } else { + const docs = [{}, {}]; + const meta = { key: 'value' }; + + callback(undefined, docs, meta); + } + + }, + 'pelias-logger': { + get: (service) => { + t.equal(service, 'api'); + return { + info: (msg) => { + infoMesssages.push(msg); + }, + debug: () => {} + }; + } + } + })(config, esclient, query); + + const req = { clean: { }, errors: [], warnings: [] }; + const res = {}; + + var next = function() { + t.deepEqual(req, { + clean: {}, + errors: [], + warnings: [] + }); + t.deepEquals(res.data, [{}, {}]); + t.deepEquals(res.meta, { key: 'value', query_type: 'this is the query type' }); + + t.ok(infoMesssages.find((msg) => { + return msg === '[controller:search] [queryType:this is the query type] [es_result_count:2]'; + })); + + t.ok(infoMesssages.find((msg) => { + return msg === 'succeeded on retry 2'; + })); + + t.end(); + }; + + controller(req, res, next); + + }); + +}; module.exports.tests.timeout = function(test, common) { test('default # of request timeout retries should be 3', (t) => { @@ -214,11 +350,11 @@ module.exports.tests.timeout = function(test, common) { var next = function() { t.equal(searchServiceCallCount, 3+1); - t.deepEqual( - infoMesssages.filter((msg)=> { return msg === 'request timed out, retrying'; } ).length, - 3, - 'there should be 3 request timed out info messages' - ); + + t.ok(infoMesssages.indexOf('request timed out on attempt 1, retrying') !== -1); + t.ok(infoMesssages.indexOf('request timed out on attempt 2, retrying') !== -1); + t.ok(infoMesssages.indexOf('request timed out on attempt 3, retrying') !== -1); + t.deepEqual(req, { clean: {}, errors: [timeoutError.message], @@ -365,7 +501,7 @@ module.exports.tests.existing_errors = function(test, common) { var esclient = function() { throw new Error('esclient should not have been called'); }; - var controller = setup( fakeDefaultConfig, esclient, mockQuery() ); + var controller = setup( {}, esclient, mockQuery() ); // the existence of `errors` means that a sanitizer detected an error, // so don't call the esclient @@ -385,10 +521,10 @@ module.exports.tests.existing_errors = function(test, common) { module.exports.tests.existing_results = function(test, common) { test('res with existing data should not call backend', function(t) { - var backend = function() { + var esclient = function() { throw new Error('backend should not have been called'); }; - var controller = setup( fakeDefaultConfig, backend, mockQuery() ); + var controller = setup( {}, esclient, mockQuery() ); var req = { }; // the existence of `data` means that there are already results so diff --git a/test/unit/mock/backend.js b/test/unit/mock/backend.js index 53df354f..04e447e9 100644 --- a/test/unit/mock/backend.js +++ b/test/unit/mock/backend.js @@ -1,8 +1,5 @@ var responses = {}; -responses['client/suggest/ok/1'] = function( cmd, cb ){ - return cb( undefined, suggestEnvelope([ { score: 1, text: 'mocktype:mockid1' } ], [ { score: 2, text: 'mocktype:mockid2' } ]) ); -}; responses['client/suggest/fail/1'] = function( cmd, cb ){ return cb( 'an elasticsearch error occurred' ); }; From a4a6a691c8f16a83a418362d4e8a4b2dbbe63742 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 17:08:48 -0500 Subject: [PATCH 27/43] cleaned up log message parts --- controller/search.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/controller/search.js b/controller/search.js index 08f2731a..4870f53a 100644 --- a/controller/search.js +++ b/controller/search.js @@ -93,8 +93,13 @@ function setup( apiConfig, esclient, query ){ // store the query_type for subsequent middleware res.meta.query_type = renderedQuery.type; - logger.info(`[controller:search] [queryType:${renderedQuery.type}] [es_result_count:` + - _.get(res, 'data', []).length + ']'); + const messageParts = [ + '[controller:search]', + `[queryType:${renderedQuery.type}]`, + `[es_result_count:${_.get(res, 'data', []).length}]` + ]; + + logger.info(messageParts.join(' ')); } logger.debug('[ES response]', docs); next(); From d5ff417bf5e74d5b8981fa92257cde3916ce541a Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 21:40:01 -0500 Subject: [PATCH 28/43] removed require's for mock backend/query --- test/unit/controller/search.js | 60 +++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 27 deletions(-) diff --git a/test/unit/controller/search.js b/test/unit/controller/search.js index 39928938..817fc821 100644 --- a/test/unit/controller/search.js +++ b/test/unit/controller/search.js @@ -1,9 +1,7 @@ 'use strict'; -var setup = require('../../../controller/search'), - mockBackend = require('../mock/backend'), - mockQuery = require('../mock/query'); -var proxyquire = require('proxyquire').noCallThru(); +const setup = require('../../../controller/search'); +const proxyquire = require('proxyquire').noCallThru(); module.exports.tests = {}; @@ -62,7 +60,7 @@ module.exports.tests.success = function(test, common) { const req = { clean: { }, errors: [], warnings: [] }; const res = {}; - var next = function() { + const next = () => { t.deepEqual(req, { clean: {}, errors: [], @@ -126,7 +124,7 @@ module.exports.tests.success = function(test, common) { const req = { clean: { }, errors: [], warnings: [] }; const res = {}; - var next = function() { + const next = () => { t.deepEqual(req, { clean: {}, errors: [], @@ -190,7 +188,7 @@ module.exports.tests.success = function(test, common) { const req = { clean: { }, errors: [], warnings: [] }; const res = {}; - var next = function() { + const next = () => { t.deepEqual(req, { clean: {}, errors: [], @@ -270,7 +268,7 @@ module.exports.tests.success = function(test, common) { const req = { clean: { }, errors: [], warnings: [] }; const res = {}; - var next = function() { + const next = () => { t.deepEqual(req, { clean: {}, errors: [], @@ -348,7 +346,7 @@ module.exports.tests.timeout = function(test, common) { const req = { clean: { }, errors: [], warnings: [] }; const res = {}; - var next = function() { + const next = () => { t.equal(searchServiceCallCount, 3+1); t.ok(infoMesssages.indexOf('request timed out on attempt 1, retrying') !== -1); @@ -399,7 +397,7 @@ module.exports.tests.timeout = function(test, common) { const req = { clean: { }, errors: [], warnings: [] }; const res = {}; - var next = function() { + const next = () => { t.equal(searchServiceCallCount, 17+1); t.end(); }; @@ -439,7 +437,7 @@ module.exports.tests.timeout = function(test, common) { const req = { clean: { }, errors: [], warnings: [] }; const res = {}; - var next = function() { + const next = () => { t.equal(searchServiceCallCount, 1); t.deepEqual(req, { clean: {}, @@ -480,7 +478,7 @@ module.exports.tests.timeout = function(test, common) { const req = { clean: { }, errors: [], warnings: [] }; const res = {}; - var next = function() { + const next = () => { t.equal(searchServiceCallCount, 1); t.deepEqual(req, { clean: {}, @@ -498,17 +496,20 @@ module.exports.tests.timeout = function(test, common) { module.exports.tests.existing_errors = function(test, common) { test('req with errors should not call backend', function(t) { - var esclient = function() { + const esclient = () => { throw new Error('esclient should not have been called'); }; - var controller = setup( {}, esclient, mockQuery() ); + const query = () => { + throw new Error('query should not have been called'); + }; + const controller = setup( {}, esclient, query ); // the existence of `errors` means that a sanitizer detected an error, // so don't call the esclient - var req = { + const req = { errors: ['error'] }; - var res = { }; + const res = { }; t.doesNotThrow(() => { controller(req, res, () => {}); @@ -521,17 +522,20 @@ module.exports.tests.existing_errors = function(test, common) { module.exports.tests.existing_results = function(test, common) { test('res with existing data should not call backend', function(t) { - var esclient = function() { - throw new Error('backend should not have been called'); + const esclient = () => { + throw new Error('esclient should not have been called'); + }; + const query = () => { + throw new Error('query should not have been called'); }; - var controller = setup( {}, esclient, mockQuery() ); + const controller = setup( {}, esclient, query ); - var req = { }; + const req = { }; // the existence of `data` means that there are already results so // don't call the backend/query - var res = { data: [{}] }; + const res = { data: [{}] }; - var next = function() { + const next = function() { t.deepEqual(res, {data: [{}]}); t.end(); }; @@ -544,18 +548,20 @@ module.exports.tests.existing_results = function(test, common) { module.exports.tests.undefined_query = function(test, common) { test('query returning undefined should not call service', function(t) { // a function that returns undefined - var query = function () { return; }; + const query = () => { + return undefined; + }; - var search_service_was_called = false; + let search_service_was_called = false; - var controller = proxyquire('../../../controller/search', { + const controller = proxyquire('../../../controller/search', { '../service/search': function() { search_service_was_called = true; throw new Error('search service should not have been called'); } })(undefined, undefined, query); - var next = function() { + const next = () => { t.notOk(search_service_was_called, 'should have returned before search service was called'); t.end(); }; @@ -571,7 +577,7 @@ module.exports.all = function (tape, common) { return tape('GET /search ' + name, testFunction); } - for( var testCase in module.exports.tests ){ + for( const testCase in module.exports.tests ){ module.exports.tests[testCase](test, common); } }; From 89d9943f263250df4ee49a46f690e87d9c950fe7 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 21:40:51 -0500 Subject: [PATCH 29/43] removed unused mock object --- test/unit/mock/query.js | 13 ------------- 1 file changed, 13 deletions(-) delete mode 100644 test/unit/mock/query.js diff --git a/test/unit/mock/query.js b/test/unit/mock/query.js deleted file mode 100644 index a3209a4d..00000000 --- a/test/unit/mock/query.js +++ /dev/null @@ -1,13 +0,0 @@ - -function setup(){ - return query; -} - -function query( clean ){ - return { - type: 'mock', - body: clean - }; -} - -module.exports = setup; \ No newline at end of file From 6425cc6444e8c90af9af24b796881174d6936bf5 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 22:29:31 -0500 Subject: [PATCH 30/43] added request retries for ES request timeouts --- controller/place.js | 78 +++++-- test/unit/controller/place.js | 419 +++++++++++++++++++++++++++------- 2 files changed, 390 insertions(+), 107 deletions(-) diff --git a/controller/place.js b/controller/place.js index 309702cd..becae8fc 100644 --- a/controller/place.js +++ b/controller/place.js @@ -1,16 +1,39 @@ -var service = { mget: require('../service/mget') }; -var logger = require('pelias-logger').get('api'); +'use strict'; + +const _ = require('lodash'); +const retry = require('retry'); + +const mgetService = require('../service/mget'); +const logger = require('pelias-logger').get('api'); function setup( apiConfig, esclient ){ - function controller( req, res, next ){ + function requestHasErrors(request) { + return _.get(request, 'errors', []).length > 0; + } - // do not run controller when a request - // validation error has occurred. - if( req.errors && req.errors.length ){ + function isRequestTimeout(err) { + return _.get(err, 'status') === 408; + } + + function controller( req, res, next ){ + // do not run controller when a request validation error has occurred. + if (requestHasErrors(req)){ return next(); } - var query = req.clean.ids.map( function(id) { + // options for retry + // maxRetries is from the API config with default of 3 + // factor of 1 means that each retry attempt will esclient requestTimeout + const operationOptions = { + retries: _.get(apiConfig, 'requestRetries', 3), + factor: 1, + minTimeout: _.get(esclient, 'transport.requestTimeout') + }; + + // setup a new operation + const operation = retry.operation(operationOptions); + + const query = req.clean.ids.map( function(id) { return { _index: apiConfig.indexName, _type: id.layers, @@ -20,19 +43,36 @@ function setup( apiConfig, esclient ){ logger.debug( '[ES req]', query ); - service.mget( esclient, query, function( err, docs ) { - // error handler - if( err ){ - req.errors.push( err ); - } - // set response data - else { - res.data = docs; - } - logger.debug('[ES response]', docs); - - next(); + operation.attempt((currentAttempt) => { + mgetService( esclient, query, function( err, docs ) { + // returns true if the operation should be attempted again + // (handles bookkeeping of maxRetries) + // only consider for status 408 (request timeout) + if (isRequestTimeout(err) && operation.retry(err)) { + logger.info(`request timed out on attempt ${currentAttempt}, retrying`); + return; + } + + // error handler + if( err ){ + req.errors.push( err ); + } + // set response data + else { + // log that a retry was successful + // most requests succeed on first attempt so this declutters log files + if (currentAttempt > 1) { + logger.info(`succeeded on retry ${currentAttempt-1}`); + } + + res.data = docs; + } + logger.debug('[ES response]', docs); + + next(); + }); }); + } return controller; diff --git a/test/unit/controller/place.js b/test/unit/controller/place.js index 216b99f3..5f54ba6d 100644 --- a/test/unit/controller/place.js +++ b/test/unit/controller/place.js @@ -1,129 +1,372 @@ -var setup = require('../../../controller/place'), - mockBackend = require('../mock/backend'); +'use strict'; + +const setup = require('../../../controller/search'); +const proxyquire = require('proxyquire').noCallThru(); module.exports.tests = {}; -module.exports.tests.interface = function(test, common) { - test('valid interface', function(t) { +module.exports.tests.interface = (test, common) => { + test('valid interface', (t) => { t.equal(typeof setup, 'function', 'setup is a function'); t.equal(typeof setup(), 'function', 'setup returns a controller'); t.end(); }); }; -// reminder: this is only the api subsection of the full config -var fakeDefaultConfig = { - indexName: 'pelias' +module.exports.tests.success = (test, common) => { + test('successful request to search service should set data and meta', (t) => { + const config = { + indexName: 'indexName value' + }; + const esclient = 'this is the esclient'; + + // request timeout messages willl be written here + const infoMesssages = []; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/place', { + '../service/mget': (esclient, query, callback) => { + t.equal(esclient, 'this is the esclient'); + t.deepEqual(query, [ + { + _index: 'indexName value', + _type: 'layer1', + _id: 'id1' + }, + { + _index: 'indexName value', + _type: 'layer2', + _id: 'id2' + } + ]); + + const docs = [{}, {}]; + + callback(undefined, docs); + } + })(config, esclient); + + const req = { + clean: { + ids: [ + { + id: 'id1', + layers: 'layer1' + }, + { + id: 'id2', + layers: 'layer2' + } + ] + }, + errors: [], + warnings: [] + }; + const res = {}; + + const next = () => { + t.deepEqual(req.errors, []); + t.deepEqual(req.warnings, []); + t.deepEquals(res.data, [{}, {}]); + t.end(); + }; + + controller(req, res, next); + + }); + }; -// functionally test controller (backend success) -module.exports.tests.functional_success = function(test, common) { - - // expected geojson features for 'client/place/ok/1' fixture - var expected = [{ - type: 'Feature', - geometry: { - type: 'Point', - coordinates: [ -50.5, 100.1 ] - }, - properties: { - id: 'myid1', - layer: 'mytype1', - text: 'test name1, city1, state1' - } - }, { - type: 'Feature', - geometry: { - type: 'Point', - coordinates: [ -51.5, 100.2 ] - }, - properties: { - id: 'myid2', - layer: 'mytype2', - text: 'test name2, city2, state2' - } - }]; - - 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'); +module.exports.tests.error_conditions = (test, common) => { + test('non-empty req.errors should ', (t) => { + const esclient = () => { + throw new Error('esclient should not have been called'); + }; + const controller = setup( {}, esclient ); + + // the existence of `errors` means that a sanitizer detected an error, + // so don't call the esclient + const req = { + errors: ['error'] + }; + const res = { }; + + t.doesNotThrow(() => { + controller(req, res, () => {}); }); - var controller = setup( fakeDefaultConfig, backend ); - var res = { - status: function( code ){ - t.equal(code, 200, 'status set'); - return res; + t.end(); + + }); + + test('mgetService returning error should add to req.errors and ignore docs', (t) => { + const config = { + indexName: 'indexName value' + }; + const esclient = 'this is the esclient'; + + // request timeout messages willl be written here + const infoMesssages = []; + + const nonTimeoutError = { + status: 500, + displayName: 'InternalServerError', + message: 'an internal server error occurred' + }; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/place', { + '../service/mget': (esclient, query, callback) => { + const docs = [{}, {}]; + + callback(nonTimeoutError, docs); + } + })(config, esclient); + + const req = { + clean: { + ids: [ + { + id: 'id1', + layers: 'layer1' + } + ] }, - json: function( json ){ - t.equal(typeof json, 'object', 'returns json'); - t.equal(typeof json.date, 'number', 'date set'); - t.equal(json.type, 'FeatureCollection', 'valid geojson'); - t.true(Array.isArray(json.features), 'features is array'); - t.deepEqual(json.features, expected, 'values correctly mapped'); + errors: [], + warnings: [] + }; + const res = {}; + + const next = () => { + t.deepEqual(req.errors, [nonTimeoutError]); + t.deepEqual(req.warnings, []); + t.deepEquals(res.data, undefined); + t.end(); + }; + + controller(req, res, next); + + }); + +}; + +module.exports.tests.timeout = function(test, common) { + test('default # of request timeout retries should be 3', (t) => { + const config = { + indexName: 'indexName value' + }; + const esclient = 'this is the esclient'; + + let searchServiceCallCount = 0; + + const timeoutError = { + status: 408, + displayName: 'RequestTimeout', + message: 'Request Timeout after 17ms' + }; + + // request timeout messages willl be written here + const infoMesssages = []; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/place', { + '../service/mget': (esclient, cmd, callback) => { + // not that the searchService got called + searchServiceCallCount++; + + callback(timeoutError); + }, + 'pelias-logger': { + get: (service) => { + t.equal(service, 'api'); + return { + info: (msg) => { + infoMesssages.push(msg); + }, + debug: () => {} + }; + } } + })(config, esclient); + + const req = { + clean: { + ids: [ + { + id: 'id1', + layers: 'layer1' + } + ] + }, + errors: [], + warnings: [] }; - var req = { clean: { ids: [ {'id' : 123, layers: [ 'a' ] } ] }, errors: [], warnings: [] }; - var next = function next() { - t.equal(req.errors.length, 0, 'next was called without error'); + const res = {}; + + const next = () => { + t.equal(searchServiceCallCount, 3+1); + + t.ok(infoMesssages.indexOf('request timed out on attempt 1, retrying') !== -1); + t.ok(infoMesssages.indexOf('request timed out on attempt 2, retrying') !== -1); + t.ok(infoMesssages.indexOf('request timed out on attempt 3, retrying') !== -1); + + t.deepEqual(req.errors, [timeoutError]); + t.deepEqual(res, {}); t.end(); }; - controller(req, res, next ); + + controller(req, res, next); + }); - test('functional success with custom index name', function(t) { - var fakeCustomizedConfig = { - indexName: 'alternateindexname' + test('explicit apiConfig.requestRetries should retry that many times', (t) => { + const config = { + indexName: 'indexName value', + requestRetries: 17 }; + const esclient = 'this is the esclient'; - var backend = mockBackend( 'client/mget/ok/1', function( cmd ){ - t.deepEqual(cmd, { body: { docs: [ { _id: 123, _index: 'alternateindexname', _type: [ 'a' ] } ] } }, 'correct backend command'); - }); - var controller = setup( fakeCustomizedConfig, backend ); - var res = { - status: function( code ){ - t.equal(code, 200, 'status set'); - return res; + let searchServiceCallCount = 0; + + const timeoutError = { + status: 408, + displayName: 'RequestTimeout', + message: 'Request Timeout after 17ms' + }; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/place', { + '../service/mget': (esclient, cmd, callback) => { + // not that the searchService got called + searchServiceCallCount++; + + callback(timeoutError); + } + })(config, esclient); + + const req = { + clean: { + ids: [ + { + id: 'id1', + layers: 'layer1' + } + ] }, - json: function( json ){ - t.equal(typeof json, 'object', 'returns json'); - t.equal(typeof json.date, 'number', 'date set'); - t.equal(json.type, 'FeatureCollection', 'valid geojson'); - t.true(Array.isArray(json.features), 'features is array'); - t.deepEqual(json.features, expected, 'values correctly mapped'); + errors: [], + warnings: [] + }; + const res = {}; + + const next = () => { + t.equal(searchServiceCallCount, 17+1); + t.end(); + }; + + controller(req, res, next); + + }); + + test('only status code 408 should be considered a retryable request', (t) => { + const config = { + indexName: 'indexName value' + }; + const esclient = 'this is the esclient'; + + let searchServiceCallCount = 0; + + const nonTimeoutError = { + status: 500, + displayName: 'InternalServerError', + message: 'an internal server error occurred' + }; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/place', { + '../service/mget': (esclient, cmd, callback) => { + // not that the searchService got called + searchServiceCallCount++; + + callback(nonTimeoutError); } + })(config, esclient); + + const req = { + clean: { + ids: [ + { + id: 'id1', + layers: 'layer1' + } + ] + }, + errors: [], + warnings: [] }; - var req = { clean: { ids: [ {'id' : 123, layers: [ 'a' ] } ] }, errors: [], warnings: [] }; - var next = function next() { - t.equal(req.errors.length, 0, 'next was called without error'); + const res = {}; + + const next = () => { + t.equal(searchServiceCallCount, 1); + t.deepEqual(req.errors, [nonTimeoutError]); t.end(); }; - controller(req, res, next ); + + controller(req, res, next); + }); -}; -// functionally test controller (backend failure) -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'); - }); - var controller = setup( fakeDefaultConfig, backend ); - var req = { clean: { ids: [ {'id' : 123, layers: [ 'b' ] } ] }, errors: [], warnings: [] }; - var next = function( message ){ - t.equal(req.errors[0],'an elasticsearch error occurred','error passed to errorHandler'); + test('string error should not retry and be logged as-is', (t) => { + const config = { + indexName: 'indexName value' + }; + const esclient = 'this is the esclient'; + + let searchServiceCallCount = 0; + + const stringTypeError = 'this is an error string'; + + // a controller that validates the esclient and cmd that was passed to the search service + const controller = proxyquire('../../../controller/place', { + '../service/mget': (esclient, cmd, callback) => { + // not that the searchService got called + searchServiceCallCount++; + + callback(stringTypeError); + } + })(config, esclient); + + const req = { + clean: { + ids: [ + { + id: 'id1', + layers: 'layer1' + } + ] + }, + errors: [], + warnings: [] + }; + const res = {}; + + const next = () => { + t.equal(searchServiceCallCount, 1); + t.deepEqual(req.errors, [stringTypeError]); t.end(); }; - controller(req, undefined, next ); + + controller(req, res, next); + }); + }; -module.exports.all = function (tape, common) { +module.exports.all = (tape, common) => { function test(name, testFunction) { return tape('GET /place ' + name, testFunction); } - for( var testCase in module.exports.tests ){ + for( const testCase in module.exports.tests ){ module.exports.tests[testCase](test, common); } }; From ece98a2ae6b2579247456b15dd650f1f46bdee2f Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 22:32:48 -0500 Subject: [PATCH 31/43] converted to let/const --- controller/search.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/controller/search.js b/controller/search.js index 4870f53a..520a490e 100644 --- a/controller/search.js +++ b/controller/search.js @@ -26,22 +26,20 @@ function setup( apiConfig, esclient, query ){ return next(); } - var cleanOutput = _.cloneDeep(req.clean); + let cleanOutput = _.cloneDeep(req.clean); if (logging.isDNT(req)) { cleanOutput = logging.removeFields(cleanOutput); } // log clean parameters for stats logger.info('[req]', 'endpoint=' + req.path, cleanOutput); - var renderedQuery = query(req.clean); + const renderedQuery = query(req.clean); // if there's no query to call ES with, skip the service if (_.isUndefined(renderedQuery)) { return next(); } - logger.debug( '[ES req]', cmd ); - // options for retry // maxRetries is from the API config with default of 3 // factor of 1 means that each retry attempt will esclient requestTimeout @@ -55,12 +53,14 @@ function setup( apiConfig, esclient, query ){ const operation = retry.operation(operationOptions); // elasticsearch command - var cmd = { + const cmd = { index: apiConfig.indexName, searchType: 'dfs_query_then_fetch', body: renderedQuery.body }; + logger.debug( '[ES req]', cmd ); + operation.attempt((currentAttempt) => { // query elasticsearch searchService( esclient, cmd, function( err, docs, meta ){ From 9da4b9d236ab97b8a1c553ab7b38477b5ce339a6 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 22:35:32 -0500 Subject: [PATCH 32/43] standardized on name to match controller/search --- controller/place.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/controller/place.js b/controller/place.js index becae8fc..5d2703cc 100644 --- a/controller/place.js +++ b/controller/place.js @@ -33,7 +33,7 @@ function setup( apiConfig, esclient ){ // setup a new operation const operation = retry.operation(operationOptions); - const query = req.clean.ids.map( function(id) { + const cmd = req.clean.ids.map( function(id) { return { _index: apiConfig.indexName, _type: id.layers, @@ -41,10 +41,10 @@ function setup( apiConfig, esclient ){ }; }); - logger.debug( '[ES req]', query ); + logger.debug( '[ES req]', cmd ); operation.attempt((currentAttempt) => { - mgetService( esclient, query, function( err, docs ) { + mgetService( esclient, cmd, function( err, docs ) { // returns true if the operation should be attempted again // (handles bookkeeping of maxRetries) // only consider for status 408 (request timeout) From ac0776e2be884c855032cc45ff75eaf1ff3c813d Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 22:37:37 -0500 Subject: [PATCH 33/43] match error handling to controller/search --- controller/place.js | 6 +++++- test/unit/controller/place.js | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/controller/place.js b/controller/place.js index 5d2703cc..7e49aee3 100644 --- a/controller/place.js +++ b/controller/place.js @@ -55,7 +55,11 @@ function setup( apiConfig, esclient ){ // error handler if( err ){ - req.errors.push( err ); + if (_.isObject(err) && err.message) { + req.errors.push( err.message ); + } else { + req.errors.push( err ); + } } // set response data else { diff --git a/test/unit/controller/place.js b/test/unit/controller/place.js index 5f54ba6d..22622e05 100644 --- a/test/unit/controller/place.js +++ b/test/unit/controller/place.js @@ -137,7 +137,7 @@ module.exports.tests.error_conditions = (test, common) => { const res = {}; const next = () => { - t.deepEqual(req.errors, [nonTimeoutError]); + t.deepEqual(req.errors, [nonTimeoutError.message]); t.deepEqual(req.warnings, []); t.deepEquals(res.data, undefined); t.end(); @@ -209,7 +209,7 @@ module.exports.tests.timeout = function(test, common) { t.ok(infoMesssages.indexOf('request timed out on attempt 2, retrying') !== -1); t.ok(infoMesssages.indexOf('request timed out on attempt 3, retrying') !== -1); - t.deepEqual(req.errors, [timeoutError]); + t.deepEqual(req.errors, [timeoutError.message]); t.deepEqual(res, {}); t.end(); }; @@ -306,7 +306,7 @@ module.exports.tests.timeout = function(test, common) { const next = () => { t.equal(searchServiceCallCount, 1); - t.deepEqual(req.errors, [nonTimeoutError]); + t.deepEqual(req.errors, [nonTimeoutError.message]); t.end(); }; From 5148198e1d007afb25fd75b5718719208e5fd0c1 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 22:39:46 -0500 Subject: [PATCH 34/43] added helper functions --- controller/search.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/controller/search.js b/controller/search.js index 520a490e..a12b620d 100644 --- a/controller/search.js +++ b/controller/search.js @@ -8,6 +8,14 @@ const logging = require( '../helper/logging' ); const retry = require('retry'); function setup( apiConfig, esclient, query ){ + function requestHasErrors(request) { + return _.get(request, 'errors', []).length > 0; + } + + function responseHasData(response) { + return _.get(response, 'data', []).length > 0; + } + function isRequestTimeout(err) { return _.get(err, 'status') === 408; } @@ -15,14 +23,14 @@ function setup( apiConfig, esclient, query ){ function controller( req, res, next ){ // do not run controller when a request // validation error has occurred. - if (_.get(req, 'errors', []).length > 0) { + if (requestHasErrors(req)) { return next(); } // do not run controller if there are already results // this was added during libpostal integration. if the libpostal parse/query // doesn't return anything then fallback to old search-engine-y behavior - if (_.get(res, 'data', []).length > 0) { + if (responseHasData(res)) { return next(); } From 3c9d39d1df9dd75797a56952281dcb6b42e983b0 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 22:41:02 -0500 Subject: [PATCH 35/43] removed unused file --- test/unit/mock/alpha3.json | 251 ------------------------------------- 1 file changed, 251 deletions(-) delete mode 100644 test/unit/mock/alpha3.json diff --git a/test/unit/mock/alpha3.json b/test/unit/mock/alpha3.json deleted file mode 100644 index 7148e878..00000000 --- a/test/unit/mock/alpha3.json +++ /dev/null @@ -1,251 +0,0 @@ -{ - "ABW": "Aruba", - "AFG": "Afghanistan", - "AGO": "Angola", - "AIA": "Anguilla", - "ALA": "Åland Islands", - "ALB": "Albania", - "AND": "Andorra", - "ARE": "United Arab Emirates", - "ARG": "Argentina", - "ARM": "Armenia", - "ASM": "American Samoa", - "ATA": "Antarctica", - "ATF": "French Southern Territories", - "ATG": "Antigua and Barbuda", - "AUS": "Australia", - "AUT": "Austria", - "AZE": "Azerbaijan", - "BDI": "Burundi", - "BEL": "Belgium", - "BEN": "Benin", - "BES": "Bonaire, Sint Eustatius and Saba", - "BFA": "Burkina Faso", - "BGD": "Bangladesh", - "BGR": "Bulgaria", - "BHR": "Bahrain", - "BHS": "Bahamas", - "BIH": "Bosnia and Herzegovina", - "BLM": "Saint Barthélemy", - "BLR": "Belarus", - "BLZ": "Belize", - "BMU": "Bermuda", - "BOL": "Bolivia, Plurinational State of", - "BRA": "Brazil", - "BRB": "Barbados", - "BRN": "Brunei Darussalam", - "BTN": "Bhutan", - "BVT": "Bouvet Island", - "BWA": "Botswana", - "CAF": "Central African Republic", - "CAN": "Canada", - "CCK": "Cocos (Keeling) Islands", - "CHE": "Switzerland", - "CHL": "Chile", - "CHN": "China", - "CIV": "Côte d'Ivoire", - "CMR": "Cameroon", - "COD": "Congo, the Democratic Republic of the", - "COG": "Congo", - "COK": "Cook Islands", - "COL": "Colombia", - "COM": "Comoros", - "CPV": "Cabo Verde", - "CRI": "Costa Rica", - "CUB": "Cuba", - "CUW": "Curaçao", - "CXR": "Christmas Island", - "CYM": "Cayman Islands", - "CYP": "Cyprus", - "CZE": "Czech Republic", - "DEU": "Germany", - "DJI": "Djibouti", - "DMA": "Dominica", - "DNK": "Denmark", - "DOM": "Dominican Republic", - "DZA": "Algeria", - "ECU": "Ecuador", - "EGY": "Egypt", - "ERI": "Eritrea", - "ESH": "Western Sahara", - "ESP": "Spain", - "EST": "Estonia", - "ETH": "Ethiopia", - "FIN": "Finland", - "FJI": "Fiji", - "FLK": "Falkland Islands (Malvinas)", - "FRA": "France", - "FRO": "Faroe Islands", - "FSM": "Micronesia, Federated States of", - "GAB": "Gabon", - "GBR": "United Kingdom", - "GEO": "Georgia", - "GGY": "Guernsey", - "GHA": "Ghana", - "GIB": "Gibraltar", - "GIN": "Guinea", - "GLP": "Guadeloupe", - "GMB": "Gambia", - "GNB": "Guinea-Bissau", - "GNQ": "Equatorial Guinea", - "GRC": "Greece", - "GRD": "Grenada", - "GRL": "Greenland", - "GTM": "Guatemala", - "GUF": "French Guiana", - "GUM": "Guam", - "GUY": "Guyana", - "HKG": "Hong Kong", - "HMD": "Heard Island and McDonald Islands", - "HND": "Honduras", - "HRV": "Croatia", - "HTI": "Haiti", - "HUN": "Hungary", - "IDN": "Indonesia", - "IMN": "Isle of Man", - "IND": "India", - "IOT": "British Indian Ocean Territory", - "IRL": "Ireland", - "IRN": "Iran, Islamic Republic of", - "IRQ": "Iraq", - "ISL": "Iceland", - "ISR": "Israel", - "ITA": "Italy", - "JAM": "Jamaica", - "JEY": "Jersey", - "JOR": "Jordan", - "JPN": "Japan", - "KAZ": "Kazakhstan", - "KEN": "Kenya", - "KGZ": "Kyrgyzstan", - "KHM": "Cambodia", - "KIR": "Kiribati", - "KNA": "Saint Kitts and Nevis", - "KOR": "Korea, Republic of", - "KWT": "Kuwait", - "LAO": "Lao People's Democratic Republic", - "LBN": "Lebanon", - "LBR": "Liberia", - "LBY": "Libya", - "LCA": "Saint Lucia", - "LIE": "Liechtenstein", - "LKA": "Sri Lanka", - "LSO": "Lesotho", - "LTU": "Lithuania", - "LUX": "Luxembourg", - "LVA": "Latvia", - "MAC": "Macao", - "MAF": "Saint Martin (French part)", - "MAR": "Morocco", - "MCO": "Monaco", - "MDA": "Moldova, Republic of", - "MDG": "Madagascar", - "MDV": "Maldives", - "MEX": "Mexico", - "MHL": "Marshall Islands", - "MKD": "Macedonia, the former Yugoslav Republic of", - "MLI": "Mali", - "MLT": "Malta", - "MMR": "Myanmar", - "MNE": "Montenegro", - "MNG": "Mongolia", - "MNP": "Northern Mariana Islands", - "MOZ": "Mozambique", - "MRT": "Mauritania", - "MSR": "Montserrat", - "MTQ": "Martinique", - "MUS": "Mauritius", - "MWI": "Malawi", - "MYS": "Malaysia", - "MYT": "Mayotte", - "NAM": "Namibia", - "NCL": "New Caledonia", - "NER": "Niger", - "NFK": "Norfolk Island", - "NGA": "Nigeria", - "NIC": "Nicaragua", - "NIU": "Niue", - "NLD": "Netherlands", - "NOR": "Norway", - "NPL": "Nepal", - "NRU": "Nauru", - "NZL": "New Zealand", - "OMN": "Oman", - "PAK": "Pakistan", - "PAN": "Panama", - "PCN": "Pitcairn", - "PER": "Peru", - "PHL": "Philippines", - "PLW": "Palau", - "PNG": "Papua New Guinea", - "POL": "Poland", - "PRI": "Puerto Rico", - "PRK": "Korea, Democratic People's Republic of", - "PRT": "Portugal", - "PRY": "Paraguay", - "PSE": "Palestine, State of", - "PYF": "French Polynesia", - "QAT": "Qatar", - "REU": "Réunion", - "ROU": "Romania", - "RUS": "Russian Federation", - "RWA": "Rwanda", - "SAU": "Saudi Arabia", - "SDN": "Sudan", - "SEN": "Senegal", - "SGP": "Singapore", - "SGS": "South Georgia and the South Sandwich Islands", - "SHN": "Saint Helena, Ascension and Tristan da Cunha", - "SJM": "Svalbard and Jan Mayen", - "SLB": "Solomon Islands", - "SLE": "Sierra Leone", - "SLV": "El Salvador", - "SMR": "San Marino", - "SOM": "Somalia", - "SPM": "Saint Pierre and Miquelon", - "SRB": "Serbia", - "SSD": "South Sudan", - "STP": "Sao Tome and Principe", - "SUR": "Suriname", - "SVK": "Slovakia", - "SVN": "Slovenia", - "SWE": "Sweden", - "SWZ": "Swaziland", - "SXM": "Sint Maarten (Dutch part)", - "SYC": "Seychelles", - "SYR": "Syrian Arab Republic", - "TCA": "Turks and Caicos Islands", - "TCD": "Chad", - "TGO": "Togo", - "THA": "Thailand", - "TJK": "Tajikistan", - "TKL": "Tokelau", - "TKM": "Turkmenistan", - "TLS": "Timor-Leste", - "TON": "Tonga", - "TTO": "Trinidad and Tobago", - "TUN": "Tunisia", - "TUR": "Turkey", - "TUV": "Tuvalu", - "TWN": "Taiwan, Province of China", - "TZA": "Tanzania, United Republic of", - "UGA": "Uganda", - "UKR": "Ukraine", - "UMI": "United States Minor Outlying Islands", - "URY": "Uruguay", - "USA": "United States", - "UZB": "Uzbekistan", - "VAT": "Holy See (Vatican City State)", - "VCT": "Saint Vincent and the Grenadines", - "VEN": "Venezuela, Bolivarian Republic of", - "VGB": "Virgin Islands, British", - "VIR": "Virgin Islands, U.S.", - "VNM": "Viet Nam", - "VUT": "Vanuatu", - "WLF": "Wallis and Futuna", - "WSM": "Samoa", - "YEM": "Yemen", - "ZAF": "South Africa", - "ZMB": "Zambia", - "ZWE": "Zimbabwe" - } \ No newline at end of file From 11b39b3f54405287bb23971e8667d3363b44819d Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Thu, 19 Jan 2017 23:17:42 -0500 Subject: [PATCH 36/43] removed unused mock backend response --- test/unit/mock/backend.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/unit/mock/backend.js b/test/unit/mock/backend.js index 04e447e9..4751acc0 100644 --- a/test/unit/mock/backend.js +++ b/test/unit/mock/backend.js @@ -1,8 +1,5 @@ var responses = {}; -responses['client/suggest/fail/1'] = function( cmd, cb ){ - return cb( 'an elasticsearch error occurred' ); -}; responses['client/search/ok/1'] = function( cmd, cb ){ return cb( undefined, searchEnvelope([{ _id: 'myid1', From 92fc161bc9bcca0cf3b2a1469b54d0f6bf3dd08e Mon Sep 17 00:00:00 2001 From: greenkeeperio-bot Date: Sat, 21 Jan 2017 17:13:01 -0500 Subject: [PATCH 37/43] chore(package): update express-http-proxy to version 0.11.0 https://greenkeeper.io/ --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 398ce8c8..faedd5ec 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,7 @@ "elasticsearch": "^12.0.1", "elasticsearch-exceptions": "0.0.4", "express": "^4.8.8", - "express-http-proxy": "^0.10.0", + "express-http-proxy": "^0.11.0", "extend": "3.0.0", "geojson": "^0.4.0", "geojson-extent": "^0.3.1", From 901b6826cac01b5a069c66b3eebe95f4227f35ed Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Mon, 23 Jan 2017 15:50:45 -0500 Subject: [PATCH 38/43] updated wording of test names and comments --- test/unit/controller/search.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/controller/search.js b/test/unit/controller/search.js index 817fc821..78e0b2d2 100644 --- a/test/unit/controller/search.js +++ b/test/unit/controller/search.js @@ -495,7 +495,7 @@ module.exports.tests.timeout = function(test, common) { }; module.exports.tests.existing_errors = function(test, common) { - test('req with errors should not call backend', function(t) { + test('req with errors should not call esclient or query', function(t) { const esclient = () => { throw new Error('esclient should not have been called'); }; @@ -521,7 +521,7 @@ module.exports.tests.existing_errors = function(test, common) { }; module.exports.tests.existing_results = function(test, common) { - test('res with existing data should not call backend', function(t) { + test('res with existing data should not call esclient or query', function(t) { const esclient = () => { throw new Error('esclient should not have been called'); }; @@ -532,7 +532,7 @@ module.exports.tests.existing_results = function(test, common) { const req = { }; // the existence of `data` means that there are already results so - // don't call the backend/query + // don't call esclient or query const res = { data: [{}] }; const next = function() { From 73ef71d863bef4c5750de850c44cd94b180e2cf4 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Mon, 23 Jan 2017 15:59:40 -0500 Subject: [PATCH 39/43] moved helper functions outside of setup scope --- controller/place.js | 14 +++++++------- controller/search.js | 20 ++++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/controller/place.js b/controller/place.js index 7e49aee3..9e41a47c 100644 --- a/controller/place.js +++ b/controller/place.js @@ -6,15 +6,15 @@ const retry = require('retry'); const mgetService = require('../service/mget'); const logger = require('pelias-logger').get('api'); -function setup( apiConfig, esclient ){ - function requestHasErrors(request) { - return _.get(request, 'errors', []).length > 0; - } +function requestHasErrors(request) { + return _.get(request, 'errors', []).length > 0; +} - function isRequestTimeout(err) { - return _.get(err, 'status') === 408; - } +function isRequestTimeout(err) { + return _.get(err, 'status') === 408; +} +function setup( apiConfig, esclient ){ function controller( req, res, next ){ // do not run controller when a request validation error has occurred. if (requestHasErrors(req)){ diff --git a/controller/search.js b/controller/search.js index a12b620d..168e47c4 100644 --- a/controller/search.js +++ b/controller/search.js @@ -7,19 +7,19 @@ const logger = require('pelias-logger').get('api'); const logging = require( '../helper/logging' ); const retry = require('retry'); -function setup( apiConfig, esclient, query ){ - function requestHasErrors(request) { - return _.get(request, 'errors', []).length > 0; - } +function requestHasErrors(request) { + return _.get(request, 'errors', []).length > 0; +} - function responseHasData(response) { - return _.get(response, 'data', []).length > 0; - } +function responseHasData(response) { + return _.get(response, 'data', []).length > 0; +} - function isRequestTimeout(err) { - return _.get(err, 'status') === 408; - } +function isRequestTimeout(err) { + return _.get(err, 'status') === 408; +} +function setup( apiConfig, esclient, query ){ function controller( req, res, next ){ // do not run controller when a request // validation error has occurred. From 62a2aa827f53fb167b4349132ad8c670769ec89d Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Mon, 23 Jan 2017 16:19:13 -0500 Subject: [PATCH 40/43] added api.requestRetries to schema and tests --- src/configValidation.js | 1 + test/unit/src/configValidation.js | 64 ++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/configValidation.js b/src/configValidation.js index 621f883b..cd738427 100644 --- a/src/configValidation.js +++ b/src/configValidation.js @@ -22,6 +22,7 @@ const schema = Joi.object().keys({ legacyUrl: Joi.string(), accessLog: Joi.string(), relativeScores: Joi.boolean(), + requestRetries: Joi.number().integer().min(0), localization: Joi.object().keys({ flipNumberAndStreetCountries: Joi.array().items(Joi.string().regex(/^[A-Z]{3}$/)) }).unknown(false) diff --git a/test/unit/src/configValidation.js b/test/unit/src/configValidation.js index 91b34312..e492fd19 100644 --- a/test/unit/src/configValidation.js +++ b/test/unit/src/configValidation.js @@ -16,7 +16,8 @@ module.exports.tests.completely_valid = function(test, common) { relativeScores: true, localization: { flipNumberAndStreetCountries: ['ABC', 'DEF'] - } + }, + requestRetries: 19 }, esclient: { requestTimeout: 17 @@ -318,6 +319,65 @@ module.exports.tests.api_validation = function(test, common) { }); + test('config with non-number api.requestRetries should throw error', function(t) { + [null, 'string', {}, [], false].forEach((value) => { + var config = { + api: { + version: 'version value', + indexName: 'index name value', + host: 'host value', + requestRetries: value + }, + esclient: {} + }; + + t.throws(function() { + configValidation.validate(config); + }, /"requestRetries" must be a number/, 'api.requestRetries should be a number'); + }); + + t.end(); + + }); + + test('config with non-integer api.requestRetries should throw error', function(t) { + var config = { + api: { + version: 'version value', + indexName: 'index name value', + host: 'host value', + requestRetries: 17.3 + }, + esclient: {} + }; + + t.throws(function() { + configValidation.validate(config); + }, /"requestRetries" must be an integer/, 'api.requestRetries should be an integer'); + + t.end(); + + }); + + test('config with negative api.requestRetries should throw error', function(t) { + var config = { + api: { + version: 'version value', + indexName: 'index name value', + host: 'host value', + requestRetries: -1 + }, + esclient: {} + }; + + t.throws(function() { + configValidation.validate(config); + }, /"requestRetries" must be larger than or equal to 0/, 'api.requestRetries must be positive'); + + t.end(); + + }); + }; module.exports.tests.esclient_validation = function(test, common) { @@ -358,7 +418,7 @@ module.exports.tests.esclient_validation = function(test, common) { }); - test('config with non-integer esclient.requestTimeout should throw error', function(t) { + test('config with non-number esclient.requestTimeout should throw error', function(t) { [null, 'string', {}, [], false].forEach((value) => { var config = { api: { From 2288e227a356494ae28bbcc339b87892ace79a8b Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Wed, 25 Jan 2017 12:22:03 -0500 Subject: [PATCH 41/43] converted configValidation to basic schema object to inject into `config.generate` fixed tests --- src/configValidation.js => schema.js | 13 +- test/unit/run.js | 2 +- .../{src/configValidation.js => schema.js} | 188 +++++++++--------- 3 files changed, 91 insertions(+), 112 deletions(-) rename src/configValidation.js => schema.js (81%) rename test/unit/{src/configValidation.js => schema.js} (62%) diff --git a/src/configValidation.js b/schema.js similarity index 81% rename from src/configValidation.js rename to schema.js index cd738427..61e6c258 100644 --- a/src/configValidation.js +++ b/schema.js @@ -14,7 +14,7 @@ const Joi = require('joi'); // * api.relativeScores (boolean) // * api.legacyUrl (string) // * api.localization (flipNumberAndStreetCountries is array of 3 character strings) -const schema = Joi.object().keys({ +module.exports = Joi.object().keys({ api: Joi.object().keys({ version: Joi.string(), indexName: Joi.string(), @@ -32,14 +32,3 @@ const schema = Joi.object().keys({ requestTimeout: Joi.number().integer().min(0) }).unknown(true) }).requiredKeys('api', 'esclient').unknown(true); - -module.exports = { - validate: function validate(config) { - Joi.validate(config, schema, (err) => { - if (err) { - throw new Error(err.details[0].message); - } - }); - } - -}; diff --git a/test/unit/run.js b/test/unit/run.js index d4ad671f..71b14e27 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -10,6 +10,7 @@ var common = { var tests = [ require('./app'), + require('./schema'), require('./controller/index'), require('./controller/place'), require('./controller/search'), @@ -62,7 +63,6 @@ var tests = [ require('./sanitizer/_deprecate_quattroshapes'), require('./sanitizer/_categories'), require('./sanitizer/nearby'), - require('./src/configValidation'), require('./sanitizer/autocomplete'), require('./sanitizer/structured_geocoding'), require('./sanitizer/place'), diff --git a/test/unit/src/configValidation.js b/test/unit/schema.js similarity index 62% rename from test/unit/src/configValidation.js rename to test/unit/schema.js index e492fd19..e1f9cfd2 100644 --- a/test/unit/src/configValidation.js +++ b/test/unit/schema.js @@ -1,11 +1,20 @@ 'use strict'; -const configValidation = require('../../../src/configValidation'); +const Joi = require('joi'); +const schema = require('../../schema'); + +function validate(config) { + Joi.validate(config, schema, (err, value) => { + if (err) { + throw new Error(err.details[0].message); + } + }); +} module.exports.tests = {}; -module.exports.tests.completely_valid = function(test, common) { - test('all valid configuration elements should not throw error', function(t) { +module.exports.tests.completely_valid = (test, common) => { + test('all valid configuration elements should not throw error', (t) => { var config = { api: { version: 'version value', @@ -24,15 +33,12 @@ module.exports.tests.completely_valid = function(test, common) { } }; - t.doesNotThrow(function() { - configValidation.validate(config); - }); - + t.doesNotThrow(validate.bind(config)); t.end(); }); - test('basic valid configurtaion should not throw error', function(t) { + test('basic valid configuration should not throw error', (t) => { var config = { api: { version: 'version value', @@ -44,30 +50,25 @@ module.exports.tests.completely_valid = function(test, common) { } }; - t.doesNotThrow(function() { - configValidation.validate(config); - }); - + t.doesNotThrow(validate.bind(config)); t.end(); }); }; -module.exports.tests.api_validation = function(test, common) { - test('config without api should throw error', function(t) { +module.exports.tests.api_validation = (test, common) => { + test('config without api should throw error', (t) => { var config = { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"api" is required/, 'api should exist'); + t.throws(validate.bind(null, config), /"api" is required/, 'api should exist'); t.end(); }); - test('config without unknown field in api should not throw error', function(t) { + test('config without unknown field in api should not throw error', (t) => { var config = { api: { version: 'version value', @@ -78,14 +79,12 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.doesNotThrow(function() { - configValidation.validate(config); - }, 'unknown properties should be allowed'); + t.doesNotThrow(validate.bind(null, config), 'unknown properties should be allowed'); t.end(); }); - test('non-string api.version should throw error', function(t) { + test('non-string api.version should throw error', (t) => { [null, 17, {}, [], true].forEach((value) => { var config = { api: { @@ -96,16 +95,15 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"version" must be a string/); + t.throws(validate.bind(null, config), /"version" must be a string/); + }); t.end(); }); - test('non-string api.indexName should throw error', function(t) { + test('non-string api.indexName should throw error', (t) => { [null, 17, {}, [], true].forEach((value) => { var config = { api: { @@ -116,16 +114,15 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"indexName" must be a string/); + t.throws(validate.bind(null, config), /"indexName" must be a string/); + }); t.end(); }); - test('non-string api.host should throw error', function(t) { + test('non-string api.host should throw error', (t) => { [null, 17, {}, [], true].forEach((value) => { var config = { api: { @@ -136,16 +133,15 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"host" must be a string/); + t.throws(validate.bind(null, config), /"host" must be a string/); + }); t.end(); }); - test('non-string api.legacyUrl should throw error', function(t) { + test('non-string api.legacyUrl should throw error', (t) => { [null, 17, {}, [], true].forEach((value) => { var config = { api: { @@ -157,16 +153,15 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"legacyUrl" must be a string/); + t.throws(validate.bind(null, config), /"legacyUrl" must be a string/); + }); t.end(); }); - test('non-string api.accessLog should throw error', function(t) { + test('non-string api.accessLog should throw error', (t) => { [null, 17, {}, [], true].forEach((value) => { var config = { api: { @@ -178,16 +173,15 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"accessLog" must be a string/); + t.throws(validate.bind(null, config), /"accessLog" must be a string/); + }); t.end(); }); - test('non-boolean api.relativeScores should throw error', function(t) { + test('non-boolean api.relativeScores should throw error', (t) => { [null, 17, {}, [], 'string'].forEach((value) => { var config = { api: { @@ -199,16 +193,15 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"relativeScores" must be a boolean/); + t.throws(validate.bind(null, config), /"relativeScores" must be a boolean/); + }); t.end(); }); - test('non-object api.localization should throw error', function(t) { + test('non-object api.localization should throw error', (t) => { [null, 17, false, [], 'string'].forEach((value) => { var config = { api: { @@ -220,16 +213,15 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"localization" must be an object/); + t.throws(validate.bind(null, config), /"localization" must be an object/); + }); t.end(); }); - test('unknown properties in api.localization should throw error', function(t) { + test('unknown properties in api.localization should throw error', (t) => { var config = { api: { version: 'version value', @@ -242,15 +234,13 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"unknown_property" is not allowed/); + t.throws(validate.bind(null, config), /"unknown_property" is not allowed/); t.end(); }); - test('non-array api.localization.flipNumberAndStreetCountries should throw error', function(t) { + test('non-array api.localization.flipNumberAndStreetCountries should throw error', (t) => { [null, 17, {}, false, 'string'].forEach((value) => { var config = { api: { @@ -264,16 +254,17 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"flipNumberAndStreetCountries" must be an array/); + t.throws( + validate.bind(null, config), + /"flipNumberAndStreetCountries" must be an array/); + }); t.end(); }); - test('non-string api.localization.flipNumberAndStreetCountries elements should throw error', function(t) { + test('non-string api.localization.flipNumberAndStreetCountries elements should throw error', (t) => { [null, 17, {}, false, []].forEach((value) => { var config = { api: { @@ -287,16 +278,15 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"0" must be a string/); + t.throws(validate.bind(null, config), /"0" must be a string/); + }); t.end(); }); - test('non-3-char api.localization.flipNumberAndStreetCountries elements should throw error', function(t) { + test('non-3-char api.localization.flipNumberAndStreetCountries elements should throw error', (t) => { ['AB', 'ABCD'].forEach((value) => { var config = { api: { @@ -310,16 +300,14 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /fails to match the required pattern/); + t.throws(validate.bind(null, config), /fails to match the required pattern/); }); t.end(); }); - test('config with non-number api.requestRetries should throw error', function(t) { + test('config with non-number api.requestRetries should throw error', (t) => { [null, 'string', {}, [], false].forEach((value) => { var config = { api: { @@ -331,16 +319,17 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"requestRetries" must be a number/, 'api.requestRetries should be a number'); + t.throws( + validate.bind(null, config), + /"requestRetries" must be a number/, 'api.requestRetries should be a number'); + }); t.end(); }); - test('config with non-integer api.requestRetries should throw error', function(t) { + test('config with non-integer api.requestRetries should throw error', (t) => { var config = { api: { version: 'version value', @@ -351,15 +340,15 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"requestRetries" must be an integer/, 'api.requestRetries should be an integer'); + t.throws( + validate.bind(null, config), + /"requestRetries" must be an integer/, 'api.requestRetries should be an integer'); t.end(); }); - test('config with negative api.requestRetries should throw error', function(t) { + test('config with negative api.requestRetries should throw error', (t) => { var config = { api: { version: 'version value', @@ -370,9 +359,9 @@ module.exports.tests.api_validation = function(test, common) { esclient: {} }; - t.throws(function() { - configValidation.validate(config); - }, /"requestRetries" must be larger than or equal to 0/, 'api.requestRetries must be positive'); + t.throws( + validate.bind(null, config), + /"requestRetries" must be larger than or equal to 0/, 'api.requestRetries must be positive'); t.end(); @@ -380,8 +369,8 @@ module.exports.tests.api_validation = function(test, common) { }; -module.exports.tests.esclient_validation = function(test, common) { - test('config without esclient should throw error', function(t) { +module.exports.tests.esclient_validation = (test, common) => { + test('config without esclient should throw error', (t) => { var config = { api: { version: 'version value', @@ -390,14 +379,14 @@ module.exports.tests.esclient_validation = function(test, common) { } }; - t.throws(function() { - configValidation.validate(config); - }, /"esclient" is required/, 'esclient should exist'); + t.throws( + validate.bind(null, config), + /"esclient" is required/, 'esclient should exist'); t.end(); }); - test('config with non-object esclient should throw error', function(t) { + test('config with non-object esclient should throw error', (t) => { [null, 17, [], 'string', true].forEach((value) => { var config = { api: { @@ -408,9 +397,9 @@ module.exports.tests.esclient_validation = function(test, common) { esclient: value }; - t.throws(function() { - configValidation.validate(config); - }, /"esclient" must be an object/, 'esclient should be an object'); + t.throws( + validate.bind(null, config), + /"esclient" must be an object/, 'esclient should be an object'); }); @@ -418,7 +407,7 @@ module.exports.tests.esclient_validation = function(test, common) { }); - test('config with non-number esclient.requestTimeout should throw error', function(t) { + test('config with non-number esclient.requestTimeout should throw error', (t) => { [null, 'string', {}, [], false].forEach((value) => { var config = { api: { @@ -431,16 +420,17 @@ module.exports.tests.esclient_validation = function(test, common) { } }; - t.throws(function() { - configValidation.validate(config); - }, /"requestTimeout" must be a number/, 'esclient.requestTimeout should be a number'); + t.throws( + validate.bind(null, config), + /"requestTimeout" must be a number/, 'esclient.requestTimeout should be a number'); + }); t.end(); }); - test('config with non-integer esclient.requestTimeout should throw error', function(t) { + test('config with non-integer esclient.requestTimeout should throw error', (t) => { var config = { api: { version: 'version value', @@ -452,15 +442,15 @@ module.exports.tests.esclient_validation = function(test, common) { } }; - t.throws(function() { - configValidation.validate(config); - }, /"requestTimeout" must be an integer/, 'esclient.requestTimeout should be an integer'); + t.throws( + validate.bind(null, config), + /"requestTimeout" must be an integer/, 'esclient.requestTimeout should be an integer'); t.end(); }); - test('config with negative esclient.requestTimeout should throw error', function(t) { + test('config with negative esclient.requestTimeout should throw error', (t) => { var config = { api: { version: 'version value', @@ -472,9 +462,9 @@ module.exports.tests.esclient_validation = function(test, common) { } }; - t.throws(function() { - configValidation.validate(config); - }, /"requestTimeout" must be larger than or equal to 0/, 'esclient.requestTimeout must be positive'); + t.throws( + validate.bind(null, config), + /"requestTimeout" must be larger than or equal to 0/, 'esclient.requestTimeout must be positive'); t.end(); @@ -482,7 +472,7 @@ module.exports.tests.esclient_validation = function(test, common) { }; -module.exports.all = function (tape, common) { +module.exports.all = (tape, common) => { function test(name, testFunction) { return tape('configValidation: ' + name, testFunction); From bb9d6e0fc9a0fcaecebe1dbfdf4cefbcd5779f64 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Wed, 25 Jan 2017 13:54:49 -0500 Subject: [PATCH 42/43] inject schema to config.generate --- app.js | 5 +---- test/unit/app.js | 16 ++++++++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/app.js b/app.js index 40c85964..2034e96b 100644 --- a/app.js +++ b/app.js @@ -1,10 +1,7 @@ var app = require('express')(); -var peliasConfig = require( 'pelias-config' ).generate(); - -// validate the configuration before attempting to load the app -require('./src/configValidation').validate(peliasConfig); +var peliasConfig = require( 'pelias-config' ).generate(require('./schema')); if( peliasConfig.api.accessLog ){ app.use( require( './middleware/access_log' ).createAccessLogger( peliasConfig.api.accessLog ) ); diff --git a/test/unit/app.js b/test/unit/app.js index 58ac8cdb..479e2af0 100644 --- a/test/unit/app.js +++ b/test/unit/app.js @@ -4,12 +4,16 @@ const proxyquire = require('proxyquire').noCallThru(); module.exports.tests = {}; -module.exports.tests.invalid_configuration = function(test, common) { - test('configuration validation throwing error should rethrow', function(t) { - t.throws(function() { +module.exports.tests.invalid_configuration = (test, common) => { + test('configuration validation throwing error should rethrow', (t) => { + t.throws(() => { proxyquire('../../app', { - './src/configValidation': { - validate: () => { + './schema': 'this is the schema', + 'pelias-config': { + generate: (schema) => { + // the schema passed to generate should be the require'd schema + t.equals(schema, 'this is the schema'); + throw Error('config is not valid'); } } @@ -23,7 +27,7 @@ module.exports.tests.invalid_configuration = function(test, common) { }; -module.exports.all = function (tape, common) { +module.exports.all = (tape, common) => { function test(name, testFunction) { return tape('app: ' + name, testFunction); From 75c655a74d8d514c19f309a9fd5ae0681eade4c7 Mon Sep 17 00:00:00 2001 From: greenkeeperio-bot Date: Wed, 25 Jan 2017 14:40:56 -0500 Subject: [PATCH 43/43] chore(package): update pelias-config to version 2.7.1 https://greenkeeper.io/ --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 7b222953..84b17064 100644 --- a/package.json +++ b/package.json @@ -52,7 +52,7 @@ "markdown": "0.5.0", "morgan": "1.7.0", "pelias-categories": "1.1.0", - "pelias-config": "2.6.0", + "pelias-config": "2.7.1", "pelias-labels": "1.5.1", "pelias-logger": "0.1.0", "pelias-model": "4.4.0",