Fix stop-code queries in stops-for-location - #1270
Conversation
Routes for a matched stop were resolved through the active-service query used by geospatial searches, and stops with nothing scheduled that day were then dropped. A stop-code lookup therefore returned nothing outside the stop's service window, while legacy resolves the stop regardless of the queried date. Split the route lookup by search mode: stop-code queries report every route serving the stop, geospatial searches keep reporting only routes active on the queried date, and only geospatial searches drop stops that have none. Fixes OneBusAway#1269
Stop-code candidates were drawn only from stops already inside the search bounds, so a code that fell outside them returned nothing, where legacy falls back to the nearest matching stop. The bounds also used the narrow default radius rather than the wider one stop-code queries expect. Candidates now come from the whole feed, are filtered to the search bounds, and fall back to the nearest candidate when none qualify. That fallback needs a limit, so bounds which miss every agency's coverage area now short-circuit to an empty list before any search runs, rather than returning the nearest match worldwide. The combined-ID sort fixture moves inside the coverage area for the same reason. Fixes OneBusAway#1239
|
Warning Review limit reached
Next review available in: 42 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 (6)
📝 WalkthroughWalkthroughAdds prepared stop-code lookup support, closest-stop fallback, and query-mode route aggregation without active-date filtering. The REST handler now preserves eligible stops without active service and skips searches outside agency coverage. ChangesStop-code lookup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@gtfsdb/query.sql`:
- Around line 315-337: Add an index for the stops.code column through the
appropriate schema change or SQL migration, ensuring the GetStopsByCode lookup
is indexed. Then run make models to regenerate the model artifacts and
regenerate gtfsdb/query.sql.go so the repository reflects the schema update.
- Around line 315-337: The GetStopsByCode query should return the complete
candidate set for stopsMatchingCode instead of limiting results to 10 before
bounds and radius filtering. Confirm duplicate stop codes are possible across
merged feeds and remove the LIMIT 10 accordingly; add an explicit deterministic
ORDER BY using stable stop fields, preferably id as the final tie-breaker.
In `@internal/gtfs/gtfs_manager.go`:
- Around line 391-428: Update stopsMatchingCode to sort the in-bounds within
candidates by distance to loc before applying the maxCount truncation, following
the existing GetStopsForLocation ORDERED_BY_CLOSEST ordering pattern. Preserve
the current fallback and return behavior, but ensure within[:maxCount] contains
the nearest stops.
In `@internal/restapi/stops_for_location_handler_test.go`:
- Around line 360-384: Add a test covering the stopsMatchingCode branch where
more than maxCount matching stops are within the search bounds. Seed multiple
stops with the same code, invoke the stops-for-location query so the in-bounds
matches exceed maxCount, and assert the response contains exactly the truncated
count and reports limitExceeded as true, following existing response-test
conventions.
In `@internal/restapi/stops_for_location_handler.go`:
- Around line 96-104: Reuse the existing outOfRange value from
CheckIfOutOfBounds(loc) at both response-building sites around the handlers’
later branches, replacing the repeated CheckIfOutOfBounds(loc) calls while
preserving the current response behavior.
🪄 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: 63f70173-6860-4214-ad8f-1bf669b38bdf
📒 Files selected for processing (6)
gtfsdb/db.gogtfsdb/query.sqlgtfsdb/query.sql.gointernal/gtfs/gtfs_manager.gointernal/restapi/stops_for_location_handler.gointernal/restapi/stops_for_location_handler_test.go
The candidate query had no ORDER BY, so which rows survived the ten-row cap depended on storage order, and stops.code had no index to serve the lookup. Order candidates by id and index the column. Sort in-bounds matches by distance before truncating to maxCount so the nearest ones are kept rather than an arbitrary subset, matching how the route-type path already orders its results. Cover the truncation branch, which merged feeds can reach by repeating a stop code.
|
Fixes #1269 and #1239 (sub-issues of the #1235 spec review).
These two are in one PR because they can't be reviewed independently: the closest-match fallback from #1239 is dead code while query mode still drops stops that have no service on the queried date (#1269). One commit each.
Return all routes for stop-code queries (#1269)
Routes were resolved through the active-service query for every search mode, and stops with nothing scheduled that day were dropped — so a stop-code lookup returned nothing outside the stop's service window. The existing test documents the workaround this forced:
Per the spec (Main Success Scenario step 6, Suspected Defect #2), stop-code queries report every route serving the stop regardless of date. Geospatial searches are unchanged.
Search stop codes feed-wide with a closest-match fallback (#1239)
Candidates were taken only from stops already inside the search bounds, so there was no fallback, and the bounds used the narrow default radius. Candidates now come from the whole feed (
GetStopsByCode, capped at 10 as the spec describes), are filtered to the bounds, and fall back to the nearest candidate when none qualify.Three things worth a reviewer's eye
Out-of-range short-circuit is scope beyond both issues. An unbounded fallback returns the nearest match worldwide —
lat=0&lon=0&query=2042returned a Redding stop. Bounds that miss every agency's coverage area now return an empty list before any search runs, which is Extension 3a and what the existingTestStopsForLocationQueryOutOfAreawas already asserting.Truncation is deterministic, not shuffled. The spec says legacy shuffles before truncating in query mode. DC-6 records that shuffle as a defect on
routes-for-location, so I didn't reproduce it here — happy to switch if you'd rather match legacy exactly.GetStopsByCodekeeps maglev's exact-match semantics. The spec describes legacy searching a stop-code index; changing match semantics felt like a separate gap rather than part of these two.Also note the combined-ID sort fixture from #1244 moves inside the RABA coverage area — the out-of-range short-circuit would otherwise empty its result. Same assertions, different coordinates.
Tests
TestStopsForLocationQueryIgnoresActiveService(lookup on a day the stop has no service) andTestStopsForLocationQueryFallsBackToClosestMatch(radius that excludes the match). Both were confirmed to fail against the pre-fix implementation.Verified: full suite in both
sqlite_fts5andpuregomodes,go vetboth, race detector onrestapi/gtfs,gofmt.Summary by CodeRabbit
New Features
Bug Fixes