added config validation, inject configuration values

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)
 };

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) {

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);
+  };
+};

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;
+};

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;
 };

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() {

test/resolversFactoryTest.js → 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,

test/resolversFactoryLocalTest.js → 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 = {