pelias / api

HTTP API for Pelias Geocoder
http://pelias.io
MIT License
218 stars 162 forks source link

Allow custom layers by removing the restriction for non-coarse layers #1631

Open mansoor-sajjad opened 1 year ago

mansoor-sajjad commented 1 year ago

:wave: We are working the custom sources and layers which are now supported by the Pelias, which is very good and we are excited about that. We have data from different sources and we have defined the layers which suits our needs and fit nicely with our data.


Here's the reason for this change :rocket:

We are testing the custom sources and layers. It works with the /autocomplete, but it doesn't works with the /reverse endpoint. We have tried to search on this topic and found other issues and discussion regarding this issue. Specially this one https://github.com/pelias/api/issues/1161, which pointed us towards the reverse.js We see that the restriction has been added to support only the non-coarse layers in the /reverse endpoint in the following commit https://github.com/pelias/api/commit/10b1d282010053e4ddece83e6351aa0f9a148a3e. We have not been able to figure out why it was necessary. @trescube If you remember anything about that change from 2017 please help us.


Here's what actually got changed :clap:

We have removed that restriction and make the /reverse endpoint to support all the layers like other endpoints. We have tested the API after that change and it works fine.

mansoor-sajjad commented 1 year ago

@missinglink Can you please review this?

missinglink commented 1 year ago

Hi @mansoor-sajjad, I believe the original intent was to avoid showing a mix of administrative data and non-administrative data in the results, unfortunately there are no screenshots of the behaviour at that time but I can imagine a worst-case scenario of something like this:

# Search for 'Torstraße 10, Berlin, Germany'

1. Torstraße 10, Berlin, Germany
2. Berlin, Germany
3. Germany

This isn't the default behaviour we'd like to see in Pelias, as results 2. and 3. are included in the properties of the first.

I haven't tested your PR but I'm guessing that it would revert to the previous behaviour?

missinglink commented 1 year ago

Just a thought, but instead of doing _.intersection(clean.layers, ['address', 'street', 'venue'])) can't we do _.difference(clean.layers, config.api.targets.layer_aliases.coarse) to establish which layers are non-coarse?

https://github.com/pelias/config/blob/master/config/defaults.json#L63

orangejulius commented 1 year ago

the original intent was to avoid showing a mix of administrative data and non-administrative data in the results

This is my understanding as well. The good news is because reverse geocoding in Elasticsearch operates only on points, only admin records with a centroid within 1km of the coordinate in question would ever be shown. So we probably wouldn't see Germany or even Berlin, but we might see neighbourhoods.

_.difference(clean.layers, config.api.targets.layer_aliases.coarse) is a perfect solution as far as I can tell.

mansoor-sajjad commented 1 year ago

Hi,

Sorry for the late response. Thanks for the review and excellent solution suggestion. @missinglink @orangejulius I have implemented the suggested solution and fixed / restored the unit tests.

orangejulius commented 1 year ago

Hi @mansoor-sajjad, This all looks good to me. Can you please check the "allow edits from maintainers" checkbox on this PR so that I can do a little quick cleanup before we merge?

Please see the docs here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

mansoor-sajjad commented 1 year ago

Hei @orangejulius,

"Allow edits by maintainers" is already checked. ✅