diff --git a/helper/fieldValue.js b/helper/fieldValue.js new file mode 100644 index 00000000..6a9c5c10 --- /dev/null +++ b/helper/fieldValue.js @@ -0,0 +1,47 @@ +'use strict'; + +const _ = require('lodash'); + +function getStringValue(property) { + // numeric value, cast to string + if (_.isNumber(property)) { + return _.toString(property); + } + + // isEmpty check works for all types of values: strings, arrays, objects + if (_.isEmpty(property)) { + return ''; + } + + if (_.isString(property)) { + return property; + } + + // array value, take first item in array (at this time only used for admin & name values) + if (_.isArray(property)) { + return property[0]; + } + + return _.toString(property); +} + +function getArrayValue(property) { + // numeric value, cast to array + if (_.isNumber(property)) { + return [property]; + } + + // isEmpty check works for all types of values: strings, arrays, objects + if (_.isEmpty(property)) { + return []; + } + + if (_.isArray(property)) { + return property; + } + + return [property]; +} + +module.exports.getStringValue = getStringValue; +module.exports.getArrayValue = getArrayValue; diff --git a/helper/geojsonify.js b/helper/geojsonify.js index 515446ce..5c571fa7 100644 --- a/helper/geojsonify.js +++ b/helper/geojsonify.js @@ -5,6 +5,7 @@ const logger = require('pelias-logger').get('geojsonify'); const collectDetails = require('./geojsonify_place_details'); const _ = require('lodash'); const Document = require('pelias-model').Document; +const field = require('./fieldValue'); function geojsonifyPlaces( params, docs ){ @@ -53,7 +54,7 @@ function geojsonifyPlace(params, place) { // assign name, logging a warning if it doesn't exist if (_.has(place, 'name.default')) { - doc.name = place.name.default; + doc.name = field.getStringValue(place.name.default); } else { logger.warn(`doc ${doc.gid} does not contain name.default`); } diff --git a/helper/geojsonify_place_details.js b/helper/geojsonify_place_details.js index c80da8b1..4c2b60b7 100644 --- a/helper/geojsonify_place_details.js +++ b/helper/geojsonify_place_details.js @@ -1,6 +1,7 @@ 'use strict'; const _ = require('lodash'); +const field = require('./fieldValue'); // Properties to be copied // If a property is identified as a single string, assume it should be presented as a string in response @@ -89,10 +90,10 @@ function collectProperties( params, source ) { switch (prop.type) { case 'string': - value = getStringValue(source[prop.name]); + value = field.getStringValue(source[prop.name]); break; case 'array': - value = getArrayValue(source[prop.name]); + value = field.getArrayValue(source[prop.name]); break; // default behavior is to copy property exactly as is default: @@ -110,35 +111,4 @@ function collectProperties( params, source ) { } -function getStringValue(property) { - // isEmpty check works for all types of values: strings, arrays, objects - if (_.isEmpty(property)) { - return ''; - } - - if (_.isString(property)) { - return property; - } - - // array value, take first item in array (at this time only used for admin values) - if (_.isArray(property)) { - return property[0]; - } - - return _.toString(property); -} - -function getArrayValue(property) { - // isEmpty check works for all types of values: strings, arrays, objects - if (_.isEmpty(property)) { - return ''; - } - - if (_.isArray(property)) { - return property; - } - - return [property]; -} - module.exports = collectProperties; diff --git a/middleware/assignLabels.js b/middleware/assignLabels.js index 1f072ddb..9ea6d74f 100644 --- a/middleware/assignLabels.js +++ b/middleware/assignLabels.js @@ -1,4 +1,4 @@ -var defaultLabelGenerator = require('pelias-labels'); +const defaultLabelGenerator = require('pelias-labels'); function setup(labelGenerator) { function middleware(req, res, next) { diff --git a/middleware/confidenceScore.js b/middleware/confidenceScore.js index 5f82ddf6..49597251 100644 --- a/middleware/confidenceScore.js +++ b/middleware/confidenceScore.js @@ -11,9 +11,10 @@ * - detection (or specification) of query type. i.e. an address shouldn't match an admin address. */ -var stats = require('stats-lite'); -var logger = require('pelias-logger').get('api'); -var check = require('check-types'); +const stats = require('stats-lite'); +const logger = require('pelias-logger').get('api'); +const check = require('check-types'); +const field = require('../helper/fieldValue'); var RELATIVE_SCORES = true; @@ -131,12 +132,12 @@ function checkDistanceFromMean(score, mean, stdev) { function checkName(text, parsed_text, hit) { // parsed_text name should take precedence if available since it's the cleaner name property if (check.assigned(parsed_text) && check.assigned(parsed_text.name) && - hit.name.default.toLowerCase() === parsed_text.name.toLowerCase()) { + field.getStringValue(hit.name.default).toLowerCase() === parsed_text.name.toLowerCase()) { return 1; } // if no parsed_text check the text value as provided against result's default name - if (hit.name.default.toLowerCase() === text.toLowerCase()) { + if (field.getStringValue(hit.name.default).toLowerCase() === text.toLowerCase()) { return 1; } diff --git a/middleware/dedupe.js b/middleware/dedupe.js index a50b0494..0a3fd7e6 100644 --- a/middleware/dedupe.js +++ b/middleware/dedupe.js @@ -1,6 +1,7 @@ -var logger = require('pelias-logger').get('api'); -var _ = require('lodash'); -var isDifferent = require('../helper/diffPlaces').isDifferent; +const logger = require('pelias-logger').get('api'); +const _ = require('lodash'); +const isDifferent = require('../helper/diffPlaces').isDifferent; +const field = require('../helper/fieldValue'); function setup() { return dedupeResults; @@ -38,7 +39,7 @@ function dedupeResults(req, res, next) { logger.info('[dupe][replacing]', { query: req.clean.text, previous: uniqueResults[dupeIndex].source, - hit: hit.name.default + ' ' + hit.source + ':' + hit._id + hit: field.getStringValue(hit.name.default) + ' ' + hit.source + ':' + hit._id }); // replace previous dupe item with current hit uniqueResults[dupeIndex] = hit; @@ -48,7 +49,7 @@ function dedupeResults(req, res, next) { logger.info('[dupe][skipping]', { query: req.clean.text, previous: uniqueResults[dupeIndex].source, - hit: hit.name.default + ' ' + hit.source + ':' + hit._id + hit: field.getStringValue(hit.name.default) + ' ' + hit.source + ':' + hit._id }); } } diff --git a/middleware/localNamingConventions.js b/middleware/localNamingConventions.js index 20c77f6b..491cc0ed 100644 --- a/middleware/localNamingConventions.js +++ b/middleware/localNamingConventions.js @@ -1,5 +1,6 @@ -var check = require('check-types'); -var _ = require('lodash'); +const check = require('check-types'); +const _ = require('lodash'); +const field = require('../helper/fieldValue'); var flipNumberAndStreetCountries = ['DEU', 'FIN', 'SWE', 'NOR', 'DNK', 'ISL', 'CZE']; @@ -49,7 +50,7 @@ function flipNumberAndStreet(place) { flipped = ( place.address_parts.street + ' ' + place.address_parts.number ); // flip street name and housenumber - if( place.name.default === standard ){ + if( field.getStringValue(place.name.default) === standard ){ place.name.default = flipped; } } diff --git a/package.json b/package.json index e94c667b..7be0cb3e 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,7 @@ "morgan": "^1.8.2", "pelias-categories": "1.2.0", "pelias-config": "2.14.0", - "pelias-labels": "1.7.0", + "pelias-labels": "1.8.0", "pelias-logger": "0.3.1", "pelias-microservice-wrapper": "1.3.0", "pelias-model": "5.3.2", diff --git a/test/unit/helper/diffPlaces.js b/test/unit/helper/diffPlaces.js index a7dd692d..6fc27c36 100644 --- a/test/unit/helper/diffPlaces.js +++ b/test/unit/helper/diffPlaces.js @@ -166,6 +166,22 @@ module.exports.tests.dedupe = function(test, common) { t.false(isDifferent(item1, item2), 'should be the same'); t.end(); }); + + test('works with name aliases', function(t) { + var item1 = { + 'name': { + 'default': ['a','b'] // note the array + } + }; + var item2 = { + 'name': { + 'default': 'a' + } + }; + + t.false(isDifferent(item1, item2), 'should be the same'); + t.end(); + }); }; module.exports.all = function (tape, common) { diff --git a/test/unit/helper/fieldValue.js b/test/unit/helper/fieldValue.js new file mode 100644 index 00000000..d3d007fe --- /dev/null +++ b/test/unit/helper/fieldValue.js @@ -0,0 +1,44 @@ +const field = require('../../../helper/fieldValue'); + +module.exports.tests = {}; + +module.exports.tests.getStringValue = function(test) { + test('getStringValue', function(t) { + t.equal(field.getStringValue([]), '', 'empty array'); + t.equal(field.getStringValue(''), '', 'empty string'); + t.equal(field.getStringValue('foo'), 'foo', 'string'); + t.equal(field.getStringValue(['foo','bar']), 'foo', 'array'); + t.equal(field.getStringValue(['']), '', 'array with empty string'); + t.equal(field.getStringValue(-0), '-0', 'number'); + t.equal(field.getStringValue(+0), '0', 'number'); + + // note: this behaviour is not desirable, it was inherited during a refactor + // see: https://github.com/pelias/api/pull/1102 + t.equal(field.getStringValue({foo: 'bar'}), '[object Object]', '_.toString'); + t.end(); + }); +}; + +module.exports.tests.getArrayValue = function(test) { + test('getArrayValue', function(t) { + t.deepEqual(field.getArrayValue([]), [], 'empty array'); + t.deepEqual(field.getArrayValue(''), [], 'empty string'); + t.deepEqual(field.getArrayValue('foo'), ['foo'], 'string'); + t.deepEqual(field.getArrayValue(['foo','bar']), ['foo','bar'], 'array'); + t.deepEqual(field.getArrayValue(['']), [''], 'array with empty string'); + t.deepEqual(field.getArrayValue(-0), [0], 'number'); + t.deepEqual(field.getArrayValue(+0), [0], 'number'); + t.deepEqual(field.getArrayValue({foo: 'bar'}), [{foo: 'bar'}], '[*]'); + t.end(); + }); +}; + +module.exports.all = function (tape, common) { + function test(name, testFunction) { + return tape('fieldValue: ' + name, testFunction); + } + + for( var testCase in module.exports.tests ){ + module.exports.tests[testCase](test, common); + } +}; diff --git a/test/unit/helper/geojsonify.js b/test/unit/helper/geojsonify.js index 72ff6392..f5176752 100644 --- a/test/unit/helper/geojsonify.js +++ b/test/unit/helper/geojsonify.js @@ -638,6 +638,51 @@ module.exports.tests.non_optimal_conditions = (test, common) => { }; +// ensure that if elasticsearch returns an array of values for name.default +// .. that we handle this case and select the first element for the label. +module.exports.tests.nameAliases = function(test, common) { + test('name aliases', function(t) { + var aliases = [{ + '_type': 'example', + '_id': '1', + 'source': 'example', + 'layer': 'example', + 'name': { + 'default': ['Example1', 'Example2'] // note the array + }, + 'center_point': { + 'lon': 0, + 'lat': 0 + } + }]; + + const expected = { + type: 'FeatureCollection', + features: [{ + type: 'Feature', + geometry: { + type: 'Point', + coordinates: [ 0, 0 ] + }, + properties: { + id: '1', + gid: 'example:example:1', + layer: 'example', + source: 'example', + source_id: undefined, + name: 'Example1' + } + }], + bbox: [ 0, 0, 0, 0 ] + }; + + var actual = geojsonify( {}, aliases ); + t.deepEquals(actual, expected); + t.end(); + }); + +}; + module.exports.all = (tape, common) => { function test(name, testFunction) { return tape(`geojsonify: ${name}`, testFunction); diff --git a/test/unit/helper/geojsonify_place_details.js b/test/unit/helper/geojsonify_place_details.js index c7f2c8ac..bb5bfa58 100644 --- a/test/unit/helper/geojsonify_place_details.js +++ b/test/unit/helper/geojsonify_place_details.js @@ -108,7 +108,7 @@ module.exports.tests.geojsonify_place_details = (test, common) => { }); test('\'empty\' string-type values should be output as \'\'', t => { - [ [], {}, '', 17, true, null, undefined ].forEach(empty_value => { + [ [], {}, '', true, null, undefined ].forEach(empty_value => { const source = { housenumber: empty_value, street: empty_value, diff --git a/test/unit/middleware/assignLabels.js b/test/unit/middleware/assignLabels.js index 26e32718..ebd7c22a 100644 --- a/test/unit/middleware/assignLabels.js +++ b/test/unit/middleware/assignLabels.js @@ -104,6 +104,33 @@ module.exports.tests.serialization = function(test, common) { }); + test('support name aliases', function(t) { + var assignLabels = require('../../../middleware/assignLabels')(); + + var res = { + data: [{ + name: { + default: ['name1','name2'] + } + }] + }; + + var expected = { + data: [{ + name: { + default: ['name1','name2'] + }, + label: 'name1' + }] + }; + + assignLabels({}, res, function () { + t.deepEqual(res, expected); + t.end(); + }); + + }); + }; module.exports.all = function (tape, common) { diff --git a/test/unit/middleware/confidenceScore.js b/test/unit/middleware/confidenceScore.js index 9e10775f..316b2de6 100644 --- a/test/unit/middleware/confidenceScore.js +++ b/test/unit/middleware/confidenceScore.js @@ -201,6 +201,38 @@ module.exports.tests.confidenceScore = function(test, common) { t.equal(res.data[0].confidence, 0.28, 'score was set'); t.end(); }); + + test('works with name aliases', function(t) { + var req = { + clean: { + text: 'example', + parsed_text: { + number: 123, + street: 'example', + state: 'EG' + } + } + }; + var res = { + data: [{ + _score: 10, + found: true, + value: 1, + center_point: { lat: 100.1, lon: -50.5 }, + name: { default: ['test name1', 'test name2'] }, // note the array + }], + 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) { diff --git a/test/unit/middleware/dedupe.js b/test/unit/middleware/dedupe.js index ca8a5cae..9b4d04c3 100644 --- a/test/unit/middleware/dedupe.js +++ b/test/unit/middleware/dedupe.js @@ -223,41 +223,71 @@ module.exports.tests.trump = function(test, common) { }); }); -test('osm with zip trumps openaddresses without zip', function (t) { - var req = { - clean: { - text: '100 Main St', - size: 100 - } - }; - var res = { - data: [ - { - 'name': { 'default': '100 Main St' }, - 'source': 'openaddresses', - 'source_id': '123456', - 'layer': 'address', - 'address_parts': {} - }, - { - 'name': { 'default': '100 Main St' }, - 'source': 'openstreetmap', - 'source_id': '654321', - 'layer': 'address', - 'address_parts': { - 'zip': '54321' + test('osm with zip trumps openaddresses without zip', function (t) { + var req = { + clean: { + text: '100 Main St', + size: 100 + } + }; + var res = { + data: [ + { + 'name': { 'default': '100 Main St' }, + 'source': 'openaddresses', + 'source_id': '123456', + 'layer': 'address', + 'address_parts': {} + }, + { + 'name': { 'default': '100 Main St' }, + 'source': 'openstreetmap', + 'source_id': '654321', + 'layer': 'address', + 'address_parts': { + 'zip': '54321' + } } + ] + }; + + var expectedCount = 1; + dedupe(req, res, function () { + t.equal(res.data.length, expectedCount, 'results have fewer items than before'); + t.deepEqual(res.data[0].source_id, '654321', 'openstreetmap result with zip won'); + t.end(); + }); + }); + + test('works with name aliases', function (t) { + var req = { + clean: { + text: '100 Main St', + size: 100 } - ] - }; + }; + var res = { + data: [ + { + 'name': { 'default': ['100 Main St'] }, // note the array + 'source': 'openaddresses', + 'source_id': '123456' + }, + { + 'name': { 'default': '100 Main St' }, + 'source': 'openstreetmap', + 'source_id': '654321' + } + ] + }; + + t.doesNotThrow(() => { + dedupe(req, res, () => {}); + }); - var expectedCount = 1; - dedupe(req, res, function () { - t.equal(res.data.length, expectedCount, 'results have fewer items than before'); - t.deepEqual(res.data[0].source_id, '654321', 'openstreetmap result with zip won'); + t.equal(res.data.length, 1, 'results have fewer items than before'); t.end(); }); -}); }; module.exports.all = function (tape, common) { diff --git a/test/unit/middleware/localNamingConventions.js b/test/unit/middleware/localNamingConventions.js index 1d4102f0..96fdc1d8 100644 --- a/test/unit/middleware/localNamingConventions.js +++ b/test/unit/middleware/localNamingConventions.js @@ -1,5 +1,5 @@ -var proxyquire = require('proxyquire'); +const proxyquire = require('proxyquire'); var customConfig = { generate: function generate() { @@ -40,7 +40,7 @@ module.exports.tests.flipNumberAndStreet = function(test, common) { var deAddress = { '_id': 'test2', '_type': 'test', - 'name': { 'default': '23 Grolmanstraße' }, + 'name': { 'default': ['23 Grolmanstraße'] }, 'center_point': { 'lon': 13.321487, 'lat': 52.506781 }, 'address_parts': { 'zip': '10623', diff --git a/test/unit/run.js b/test/unit/run.js index 1a557733..52b6e519 100644 --- a/test/unit/run.js +++ b/test/unit/run.js @@ -32,6 +32,7 @@ var tests = [ require('./controller/predicates/is_request_sources_only_whosonfirst'), require('./helper/debug'), require('./helper/diffPlaces'), + require('./helper/fieldValue'), require('./helper/geojsonify_place_details'), require('./helper/geojsonify'), require('./helper/logging'),