Honor latSpan and lonSpan in routes-for-location - #1275
Open
ARCoder181105 wants to merge 2 commits into
Open
Conversation
The handler assigned a default radius to LocationParams whenever no radius was supplied, before passing the params to BoundsFromParams. That helper sizes the box from latSpan/lonSpan only when no radius is set, so the pre-filled default meant the span branch was never reached and any latSpan/lonSpan a client sent was silently discarded. Apply the default radius only when neither a radius nor a usable pair of spans was supplied. The predicate mirrors the one in BoundsFromParams: both spans must be positive, since a single span leaves the box unsized and must still fall back to the query-aware default radius.
The previous span test requested a 0.01 degree box, which at the fixture latitude is close enough to the default radius box that both returned the same single route. It passed whether or not the spans were honored, which is why the gap went unnoticed. Replace it with a table covering the three sizing rules from the spec: spans widening and narrowing the box relative to the default radius, and radius taking precedence when both are supplied. Add a case for a lone latSpan, which must still fall back to the 10km query radius rather than the 600m one. Each case is built to return a different result than the default box, so it fails if the spans are ignored. A fresh API per subtest avoids the shared key's rate limit, and results are filtered to the RABA agency because other tests leave synthetic agencies in the shared test database.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe routes-for-location handler now sanitizes queries and preserves provided positive latitude and longitude spans instead of applying a default radius. Table-driven tests cover bounding-box sizing, radius precedence, and query-aware fallback behavior. ChangesLocation bounds
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1226
routes-for-locationacceptedlatSpan/lonSpan, then ignored them , a 10km × 10km box request got a 600m box.BoundsFromParamssizes the box from the spans only when no radius is set. The handler assigned the default radius before calling it, so the span branch was unreachable.Change
Apply the default radius only when neither a radius nor a usable pair of spans was supplied.
Guard is
Radius == 0 && !(LatSpan > 0 && LonSpan > 0), not theLatSpan == 0 && LonSpan == 0the issue specifies. With one span plus aquery, the literal version leavesRadiusat 0 andBoundsFromParamsfalls back to 600m instead of the specced 10km — its fallback isn't query-aware.Tests
Replaced
TestRoutesForLocationLatSpanAndLonSpanwith a table-drivenTestRoutesForLocationBoundingBoxSizing. The old test used a 0.01° box, near-identical to the default-radius box at the fixture latitude, so it passed whether or not spans were honoured.Four cases, each returning a different result than the default box: spans widening the box, spans narrowing it, radius taking precedence over spans, and a lone
latSpanfalling back to the 10km query radius.Summary by CodeRabbit
Bug Fixes
Tests