feat: Implement arrivals-and-departures-for-location endpoint#787
feat: Implement arrivals-and-departures-for-location endpoint#787ARCoder181105 wants to merge 17 commits into
Conversation
ARCoder181105
changed the title
arrivals-and-departures-for-location endpoint
ARCoder181105
changed the title
arrivals-and-departures-for-location endpoint
|
Hi @aaronbrethorst, I've ported the A few technical notes and questions for your review:
Finally, I know some of my inline code comments might be a bit vague. I wrote them based on my current understanding of the pipeline, but I would really appreciate your suggestions on how I can improve the documentation for future maintainers. Let me know what you think! |
|
Also I know the commits are also a bit vague once these changes approves I will squash and rebase the branch into single atomic commit... |
|
Note: |
Port Java arrivals-and-departures-for-location to Go maglev architecture. - Geospatial stop lookup via in-memory R-tree index - 3-day window queries (yesterday/today/tomorrow) for overnight trip support - Batch-fetch routes/trips/stop-times to avoid N+1 queries - Deduplicated references block (Agencies, Routes, Stops, Trips, Situations) - nearbyStopIds via haversine distance, excluding stops already in stopIds - arrivalStatus derived from schedule deviation (LATE/EARLY/ON_TIME/default) - limitExceeded flag honoring maxCount (mirrors Java MaxCountSupport) - Add StopWithDistance model and NewArrivalsAndDeparturesForLocationResponse - Add ORDER BY to GetAgenciesForStops for deterministic results - Fix nil vehicle passed to BuildTripStatus in arrivals_and_departure_for_stop.go - Register GET /api/where/arrivals-and-departures-for-location.json - Add unit, E2E, and context cancellation tests Closes OneBusAway#787 Fixes OneBusAway#799
ARCoder181105
force-pushed
the
feature/arrivals-departures-for-location
branch
from
e3c9b4d to
b056442
Compare
|
Hey @ARCoder181105 Great work on this endpoint! |
|
Hi @3rabiii, thanks for the review! I have implemented the missing parameters you mentioned ( I also addressed a few other minor parity gaps (added Everything has been tested and verified against the live PugetSound schema. Ready for another look whenever you have time! |
|
Hi @aaronbrethorst , @Ahmedhossamdev The PR is ready for review ... |
|
Hi @ARCoder181105 |
|
@Ahmedhossamdev I already updated it here #807 |
Port Java arrivals-and-departures-for-location to Go maglev architecture. - Geospatial stop lookup via in-memory R-tree index - 3-day window queries (yesterday/today/tomorrow) for overnight trip support - Batch-fetch routes/trips/stop-times to avoid N+1 queries - Deduplicated references block (Agencies, Routes, Stops, Trips, Situations) - nearbyStopIds via haversine distance, excluding stops already in stopIds - arrivalStatus derived from schedule deviation (LATE/EARLY/ON_TIME/default) - limitExceeded flag honoring maxCount (mirrors Java MaxCountSupport) - Add StopWithDistance model and NewArrivalsAndDeparturesForLocationResponse - Add ORDER BY to GetAgenciesForStops for deterministic results - Fix nil vehicle passed to BuildTripStatus in arrivals_and_departure_for_stop.go - Register GET /api/where/arrivals-and-departures-for-location.json - Add unit, E2E, and context cancellation tests Closes OneBusAway#787 Fixes OneBusAway#799
…dpoint - Add parsing and handling for emptyReturnsNotFound parameter - Add parsing and GTFS filtering for routeType parameter - Add frequencyMinutesBefore and frequencyMinutesAfter parsing - Fix nearbyStopIds logic to match Java's includeInputIdsInNearby=true override
…stopSequence offset in arrivals-and-departures-for-location
ARCoder181105
force-pushed
the
feature/arrivals-departures-for-location
branch
from
e4feabe to
4c74707
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new GET /api/where/arrivals-and-departures-for-location.json endpoint (params, stop resolution, 3-day stop_times aggregation, GTFS-RT merging, response builder), adds StopWithDistance and response constructor, changes ArrivalAndDeparture JSON tag, normalizes situation IDs, wires vehicle into trip status, adds tests, and makes small GTFS SQL and helper fixes. ChangesArrivals-and-departures-for-location feature
GTFS / DB and helper fixes (separate concerns)
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as RestAPI Handler
participant Resolver as Stop Resolver
participant DB as GTFS DB
participant RT as GTFS-RT Service
participant Builder as Response Builder
Client->>API: GET /arrivals-and-departures-for-location.json (lat,lon,radius,...)
API->>Resolver: validate params, compute bbox/radius
Resolver->>DB: query stops within bbox/radius
DB-->>Resolver: stop list
API->>DB: fetch active stop_times/trips/routes (3-day window)
DB-->>API: scheduled stop_times + references
API->>RT: fetch vehicle positions & alerts for relevant trips
RT-->>API: vehicles, alerts
API->>Builder: compute predicted/scheduled times, statuses, distances, nearby stops
Builder-->>API: arrivalsAndDepartures + references
API-->>Client: 200 OK with JSON envelope
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🧹 Nitpick comments (2)
internal/restapi/context_cancellation_test.go (1)
84-95:429as an allowed outcome may dilute cancellation-signal quality.Because all sub-tests reuse the same API/rate limiter, a rate-limit hit can pass this assertion without validating context-cancel handling. Consider isolating limiter state per sub-test (fresh API/mux) for this suite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/restapi/context_cancellation_test.go` around lines 84 - 95, The test currently allows HTTP 429 as an acceptable response which can mask failures in context-cancellation handling because all sub-tests share the same rate limiter; update the test to initialize a fresh API/mux (and its rate limiter) for each sub-test so limiter state is isolated, and remove http.StatusTooManyRequests from the accepted status list in the assertion; specifically, change the sub-test setup to create a new server/handler (new API/mux/rate limiter instance) per t.Run and assert only 200, 401, 400, 404, 408, 500, or 504 (no 429).internal/models/response.go (1)
80-90: Minor: Consider usingmap[string]anyfor consistency.The rest of this file uses
map[string]any(lines 22, 31, 41, 55), but this function usesmap[string]interface{}. While functionally equivalent since Go 1.18, usinganywould maintain consistency.♻️ Optional consistency fix
- entryData := map[string]interface{}{ + entryData := map[string]any{ "arrivalsAndDepartures": arrivalsAndDepartures, "limitExceeded": limitExceeded, "nearbyStopIds": nearbyStopIds, "situationIds": situationIds, "stopIds": stopIds, } - data := map[string]interface{}{ + data := map[string]any{ "entry": entryData, "references": references, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/models/response.go` around lines 80 - 90, The maps entryData and data are declared with map[string]interface{} but the rest of the file uses map[string]any; update the declarations for both entryData and data to use map[string]any (keeping the same keys/values) to maintain type-consistency with other maps in this file (look for symbols entryData and data in response.go and replace interface{} with any).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/restapi/arrivals_and_departures_for_location.go`:
- Around line 686-694: Replace direct accesses to nullable fields
trip.TripHeadsign.String and trip.DirectionID.Int64 with explicit .Valid checks:
compute a headsign variable (e.g., headsign := "" ; if trip.TripHeadsign.Valid {
headsign = trip.TripHeadsign.String }) and a direction string (e.g., dir := "" ;
if trip.DirectionID.Valid { dir = strconv.FormatInt(trip.DirectionID.Int64, 10)
}) and pass those into utils.FormCombinedID/strconv.FormatInt call sites instead
of accessing .String/.Int64 directly; apply the same pattern for any other
sql.NullString/sql.NullInt64 usages across handlers in internal/restapi (look
for trip.TripHeadsign and trip.DirectionID to locate occurrences).
---
Nitpick comments:
In `@internal/models/response.go`:
- Around line 80-90: The maps entryData and data are declared with
map[string]interface{} but the rest of the file uses map[string]any; update the
declarations for both entryData and data to use map[string]any (keeping the same
keys/values) to maintain type-consistency with other maps in this file (look for
symbols entryData and data in response.go and replace interface{} with any).
In `@internal/restapi/context_cancellation_test.go`:
- Around line 84-95: The test currently allows HTTP 429 as an acceptable
response which can mask failures in context-cancellation handling because all
sub-tests share the same rate limiter; update the test to initialize a fresh
API/mux (and its rate limiter) for each sub-test so limiter state is isolated,
and remove http.StatusTooManyRequests from the accepted status list in the
assertion; specifically, change the sub-test setup to create a new
server/handler (new API/mux/rate limiter instance) per t.Run and assert only
200, 401, 400, 404, 408, 500, or 504 (no 429).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bc9fb6dd-17be-430c-ba39-c3c40fb2fe67
📒 Files selected for processing (12)
gtfsdb/query.sqlgtfsdb/query.sql.gointernal/models/arrival_and_departure.gointernal/models/response.gointernal/models/stops.gointernal/restapi/arrivals_and_departures_for_location.gointernal/restapi/arrivals_and_departures_for_location_test.gointernal/restapi/arrivals_and_departures_for_stop_handler.gointernal/restapi/context_cancellation_test.gointernal/restapi/input_validation_integration_test.gointernal/restapi/routes.gointernal/restapi/trips_helper.go
….go for consistency
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/models/response.go`:
- Around line 62-92: In NewArrivalsAndDeparturesForLocationResponse ensure the
arrivalsAndDepartures parameter is normalized to an empty slice when nil so it
serializes as [] instead of null; update the function near the existing nil
checks for nearbyStopIds/situationIds/stopIds to add a check for
arrivalsAndDepartures and set it to an empty []ArrivalAndDeparture when nil
before building entryData so the response schema stays stable.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 36c1862c-4a85-4e03-9758-55bb99397822
📒 Files selected for processing (1)
internal/models/response.go
|
Hi @aaronbrethorst , @Ahmedhossamdev I resolved merge conflicts and failing test The PR is ready for review.... |
Remaining API Parity GapsTested against:
|
| Field | Local | Production | Notes |
|---|---|---|---|
status |
"ON_TIME" for block-propagated |
"default" |
Should only be ON_TIME/LATE/EARLY for direct GTFS-RT trip updates; block-propagated predictions should stay "default" |
arrivalEnabled |
always true |
false at first stop |
Origin stop (stopSequence 0) should have arrivalEnabled: false |
distanceFromStop |
0 |
real distance (e.g. 710m) |
Depends on vehicle position propagation logic |
numberOfStopsAway |
-1 |
real count (e.g. 2, 19) |
Same dependency as above |
tripStatus object
| Field | Local | Production | Notes |
|---|---|---|---|
scheduled |
present (false) |
absent | Extra field not in the OBA OpenAPI spec |
frequency |
absent | null |
Should serialize as null when trip is not frequency-based |
Top-level entry fields
| Field | Local | Production | Notes |
|---|---|---|---|
situationIds |
[] |
populated (e.g. ["40_17550", ...]) |
Requires agency-wide and route-wide alert matching, not just trip-level |
These are good candidates for follow-up issues. The core work (block-based prediction propagation, blockTripSequence correction, nearbyStopIds, vehicleId, tripStatus presence, stopSequence, lastUpdateTime) is already merged and working. 🚌
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gtfsdb/query.sql (1)
677-677: 💤 Low valueConsider making sort direction explicit.
SQL defaults to ascending order, but explicitly specifying
ASCimproves readability and makes intent clearer.✨ Proposed change for clarity
-ORDER BY a.id, stop_times.stop_id; +ORDER BY a.id ASC, stop_times.stop_id ASC;🤖 Prompt for 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. In `@gtfsdb/query.sql` at line 677, The ORDER BY clause at the end of the query ("ORDER BY a.id, stop_times.stop_id") relies on implicit ascending order; update it to be explicit by appending ASC to each sort key (e.g., ORDER BY a.id ASC, stop_times.stop_id ASC) to improve readability and make intent clear when editing or reviewing the query.
🤖 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.
Nitpick comments:
In `@gtfsdb/query.sql`:
- Line 677: The ORDER BY clause at the end of the query ("ORDER BY a.id,
stop_times.stop_id") relies on implicit ascending order; update it to be
explicit by appending ASC to each sort key (e.g., ORDER BY a.id ASC,
stop_times.stop_id ASC) to improve readability and make intent clear when
editing or reviewing the query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b49acfa-42da-4c3b-bcaf-5b07cf917b5a
📒 Files selected for processing (6)
gtfsdb/query.sqlgtfsdb/query.sql.gointernal/models/arrival_and_departure.gointernal/restapi/arrivals_and_departures_for_stop_handler.gointernal/restapi/routes.gointernal/restapi/trips_helper.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/models/arrival_and_departure.go
- internal/restapi/routes.go
- internal/restapi/arrivals_and_departures_for_stop_handler.go
- internal/restapi/trips_helper.go
- gtfsdb/query.sql.go
…complexity and fix GTFS layover constraint
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/restapi/arrivals_and_departures_for_location.go (2)
586-589: ⚡ Quick winDon't swallow batch-fetch errors; propagate at least context cancellation.
When
batchFetchLocationRoutesAndTripsfails (e.g., ctx cancellation, DB outage), this returns(false, nil), so the outer per-stop loop inarrivalsAndDeparturesForLocationHandlercontinues iterating over the remaining stops despite the client having gone away or the DB being unhealthy. The internal warning log in the batch helper doesn't help the request loop short-circuit. The inner loop already callsclientCanceledResponseforctx.Err()at line 599–602; do the same here so cancellation/timeout failures actually abort the handler.♻️ Proposed change
routesLookup, tripsLookup, tripStopCountMap, bErr := api.batchFetchLocationRoutesAndTrips(ctx, stopCode, allActiveStopTimes) if bErr != nil { - return false, nil + if ctx.Err() != nil { + api.clientCanceledResponse(w, r, ctx.Err()) + return false, ctx.Err() + } + return false, nil }🤖 Prompt for 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. In `@internal/restapi/arrivals_and_departures_for_location.go` around lines 586 - 589, The batchFetchLocationRoutesAndTrips error is being swallowed (returning false, nil) which prevents request-level cancellation/DB errors from aborting arrivalsAndDeparturesForLocationHandler; change the handling after calling batchFetchLocationRoutesAndTrips so that if bErr != nil you check ctx.Err(): if ctx.Err() != nil call clientCanceledResponse(ctx) (as done in the inner loop) and return its result, otherwise propagate/return the bErr (or a wrapped error) so the handler short-circuits on DB failures instead of continuing to the next stop. Ensure you update the branch that currently returns (false, nil) to return the appropriate cancellation response or the error from batchFetchLocationRoutesAndTrips.
297-309: ⚡ Quick winSeveral DB/spatial errors are silently dropped.
A few call sites in this handler discard returned errors with
_:
- Line 297:
stops, _ := api.GtfsManager.GetStopsForLocation(...)— a failure in the bbox query yields the same "no stops" path as a legitimately empty result, including triggeringemptyReturnsNotFound404s. The caller can't distinguish "out of area" from "lookup failed".- Lines 898–899:
GetStopsByIDsandGetRoutesForStopserrors are dropped during references assembly, which can yield a silently truncatedreferencesblock (missing stops/routes) without any log line.At minimum, log these errors (e.g., via
api.Logger.Warn) so failures are observable; the line-297 error in particular is worth surfacing as a 5xx since it gates the entire response shape. This matches the warn-and-degrade pattern you already use elsewhere in this file (e.g.,fetchStopTimesForDayOffset).Also applies to: 898-899
🤖 Prompt for 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. In `@internal/restapi/arrivals_and_departures_for_location.go` around lines 297 - 309, The handler currently ignores errors from api.GtfsManager.GetStopsForLocation and later calls GetStopsByIDs/GetRoutesForStops, causing silent failures; update the call sites (GetStopsForLocation in the arrivals_and_departures_for_location handler and the GetStopsByIDs/GetRoutesForStops usages during references assembly) to capture the returned error, log it with api.Logger.Warn (including context like the location params or stop IDs), and for the initial GetStopsForLocation error return an appropriate 5xx response instead of treating it as an empty result (preserve the existing emptyReturnsNotFound flow only for legitimately empty slices); follow the existing warn-and-degrade pattern used by fetchStopTimesForDayOffset for non-fatal downstream errors so references are logged when truncated.
🤖 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.
Nitpick comments:
In `@internal/restapi/arrivals_and_departures_for_location.go`:
- Around line 586-589: The batchFetchLocationRoutesAndTrips error is being
swallowed (returning false, nil) which prevents request-level cancellation/DB
errors from aborting arrivalsAndDeparturesForLocationHandler; change the
handling after calling batchFetchLocationRoutesAndTrips so that if bErr != nil
you check ctx.Err(): if ctx.Err() != nil call clientCanceledResponse(ctx) (as
done in the inner loop) and return its result, otherwise propagate/return the
bErr (or a wrapped error) so the handler short-circuits on DB failures instead
of continuing to the next stop. Ensure you update the branch that currently
returns (false, nil) to return the appropriate cancellation response or the
error from batchFetchLocationRoutesAndTrips.
- Around line 297-309: The handler currently ignores errors from
api.GtfsManager.GetStopsForLocation and later calls
GetStopsByIDs/GetRoutesForStops, causing silent failures; update the call sites
(GetStopsForLocation in the arrivals_and_departures_for_location handler and the
GetStopsByIDs/GetRoutesForStops usages during references assembly) to capture
the returned error, log it with api.Logger.Warn (including context like the
location params or stop IDs), and for the initial GetStopsForLocation error
return an appropriate 5xx response instead of treating it as an empty result
(preserve the existing emptyReturnsNotFound flow only for legitimately empty
slices); follow the existing warn-and-degrade pattern used by
fetchStopTimesForDayOffset for non-fatal downstream errors so references are
logged when truncated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0456dcd8-41d9-4b3b-8e18-40b34e00e525
📒 Files selected for processing (3)
gtfsdb/helpers.gogtfsdb/query.sqlinternal/restapi/arrivals_and_departures_for_location.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gtfsdb/query.sql
3 participants
Overview
This PR ports the
arrivals-and-departures-for-locationendpoint from the legacy Java codebase into the Gomaglevarchitecture. This endpoint allows clients to retrieve active arrivals and departures for multiple stops within a specified geographic bounding box or radius.Key Features
referencesblock to strictly adhere to the legacy JSON schema and optimize payload size.nearbyStopIdsto identify and include stops within the radius that may not currently have active arrivals.Bug Fixes
arrivals_and_departure_for_stop.gowhere thevehicleparameter was incorrectly passed asniltoBuildTripStatus.Testing
fix #799
Summary by CodeRabbit
New Features
Bug Fixes
Tests