Randomize routes-for-location truncation to match spec - #1290
Randomize routes-for-location truncation to match spec#1290ARCoder181105 wants to merge 3 commits into
Conversation
queryRoutesInBounds asked the SQL layer for maxCount+1 rows ordered by distance and dropped the tail, so truncated responses always kept the nearest routes and always dropped the same ones on repeat queries. The wiki spec calls for a random shuffle of the full match set before truncation when limitExceeded is set, matching legacy behavior and how GetStopsForLocation already handles its BOUNDS mode. Fetch every in-bounds match instead of capping at the SQL layer, then shuffle and truncate in Go, consistent with the existing stop-location shuffle.
TestRoutesForLocationHandlerLimitExceeded pinned the exact pair of routes returned when maxCount truncates the result set, which no longer holds now that truncation is randomized. Replace it with checks that the count, sort order, and membership in the full match set still hold, and add a test that repeats the same request many times to confirm every candidate route can appear in the response.
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesRoute truncation behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 16 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/gtfs/gtfs_manager.go`:
- Around line 500-508: Update the route-set query in the truncation flow around
queryRoutesInBounds to replace MaxCount: noRowLimit with a bounded,
deterministic sampling limit derived from the requested maxCount and geographic
bounds. Ensure the query never materializes every matching route, while
preserving the returned result cap and deterministic sampling behavior.
In `@internal/restapi/routes_for_location_handler_test.go`:
- Around line 173-177: Correct the probability expression in the comment above
routesForLocationShuffleIterations: state that the probability of any specific
route being omitted across all iterations is
(1/3)^routesForLocationShuffleIterations, while preserving the existing
explanation and constant value.
- Around line 179-190: Update
TestRoutesForLocationHandlerLimitExceededIsRandomized to validate each sampled
response before collecting route IDs: assert the successful HTTP status,
LimitExceeded flag, exactly two results, expected route membership, and
randomized ordering. Keep the existing aggregate seen-ID assertion, and cover
every newly introduced response-condition branch in the test.
- Around line 163-169: Update the test around the truncation assertions to fail
immediately when model.Data.List does not contain exactly two routes, using a
fatal length assertion or an equivalent guard before indexing. Keep the existing
subset and ID-order checks unchanged once the length requirement is satisfied.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8436054e-0236-42fc-b766-81a6f7cf92a8
📒 Files selected for processing (2)
internal/gtfs/gtfs_manager.gointernal/restapi/routes_for_location_handler_test.go
queryRoutesInBounds dropped its SQL row limit entirely to let truncation shuffle over the full match set. BoundsFromParams is called here without clamping, so a caller-supplied radius or span can be arbitrarily large; add a generous ceiling as a defensive backstop against a pathological bounding box, well above any real GTFS feed's route count so it never affects the uniform-random selection the spec requires. Also fix the randomized-truncation test: a specific route is dropped with probability 1/3 per call, not 2/3, correcting the comment's flake bound; assert per-iteration on status, LimitExceeded, count, and sort order instead of only aggregating results after the loop, using require so a bad response fails fast instead of index-panicking; and switch the burst of 50 rapid calls to the rate-limit-exempt test key so they don't intermittently 429 against the low test rate limit.
|
3 participants
Summary
Note for reviewer
Wiki spec lists this shuffle as Suspected Defect 1 (deterministic selection suspected to be the intended behavior) but has no Implementation Decisions entry. This PR replicates legacy/spec behavior as requested in the linked issue. Flagging per contributing guidelines — recording a deviation, if desired, is a wiki/reviewer decision.
Test plan
Closes #1229
Summary by CodeRabbit