Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mansoor-sajjad
Copy link

👋
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 🚀

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 #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 10b1d28. 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 👏

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
Copy link
Author

@missinglink Can you please review this?

@missinglink
Copy link
Member

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
Copy link
Member

missinglink commented Sep 15, 2022

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
Copy link
Member

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.

test/unit/query/reverse.js Outdated Show resolved Hide resolved
…ed non-coarse layers, inorder to support the custom layers for reverse endpoint.
@mansoor-sajjad
Copy link
Author

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.

query/reverse.js Outdated Show resolved Hide resolved
@mansoor-sajjad mansoor-sajjad requested review from missinglink and orangejulius and removed request for orangejulius and missinglink September 30, 2022 07:55
Copy link
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and simple, happy to merge this.

@orangejulius you'll need to remove your block before we can proceed.

@orangejulius
Copy link
Member

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
Copy link
Author

Hei @orangejulius,

"Allow edits by maintainers" is already checked. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants