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

clamp boundary.circle.radius for reverse queries #1618

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

missinglink
Copy link
Member

@missinglink missinglink commented Mar 22, 2022

I noticed that the boundary.circle.radius param is currently unbounded for /v1/reverse.
If a users sets this value very high it can result in a lot of disk I/O as ES pages in the docvalues to score all the results.

This PR defines minimum and maximum values for this radius value, I've selected 5km as the max and an arbitrarily low value of 0.00001km for the min; although I'm open to changing either of these.

@@ -1,10 +1,9 @@
var geo_common = require ('./_geo_common');
var _ = require('lodash');
var defaults = require('../query/reverse_defaults');
Copy link
Member Author

Choose a reason for hiding this comment

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

this was unused

var LAT_LON_IS_REQUIRED = true,
CIRCLE_IS_REQUIRED = false;

const non_coarse_layers = ['venue', 'address', 'street'];
Copy link
Member Author

Choose a reason for hiding this comment

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

this was unused

Copy link
Member

Choose a reason for hiding this comment

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

Huh, I wonder when we last used it... We have long needed to improve reverse geocoding support for custom layers, as mentioned in #1161. Not something to solve in this PR though.

const raw = {
'point.lat': '12.121212',
'point.lon': '21.212121',
'boundary.circle.radius': '1km' // note the 'km'
Copy link
Member Author

Choose a reason for hiding this comment

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

this is weird, I'd like to change it, but in another PR.
right now you can specify 5km or 5m and in both cases the unit designation is stripped and results in 5, which is km.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah that is unfortunate. Very Javascript-y 😆

@missinglink missinglink force-pushed the clamp-boundary-circle-reverse branch from b0ddb04 to 44ea887 Compare March 22, 2022 14:51
Copy link
Member

@orangejulius orangejulius left a comment

Choose a reason for hiding this comment

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

Looks good. I have a suggestion on the tests but it's very much optional. :)

Comment on lines 136 to 138
t.equals(raw['boundary.circle.lat'], 12.121212);
t.equals(raw['boundary.circle.lon'], 21.212121);
t.equals(raw['boundary.circle.radius'], '0.000000000000001');
Copy link
Member

@orangejulius orangejulius Mar 22, 2022

Choose a reason for hiding this comment

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

I think you can skip testing the raw values, since you specified them above.

IMO it just makes the test longer and it took me a second to realize it was checking the raw value, not clean. At first glance it looked to me like the functionality is not working and the radius is not being clamped!

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah good point, I just copied the existing format, it also caught me out writing the tests.
I guess the purpose is to show that the $raw values are not mutated, I can have a single test for that and clean up th other ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm actually no, that's not right, because if so, then the raw values would all be strings 🤷‍♂️
Anyway, I just cleaned up the tests by removing the assertions for $raw

const raw = {
'point.lat': '12.121212',
'point.lon': '21.212121',
'boundary.circle.radius': '1km' // note the 'km'
Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah that is unfortunate. Very Javascript-y 😆

var LAT_LON_IS_REQUIRED = true,
CIRCLE_IS_REQUIRED = false;

const non_coarse_layers = ['venue', 'address', 'street'];
Copy link
Member

Choose a reason for hiding this comment

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

Huh, I wonder when we last used it... We have long needed to improve reverse geocoding support for custom layers, as mentioned in #1161. Not something to solve in this PR though.

@missinglink missinglink force-pushed the clamp-boundary-circle-reverse branch from 44ea887 to 9f741b1 Compare March 22, 2022 15:21
@missinglink missinglink merged commit 04617ba into master Mar 22, 2022
@missinglink missinglink deleted the clamp-boundary-circle-reverse branch March 22, 2022 15:24
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.

2 participants