From 3bf40fe51eea655840c771a385aecb1790ae292d Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Tue, 2 May 2017 14:20:04 -0400 Subject: [PATCH] refactored ServiceConfiguration to be a class --- routes/v1.js | 6 +- service/configurations/PlaceHolder.js | 30 ++ .../configurations/ServiceConfiguration.js | 48 +++ service/configurations/placeholder.js | 36 -- service/http_json.js | 117 +++--- test/unit/run.js | 3 +- .../{placeholder.js => PlaceHolder.js} | 65 +++- .../configurations/ServiceConfiguration.js | 58 +++ test/unit/service/http_json.js | 356 +++++++++++------- 9 files changed, 475 insertions(+), 244 deletions(-) create mode 100644 service/configurations/PlaceHolder.js create mode 100644 service/configurations/ServiceConfiguration.js delete mode 100644 service/configurations/placeholder.js rename test/unit/service/configurations/{placeholder.js => PlaceHolder.js} (58%) create mode 100644 test/unit/service/configurations/ServiceConfiguration.js diff --git a/routes/v1.js b/routes/v1.js index c3ee4fa4..347b9af2 100644 --- a/routes/v1.js +++ b/routes/v1.js @@ -84,12 +84,12 @@ function addRoutes(app, peliasConfig) { const esclient = elasticsearch.Client(peliasConfig.esclient); const isPipServiceEnabled = require('../controller/predicates/is_service_enabled')(peliasConfig.api.pipService); - const isPlaceholderServiceEnabled = require('../controller/predicates/is_service_enabled')(peliasConfig.api.placeholderService); const pipService = require('../service/pointinpolygon')(peliasConfig.api.pipService); - const placeholderService = require('../service/http_json')( - new PlaceHolder(peliasConfig.api.placeholderService)); + const placeholderConfiguration = new PlaceHolder(peliasConfig.api.services.placeholder); + const placeholderService = require('../service/http_json')(placeholderConfiguration); + const isPlaceholderServiceEnabled = require('../controller/predicates/is_service_enabled')(placeholderConfiguration.getBaseUrl()); const coarse_reverse_should_execute = all( not(hasRequestErrors), isPipServiceEnabled, isCoarseReverse diff --git a/service/configurations/PlaceHolder.js b/service/configurations/PlaceHolder.js new file mode 100644 index 00000000..5f7fb464 --- /dev/null +++ b/service/configurations/PlaceHolder.js @@ -0,0 +1,30 @@ +'use strict'; + +const _ = require('lodash'); + +const ServiceConfiguration = require('./ServiceConfiguration'); + +class PlaceHolder extends ServiceConfiguration { + constructor(o) { + super('placeholder', o); + } + + getParameters(req) { + const parameters = { + text: req.clean.text + }; + + if (_.has(req.clean, 'lang.iso6393')) { + parameters.lang = req.clean.lang.iso6393; + } + + return parameters; + } + + getUrl(req) { + return `${this.baseUrl}/search`; + } + +} + +module.exports = PlaceHolder; diff --git a/service/configurations/ServiceConfiguration.js b/service/configurations/ServiceConfiguration.js new file mode 100644 index 00000000..3fe0d75f --- /dev/null +++ b/service/configurations/ServiceConfiguration.js @@ -0,0 +1,48 @@ +'use strict'; + +const _ = require('lodash'); + +class ServiceConfiguration { + constructor(name, config) { + if (_.isEmpty(name)) { + throw 'name is required'; + } + + this.name = name; + this.baseUrl = config.url; + this.timeout = config.timeout || 250; + this.retries = config.retries || 3; + + } + + getName() { + return this.name; + } + + getBaseUrl() { + return this.baseUrl; + } + + getUrl() { + return this.baseUrl; + } + + getRetries() { + return this.retries; + } + + getTimeout() { + return this.timeout; + } + + getParameters() { + return {}; + } + + getHeaders() { + return {}; + } + +} + +module.exports = ServiceConfiguration; diff --git a/service/configurations/placeholder.js b/service/configurations/placeholder.js deleted file mode 100644 index 631b64b4..00000000 --- a/service/configurations/placeholder.js +++ /dev/null @@ -1,36 +0,0 @@ -const _ = require('lodash'); - -function PlaceHolder( baseUrl ){ - this.baseUrl = baseUrl; -} - -PlaceHolder.prototype.getName = function() { - return 'placeholder'; -}; - -PlaceHolder.prototype.getBaseUrl = function() { - return this.baseUrl; -}; - -PlaceHolder.prototype.getUrl = function(req) { - return `${this.baseUrl}/search`; -}; - -PlaceHolder.prototype.getParameters = function(req) { - const parameters = { - text: req.clean.text - }; - - if (_.has(req.clean, 'lang.iso6393')) { - parameters.lang = req.clean.lang.iso6393; - } - - return parameters; -}; - -PlaceHolder.prototype.getHeaders = function(req) { - return {}; -}; - -// export -module.exports = PlaceHolder; diff --git a/service/http_json.js b/service/http_json.js index 969c5baf..7c9a5716 100644 --- a/service/http_json.js +++ b/service/http_json.js @@ -1,19 +1,27 @@ -const request = require('request'); -const bl = require('bl'); +const request = require('superagent'); const _ = require('lodash'); const isDNT = require( '../helper/logging' ).isDNT; const logger = require( 'pelias-logger' ).get( 'placeholder' ); +const ServiceConfiguration = require('./configurations/ServiceConfiguration'); + +function synthesizeUrl(serviceConfig, req) { + const parameters = _.map(serviceConfig.getParameters(), (value, key) => { + return `${key}=${value}`; + }).join('&'); + + if (parameters) { + return encodeURI(`${serviceConfig.getUrl(req)}?${parameters}`); + } else { + return serviceConfig.getUrl(req); + } + +} + module.exports = function setup(serviceConfig) { - if (!_.conformsTo(serviceConfig, { - getName: _.isFunction, - getBaseUrl: _.isFunction, - getUrl: _.isFunction, - getParameters: _.isFunction, - getHeaders: _.isFunction - })) { - throw Error('serviceConfig should have a bunch of functions exposed'); + if (!(serviceConfig instanceof ServiceConfiguration)) { + throw Error('serviceConfig be an instance of ServiceConfiguration'); } if (_.isEmpty(serviceConfig.getBaseUrl())) { @@ -28,63 +36,64 @@ module.exports = function setup(serviceConfig) { logger.info(`using ${serviceConfig.getName()} service at ${serviceConfig.getBaseUrl()}`); return (req, callback) => { - const options = { - method: 'GET', - url: serviceConfig.getUrl(req), - qs: serviceConfig.getParameters(req), - headers: serviceConfig.getHeaders(req) || {} - }; - + const headers = serviceConfig.getHeaders(req) || {}; const do_not_track = isDNT(req); if (do_not_track) { - options.headers.dnt = '1'; + headers.dnt = '1'; } - request(options).on('response', (response) => { - // pipe the response thru bl which will accumulate the entire body - response.pipe(bl((err, data) => { - if (response.statusCode === 200) { - // parse and return w/o error unless response wasn't JSON - try { - const parsed = JSON.parse(data); - return callback(null, parsed); - + request + .get(serviceConfig.getUrl(req)) + .set(headers) + .timeout(serviceConfig.getTimeout()) + .retry(serviceConfig.getRetries()) + .accept('json') + .query(serviceConfig.getParameters(req)) + .on('error', (err) => { + if (err.status) { + // first handle case where a non-200 was returned + if (do_not_track) { + logger.error(`${serviceConfig.getBaseUrl()} [do_not_track] returned status ${err.status}: ${err.response.text}`); + return callback(`${serviceConfig.getBaseUrl()} [do_not_track] returned status ${err.status}: ${err.response.text}`); + } else { + logger.error(`${synthesizeUrl(serviceConfig, req)} returned status ${err.status}: ${err.response.text}`); + return callback(`${synthesizeUrl(serviceConfig, req)} returned status ${err.status}: ${err.response.text}`); } - catch (err) { - if (do_not_track) { - logger.error(`${serviceConfig.getBaseUrl()} [do_not_track] could not parse response: ${data}`); - return callback(`${serviceConfig.getBaseUrl()} [do_not_track] could not parse response: ${data}`); - } else { - logger.error(`${response.request.href} could not parse response: ${data}`); - return callback(`${response.request.href} could not parse response: ${data}`); - } - } } - else { - // otherwise there was a non-200 status so handle generically + + // handle case that something catastrophic happened while contacting the server + if (do_not_track) { + logger.error(`${serviceConfig.getBaseUrl()} [do_not_track]: ${JSON.stringify(err)}`); + return callback(err); + } else { + logger.error(`${serviceConfig.getUrl(req)}: ${JSON.stringify(err)}`); + return callback(err); + } + + }) + .end((err, response) => { + // bail early if there's an error (shouldn't happen since it was already handled above) + if (err) { + return; + } + + if (response.type === 'application/json') { + return callback(null, response.body); + + } else { if (do_not_track) { - logger.error(`${serviceConfig.getBaseUrl()} [do_not_track] returned status ${response.statusCode}: ${data}`); - return callback(`${serviceConfig.getBaseUrl()} [do_not_track] returned status ${response.statusCode}: ${data}`); + logger.error(`${serviceConfig.getBaseUrl()} [do_not_track] could not parse response: ${response.text}`); + return callback(`${serviceConfig.getBaseUrl()} [do_not_track] could not parse response: ${response.text}`); } else { - logger.error(`${response.request.href} returned status ${response.statusCode}: ${data}`); - return callback(`${response.request.href} returned status ${response.statusCode}: ${data}`); + logger.error(`${synthesizeUrl(serviceConfig, req)} could not parse response: ${response.text}`); + return callback(`${synthesizeUrl(serviceConfig, req)} could not parse response: ${response.text}`); } } - })); - - }) - .on('error', (err) => { - if (do_not_track) { - logger.error(`${serviceConfig.getBaseUrl()} [do_not_track]: ${JSON.stringify(err)}`); - callback(err); - } else { - logger.error(`${options.url}: ${JSON.stringify(err)}`); - callback(err); - } - }); + + }); }; diff --git a/test/unit/run.js b/test/unit/run.js index 878a1b58..039ed927 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -81,7 +81,8 @@ var tests = [ require('./sanitizer/search_fallback'), require('./sanitizer/wrap'), require('./service/http_json'), - require('./service/configurations/placeholder'), + require('./service/configurations/PlaceHolder'), + require('./service/configurations/ServiceConfiguration'), require('./service/mget'), require('./service/search'), require('./service/interpolation'), diff --git a/test/unit/service/configurations/placeholder.js b/test/unit/service/configurations/PlaceHolder.js similarity index 58% rename from test/unit/service/configurations/placeholder.js rename to test/unit/service/configurations/PlaceHolder.js index cb1191bb..055f6318 100644 --- a/test/unit/service/configurations/placeholder.js +++ b/test/unit/service/configurations/PlaceHolder.js @@ -1,34 +1,47 @@ module.exports.tests = {}; -const PlaceHolder = require('../../../../service/configurations/placeholder'); +const PlaceHolder = require('../../../../service/configurations/PlaceHolder'); module.exports.tests.all = (test, common) => { test('getName should return \'placeholder\'', (t) => { - const placeholder = new PlaceHolder('base url'); - - t.equals(placeholder.getName(), 'placeholder'); - t.end(); - - }); + const configBlob = { + url: 'base url', + timeout: 17, + retries: 19 + }; - test('getBaseUrl should return value passed to constructor', (t) => { - const placeholder = new PlaceHolder('base url'); + const placeholder = new PlaceHolder(configBlob); + t.equals(placeholder.getName(), 'placeholder'); t.equals(placeholder.getBaseUrl(), 'base url'); + t.equals(placeholder.getTimeout(), 17); + t.equals(placeholder.getRetries(), 19); t.end(); }); test('getUrl should return value passed to constructor', (t) => { - const placeholder = new PlaceHolder('base url'); + const configBlob = { + url: 'base url', + timeout: 17, + retries: 19 + }; + + const placeholder = new PlaceHolder(configBlob); - t.equals(placeholder.getUrl('this is not an object'), 'base url/search'); + t.equals(placeholder.getUrl(), 'base url/search'); t.end(); }); test('getParameters should return object with text and lang from req', (t) => { - const placeholder = new PlaceHolder('base url'); + const configBlob = { + url: 'base url', + timeout: 17, + retries: 19 + }; + + const placeholder = new PlaceHolder(configBlob); const req = { clean: { @@ -45,15 +58,27 @@ module.exports.tests.all = (test, common) => { }); test('getHeaders should return empty object', (t) => { - const placeholder = new PlaceHolder('base url'); + const configBlob = { + url: 'base url', + timeout: 17, + retries: 19 + }; + + const placeholder = new PlaceHolder(configBlob); - t.deepEquals(placeholder.getHeaders('this is not an object'), {}); + t.deepEquals(placeholder.getHeaders(), {}); t.end(); }); test('getParameters should not include lang if req.clean.lang is unavailable', (t) => { - const placeholder = new PlaceHolder('base url'); + const configBlob = { + url: 'base url', + timeout: 17, + retries: 19 + }; + + const placeholder = new PlaceHolder(configBlob); const req = { clean: { @@ -67,7 +92,13 @@ module.exports.tests.all = (test, common) => { }); test('getParameters should not include lang if req.clean.lang.iso6393 is unavailable', (t) => { - const placeholder = new PlaceHolder('base url'); + const configBlob = { + url: 'base url', + timeout: 17, + retries: 19 + }; + + const placeholder = new PlaceHolder(configBlob); const req = { clean: { @@ -85,7 +116,7 @@ module.exports.tests.all = (test, common) => { module.exports.all = (tape, common) => { function test(name, testFunction) { - return tape(`SERVICE CONFIGURATION /placeholder ${name}`, testFunction); + return tape(`SERVICE CONFIGURATION /PlaceHolder ${name}`, testFunction); } for( var testCase in module.exports.tests ){ diff --git a/test/unit/service/configurations/ServiceConfiguration.js b/test/unit/service/configurations/ServiceConfiguration.js new file mode 100644 index 00000000..ca31b327 --- /dev/null +++ b/test/unit/service/configurations/ServiceConfiguration.js @@ -0,0 +1,58 @@ +module.exports.tests = {}; + +const ServiceConfiguration = require('../../../../service/configurations/ServiceConfiguration'); + +module.exports.tests.all = (test, common) => { + test('timeout and retries overrides should be returned by getters', (t) => { + const configBlob = { + url: 'base url', + timeout: 17, + retries: 19 + }; + + const serviceConfiguration = new ServiceConfiguration('service name', configBlob); + + t.equals(serviceConfiguration.getName(), 'service name'); + t.equals(serviceConfiguration.getBaseUrl(), 'base url'); + t.deepEquals(serviceConfiguration.getParameters(), {}); + t.deepEquals(serviceConfiguration.getHeaders(), {}); + t.equals(serviceConfiguration.getUrl(), 'base url'); + t.equals(serviceConfiguration.getRetries(), 19); + t.equals(serviceConfiguration.getTimeout(), 17); + t.end(); + + }); + + test('configBlob w/o timeout or retries should default to 250 and 3, respectively', (t) => { + const configBlob = { + url: 'base url' + }; + + const serviceConfiguration = new ServiceConfiguration('service name', configBlob); + + t.equals(serviceConfiguration.getTimeout(), 250, 'should be a default of 250'); + t.equals(serviceConfiguration.getRetries(), 3, 'should be a default of 3'); + t.end(); + + }); + + test('missing name should throw error', (t) => { + t.throws(() => { + // lint complains if using `new` and not assigning to something + const config = new ServiceConfiguration(undefined, { url: 'base url' }); + }, /^name is required$/); + t.end(); + + }); + +}; + +module.exports.all = (tape, common) => { + function test(name, testFunction) { + return tape(`SERVICE CONFIGURATION /ServiceConfiguration ${name}`, testFunction); + } + + for( var testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; diff --git a/test/unit/service/http_json.js b/test/unit/service/http_json.js index b9c4d528..7d14f7ae 100644 --- a/test/unit/service/http_json.js +++ b/test/unit/service/http_json.js @@ -1,7 +1,10 @@ +'use strict'; + const proxyquire = require('proxyquire').noCallThru(); const express = require('express'); const setup = require('../../../service/http_json'); +const ServiceConfiguration = require('../../../service/configurations/ServiceConfiguration'); module.exports.tests = {}; @@ -19,72 +22,10 @@ module.exports.tests.interface = (test, common) => { }; module.exports.tests.conforms_to = (test, common) => { - test('serviceConfig with non-function getName property should throw error', (t) => { - const serviceConfig = { - getName: 'this is not a function', - getBaseUrl: () => {}, - getUrl: () => {}, - getParameters: () => {}, - getHeaders: () => {} - }; - - t.throws(setup.bind(null, serviceConfig), /serviceConfig should have a bunch of functions exposed/); - t.end(); - - }); - - test('serviceConfig with non-function getBaseUrl property should throw error', (t) => { - const serviceConfig = { - getName: () => {}, - getBaseUrl: 'this is not a function', - getUrl: () => {}, - getParameters: () => {}, - getHeaders: () => {} - }; - - t.throws(setup.bind(null, serviceConfig), /serviceConfig should have a bunch of functions exposed/); - t.end(); - - }); - - test('serviceConfig with non-function getUrl property should throw error', (t) => { - const serviceConfig = { - getName: () => {}, - getBaseUrl: () => {}, - getUrl: 'this is not a function', - getParameters: () => {}, - getHeaders: () => {} - }; - - t.throws(setup.bind(null, serviceConfig), /serviceConfig should have a bunch of functions exposed/); - t.end(); + test('non-ServiceConfiguration instance should throw error', (t) => { + const serviceConfig = 'not an instance of serviceConfiguration'; - }); - - test('serviceConfig with non-function getParameters property should throw error', (t) => { - const serviceConfig = { - getName: () => {}, - getBaseUrl: () => {}, - getUrl: () => {}, - getParameters: 'this is not a function', - getHeaders: () => {} - }; - - t.throws(setup.bind(null, serviceConfig), /serviceConfig should have a bunch of functions exposed/); - t.end(); - - }); - - test('serviceConfig with non-function getHeaders property should throw error', (t) => { - const serviceConfig = { - getName: () => {}, - getBaseUrl: () => {}, - getUrl: () => {}, - getParameters: () => {}, - getHeaders: 'this is not a function' - }; - - t.throws(setup.bind(null, serviceConfig), /serviceConfig should have a bunch of functions exposed/); + t.throws(setup.bind(null, serviceConfig), /serviceConfig be an instance of ServiceConfiguration/); t.end(); }); @@ -95,19 +36,15 @@ module.exports.tests.do_nothing_service = (test, common) => { test('undefined config.url should return service that logs that config.name service is not available', (t) => { const logger = require('pelias-mock-logger')(); - const serviceConfig = { - getName: () => { return 'foo'; }, - getBaseUrl: () => { - return undefined; - }, - getUrl: () => { return undefined; }, - getParameters: (req) => {}, - getHeaders: (req) => {} + const MockServiceConfig = class extends ServiceConfiguration { + constructor(o) { + super('foo', { } ); + } }; const service = proxyquire('../../../service/http_json', { 'pelias-logger': logger - })(serviceConfig); + })(new MockServiceConfig()); t.ok(logger.isWarnMessage(/^foo service disabled$/)); @@ -130,17 +67,18 @@ module.exports.tests.failure_conditions = (test, common) => { const logger = require('pelias-mock-logger')(); - const serviceConfig = { - getName: () => { return 'foo'; }, - getBaseUrl: () => { return `http://localhost:${port}`; }, - getUrl: (req) => { return `http://localhost:${port}/built_url`; }, - getParameters: (req) => { return { param1: 'param1 value', param2: 'param2 value' }; }, - getHeaders: (req) => { return { header1: 'header1 value' }; } + const MockServiceConfig = class extends ServiceConfiguration { + constructor(o) { + super('foo', { url: `http://localhost:${port}` } ); + } + getUrl(req) { + return `http://localhost:${port}/built_url`; + } }; const service = proxyquire('../../../service/http_json', { 'pelias-logger': logger - })(serviceConfig); + })(new MockServiceConfig()); t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`))); @@ -166,12 +104,13 @@ module.exports.tests.failure_conditions = (test, common) => { const logger = require('pelias-mock-logger')(); - const serviceConfig = { - getName: () => { return 'foo'; }, - getBaseUrl: () => { return `http://localhost:${port}`; }, - getUrl: (req) => { return `http://localhost:${port}/built_url`; }, - getParameters: (req) => { return { param1: 'param1 value', param2: 'param2 value' }; }, - getHeaders: (req) => { return { header1: 'header1 value' }; } + const MockServiceConfig = class extends ServiceConfiguration { + constructor(o) { + super('foo', { url: `http://localhost:${port}` } ); + } + getUrl(req) { + return `http://localhost:${port}/built_url`; + } }; const service = proxyquire('../../../service/http_json', { @@ -179,7 +118,7 @@ module.exports.tests.failure_conditions = (test, common) => { '../helper/logging': { isDNT: () => { return true; } } - })(serviceConfig); + })(new MockServiceConfig()); t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`))); @@ -212,17 +151,24 @@ module.exports.tests.failure_conditions = (test, common) => { const logger = require('pelias-mock-logger')(); - const serviceConfig = { - getName: () => { return 'foo'; }, - getBaseUrl: () => { return `http://localhost:${port}`; }, - getUrl: (req) => { return `http://localhost:${port}/some_endpoint`; }, - getParameters: (req) => { return { param1: 'param1 value', param2: 'param2 value' }; }, - getHeaders: (req) => { return { header1: 'header1 value' }; } + const MockServiceConfig = class extends ServiceConfiguration { + constructor(o) { + super('foo', { url: `http://localhost:${port}` } ); + } + getUrl(req) { + return `http://localhost:${port}/some_endpoint`; + } + getParameters(req) { + return { param1: 'param1 value', param2: 'param2 value' }; + } + getHeaders(req) { + return { header1: 'header1 value' }; + } }; const service = proxyquire('../../../service/http_json', { 'pelias-logger': logger - })(serviceConfig); + })(new MockServiceConfig()); t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`))); @@ -256,12 +202,19 @@ module.exports.tests.failure_conditions = (test, common) => { const logger = require('pelias-mock-logger')(); - const serviceConfig = { - getName: () => { return 'foo'; }, - getBaseUrl: () => { return `http://localhost:${port}`; }, - getUrl: (req) => { return `http://localhost:${port}/some_endpoint`; }, - getParameters: (req) => { return { param1: 'param1 value', param2: 'param2 value' }; }, - getHeaders: (req) => { return { header1: 'header1 value' }; } + const MockServiceConfig = class extends ServiceConfiguration { + constructor(o) { + super('foo', { url: `http://localhost:${port}` } ); + } + getUrl(req) { + return `http://localhost:${port}/some_endpoint`; + } + getParameters(req) { + return { param1: 'param1 value', param2: 'param2 value' }; + } + getHeaders(req) { + return { header1: 'header1 value' }; + } }; const service = proxyquire('../../../service/http_json', { @@ -269,7 +222,7 @@ module.exports.tests.failure_conditions = (test, common) => { '../helper/logging': { isDNT: () => { return true; } } - })(serviceConfig); + })(new MockServiceConfig()); t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`))); @@ -294,7 +247,7 @@ module.exports.tests.failure_conditions = (test, common) => { t.equals(req.headers.header1, 'header1 value', 'all headers should have been passed'); t.deepEquals(req.query, { param1: 'param1 value', param2: 'param2 value' }); - res.status(200).send('this is not parseable as JSON'); + res.set('Content-Type', 'text/plain').status(200).send('this is not parseable as JSON'); }); const server = webServer.listen(); @@ -302,17 +255,24 @@ module.exports.tests.failure_conditions = (test, common) => { const logger = require('pelias-mock-logger')(); - const serviceConfig = { - getName: () => { return 'foo'; }, - getBaseUrl: () => { return `http://localhost:${port}`; }, - getUrl: (req) => { return `http://localhost:${port}/some_endpoint`; }, - getParameters: (req) => { return { param1: 'param1 value', param2: 'param2 value' }; }, - getHeaders: (req) => { return { header1: 'header1 value' }; } + const MockServiceConfig = class extends ServiceConfiguration { + constructor(o) { + super('foo', { url: `http://localhost:${port}` } ); + } + getUrl(req) { + return `http://localhost:${port}/some_endpoint`; + } + getParameters(req) { + return { param1: 'param1 value', param2: 'param2 value' }; + } + getHeaders(req) { + return { header1: 'header1 value' }; + } }; const service = proxyquire('../../../service/http_json', { 'pelias-logger': logger - })(serviceConfig); + })(new MockServiceConfig()); t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`))); @@ -346,12 +306,19 @@ module.exports.tests.failure_conditions = (test, common) => { const logger = require('pelias-mock-logger')(); - const serviceConfig = { - getName: () => { return 'foo'; }, - getBaseUrl: () => { return `http://localhost:${port}`; }, - getUrl: (req) => { return `http://localhost:${port}/some_endpoint`; }, - getParameters: (req) => { return { param1: 'param1 value', param2: 'param2 value' }; }, - getHeaders: (req) => { return { header1: 'header1 value' }; } + const MockServiceConfig = class extends ServiceConfiguration { + constructor(o) { + super('foo', { url: `http://localhost:${port}` } ); + } + getUrl(req) { + return `http://localhost:${port}/some_endpoint`; + } + getParameters(req) { + return { param1: 'param1 value', param2: 'param2 value' }; + } + getHeaders(req) { + return { header1: 'header1 value' }; + } }; const service = proxyquire('../../../service/http_json', { @@ -359,7 +326,7 @@ module.exports.tests.failure_conditions = (test, common) => { '../helper/logging': { isDNT: () => { return true; } } - })(serviceConfig); + })(new MockServiceConfig()); t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`))); @@ -377,6 +344,58 @@ module.exports.tests.failure_conditions = (test, common) => { }); + test('server timing out on all requests should log and return error', (t) => { + const webServer = express(); + let requestCount = 0; + webServer.get('/some_endpoint', (req, res, next) => { + requestCount++; + res.set('Content-Type', 'text/plain').status(503).send('request timeout'); + }); + + const server = webServer.listen(); + const port = server.address().port; + + const logger = require('pelias-mock-logger')(); + + const MockServiceConfig = class extends ServiceConfiguration { + constructor(o) { + super('foo', { url: `http://localhost:${port}` } ); + } + getUrl(req) { + return `http://localhost:${port}/some_endpoint`; + } + getParameters(req) { + return { param1: 'param1 value', param2: 'param2 value' }; + } + getHeaders(req) { + return { header1: 'header1 value' }; + } + getRetries() { + return 1; + } + }; + + const service = proxyquire('../../../service/http_json', { + 'pelias-logger': logger + })(new MockServiceConfig()); + + t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`))); + + service({}, (err, results) => { + t.equals(err, `http://localhost:${port}/some_endpoint?param1=param1%20value¶m2=param2%20value ` + + 'returned status 503: request timeout'); + t.notOk(results); + t.ok(logger.isErrorMessage(`http://localhost:${port}/some_endpoint?param1=param1%20value¶m2=param2%20value ` + + `returned status 503: request timeout`)); + t.equals(requestCount, 2); + t.end(); + + server.close(); + + }); + + }); + }; module.exports.tests.success_conditions = (test, common) => { @@ -388,7 +407,7 @@ module.exports.tests.success_conditions = (test, common) => { t.equals(req.headers.header1, 'header1 value', 'all headers should have been passed'); t.deepEquals(req.query, { param1: 'param1 value', param2: 'param2 value' }); - res.status(200).send('[1, 2, 3]'); + res.status(200).json([1, 2, 3]); }); const server = webServer.listen(); @@ -396,17 +415,24 @@ module.exports.tests.success_conditions = (test, common) => { const logger = require('pelias-mock-logger')(); - const serviceConfig = { - getName: () => { return 'foo'; }, - getBaseUrl: () => { return `http://localhost:${port}`; }, - getUrl: (req) => { return `http://localhost:${port}/some_endpoint`; }, - getParameters: (req) => { return { param1: 'param1 value', param2: 'param2 value' }; }, - getHeaders: (req) => { return { header1: 'header1 value' }; } + const MockServiceConfig = class extends ServiceConfiguration { + constructor(o) { + super('foo', { url: `http://localhost:${port}` } ); + } + getUrl(req) { + return `http://localhost:${port}/some_endpoint`; + } + getParameters(req) { + return { param1: 'param1 value', param2: 'param2 value' }; + } + getHeaders(req) { + return { header1: 'header1 value' }; + } }; const service = proxyquire('../../../service/http_json', { 'pelias-logger': logger - })(serviceConfig); + })(new MockServiceConfig()); t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`))); @@ -429,7 +455,7 @@ module.exports.tests.success_conditions = (test, common) => { t.deepEquals(req.query, { param1: 'param1 value', param2: 'param2 value' }); - res.status(200).send('[1, 2, 3]'); + res.status(200).json([1, 2, 3]); }); const server = webServer.listen(); @@ -437,12 +463,19 @@ module.exports.tests.success_conditions = (test, common) => { const logger = require('pelias-mock-logger')(); - const serviceConfig = { - getName: () => { return 'foo'; }, - getBaseUrl: () => { return `http://localhost:${port}`; }, - getUrl: (req) => { return `http://localhost:${port}/some_endpoint`; }, - getParameters: (req) => { return { param1: 'param1 value', param2: 'param2 value' }; }, - getHeaders: (req) => { } + const MockServiceConfig = class extends ServiceConfiguration { + constructor(o) { + super('foo', { url: `http://localhost:${port}` } ); + } + getUrl(req) { + return `http://localhost:${port}/some_endpoint`; + } + getParameters(req) { + return { param1: 'param1 value', param2: 'param2 value' }; + } + getHeaders(req) { + return undefined; + } }; const service = proxyquire('../../../service/http_json', { @@ -450,7 +483,63 @@ module.exports.tests.success_conditions = (test, common) => { '../helper/logging': { isDNT: () => { return true; } } - })(serviceConfig); + })(new MockServiceConfig()); + + t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`))); + + service({}, (err, results) => { + t.notOk(err, 'should be no error'); + t.deepEquals(results, [1, 2, 3]); + t.notOk(logger.hasErrorMessages()); + t.end(); + + server.close(); + + }); + + }); + + test('server succeeding on last timeout chance should return no error and parsed output', (t) => { + const webServer = express(); + let requestCount = 0; + webServer.get('/some_endpoint', (req, res, next) => { + if (++requestCount < 3) { + res.status(503); + + } else { + t.notOk(req.headers.hasOwnProperty('dnt'), 'dnt header should not have been passed'); + t.equals(req.headers.header1, 'header1 value', 'all headers should have been passed'); + t.deepEquals(req.query, { param1: 'param1 value', param2: 'param2 value' }); + + res.status(200).json([1, 2, 3]); + + } + + }); + + const server = webServer.listen(); + const port = server.address().port; + + const logger = require('pelias-mock-logger')(); + + const MockServiceConfig = class extends ServiceConfiguration { + constructor(o) { + super('foo', { url: `http://localhost:${port}` } ); + } + getUrl(req) { + return `http://localhost:${port}/some_endpoint`; + } + getParameters(req) { + return { param1: 'param1 value', param2: 'param2 value' }; + } + getHeaders(req) { + return { header1: 'header1 value' }; + } + }; + + const service = proxyquire('../../../service/http_json', { + 'pelias-logger': logger + })(new MockServiceConfig()); t.ok(logger.isInfoMessage(new RegExp(`using foo service at http://localhost:${port}`))); @@ -458,6 +547,7 @@ module.exports.tests.success_conditions = (test, common) => { t.notOk(err, 'should be no error'); t.deepEquals(results, [1, 2, 3]); t.notOk(logger.hasErrorMessages()); + t.equals(requestCount, 3); t.end(); server.close();