Support custom source for parents

[Joxit]

Background

At the time of this PR, when we build the hierarchy, we use pelias/pip-service which is an In Memory Point-In-Polygon lookup service based on WOF data. That means, when we have something in the hierarchy, it's necessarily from WOF. As a result, in the API, when we retrieve data from PIP Service, the source WOF is hard-codded.

The future of PIP is pelias/spatial which supports a plethora of sources. I start the work for a smooth integration from the beginning.

What's new ?

Since spatial is not yet used, the source will always be null and the code is 100% backward compatible. ~~I changed the fourth parameter to an object because the linter says : Document.js: line 319, col 40, This function has too many parameters. (5) for a new parameter. If the fourth parameter is a string, that means that it's an abbreviation, and if it's an object, it should contains abbr and/or source.~~ I added a fifth parameter for the source.

[missinglink]

It's possible to change this to 5 if you like? I. think it was picked fairly arbitrarily 😆 https://github.com/pelias/model/blob/master/.jshintrc#L19

[Joxit]

Ha ha okay thx, I updated to 6 :man_shrugging:

[missinglink]

Okay so schema is merged, I'm not sure what to do about this breaking change... I think we're going to need to mark this as a breaking change and then mark all the importers as a breaking change too?

@orangejulius what's the safest way to release this?

[orangejulius]

Yes that's right, this should be marked as a breaking change and likewise we should do major version bumps on all the importers when we merge the dependency update. Since we merged the schema PR already, we can be extra sure that the commit messages and therefore release notes mention the required schema version to minimize confusion.

We've also found that waiting a few weeks after the schema change is usually enough to prevent too many people from having problems.

[missinglink]

I guess one other option is to have a post processing step which checks if these fields are just filled with null values, and if so, then delete the property completely so it never gets sent to ES.

That would mean it remains backwards compatible as long as the importers don't ever pass a 5th argument to addParent()

And realistically that's not going to happen until we do a general release of Spatial for PIP, I'd hope by then enough time has passed.

[orangejulius]

Yeah actually that sounds a lot simpler. It's still a breaking change but since none of the importers (except maybe CSV) will use it in the near future, it probably won't impact anyone.

[missinglink]

Also worth noting that if someone is using a modern version of pelias/model on a schema version prior to today then it should fail loudly and early, which is how we like it 😆

The error message might not be super user friendly though...

[orangejulius]

Hmm yeah. Random errors that are thrown by Elasticsearch when an import attempts to write to a missing field will probably not be user friendly for people running the importers.

What would be amazing but probably a lot more work is if the Pelias Model code made a single call to get the Elasticsearch mapping when it first started up, and then used that to validate required fields before writing any documents.

Might be worth implementing in the long run though (as a separate PR).

[Joxit]

So, I should remove the else clause here for backward compatibility ?

https://github.com/pelias/model/blob/8aef69949e3808d8f9f16865b8b477d9ec802f5f/Document.js#L375-L379

[missinglink]

Let's hold off for now, I'm hoping we can just merge it as-is.

After another couple weeks the only developers who are going to get affected by the breaking change are those trying to reimport using a new importer version on an old schema version, which I'm assuming to be fairly rare.

[missinglink]

There is another potentially breaking change we might consider merging at the same time as this https://github.com/pelias/model/pull/134

[missinglink]

Happy 2021, @orangejulius @Joxit what's the plan for getting this out the door, suggestions?

[Joxit]

Happy 2021 :smile: pelias/schema#459 was merged in August, so breaking change here and feature change in importers ?

[Joxit]

rebased to master

[missinglink]

please mark it as a BREAKING CHANGE and we can merge it https://github.com/semantic-release/semantic-release#commit-message-format

[Joxit]

Marked as BREAKING CHANGE :+1:

[missinglink]

🥳

[orangejulius]

Very nice and gotta say the commit translated to release notes perfectly, with a link to the relevant schema PR and everything (https://github.com/pelias/model/releases/tag/v9.0.0). This may not sound impressive, but we often get it wrong too, so ... 👍

[Joxit]

thanks :blush: