Cap route-type searches after the service filter - #1272
Conversation
Route-type searches capped the candidate list before stops with no service on the queried date were dropped, so the response could fall far below maxCount while limitExceeded still claimed results were withheld. On the RABA fixture, maxCount=2 returned nothing at all where 122 stops matched, and maxCount=5 returned one. Move that cap downstream of the service filter so the list fills to maxCount and limitExceeded reports a real overflow. Bounds searches keep capping upstream, which is the documented legacy ordering. Fixes OneBusAway#1268
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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)
📝 WalkthroughWalkthroughThe route-type stop search now applies ChangesRoute-type result capping
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/restapi/stops_for_location_handler_test.go`:
- Around line 151-175: Update
TestStopsForLocationRouteTypeCapsAfterServiceFilter to assert the returned stop
IDs, or their set, for each capped maxCount case in addition to length and
LimitExceeded. Verify the IDs represent the nearest route-type matches selected
before combined-ID sorting, while preserving the existing response-status and
count assertions.
In `@internal/restapi/stops_for_location_handler.go`:
- Around line 230-236: After the route-type truncation in the stops-for-location
handler, rebuild agencyIDs and routeIDs from the retained resultRawStopIDs so
references.agencies and references.routes only include IDs belonging to returned
stops. Keep the existing result and limit-exceeded behavior unchanged, and
perform this before constructing or returning the response.
🪄 Autofix (Beta)
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: 6cbeb49e-b9bf-485b-b1db-cb0803bbde0f
📒 Files selected for processing (3)
internal/gtfs/gtfs_manager.gointernal/restapi/stops_for_location_handler.gointernal/restapi/stops_for_location_handler_test.go
There was a problem hiding this comment.
Pull request overview
Moves stops-for-location route-type result truncation to occur after the handler’s active-service filter so maxCount and limitExceeded reflect the final (post-filter) stop list, matching the intended endpoint behavior.
Changes:
- Shift route-type truncation from
GtfsManager.GetStopsForLocationinto the REST handler after inactive stops are dropped. - Preserve bounds-search behavior (still capped upstream).
- Add regression tests covering capped and under-cap route-type searches.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/restapi/stops_for_location_handler.go | Applies route-type truncation after service-date filtering (downstream cap). |
| internal/restapi/stops_for_location_handler_test.go | Adds regression coverage for route-type cap ordering and under-cap behavior. |
| internal/gtfs/gtfs_manager.go | Removes the upstream cap for route-type mode to defer truncation downstream. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(routeTypes) > 0 && len(results) > maxCount { | ||
| results = results[:maxCount] | ||
| resultRawStopIDs = resultRawStopIDs[:maxCount] | ||
| isLimitExceeded = true | ||
| } |
Agency and route references were collected from every candidate stop, so capping the list left references describing stops the caller never saw. With routeType=3 and maxCount=2 the response carried twelve routes for two stops that use one between them. Collect them from the returned stops instead, after the cap is applied. Assert the surviving stop IDs as well as the count, so the test covers which stops the cap keeps rather than only how many.
|
Fixes #1268 (sub-issue of the #1235 spec review).
Route-type searches capped the candidate list in
GetStopsForLocation, but stops with no service on the queried date are dropped afterwards in the handler. The cap therefore ran against a set that was still about to shrink, and the response could fall far belowmaxCountwhilelimitExceededclaimed results had been withheld.Measured on the RABA fixture (clock 2025-12-26,
lat=40.583321&lon=-122.426966&radius=5000&routeType=3):maxCountlist=0,limitExceeded=truelist=2,limitExceeded=truelist=1,limitExceeded=truelist=5,limitExceeded=truelist=122,limitExceeded=falseAsking for two stops returned none, out of 122 matches.
This is the ordering issue recorded as Suspected Defect #4 ("the limit check in the geospatial path runs before route filtering… a clean reimplementation would not have this ordering issue"), and the result contradicts DC-6's intent that the list is capped at
maxCountwithlimitExceededsignalling discarded results.Approach
The cap moves downstream of the service filter, into the handler, where the surviving stops are known. Truncation still happens before the combined-ID sort, so the nearest matches are the ones kept.
Bounds searches (no
routeType) are untouched — they keep capping upstream, which is the documented legacy ordering from Main Success Scenario step 5, not a defect.Note
The candidate set handed to the batch queries is now the full route-type match within the search bounds rather than a pre-capped slice, since which stops survive the service filter isn't knowable before running it. That's the same shape legacy has.
Tests cover both cap sizes and the under-cap case; the capped cases were confirmed to fail against the previous ordering.
Verified: full suite in both
sqlite_fts5andpuregomodes,go vetboth, race detector onrestapi/gtfs,gofmt.Summary by CodeRabbit
Bug Fixes
Tests