Skip to content

Commit

Permalink
fix(addParent): support explicitly passing null abbreviation
Browse files Browse the repository at this point in the history
For some time, we have [used null values](11bd4f6) to ensure Elasticsearch properly handles the optional abbreviation value in parent records.

As a result, we have been very strict in what we allow for values of the
abbreviation field. Currently, the Javascript value `undefined` is
converted to `null` (Elasticsearch has `null` but not `undefined`), and
passing `null` in is forbidden.

However, this means that if we wish to take a value that has been
already added with `addParent` and "re-add" it (perhaps after some
processing, as in pelias/wof-admin-lookup#232), the model
code will throw a validation error.

This PR changes `addParent` to accept any value with `typeof` `string`.
All others are stored as `null`s. After testing of importing around 5
million OSM records from around the world, no numeric abbreviation
values was found, so this should cover all cases well.

As it relates to pelias/wof-admin-lookup#232,
this fixes a problem where many many otherwise valid records will be
skipped with postal cities admin lookup is enabled.
  • Loading branch information
orangejulius authored and missinglink committed Sep 19, 2018
1 parent 4d91814 commit e701112
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 3 deletions.
6 changes: 3 additions & 3 deletions Document.js
Original file line number Diff line number Diff line change
Expand Up @@ -334,10 +334,10 @@ Document.prototype.addParent = function( field, name, id, abbr ){
== you can now be sure that the abbreviation 'bingo' belongs to '2' and not '1'.
**/
if (_.isUndefined(abbr)) {
add( field + '_a', null );
} else {
if (typeof abbr === 'string') {
addValidate( field + '_a', abbr );
} else {
add( field + '_a', null );
}

// chainable
Expand Down
23 changes: 23 additions & 0 deletions test/document/parent.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,29 @@ module.exports.tests.addParent = function(test) {
t.equal(doc.parent.country_a[0], null, 'adder works');
t.end();
});

test('addParent - pass null for abbr', function(t) {
var doc = new Document('mysource','mylayer','myid');

doc.addParent('country','liberland', 'liber_id', null);

t.equal(doc.parent.country[0], 'liberland', 'adder works');
t.equal(doc.parent.country_id[0], 'liber_id', 'adder works');
t.equal(doc.parent.country_a[0], null, 'adder works');
t.end();
});

test('addParent - treat numbers as null for abbr', function(t) {
var doc = new Document('mysource','mylayer','myid');

doc.addParent('country','liberland', 'liber_id', 100);

t.equal(doc.parent.country[0], 'liberland', 'adder works');
t.equal(doc.parent.country_id[0], 'liber_id', 'adder works');
t.equal(doc.parent.country_a[0], null, 'adder works');
t.end();
});

test('addParent - validate', function(t) {
var doc = new Document('mysource','mylayer','myid');
t.throws( doc.addParent.bind(doc, 1), null, 'invalid type' );
Expand Down

0 comments on commit e701112

Please sign in to comment.