From 05da79923f7bf81e86c3c017843c57cd67bb8802 Mon Sep 17 00:00:00 2001 From: Stephen Hess Date: Mon, 2 Oct 2017 08:57:46 -0400 Subject: [PATCH] wrap interpolation service call to intercept errors --- middleware/interpolate.js | 34 +++- test/unit/middleware/interpolate.js | 291 ++++++++++++++++------------ 2 files changed, 193 insertions(+), 132 deletions(-) diff --git a/middleware/interpolate.js b/middleware/interpolate.js index 0c7fbd99..a7d3e1d8 100644 --- a/middleware/interpolate.js +++ b/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]; diff --git a/test/unit/middleware/interpolate.js b/test/unit/middleware/interpolate.js index de855854..f020628d 100644 --- a/test/unit/middleware/interpolate.js +++ b/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) {