feat: Merge pull request #1013 from pelias/rework-interpolation-error-handling

wrap interpolation service call to intercept errors

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

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