From ad8258e6801134abaa509f5d682418ff2321123c Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Wed, 11 Jan 2017 17:02:05 -0500 Subject: [PATCH] added config validation, inject configuration values --- index.js | 14 ++-- src/configValidation.js | 16 ++-- src/httpPipResolver.js | 21 ++--- src/localPipResolver.js | 9 ++- src/lookupStream.js | 25 +++--- src/resolversFactory.js | 4 - test/configValidationTest.js | 81 +++++++++++++++++-- ...sFactoryTest.js => httpPipResolverTest.js} | 10 +-- ...ryLocalTest.js => localPipResolverTest.js} | 4 +- test/lookupStreamTest.js | 39 ++++----- test/test.js | 4 +- 11 files changed, 150 insertions(+), 77 deletions(-) delete mode 100644 src/resolversFactory.js rename test/{resolversFactoryTest.js => httpPipResolverTest.js} (93%) rename test/{resolversFactoryLocalTest.js => localPipResolverTest.js} (91%) diff --git a/index.js b/index.js index 959e1ca..f4d86ce 100644 --- a/index.js +++ b/index.js @@ -1,11 +1,15 @@ const peliasConfig = require('pelias-config').generate(); require('./src/configValidation').validate(peliasConfig); -var createLookupStream = require('./src/lookupStream'); -var createWofPipResolver = require('./src/resolversFactory'); +const _ = require('lodash'); + +const options = { + maxConcurrentReqs: _.get(peliasConfig, 'imports.adminLookup.maxConcurrentReqs', 1), + suspectFile: _.get(peliasConfig, 'logger.suspectFile', false) +}; module.exports = { - createLookupStream: createLookupStream.createLookupStream, - createWofPipResolver: createWofPipResolver.createWofPipResolver, - createLocalWofPipResolver: createWofPipResolver.createLocalPipResolver + createLookupStream: require('./src/lookupStream')(options), + createWofPipResolver: require('./src/httpPipResolver')(options), + createLocalWofPipResolver: require('./src/localPipResolver')(peliasConfig.imports.whosonfirst.datapath) }; diff --git a/src/configValidation.js b/src/configValidation.js index 9466857..857cade 100644 --- a/src/configValidation.js +++ b/src/configValidation.js @@ -2,14 +2,18 @@ const Joi = require('joi'); -// requires just `maxConcurrentReqs` +// requires just `imports.whosonfirst.datapath` +// `imports.adminLookup.maxConcurrentReqs` is optional const schema = Joi.object().keys({ - imports: { - adminLookup: { + imports: Joi.object().keys({ + adminLookup: Joi.object().keys({ maxConcurrentReqs: Joi.number().integer() - } - } -}).unknown(true); + }), + whosonfirst: Joi.object().keys({ + datapath: Joi.string() + }).requiredKeys('datapath').unknown(true) + }).requiredKeys('whosonfirst').unknown(true) +}).requiredKeys('imports').unknown(true); module.exports = { validate: function validate(config) { diff --git a/src/httpPipResolver.js b/src/httpPipResolver.js index 1bfff82..3d6972c 100644 --- a/src/httpPipResolver.js +++ b/src/httpPipResolver.js @@ -3,19 +3,13 @@ var util = require('util'); var http = require('http'); var request = require('request'); -var peliasConfig = require( 'pelias-config' ).generate(); var _ = require('lodash'); +let maxConcurrentReqs; -function RemotePIPResolver(url, config) { +function RemotePIPResolver(url) { // prepend url with 'http://' if not already this.normalizedUrl = _.startsWith(url, 'http://') ? url : 'http://' + url; - this.config = config || peliasConfig; - - this.maxConcurrentReqs = 1; - if (this.config.imports.adminLookup && this.config.imports.adminLookup.maxConcurrentReqs) { - this.maxConcurrentReqs = this.config.imports.adminLookup.maxConcurrentReqs; - } this.httpAgent = new http.Agent({ keepAlive: true, @@ -80,8 +74,9 @@ RemotePIPResolver.prototype.end = function end() { this.httpAgent.destroy(); }; -function createWofPipResolver(url, config) { - return new RemotePIPResolver(url, config); -} - -module.exports = createWofPipResolver; +module.exports = function(options) { + maxConcurrentReqs = _.get(options, 'maxConcurrentReqs', 1); + return (url) => { + return new RemotePIPResolver(url); + }; +}; diff --git a/src/localPipResolver.js b/src/localPipResolver.js index 4c48e14..5ead98f 100644 --- a/src/localPipResolver.js +++ b/src/localPipResolver.js @@ -3,6 +3,8 @@ var logger = require('pelias-logger').get('wof-admin-lookup'); var createPIPService = require('pelias-wof-pip-service').create; +let datapath; + /** * LocalPIPService class * @@ -15,7 +17,7 @@ function LocalPIPService(lookupService) { if (!this.lookupService) { var self = this; - createPIPService(function (err, service) { + createPIPService(datapath, function (err, service) { self.lookupService = service; }); } @@ -84,4 +86,7 @@ function createLocalPipResolver(service) { return new LocalPIPService(service); } -module.exports = createLocalPipResolver; +module.exports = function(_datapath) { + datapath = _datapath; + return createLocalPipResolver; +}; diff --git a/src/lookupStream.js b/src/lookupStream.js index acae430..02ca823 100644 --- a/src/lookupStream.js +++ b/src/lookupStream.js @@ -1,16 +1,20 @@ +'use strict'; + var _ = require('lodash'); var parallelStream = require('pelias-parallel-stream'); -var peliasConfig = require( 'pelias-config' ).generate(); var regions = require('../data/regions'); var peliasLogger = require( 'pelias-logger' ); var getAdminLayers = require( './getAdminLayers' ); +let maxConcurrentReqs; +let suspectFile; + //defaults to nowhere var optsArg = { transports: [] }; //only prints to suspect records log if flag is set -if (peliasConfig.logger.suspectFile === true){ +if (suspectFile){ optsArg.transports.push(new peliasLogger.winston.transports.File( { filename: 'suspect_wof_records.log', timestamp: false @@ -67,21 +71,12 @@ function hasAnyMultiples(result) { }); } -function createLookupStream(resolver, config) { - +function createLookupStream(resolver) { if (!resolver) { throw new Error('createLookupStream requires a valid resolver to be passed in as the first parameter'); } - config = config || peliasConfig; - - var maxConcurrentReqs = 1; - if (config.imports.adminLookup && config.imports.adminLookup.maxConcurrentReqs) { - maxConcurrentReqs = config.imports.adminLookup.maxConcurrentReqs; - } - var stream = parallelStream(maxConcurrentReqs, function (doc, enc, callback) { - // don't do anything if there's no centroid if (_.isEmpty(doc.getCentroid())) { return callback(null, doc); @@ -154,6 +149,8 @@ function getCountryCode(result) { return undefined; } -module.exports = { - createLookupStream: createLookupStream +module.exports = (options) => { + maxConcurrentReqs = _.get(options, 'maxConcurrentReqs', 1); + suspectFile = _.get(options, 'suspectFile', false); + return createLookupStream; }; diff --git a/src/resolversFactory.js b/src/resolversFactory.js deleted file mode 100644 index 7559b03..0000000 --- a/src/resolversFactory.js +++ /dev/null @@ -1,4 +0,0 @@ -module.exports = { - createWofPipResolver: require('./httpPipResolver'), - createLocalPipResolver: require('./localPipResolver') -}; diff --git a/test/configValidationTest.js b/test/configValidationTest.js index 0cc7442..e87f75e 100644 --- a/test/configValidationTest.js +++ b/test/configValidationTest.js @@ -6,12 +6,12 @@ const configValidation = require('../src/configValidation'); const proxyquire = require('proxyquire').noCallThru(); tape('tests configuration scenarios', function(test) { - test.test('missing imports should not throw error', function(t) { + test.test('missing imports should throw error', function(t) { const config = {}; - t.doesNotThrow(function() { + t.throws(function() { configValidation.validate(config); - }); + }, /"imports" is required/); t.end(); }); @@ -29,9 +29,38 @@ tape('tests configuration scenarios', function(test) { }); + test.test('missing imports.whosonfirst should throw error', function(t) { + const config = { + imports: {} + }; + + t.throws(function() { + configValidation.validate(config); + }, /"whosonfirst" is required/); + t.end(); + + }); + + test.test('missing imports.whosonfirst.datapath should throw error', function(t) { + const config = { + imports: { + whosonfirst: {} + } + }; + + t.throws(function() { + configValidation.validate(config); + }, /"datapath" is required/); + t.end(); + + }); + test.test('missing imports.adminLookup should not throw error', function(t) { const config = { imports: { + whosonfirst: { + datapath: 'datapath value' + } } }; @@ -46,7 +75,10 @@ tape('tests configuration scenarios', function(test) { [null, 17, 'string', [], true].forEach((value) => { const config = { imports: { - adminLookup: value + adminLookup: value, + whosonfirst: { + datapath: 'datapath value' + } } }; @@ -65,6 +97,9 @@ tape('tests configuration scenarios', function(test) { imports: { adminLookup: { maxConcurrentReqs: value + }, + whosonfirst: { + datapath: 'datapath value' } } }; @@ -84,6 +119,9 @@ tape('tests configuration scenarios', function(test) { imports: { adminLookup: { maxConcurrentReqs: 17.3 + }, + whosonfirst: { + datapath: 'datapath value' } } }; @@ -100,6 +138,9 @@ tape('tests configuration scenarios', function(test) { const config = { imports: { adminLookup: { + }, + whosonfirst: { + datapath: 'datapath value' } } }; @@ -117,6 +158,9 @@ tape('tests configuration scenarios', function(test) { imports: { adminLookup: { maxConcurrentReqs: 17 + }, + whosonfirst: { + datapath: 'datapath value' } } }; @@ -129,14 +173,39 @@ tape('tests configuration scenarios', function(test) { }); + test.test('non-string imports.whosonfirst.datapath should throw error', function(t) { + [null, 17, {}, [], true].forEach((value) => { + const config = { + imports: { + whosonfirst: { + datapath: value + } + } + }; + + t.throws(function() { + configValidation.validate(config); + }, /"datapath" must be a string/); + }); + + t.end(); + + }); + test.test('unknown properties should not throw errors', function(t) { const config = { imports: { adminLookup: { maxConcurrentReqs: 17, unknown_property: 'property value' - } - } + }, + whosonfirst: { + datapath: 'datapath value', + unknown_property: 'property value' + }, + unknown_property: 'property value' + }, + unknown_property: 'property value' }; t.doesNotThrow(function() { diff --git a/test/resolversFactoryTest.js b/test/httpPipResolverTest.js similarity index 93% rename from test/resolversFactoryTest.js rename to test/httpPipResolverTest.js index e457028..f5f244d 100644 --- a/test/resolversFactoryTest.js +++ b/test/httpPipResolverTest.js @@ -2,7 +2,7 @@ var tape = require('tape'); var http = require('http'); var intercept = require('intercept-stdout'); -var resolvers = require('../src/resolversFactory'); +var httpPipResolver = require('../src/httpPipResolver')({ maxConcurrentReqs: 1 }); tape('tests', function(test) { test.test('return value should be parsed from server response', function(t) { @@ -85,7 +85,7 @@ tape('tests', function(test) { server.listen(0); - var resolver = resolvers.createWofPipResolver('http://localhost:' + server.address().port + '/?'); + var resolver = httpPipResolver('http://localhost:' + server.address().port + '/?'); var centroid = { lat: 12.121212, @@ -161,7 +161,7 @@ tape('tests', function(test) { server.listen(0); - var resolver = resolvers.createWofPipResolver('localhost:' + server.address().port + '/?'); + var resolver = httpPipResolver('localhost:' + server.address().port + '/?'); var centroid = { lat: 12.121212, @@ -199,7 +199,7 @@ tape('tests', function(test) { }); test.test('error condition', function(t) { - var resolver = resolvers.createWofPipResolver('http://localhost:12345/?'); + var resolver = httpPipResolver('http://localhost:12345/?'); var centroid = { lat: 12.121212, @@ -236,7 +236,7 @@ tape('tests', function(test) { server.listen(0); - var resolver = resolvers.createWofPipResolver('http://localhost:' + server.address().port + '/?'); + var resolver = httpPipResolver('http://localhost:' + server.address().port + '/?'); var centroid = { lat: 12.121212, diff --git a/test/resolversFactoryLocalTest.js b/test/localPipResolverTest.js similarity index 91% rename from test/resolversFactoryLocalTest.js rename to test/localPipResolverTest.js index 7a468f6..7346a9a 100644 --- a/test/resolversFactoryLocalTest.js +++ b/test/localPipResolverTest.js @@ -1,6 +1,6 @@ var tape = require('tape'); -var resolvers = require('../src/resolversFactory'); +var localPipResolver = require('../src/localPipResolver'); tape('tests', function(test) { @@ -47,7 +47,7 @@ tape('tests', function(test) { var lookupServiceMock = makeLookupMock(t, expectedLookupParams, null, results); - var resolver = resolvers.createLocalPipResolver(lookupServiceMock); + var resolver = localPipResolver()(lookupServiceMock); var callback = function(err, result) { var expected = { diff --git a/test/lookupStreamTest.js b/test/lookupStreamTest.js index 4bcdffb..c1c31ac 100644 --- a/test/lookupStreamTest.js +++ b/test/lookupStreamTest.js @@ -3,7 +3,7 @@ var event_stream = require('event-stream'); var Document = require('pelias-model').Document; var _ = require('lodash'); -var stream = require('../src/lookupStream'); +var stream = require('../src/lookupStream')(); function test_stream(input, testedStream, callback) { var input_stream = event_stream.readArray(input); @@ -14,11 +14,17 @@ function test_stream(input, testedStream, callback) { tape('tests', function(test) { test.test('doc without centroid should not modify input', function(t) { - var input = [ + const input = [ new Document( 'whosonfirst', 'placetype', '1') ]; - var lookupStream = stream.createLookupStream({}); + const resolver = { + lookup: () => { + throw new Error('lookup should not have been called'); + } + }; + + const lookupStream = stream(resolver); test_stream(input, lookupStream, function(err, actual) { t.deepEqual(actual, input, 'nothing should have changed'); @@ -93,7 +99,7 @@ tape('tests', function(test) { } }; - var lookupStream = stream.createLookupStream(resolver); + var lookupStream = stream(resolver); test_stream(input, lookupStream, function(err, actual) { t.deepEqual(actual, expected, 'all fields should have been set'); @@ -129,7 +135,7 @@ tape('tests', function(test) { } }; - var lookupStream = stream.createLookupStream(resolver); + var lookupStream = stream(resolver); test_stream(input, lookupStream, function(err, actual) { t.deepEqual(actual, expected, 'result with missing fields should not set anything in doc'); @@ -151,10 +157,7 @@ tape('tests', function(test) { } }; - var config = { - imports: { adminLookup: { maxConcurrentReqs: 1 } } - }; - var lookupStream = stream.createLookupStream(resolver, config); + var lookupStream = stream(resolver); var input_stream = event_stream.readArray([input]); var destination_stream = event_stream.writeArray(function() { @@ -198,7 +201,7 @@ tape('tests', function(test) { } }; - var lookupStream = stream.createLookupStream(resolver); + var lookupStream = stream(resolver); test_stream(input, lookupStream, function(err, actual) { t.deepEqual(actual, expected, 'region abbreviation should have been set'); @@ -235,7 +238,7 @@ tape('tests', function(test) { } }; - var lookupStream = stream.createLookupStream(resolver); + var lookupStream = stream(resolver); test_stream(input, lookupStream, function(err, actual) { t.deepEqual(actual, expected, 'no region abbreviation should have been set'); @@ -271,7 +274,7 @@ tape('tests', function(test) { } }; - var lookupStream = stream.createLookupStream(resolver); + var lookupStream = stream(resolver); test_stream(input, lookupStream, function(err, actual) { t.deepEqual(actual, expected, 'no region abbreviation should have been set'); @@ -300,7 +303,7 @@ tape('tests', function(test) { } }; - var lookupStream = stream.createLookupStream(resolver); + var lookupStream = stream(resolver); test_stream([inputDoc], lookupStream, function(err, actual) { t.deepEqual(actual, [expectedDoc], 'no region abbreviation should have been set'); @@ -330,7 +333,7 @@ tape('tests', function(test) { } }; - var lookupStream = stream.createLookupStream(resolver); + var lookupStream = stream(resolver); test_stream([inputDoc], lookupStream, function(err, actual) { t.deepEqual(actual, [expectedDoc], 'alpha3 should have been set'); @@ -363,7 +366,7 @@ tape('tests', function(test) { } }; - var lookupStream = stream.createLookupStream(resolver); + var lookupStream = stream(resolver); t.doesNotThrow( function () { @@ -399,7 +402,7 @@ tape('tests', function(test) { } }; - var lookupStream = stream.createLookupStream(resolver); + var lookupStream = stream(resolver); test_stream(input, lookupStream, function(err, actual) { t.deepEqual(actual, expected, 'all fields should have been set'); @@ -436,7 +439,7 @@ tape('tests', function(test) { } }; - var lookupStream = stream.createLookupStream(resolver); + var lookupStream = stream(resolver); test_stream(input, lookupStream, function(err, actual) { t.deepEqual(actual, expected, 'all fields should have been set'); @@ -456,7 +459,7 @@ tape('tests', function(test) { } }; - var lookupStream = stream.createLookupStream(resolver); + var lookupStream = stream(resolver); lookupStream.end(); }); diff --git a/test/test.js b/test/test.js index f5f1087..3c7f90b 100644 --- a/test/test.js +++ b/test/test.js @@ -1,4 +1,4 @@ require ('./configValidationTest.js'); require ('./lookupStreamTest.js'); -require ('./resolversFactoryTest.js'); -require ('./resolversFactoryLocalTest.js'); +require ('./httpPipResolverTest.js'); +require ('./localPipResolverTest.js');