From bae534dfe8597eba3dd4173ab86734232e6bf510 Mon Sep 17 00:00:00 2001 From: Joxit Date: Tue, 16 Feb 2021 14:35:50 +0100 Subject: [PATCH] fix(normalizeParentIds): cannot read property toLowerCase of null when source is not set --- middleware/normalizeParentIds.js | 11 ++- package.json | 4 +- test/unit/controller/coarse_reverse.js | 35 +++++++-- test/unit/controller/placeholder.js | 36 ++++++--- test/unit/middleware/normalizeParentIds.js | 89 ++++++++++++++++++++++ 5 files changed, 151 insertions(+), 24 deletions(-) diff --git a/middleware/normalizeParentIds.js b/middleware/normalizeParentIds.js index 5de656bb7..3aa9a6a3a 100644 --- a/middleware/normalizeParentIds.js +++ b/middleware/normalizeParentIds.js @@ -34,20 +34,19 @@ function normalizeParentIds(place) { if (place[placeType] && place[placeType].length > 0 && place[placeType][0]) { // This is a solution for geonames hack. // We can store in ES the source and defaulted to wof for backward compatibility. - let source = _.get(place, `${placeType}_source`, ['whosonfirst']); + // The default values via lodash _.get is used only when the value is `undefined`, in our case it may be null. + let source = _.get(place, `${placeType}_source[0]`) || 'whosonfirst'; - const placetype_ids = _.get(place, `${placeType}_gid`, [null]); + const placetype_ids = _.get(place, `${placeType}_gid[0]`, null); // looking forward to the day we can remove all geonames specific hacks, but until then... // geonames sometimes has its own ids in the parent hierarchy, so it's dangerous to assume that // it's always WOF ids and hardcode to that - if (place.source === 'geonames' && place.source_id === placetype_ids[0]) { + if (place.source === 'geonames' && place.source_id === placetype_ids) { source = place.source; - } else { - source = _.head(source); } - place[`${placeType}_gid`] = [ makeNewId(source, placeType, placetype_ids[0]) ]; + place[`${placeType}_gid`] = [ makeNewId(source, placeType, placetype_ids) ]; } }); } diff --git a/package.json b/package.json index 62ccc8c2c..2b1dd71db 100644 --- a/package.json +++ b/package.json @@ -53,7 +53,7 @@ "pelias-labels": "^1.16.0", "pelias-logger": "^1.2.0", "pelias-microservice-wrapper": "^1.7.0", - "pelias-model": "^7.0.0", + "pelias-model": "^9.0.0", "pelias-parser": "1.53.0", "pelias-query": "^11.0.0", "pelias-sorting": "^1.2.0", @@ -75,7 +75,7 @@ "precommit-hook": "^3.0.0", "proxyquire": "^2.0.0", "tap-dot": "^2.0.0", - "tape": "^5.0.0" + "tape": "^5.1.1" }, "pre-commit": [ "lint", diff --git a/test/unit/controller/coarse_reverse.js b/test/unit/controller/coarse_reverse.js index d0e07fee0..1cad1a72f 100644 --- a/test/unit/controller/coarse_reverse.js +++ b/test/unit/controller/coarse_reverse.js @@ -269,45 +269,59 @@ module.exports.tests.success_conditions = (test, common) => { neighbourhood: ['neighbourhood name'], neighbourhood_id: ['10'], neighbourhood_a: ['neighbourhood abbr'], + neighbourhood_source: [null], borough: ['borough name'], borough_id: ['20'], borough_a: ['borough abbr'], + borough_source: [null], locality: ['locality name'], locality_id: ['30'], locality_a: ['locality abbr'], + locality_source: [null], localadmin: ['localadmin name'], localadmin_id: ['40'], localadmin_a: ['localadmin abbr'], + localadmin_source: [null], county: ['county name'], county_id: ['50'], county_a: ['county abbr'], + county_source: [null], macrocounty: ['macrocounty name'], macrocounty_id: ['60'], macrocounty_a: ['macrocounty abbr'], + macrocounty_source: [null], region: ['region name'], region_id: ['70'], region_a: ['region abbr'], + region_source: [null], macroregion: ['macroregion name'], macroregion_id: ['80'], macroregion_a: ['macroregion abbr'], + macroregion_source: [null], dependency: ['dependency name'], dependency_id: ['90'], dependency_a: ['dependency abbr'], + dependency_source: [null], country: ['country name'], country_id: ['100'], country_a: ['xyz'], + country_source: [null], empire: ['empire name'], empire_id: ['110'], empire_a: ['empire abbr'], + empire_source: [null], continent: ['continent name'], continent_id: ['120'], continent_a: ['continent abbr'], + continent_source: [null], ocean: ['ocean name'], ocean_id: ['130'], ocean_a: ['ocean abbr'], + ocean_source: [null], marinearea: ['marinearea name'], marinearea_id: ['140'], marinearea_a: ['marinearea abbr'], + marinearea_source: [null], }, center_point: { lat: 12.121212, @@ -385,7 +399,8 @@ module.exports.tests.success_conditions = (test, common) => { parent: { neighbourhood: ['neighbourhood name'], neighbourhood_id: ['10'], - neighbourhood_a: [null] + neighbourhood_a: [null], + neighbourhood_source: [null] }, center_point: { lat: 12.121212, @@ -460,7 +475,8 @@ module.exports.tests.success_conditions = (test, common) => { parent: { neighbourhood: ['neighbourhood name'], neighbourhood_id: ['10'], - neighbourhood_a: ['neighbourhood abbr'] + neighbourhood_a: ['neighbourhood abbr'], + neighbourhood_source: [null] }, bounding_box: '{"min_lat":40.006751,"max_lat":40.072939,"min_lon":-76.345902,"max_lon":-76.254038}' } @@ -534,7 +550,8 @@ module.exports.tests.success_conditions = (test, common) => { parent: { neighbourhood: ['neighbourhood name'], neighbourhood_id: ['10'], - neighbourhood_a: ['neighbourhood abbr'] + neighbourhood_a: ['neighbourhood abbr'], + neighbourhood_source: [null] }, center_point: { lat: 12.121212, @@ -615,7 +632,8 @@ module.exports.tests.success_conditions = (test, common) => { parent: { neighbourhood: ['neighbourhood name'], neighbourhood_id: ['10'], - neighbourhood_a: ['neighbourhood abbr'] + neighbourhood_a: ['neighbourhood abbr'], + neighbourhood_source: [null] } } ] @@ -696,7 +714,8 @@ module.exports.tests.success_conditions = (test, common) => { parent: { neighbourhood: ['neighbourhood name'], neighbourhood_id: ['10'], - neighbourhood_a: ['neighbourhood abbr'] + neighbourhood_a: ['neighbourhood abbr'], + neighbourhood_source: [null] } } ] @@ -779,7 +798,8 @@ module.exports.tests.success_conditions = (test, common) => { parent: { neighbourhood: ['neighbourhood name'], neighbourhood_id: ['10'], - neighbourhood_a: ['neighbourhood abbr'] + neighbourhood_a: ['neighbourhood abbr'], + neighbourhood_source: [null] } } ] @@ -989,7 +1009,8 @@ module.exports.tests.failure_conditions = (test, common) => { parent: { neighbourhood: [ 'Example' ], neighbourhood_id: [ '20' ], - neighbourhood_a: [ null ] + neighbourhood_a: [ null ], + neighbourhood_source: [null] }, source: 'whosonfirst', layer: 'neighbourhood', diff --git a/test/unit/controller/placeholder.js b/test/unit/controller/placeholder.js index 352c0119d..b1a6866dd 100644 --- a/test/unit/controller/placeholder.js +++ b/test/unit/controller/placeholder.js @@ -237,33 +237,43 @@ module.exports.tests.success = (test, common) => { neighbourhood: ['neighbourhood name 1', 'neighbourhood name 2'], neighbourhood_id: ['10', '20'], neighbourhood_a: [null, 'neighbourhood abbr 2'], + neighbourhood_source: [null], borough: ['borough name 1', 'borough name 2'], borough_id: ['9', '19'], borough_a: [null, 'borough abbr 2'], + borough_source: [null], locality: ['locality name 1', 'locality name 2'], locality_id: ['8', '18'], locality_a: [null, 'locality abbr 2'], + locality_source: [null], localadmin: ['localadmin name 1', 'localadmin name 2'], localadmin_id: ['7', '17'], localadmin_a: [null, 'localadmin abbr 2'], + localadmin_source: [null], county: ['county name 1', 'county name 2'], county_id: ['6', '16'], county_a: [null, 'county abbr 2'], + county_source: [null], macrocounty: ['macrocounty name 1', 'macrocounty name 2'], macrocounty_id: ['5', '15'], macrocounty_a: [null, 'macrocounty abbr 2'], + macrocounty_source: [null], region: ['region name 1', 'region name 2'], region_id: ['4', '14'], region_a: [null, 'region abbr 2'], + region_source: [null], macroregion: ['macroregion name 1', 'macroregion name 2'], macroregion_id: ['3', '13'], macroregion_a: [null, 'macroregion abbr 2'], + macroregion_source: [null], dependency: ['dependency name 1', 'dependency name 2'], dependency_id: ['2', '12'], dependency_a: [null, 'dependency abbr 2'], + dependency_source: [null], country: ['country name 1', 'country name 2'], country_id: ['1', '11'], - country_a: ['ABC', 'XYZ'] + country_a: ['ABC', 'XYZ'], + country_source: [null] } }, { @@ -1627,7 +1637,8 @@ module.exports.tests.result_filtering = (test, common) => { parent: { country: ['country name 1'], country_id: ['1'], - country_a: ['ABC'] + country_a: ['ABC'], + country_source: [null] } }, { @@ -1648,7 +1659,8 @@ module.exports.tests.result_filtering = (test, common) => { parent: { country: ['country name 4'], country_id: ['4'], - country_a: ['ABC'] + country_a: ['ABC'], + country_source: [null] } } ] @@ -1775,7 +1787,8 @@ module.exports.tests.result_filtering = (test, common) => { parent: { country: ['country name 1', 'country name 2'], country_id: ['1', '2'], - country_a: ['ABC', 'DEF'] + country_a: ['ABC', 'DEF'], + country_source: [null] } }, { @@ -1796,7 +1809,8 @@ module.exports.tests.result_filtering = (test, common) => { parent: { country: ['country name 3'], country_id: ['3'], - country_a: ['ABC'] + country_a: ['ABC'], + country_source: [null] } }, { @@ -1817,7 +1831,8 @@ module.exports.tests.result_filtering = (test, common) => { parent: { country: ['country name 4'], country_id: ['4'], - country_a: ['GHI'] + country_a: ['GHI'], + country_source: [null] } } ] @@ -1895,7 +1910,8 @@ module.exports.tests.lineage_errors = (test, common) => { parent: { country: ['country name 1'], country_id: ['1'], - country_a: ['country abbr 1'] + country_a: ['country abbr 1'], + country_source: [null] } } ] @@ -1968,7 +1984,8 @@ module.exports.tests.lineage_errors = (test, common) => { parent: { country: ['country name 1'], country_id: ['1'], - country_a: ['country abbr 1'] + country_a: ['country abbr 1'], + country_source: [null] } } ] @@ -2040,7 +2057,8 @@ module.exports.tests.lineage_errors = (test, common) => { parent: { country: ['country name 1'], country_id: ['1'], - country_a: ['country abbr 1'] + country_a: ['country abbr 1'], + country_source: [null] } } ] diff --git a/test/unit/middleware/normalizeParentIds.js b/test/unit/middleware/normalizeParentIds.js index be0544973..6d51879f6 100644 --- a/test/unit/middleware/normalizeParentIds.js +++ b/test/unit/middleware/normalizeParentIds.js @@ -78,6 +78,95 @@ module.exports.tests.interface = function(test, common) { }); + test('default source should be whosonfirst', function(t) { + + var input = { + data: [{ + 'parent': { + 'country': ['United States'], // these shouldn't change + 'country_id': ['85633793'], + 'country_a': ['USA'] + }, + 'country': ['United States'], + 'country_gid': ['85633793'], + 'country_source': [ null ], + 'country_a': ['USA'], + 'macroregion': ['MacroRegion Name'], + 'macroregion_gid': ['foobar'], + 'macroregion_a': ['MacroRegion Abbreviation'], + 'macroregion_source': [ undefined ], + 'region': ['New York'], + 'region_gid': ['85688543'], + 'region_a': ['NY'], + 'region_source': null, + 'macrocounty': ['MacroCounty Name'], + 'macrocounty_gid': ['~~~~~'], + 'macrocounty_a': ['MacroCounty Abbreviation'], + 'macrocounty_source': undefined, + 'county': ['Kings County'], + 'county_gid': ['102082361'], + 'county_a': [null], + 'county_source': ['whosonfirst'], + 'localadmin': ['Brooklyn'], + 'localadmin_gid': ['404521211'], + 'localadmin_a': [null], + 'localadmin_source': ['whosonfirst', '~~~~~'], + 'locality': ['Some Locality'], + 'locality_gid': ['85977539'], + 'locality_a': [null], + 'locality_source': [], + 'neighbourhood': [], + 'neighbourhood_gid': [] + }] + }; + + var expected = { + data: [{ + 'parent': { + 'country': ['United States'], + 'country_id': ['85633793'], + 'country_a': ['USA'] + }, + 'country': ['United States'], + 'country_gid': ['whosonfirst:country:85633793'], + 'country_a': ['USA'], + 'country_source': [ null ], + 'macroregion': ['MacroRegion Name'], + 'macroregion_gid': ['whosonfirst:macroregion:foobar'], + 'macroregion_a': ['MacroRegion Abbreviation'], + 'macroregion_source': [ undefined ], + 'region': ['New York'], + 'region_gid': ['whosonfirst:region:85688543'], + 'region_a': ['NY'], + 'region_source': null, + 'macrocounty': ['MacroCounty Name'], + 'macrocounty_gid': ['whosonfirst:macrocounty:~~~~~'], + 'macrocounty_a': ['MacroCounty Abbreviation'], + 'macrocounty_source': undefined, + 'county': ['Kings County'], + 'county_gid': ['whosonfirst:county:102082361'], + 'county_a': [null], + 'county_source': ['whosonfirst'], + 'localadmin': ['Brooklyn'], + 'localadmin_gid': ['whosonfirst:localadmin:404521211'], + 'localadmin_a': [null], + 'localadmin_source': ['whosonfirst', '~~~~~'], + 'locality': ['Some Locality'], + 'locality_gid': ['whosonfirst:locality:85977539'], + 'locality_a': [null], + 'locality_source': [], + 'neighbourhood': [], + 'neighbourhood_gid': [] + }] + }; + + normalizer({}, input, function () { + t.deepEqual(input, expected); + t.end(); + }); + + }); + test('Geonames ids do not override parent hierarchy with WOF equivalents', function(t) { var input = {