Merge pull request #1020 from pelias/staging

Merge staging into production

helper/geojsonify_place_details.js

@@ -46,6 +46,9 @@ const DETAILS_PROPS = [
   { name: 'continent',         type: 'string' },
   { name: 'continent_gid',     type: 'string' },
   { name: 'continent_a',       type: 'string' },
+  { name: 'empire',            type: 'string',    condition: _.negate(hasCountry) },
+  { name: 'empire_gid',        type: 'string',    condition: _.negate(hasCountry) },
+  { name: 'empire_a',          type: 'string',    condition: _.negate(hasCountry) },
   { name: 'ocean',             type: 'string' },
   { name: 'ocean_gid',         type: 'string' },
   { name: 'ocean_a',           type: 'string' },
@@ -57,6 +60,11 @@ const DETAILS_PROPS = [
   { name: 'category',          type: 'array',     condition: checkCategoryParam }
 ];
 
+// returns true IFF source a country_gid property
+function hasCountry(params, source) {
+  return source.hasOwnProperty('country_gid');
+}
+
 function checkCategoryParam(params) {
   return _.isObject(params) && params.hasOwnProperty('categories');
 }
@@ -72,7 +80,7 @@ function checkCategoryParam(params) {
 function collectProperties( params, source ) {
   return DETAILS_PROPS.reduce((result, prop) => {
     // if condition isn't met, don't set the property
-    if (_.isFunction(prop.condition) && !prop.condition(params)) {
+    if (_.isFunction(prop.condition) && !prop.condition(params, source)) {
       return result;
     }
 

helper/type_mapping.js

@@ -1,5 +1,4 @@
-var extend = require('extend'),
+const _ = require('lodash');
-  _ = require('lodash');
 
 function addStandardTargetsToAliases(standard, aliases) {
   var combined = _.extend({}, aliases);

middleware/geocodeJSON.js

@@ -1,5 +1,4 @@
 var url = require('url');
-var extend = require('extend');
 var geojsonify = require('../helper/geojsonify');
 var _ = require('lodash');
 
@@ -74,7 +73,7 @@ function convertToGeocodeJSON(req, res, next, opts) {
   res.body.geocoding.timestamp = new Date().getTime();
 
   // convert docs to geojson and merge with geocoding block
-  extend(res.body, geojsonify(req.clean, res.data || []));
+  _.extend(res.body, geojsonify(req.clean, res.data || []));
 
   next();
 }

middleware/interpolate.js

@@ -21,15 +21,34 @@ example response from interpolation web service:
 }
 **/
 
+// The interpolation middleware layer uses async.map to iterate over the results
+//  since the interpolation service only operates on single inputs.  The problem
+//  with async.map is that if a single error is returned then the entire batch
+//  exits early.  This function wraps the service call to intercept the error so
+//  that async.map never returns an error.
+function error_intercepting_service(service, req) {
+  return (street_result, next) => {
+    service(req, street_result, (err, interpolation_result) => {
+      if (err) {
+        logger.error(`[middleware:interpolation] ${_.defaultTo(err.message, err)}`);
+        // now that the error has been caught and reported, act as if there was no error
+        return next(null, null);
+      }
+
+      // no error occurred, so pass along the result
+      return next(null, interpolation_result);
+
+    });
+  };
+}
+
 function setup(service, should_execute) {
   return function controller(req, res, next) {
+    // bail early if the service shouldn't execute
     if (!should_execute(req, res)) {
       return next();
     }
 
-    // bind the service to the req which doesn't change
-    const req_bound_service = _.partial(service, req);
-
     // only interpolate the street-layer results
     // save this off into a separate array so that when docs are annotated
     //  after the interpolate results are returned, no complicated bookkeeping is needed
@@ -37,12 +56,11 @@ function setup(service, should_execute) {
 
     // perform interpolations asynchronously for all relevant hits
     const start = (new Date()).getTime();
-    async.map(street_results, req_bound_service, (err, interpolation_results) => {
-      if (err) {
-        logger.error(`[middleware:interpolation] ${_.defaultTo(err.message, err)}`);
-        return next();
-      }
 
+    // call the interpolation service asynchronously on every street result
+    async.map(street_results, error_intercepting_service(service, req), (err, interpolation_results) => {
+
+      // iterate the interpolation results, mapping back into the source results
       interpolation_results.forEach((interpolation_result, idx) => {
         const source_result = street_results[idx];
 

package.json

@@ -44,7 +44,6 @@
     "elasticsearch": "^13.0.0",
     "elasticsearch-exceptions": "0.0.4",
     "express": "^4.8.8",
-    "extend": "^3.0.1",
     "geojson": "^0.5.0",
     "@mapbox/geojson-extent": "^0.3.1",
     "geolib": "^2.0.18",
@@ -57,7 +56,7 @@
     "morgan": "^1.8.2",
     "pelias-categories": "1.2.0",
     "pelias-config": "2.12.1",
-    "pelias-labels": "1.6.0",
+    "pelias-labels": "1.7.0",
     "pelias-logger": "0.2.0",
     "pelias-microservice-wrapper": "1.2.1",
     "pelias-model": "5.1.0",

sanitizer/_text_addressit.js

@@ -1,6 +1,5 @@
 var check = require('check-types');
 var parser = require('addressit');
-var extend = require('extend');
 var _      = require('lodash');
 var logger = require('pelias-logger').get('api');
 
@@ -82,8 +81,8 @@ function parse(query) {
   var addressWithAdminParts  = getAdminPartsBySplittingOnDelim(queryParts);
   var addressWithAddressParts= getAddressParts(queryParts.join(DELIM + ' '));
 
-  var parsedAddress  = extend(addressWithAdminParts,
+  // combine the 2 objects
-                              addressWithAddressParts);
+  _.extend(addressWithAdminParts, addressWithAddressParts);
 
   var address_parts  =  [ 'name',
                           'number',
@@ -99,8 +98,8 @@ function parse(query) {
   var parsed_text = {};
 
   address_parts.forEach(function(part){
-    if (parsedAddress[part]) {
+    if (addressWithAdminParts[part]) {
-      parsed_text[part] = parsedAddress[part];
+      parsed_text[part] = addressWithAdminParts[part];
     }
   });
 

test/unit/helper/geojsonify_place_details.js

@@ -532,6 +532,52 @@ module.exports.tests.geojsonify_place_details = (test, common) => {
 
 };
 
+module.exports.tests.empire_specific = (test, common) => {
+  test('empire* properties should not be output when country_gid property is available', t => {
+    const source = {
+      country_gid: 'country_gid value',
+      empire: 'empire value',
+      empire_gid: 'empire_gid value',
+      empire_a: 'empire_a value',
+      label: 'label value'
+    };
+
+    const expected = {
+      country_gid: 'country_gid value',
+      label: 'label value'
+    };
+
+    const actual = geojsonify({}, source);
+
+    t.deepEqual(actual, expected);
+    t.end();
+
+  });
+
+  test('empire* properties should be output when country_gid property is not available', t => {
+    const source = {
+      empire: 'empire value',
+      empire_gid: 'empire_gid value',
+      empire_a: 'empire_a value',
+      label: 'label value'
+    };
+
+    const expected = {
+      empire: 'empire value',
+      empire_gid: 'empire_gid value',
+      empire_a: 'empire_a value',
+      label: 'label value'
+    };
+
+    const actual = geojsonify({}, source);
+
+    t.deepEqual(actual, expected);
+    t.end();
+
+  });
+
+};
+
 module.exports.all = (tape, common) => {
 
   function test(name, testFunction) {

test/unit/middleware/interpolate.js

@@ -38,129 +38,6 @@ module.exports.tests.early_exit_conditions = (test, common) => {
 
 };
 
-module.exports.tests.error_conditions = (test, common) => {
-  test('service error string should log and not modify any results', t => {
-    t.plan(2);
-
-    const service = (req, res, callback) => {
-      callback('this is an error', {
-        properties: {
-          number: 17,
-          source: 'OSM',
-          source_id: 'openstreetmap source id',
-          lat: 12.121212,
-          lon: 21.212121
-        }
-      });
-
-    };
-
-    const logger = require('pelias-mock-logger')();
-
-    const controller = proxyquire('../../../middleware/interpolate', {
-      'pelias-logger': logger
-    })(service, () => true);
-
-    const req = { a: 1 };
-    const res = {
-      data: [
-        {
-          id: 1,
-          layer: 'street',
-          name: {
-            default: 'street name 1'
-          },
-          address_parts: {},
-          // bounding_box should be removed
-          bounding_box: {}
-        }
-      ]
-    };
-
-    controller(req, res, () => {
-      t.ok(logger.isErrorMessage('[middleware:interpolation] this is an error'));
-
-      t.deepEquals(res, {
-        data: [
-          {
-            id: 1,
-            layer: 'street',
-            name: {
-              default: 'street name 1'
-            },
-            address_parts: {},
-            // bounding_box should be removed
-            bounding_box: {}
-          }
-        ]
-      }, 'res should not have been modified');
-
-    });
-
-  });
-
-  test('service error object should log message and not modify any results', t => {
-    t.plan(2);
-
-    const service = (req, res, callback) => {
-      callback({ message: 'this is an error' }, {
-        properties: {
-          number: 17,
-          source: 'OSM',
-          source_id: 'openstreetmap source id',
-          lat: 12.121212,
-          lon: 21.212121
-        }
-      });
-
-    };
-
-    const logger = require('pelias-mock-logger')();
-
-    const controller = proxyquire('../../../middleware/interpolate', {
-      'pelias-logger': logger
-    })(service, () => true);
-
-    const req = { a: 1 };
-    const res = {
-      data: [
-        {
-          id: 1,
-          layer: 'street',
-          name: {
-            default: 'street name 1'
-          },
-          address_parts: {},
-          // bounding_box should be removed
-          bounding_box: {}
-        }
-      ]
-    };
-
-    controller(req, res, () => {
-      t.ok(logger.isErrorMessage('[middleware:interpolation] this is an error'));
-
-      t.deepEquals(res, {
-        data: [
-          {
-            id: 1,
-            layer: 'street',
-            name: {
-              default: 'street name 1'
-            },
-            address_parts: {},
-            // bounding_box should be removed
-            bounding_box: {}
-          }
-        ]
-      }, 'res should not have been modified');
-
-    });
-
-  });
-
-};
-
 module.exports.tests.success_conditions = (test, common) => {
   test('undefined res should not cause errors', t => {
     const service = (req, res, callback) => {
@@ -219,7 +96,7 @@ module.exports.tests.success_conditions = (test, common) => {
 
   });
 
-  test('interpolated results should be mapped in', t => {
+  test('only \'street\' layer results should attempt interpolation', t => {
     const service = (req, res, callback) => {
       if (res.id === 1) {
         callback(null, {
@@ -294,6 +171,7 @@ module.exports.tests.success_conditions = (test, common) => {
           bounding_box: {}
         },
         {
+          // this is not a street result and should not attempt interpolation
           id: 2,
           layer: 'not street',
           name: {
@@ -398,6 +276,171 @@ module.exports.tests.success_conditions = (test, common) => {
 
   });
 
+  test('service call returning error should not map in interpolated results for non-errors', t => {
+    const service = (req, res, callback) => {
+      if (res.id === 1) {
+        callback(null, {
+          properties: {
+            number: 17,
+            source: 'Source Abbr 1',
+            source_id: 'source 1 source id',
+            lat: 12.121212,
+            lon: 21.212121
+          }
+        });
+
+      } else if (res.id === 2) {
+        callback('id 2 produced an error string', {});
+
+      } else if (res.id === 3) {
+        callback({ message: 'id 3 produced an error object' }, {});
+
+      } else if (res.id === 4) {
+        callback(null, {
+          properties: {
+            number: 18,
+            source: 'Source Abbr 4',
+            source_id: 'source 4 source id',
+            lat: 13.131313,
+            lon: 31.313131
+          }
+        });
+
+      } else {
+        console.error(res.id);
+        t.fail(`unexpected id ${res.id}`);
+
+      }
+
+    };
+
+    const logger = require('pelias-mock-logger')();
+
+    const controller = proxyquire('../../../middleware/interpolate', {
+      'pelias-logger': logger,
+      '../helper/type_mapping': {
+        source_mapping: {
+          'source abbr 1': ['full source name 1'],
+          'source abbr 4': ['full source name 4']
+        }
+      }
+    })(service, () => true);
+
+    const req = {
+      clean: {
+        parsed_text: 'this is req.clean.parsed_text'
+      }
+    };
+
+    const res = {
+      data: [
+        {
+          id: 1,
+          layer: 'street',
+          name: {
+            default: 'street name 1'
+          },
+          address_parts: {}
+        },
+        {
+          id: 2,
+          layer: 'street',
+          name: {
+            default: 'street name 2'
+          },
+          address_parts: {}
+        },
+        {
+          id: 3,
+          layer: 'street',
+          name: {
+            default: 'street name 3'
+          },
+          address_parts: {}
+        },
+        {
+          id: 4,
+          layer: 'street',
+          name: {
+            default: 'street name 4'
+          },
+          address_parts: {}
+        }
+      ]
+    };
+
+    controller(req, res, () => {
+      t.deepEquals(logger.getErrorMessages(), [
+        '[middleware:interpolation] id 2 produced an error string',
+        '[middleware:interpolation] id 3 produced an error object'
+      ]);
+      t.ok(logger.isInfoMessage(/\[interpolation\] \[took\] \d+ ms/), 'timing should be info-logged');
+
+      // test debug messages very vaguely to avoid brittle tests
+      t.ok(logger.isDebugMessage(/^\[interpolation\] \[hit\] this is req.clean.parsed_text \{.+?\}$/),
+        'hits should be debug-logged');
+
+      t.deepEquals(res, {
+        data: [
+          {
+            id: 1,
+            layer: 'address',
+            match_type: 'interpolated',
+            name: {
+              default: '17 street name 1'
+            },
+            source: 'full source name 1',
+            source_id: 'source 1 source id',
+            address_parts: {
+              number: 17
+            },
+            center_point: {
+              lat: 12.121212,
+              lon: 21.212121
+            }
+          },
+          {
+            id: 4,
+            layer: 'address',
+            match_type: 'interpolated',
+            name: {
+              default: '18 street name 4'
+            },
+            source: 'full source name 4',
+            source_id: 'source 4 source id',
+            address_parts: {
+              number: 18
+            },
+            center_point: {
+              lat: 13.131313,
+              lon: 31.313131
+            }
+          },
+          {
+            id: 2,
+            layer: 'street',
+            name: {
+              default: 'street name 2'
+            },
+            address_parts: {}
+          },
+          {
+            id: 3,
+            layer: 'street',
+            name: {
+              default: 'street name 3'
+            },
+            address_parts: {}
+          }
+        ]
+      }, 'hits should be mapped in and res.data sorted with addresses first and non-addresses last');
+
+      t.end();
+
+    });
+
+  });
+
   test('interpolation result without source_id should remove all together', t => {
     const service = (req, res, callback) => {
       if (res.id === 1) {