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.

Sign up for GitHub

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
Merged

clamp boundary.circle.radius for reverse queries #1618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions sanitizer/_geo_reverse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 _ = require('lodash');
const geo_common = require ('./_geo_common');
const LAT_LON_IS_REQUIRED = true;
const CIRCLE_IS_REQUIRED = false;
const CIRCLE_MIN_RADIUS = 0.00001;
const CIRCLE_MAX_RADIUS = 5.0;

// validate inputs, convert types and apply defaults
function _sanitize( raw, clean ){
Expand Down Expand Up @@ -32,6 +31,15 @@ function _sanitize( raw, clean ){
// santize the boundary.circle
geo_common.sanitize_circle( 'boundary.circle', clean, raw, CIRCLE_IS_REQUIRED );

// clamp the 'boundary.circle.radius' param to an absolute value range.
// note: large radius values have a severe performance impact.
if (_.isNumber(clean['boundary.circle.radius']) ){
clean['boundary.circle.radius'] = _.clamp(
clean['boundary.circle.radius'],
CIRCLE_MIN_RADIUS, CIRCLE_MAX_RADIUS
);
}

}
catch (err) {
messages.errors.push( err.message );
Expand Down
59 changes: 54 additions & 5 deletions test/unit/sanitizer/_geo_reverse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,27 +61,76 @@ module.exports.tests.warning_situations = (test, common) => {
};

module.exports.tests.success_conditions = (test, common) => {
// note: this behaviour is historic and should probably be removed.
// it's possible to add non-numeric tokens to the boundary.circle.radius
// value and they are simply stripped out by parseFloat() with warning.
test('boundary.circle.radius must be a positive number', (t) => {
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 😆

};
const clean = {};
const errorsAndWarnings = sanitizer.sanitize(raw, clean);

t.equals(clean['boundary.circle.lat'], 12.121212);
t.equals(clean['boundary.circle.lon'], 21.212121);
t.equals(clean['boundary.circle.radius'], 1.0);

t.deepEquals(errorsAndWarnings, { errors: [], warnings: [] });
t.end();
});

test('boundary.circle.radius specified in request should override default', (t) => {
const raw = {
'point.lat': '12.121212',
'point.lon': '21.212121',
'boundary.circle.radius': '3248732857km' // this will never be the default
'boundary.circle.radius': '2'
};
const clean = {};
const errorsAndWarnings = sanitizer.sanitize(raw, clean);

t.equals(raw['boundary.circle.lat'], 12.121212);
t.equals(raw['boundary.circle.lon'], 21.212121);
t.equals(raw['boundary.circle.radius'], '3248732857km');
t.equals(clean['boundary.circle.lat'], 12.121212);
t.equals(clean['boundary.circle.lon'], 21.212121);
t.equals(clean['boundary.circle.radius'], 3248732857.0);
t.equals(clean['boundary.circle.radius'], 2.0);

t.deepEquals(errorsAndWarnings, { errors: [], warnings: [] });
t.end();
});

test('boundary.circle.radius specified should be clamped to MAX', (t) => {
const raw = {
'point.lat': '12.121212',
'point.lon': '21.212121',
'boundary.circle.radius': '10000'
};
const clean = {};
const errorsAndWarnings = sanitizer.sanitize(raw, clean);

t.equals(clean['boundary.circle.lat'], 12.121212);
t.equals(clean['boundary.circle.lon'], 21.212121);
t.equals(clean['boundary.circle.radius'], 5.0); // CIRCLE_MAX_RADIUS

t.deepEquals(errorsAndWarnings, { errors: [], warnings: [] });
t.end();
});

test('boundary.circle.radius specified should be clamped to MIN', (t) => {
const raw = {
'point.lat': '12.121212',
'point.lon': '21.212121',
'boundary.circle.radius': '0.000000000000001'
};
const clean = {};
const errorsAndWarnings = sanitizer.sanitize(raw, clean);

t.equals(clean['boundary.circle.lat'], 12.121212);
t.equals(clean['boundary.circle.lon'], 21.212121);
t.equals(clean['boundary.circle.radius'], 0.00001); // CIRCLE_MIN_RADIUS

t.deepEquals(errorsAndWarnings, { errors: [], warnings: [] });
t.end();
});
};

module.exports.all = (tape, common) => {
Expand Down