From b88d2a0abe7ca72861abcd5091f98a94c22db0c0 Mon Sep 17 00:00:00 2001 From: missinglink Date: Mon, 25 Sep 2017 17:40:47 +0200 Subject: [PATCH 1/3] feat(coarse_reverse): add try/catch block around synthesizeDoc code --- controller/coarse_reverse.js | 71 +++++++++++++++----------- test/unit/controller/coarse_reverse.js | 54 ++++++++++++++++++++ 2 files changed, 96 insertions(+), 29 deletions(-) diff --git a/controller/coarse_reverse.js b/controller/coarse_reverse.js index e49dc8c9..1ed47c58 100644 --- a/controller/coarse_reverse.js +++ b/controller/coarse_reverse.js @@ -62,40 +62,50 @@ function synthesizeDoc(results) { const most_granular_layer = getMostGranularLayerOfResult(_.keys(results)); const id = results[most_granular_layer][0].id; - const doc = new Document('whosonfirst', most_granular_layer, id.toString()); - doc.setName('default', results[most_granular_layer][0].name); + try { + const doc = new Document('whosonfirst', most_granular_layer, id.toString()); + doc.setName('default', results[most_granular_layer][0].name); - // assign the administrative hierarchy - _.keys(results).forEach((layer) => { - doc.addParent(layer, results[layer][0].name, results[layer][0].id.toString(), results[layer][0].abbr); - }); + // assign the administrative hierarchy + _.keys(results).forEach((layer) => { + doc.addParent(layer, results[layer][0].name, results[layer][0].id.toString(), results[layer][0].abbr); + }); - // set centroid if available - if (_.has(results[most_granular_layer][0], 'centroid')) { - doc.setCentroid( results[most_granular_layer][0].centroid ); - } + // set centroid if available + if (_.has(results[most_granular_layer][0], 'centroid')) { + doc.setCentroid( results[most_granular_layer][0].centroid ); + } - // set bounding box if available - if (_.has(results[most_granular_layer][0], 'bounding_box')) { - const parsed_bounding_box = results[most_granular_layer][0].bounding_box.split(',').map(parseFloat); - doc.setBoundingBox({ - upperLeft: { - lat: parsed_bounding_box[3], - lon: parsed_bounding_box[0] - }, - lowerRight: { - lat: parsed_bounding_box[1], - lon: parsed_bounding_box[2] - } - }); + // set bounding box if available + if (_.has(results[most_granular_layer][0], 'bounding_box')) { + const parsed_bounding_box = results[most_granular_layer][0].bounding_box.split(',').map(parseFloat); + doc.setBoundingBox({ + upperLeft: { + lat: parsed_bounding_box[3], + lon: parsed_bounding_box[0] + }, + lowerRight: { + lat: parsed_bounding_box[1], + lon: parsed_bounding_box[2] + } + }); - } + } + + const esDoc = doc.toESDocument(); + esDoc.data._id = esDoc._id; + esDoc.data._type = esDoc._type; + return esDoc.data; - const esDoc = doc.toESDocument(); - esDoc.data._id = esDoc._id; - esDoc.data._type = esDoc._type; - return esDoc.data; + } catch( e ) { + // an error occurred when generating a new Document + logger.info(`[controller:coarse_reverse][error]`); + logger.error(e); + logger.error(results); + + return null; + } } function setup(service, should_execute) { @@ -141,7 +151,10 @@ function setup(service, should_execute) { // if there's a result at the requested layer(s), synthesize a doc from results if (hasResultsAtRequestedLayers(applicable_results, effective_layers)) { - res.data.push(synthesizeDoc(applicable_results)); + const doc = synthesizeDoc(applicable_results); + if (doc){ + res.data.push(doc); + } } debugLog.stopTimer(req, initialTime); return next(); diff --git a/test/unit/controller/coarse_reverse.js b/test/unit/controller/coarse_reverse.js index fa42ef0a..cc7c5443 100644 --- a/test/unit/controller/coarse_reverse.js +++ b/test/unit/controller/coarse_reverse.js @@ -860,6 +860,60 @@ module.exports.tests.failure_conditions = (test, common) => { }); + test('service returns 0 length name', (t) => { + t.plan(5); + + const service = (req, callback) => { + t.deepEquals(req, { clean: { layers: ['neighbourhood'] } } ); + + const results = { + neighbourhood: [ + { id: 20, name: '' } + ] + }; + + callback(undefined, results); + }; + + const logger = require('pelias-mock-logger')(); + + const controller = proxyquire('../../../controller/coarse_reverse', { + 'pelias-logger': logger + })(service, _.constant(true)); + + const req = { + clean: { + layers: ['neighbourhood'] + } + }; + + const res = { }; + + // verify that next was called + const next = () => { + t.pass('next() was called'); + }; + + controller(req, res, next); + + const expected = { + meta: {}, + data: [] + }; + + t.deepEquals(res, expected); + t.deepEquals(logger.getMessages('info'), [ + '[controller:coarse_reverse][queryType:pip][result_count:1]', + '[controller:coarse_reverse][error]' + ]); + t.deepEquals(logger.getMessages('error'), [ + '{ [PeliasModelError: invalid document type, expecting: truthy, ' + + 'got: ]\n name: \'PeliasModelError\',\n message: \'invalid document type, expecting: truthy, got: \' }', + '{ neighbourhood: [ { id: 20, name: \'\' } ] }' + ]); + t.end(); + + }); }; module.exports.all = (tape, common) => { From 3913c836e2bbc0144910513b6a2aee9e0a505a57 Mon Sep 17 00:00:00 2001 From: missinglink Date: Mon, 25 Sep 2017 18:05:25 +0200 Subject: [PATCH 2/3] feat(coarse_reverse): improve logger assertions --- controller/coarse_reverse.js | 2 +- test/unit/controller/coarse_reverse.js | 17 +++++++---------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/controller/coarse_reverse.js b/controller/coarse_reverse.js index 1ed47c58..cae79a78 100644 --- a/controller/coarse_reverse.js +++ b/controller/coarse_reverse.js @@ -102,7 +102,7 @@ function synthesizeDoc(results) { // an error occurred when generating a new Document logger.info(`[controller:coarse_reverse][error]`); logger.error(e); - logger.error(results); + logger.info(results); return null; } diff --git a/test/unit/controller/coarse_reverse.js b/test/unit/controller/coarse_reverse.js index cc7c5443..d4e357cb 100644 --- a/test/unit/controller/coarse_reverse.js +++ b/test/unit/controller/coarse_reverse.js @@ -861,7 +861,7 @@ module.exports.tests.failure_conditions = (test, common) => { }); test('service returns 0 length name', (t) => { - t.plan(5); + t.plan(6); const service = (req, callback) => { t.deepEquals(req, { clean: { layers: ['neighbourhood'] } } ); @@ -902,15 +902,12 @@ module.exports.tests.failure_conditions = (test, common) => { }; t.deepEquals(res, expected); - t.deepEquals(logger.getMessages('info'), [ - '[controller:coarse_reverse][queryType:pip][result_count:1]', - '[controller:coarse_reverse][error]' - ]); - t.deepEquals(logger.getMessages('error'), [ - '{ [PeliasModelError: invalid document type, expecting: truthy, ' + - 'got: ]\n name: \'PeliasModelError\',\n message: \'invalid document type, expecting: truthy, got: \' }', - '{ neighbourhood: [ { id: 20, name: \'\' } ] }' - ]); + + // logger messages + t.true(logger.hasMessages('info'), '[controller:coarse_reverse][error]'); + t.true(logger.hasMessages('error'), 'invalid document type, expecting: truthy, got: '); + t.true(logger.hasMessages('info'), '{ neighbourhood: [ { id: 20, name: \'\' } ] }'); + t.end(); }); From 40870c4e048e122e09de9236f309373d64b5113f Mon Sep 17 00:00:00 2001 From: Diana Shkolnikov Date: Mon, 25 Sep 2017 12:44:00 -0400 Subject: [PATCH 3/3] check for missing parent object in confidence score computation --- middleware/confidenceScore.js | 7 +++--- test/unit/middleware/confidenceScore.js | 32 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/middleware/confidenceScore.js b/middleware/confidenceScore.js index 8f5b61fb..5f82ddf6 100644 --- a/middleware/confidenceScore.js +++ b/middleware/confidenceScore.js @@ -94,7 +94,8 @@ function checkForDealBreakers(req, hit) { return false; } - if (check.assigned(req.clean.parsed_text.state) && hit.parent.region_a && req.clean.parsed_text.state !== hit.parent.region_a[0]) { + if (check.assigned(req.clean.parsed_text.state) && check.assigned(hit.parent) && + hit.parent.region_a && req.clean.parsed_text.state !== hit.parent.region_a[0]) { logger.debug('[confidence][deal-breaker]: state !== region_a'); return true; } @@ -220,8 +221,8 @@ function checkAddress(text, hit) { res += propMatch(text.number, (hit.address_parts ? hit.address_parts.number : null), false); res += propMatch(text.street, (hit.address_parts ? hit.address_parts.street : null), false); res += propMatch(text.postalcode, (hit.address_parts ? hit.address_parts.zip: null), true); - res += propMatch(text.state, (hit.parent.region_a ? hit.parent.region_a[0] : null), true); - res += propMatch(text.country, (hit.parent.country_a ? hit.parent.country_a[0] :null), true); + res += propMatch(text.state, ((hit.parent && hit.parent.region_a) ? hit.parent.region_a[0] : null), true); + res += propMatch(text.country, ((hit.parent && hit.parent.country_a) ? hit.parent.country_a[0] :null), true); res /= checkCount; } diff --git a/test/unit/middleware/confidenceScore.js b/test/unit/middleware/confidenceScore.js index a0b4de6f..9e10775f 100644 --- a/test/unit/middleware/confidenceScore.js +++ b/test/unit/middleware/confidenceScore.js @@ -169,6 +169,38 @@ module.exports.tests.confidenceScore = function(test, common) { t.false(res.data[0].hasOwnProperty('confidence'), 'score was not set'); t.end(); }); + + test('missing parent object should not throw an exception', function(t) { + var req = { + clean: { + text: '123 Main St, City, NM', + parsed_text: { + number: 123, + street: 'Main St', + state: 'NM' + } + } + }; + var res = { + data: [{ + _score: 10, + found: true, + value: 1, + center_point: { lat: 100.1, lon: -50.5 }, + name: { default: 'test name1' }, + }], + meta: { + scores: [10], + query_type: 'original' + } + }; + + t.doesNotThrow(() => { + confidenceScore(req, res, () => {}); + }); + t.equal(res.data[0].confidence, 0.28, 'score was set'); + t.end(); + }); }; module.exports.all = function (tape, common) {