Skip to content

Commit

Permalink
Merge pull request #1513 from pelias/joxit/fix/normalizeParentIds
Browse files Browse the repository at this point in the history 
normalizeParentIds: cannot read property toLowerCase of null when source is not set
  • Loading branch information
@orangejulius
orangejulius authored Feb 23, 2021
2 parents 44fed98 + bae534d commit de97774
Show file tree
Hide file tree
Showing 5 changed files with 151 additions and 24 deletions.
11 changes: 5 additions & 6 deletions middleware/normalizeParentIds.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) ];
}
});
}
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
35 changes: 28 additions & 7 deletions test/unit/controller/coarse_reverse.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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}'
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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]
}
}
]
Expand Down Expand Up @@ -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]
}
}
]
Expand Down Expand Up @@ -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]
}
}
]
Expand Down Expand Up @@ -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',
Expand Down
36 changes: 27 additions & 9 deletions test/unit/controller/placeholder.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
},
{
Expand Down Expand Up @@ -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]
}
},
{
Expand All @@ -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]
}
}
]
Expand Down Expand Up @@ -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]
}
},
{
Expand All @@ -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]
}
},
{
Expand All @@ -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]
}
}
]
Expand Down Expand Up @@ -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]
}
}
]
Expand Down Expand Up @@ -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]
}
}
]
Expand Down Expand Up @@ -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]
}
}
]
Expand Down
89 changes: 89 additions & 0 deletions test/unit/middleware/normalizeParentIds.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down

0 comments on commit de97774

Please sign in to comment.