feat: Implement arrivals-and-departures-for-location endpoint#787
Open
ARCoder181105 wants to merge 39 commits into
Open
feat: Implement arrivals-and-departures-for-location endpoint#787ARCoder181105 wants to merge 39 commits into
ARCoder181105 wants to merge 39 commits into
Conversation
ARCoder181105
changed the title
arrivals-and-departures-for-location endpoint
ARCoder181105
changed the title
arrivals-and-departures-for-location endpoint
ARCoder181105
added a commit
to ARCoder181105/maglev
that referenced
this pull request
Mar 28, 2026
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
Collaborator
|
Hey @ARCoder181105 Great work on this endpoint! |
Collaborator
Author
|
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! |
Collaborator
Author
|
Hi @aaronbrethorst , @Ahmedhossamdev The PR is ready for review ... |
Member
|
Hi @ARCoder181105 |
Collaborator
|
@Ahmedhossamdev I already updated it here #807 |
Collaborator
Author
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:
📝 WalkthroughWalkthroughImplements the arrivals-and-departures-for-location endpoint with models and a response constructor; adds param parsing/validation, batched GTFS queries, arrival construction and enrichment (predictions, vehicle inference, alerts), reference building, deterministic SQL ordering, helper fixes, and tests. ChangesArrivals-and-Departures-for-Location Endpoint
Sequence Diagram(s)sequenceDiagram
participant Client
participant LocationHandler
participant StopResolver
participant GTFSDB
participant ArrivalBuilder
participant References
participant Response
Client->>LocationHandler: GET /arrivals-and-departures-for-location.json (lat,lon,time,filters)
LocationHandler->>StopResolver: find stops near location (radius/spans)
StopResolver->>GTFSDB: query stops and stop_times (batched)
LocationHandler->>ArrivalBuilder: build arrivals per stop (routes,trips,vehicles)
ArrivalBuilder->>GTFSDB: batch fetch routes, trips, stop_times, services
ArrivalBuilder->>ArrivalBuilder: apply predictions, trip status, infer stops from vehicle
ArrivalBuilder->>References: collect trips/routes/stops/agencies and situations
LocationHandler->>Response: assemble entry + references + nearbyStopIds
Response-->>Client: JSON payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
Collaborator
Author
|
Hi @aaronbrethorst , @Ahmedhossamdev I resolved merge conflicts and failing test The PR is ready for review.... |
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
ARCoder181105
force-pushed
the
feature/arrivals-departures-for-location
branch
from
0301916 to
ccf440f
Compare
Collaborator
Author
|
Hi @aaronbrethorst , @Ahmedhossamdev , This PR is ready for review! I was able to successfully replicate the behavior of the legacy Java OBA output for this endpoint. I verified the data parity using this PROD link: Please take a look whenever you have time. |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/restapi/context_cancellation_test.go (1)
84-94: ⚡ Quick winAvoid masking cancellation behavior with rate-limit outcomes.
Allowing 429 here makes the test pass even when cancellation logic regresses. Prefer isolating this test from rate limiting (e.g., dedicated high-limit test API / key) and keep expected statuses focused on cancellation and handler outcomes.
🤖 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/context_cancellation_test.go` around lines 84 - 94, The test currently allows http.StatusTooManyRequests which masks cancellation regressions; in the assertion in context_cancellation_test.go (the block checking statusCode) remove StatusTooManyRequests from the allowed statuses and instead ensure the test uses an isolated high-rate test key or test server (e.g., configure the request setup that exercises the cancellation path in the same test harness) so that rate limiting cannot produce 429s—update the assertion to only accept 200, 401, 400, 404, 500, 408, or 504 and adjust the test setup (API key/limit or dedicated endpoint) to prevent 429 responses during this test.internal/restapi/arrivals_and_departures_for_location_handler_test.go (1)
476-493: ⚡ Quick winAssert
entry.limitExceededexplicitly in the limit test.The test currently only checks returned item count. Please also assert
entry["limitExceeded"] == truewhen truncation happens so the API contract is fully covered.🤖 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_handler_test.go` around lines 476 - 493, The test TestArrivalsAndDeparturesForLocationLimitExceeded only asserts the number of arrivals but not the API contract flag; update the test after extracting entry (in TestArrivalsAndDeparturesForLocationLimitExceeded) to explicitly assert that entry["limitExceeded"] is present and true (e.g., require.True/assert.Equal on entry["limitExceeded"] == true) so the case where results are truncated is validated alongside the length check.
🤖 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/arrivals_and_departures_for_location_handler.go`:
- Around line 483-494: The code computes maxBefore/maxAfter but still uses
params.MinutesBefore and params.MinutesAfter when building the time window;
update the stopWindowStart and stopWindowEnd calculations inside the handler so
they use maxBefore and maxAfter instead of
params.MinutesBefore/params.MinutesAfter. Locate the block where stopWindowStart
and stopWindowEnd are set (references: maxBefore, maxAfter, stopWindowStart,
stopWindowEnd, spc.QueryTime) and replace the direct params values with the
computed maxima that include params.FrequencyMinutesBefore and
params.FrequencyMinutesAfter.
---
Nitpick comments:
In `@internal/restapi/arrivals_and_departures_for_location_handler_test.go`:
- Around line 476-493: The test
TestArrivalsAndDeparturesForLocationLimitExceeded only asserts the number of
arrivals but not the API contract flag; update the test after extracting entry
(in TestArrivalsAndDeparturesForLocationLimitExceeded) to explicitly assert that
entry["limitExceeded"] is present and true (e.g., require.True/assert.Equal on
entry["limitExceeded"] == true) so the case where results are truncated is
validated alongside the length check.
In `@internal/restapi/context_cancellation_test.go`:
- Around line 84-94: The test currently allows http.StatusTooManyRequests which
masks cancellation regressions; in the assertion in context_cancellation_test.go
(the block checking statusCode) remove StatusTooManyRequests from the allowed
statuses and instead ensure the test uses an isolated high-rate test key or test
server (e.g., configure the request setup that exercises the cancellation path
in the same test harness) so that rate limiting cannot produce 429s—update the
assertion to only accept 200, 401, 400, 404, 500, 408, or 504 and adjust the
test setup (API key/limit or dedicated endpoint) to prevent 429 responses during
this test.
🪄 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: 1d368b31-47f2-4495-97ca-cf22a78b85ce
📒 Files selected for processing (18)
gtfsdb/helpers.gogtfsdb/query.sqlgtfsdb/query.sql.gointernal/models/arrival_and_departure.gointernal/models/response.gointernal/models/situation.gointernal/models/stops.gointernal/restapi/arrival_and_departure_for_stop_handler.gointernal/restapi/arrivals_and_departures_for_location_handler.gointernal/restapi/arrivals_and_departures_for_location_handler_test.gointernal/restapi/arrivals_and_departures_for_stop_handler.gointernal/restapi/context_cancellation_test.gointernal/restapi/coverage_test.gointernal/restapi/input_validation_integration_test.gointernal/restapi/reference_utils.gointernal/restapi/routes.gointernal/restapi/trips_helper.gointernal/restapi/trips_helper_test.go
💤 Files with no reviewable changes (1)
- internal/restapi/trips_helper_test.go
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Collaborator
Author
Reference: Arrivals and Departures for Location Endpoint1. OverviewThis document serves as a technical reference for the Purpose: To retrieve all active scheduled and real-time arrivals/departures for transit stops within a specified geographic bounding box ( 2. Architectural Flow & Performance
To maintain high performance across a wide geographic radius, the data fetching is heavily batched to avoid N+1 query bottlenecks.
3. Key Heuristics & Edge Cases3.1 Stop Sequence Inference (Nearest-Stop Fallback)Not all transit agencies provide 3.2 Arrival Status DerivationArrival statuses (
4. Known Parity DeviationsThe core math (deviations, distances, sequences) is strictly verified against Java OBA. However, there are a few intentional formatting and serialization differences to align with Go's standard library and OpenAPI conformance:
5. Testing & VerificationParity was verified using the following production payload: All |
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/restapi/arrival_and_departure_for_stop_handler.go (1)
717-743:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInfer stop sequence from the vehicle’s active trip, not always the target trip.
If the vehicle has moved to another trip in the block, inferring with
targetTripIDcan compute an invalid sequence and incorrectnumberOfStopsAway.💡 Proposed fix
func (api *RestAPI) getNumberOfStopsAway(ctx context.Context, targetTripID string, targetStopSequence int, vehicle *gtfs.Vehicle, serviceDate time.Time) *int { currentVehicleStopSequence := getCurrentVehicleStopSequence(vehicle) + activeTripID := GetVehicleActiveTripID(vehicle) + if activeTripID == "" { + activeTripID = targetTripID + } if currentVehicleStopSequence == nil { // Fallback: infer the vehicle's current stop from its lat/lon position. // This handles agencies (e.g. Sound Transit Link light rail) that don't // publish current_stop_sequence in GTFS-RT vehicle positions. if vehicle == nil || vehicle.Position == nil || vehicle.Position.Latitude == nil || vehicle.Position.Longitude == nil { return nil } inferred := api.inferStopSequenceFromPosition( - ctx, targetTripID, + ctx, activeTripID, float64(*vehicle.Position.Latitude), float64(*vehicle.Position.Longitude), ) if inferred == nil { return nil } currentVehicleStopSequence = inferred } - - activeTripID := GetVehicleActiveTripID(vehicle) - if activeTripID == "" { - activeTripID = targetTripID - }🤖 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/arrival_and_departure_for_stop_handler.go` around lines 717 - 743, Compute the vehicle's activeTripID (via GetVehicleActiveTripID(vehicle) with fallback to targetTripID) before calling inferStopSequenceFromPosition and pass that activeTripID into inferStopSequenceFromPosition instead of always using targetTripID; this ensures currentVehicleStopSequence is inferred for the trip the vehicle is actually on. Update the code paths referencing activeTripID/currentVehicleStopSequence (including the later call to getBlockSequenceForStopSequence that assigns vehicleGlobalSeq) to use the same activeTripID variable.
🧹 Nitpick comments (1)
internal/restapi/arrival_and_departure_for_stop_handler.go (1)
756-767: 🏗️ Heavy liftCache trip stop geometry for fallback sequence inference.
This fallback does two DB calls per invocation. In endpoints that compute many arrivals, this can reintroduce query amplification for agencies without
current_stop_sequence.🤖 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/arrival_and_departure_for_stop_handler.go` around lines 756 - 767, The fallback sequence inference currently issues GetStopTimesForTrip and GetStopsByIDs on every call, causing repeated DB queries; add a cache for trip stop geometry (e.g., a TripStopGeometryCache on GtfsManager) and change the handler to query that cache first and only call GetStopTimesForTrip/GetStopsByIDs on cache miss, storing the resolved stop geometry (stopIDs -> stops) with a sensible TTL/eviction; update the code paths in arrival_and_departure_for_stop_handler.go that call GetStopTimesForTrip and GetStopsByIDs to use the new cache accessors on GtfsManager (e.g., GetCachedTripStops/GetOrLoadTripStops) and keep current_stop_sequence logic unchanged.
🤖 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/arrivals_and_departures_for_location_handler_test.go`:
- Around line 38-50: The test only checks params.Time is non-zero; change it to
assert the exact instant parsed from the unix-millis value to catch
seconds-vs-milliseconds bugs: in the test for
parseArrivalsAndDeparturesForLocationParams replace the assert.False(t,
params.Time.IsZero()) with an assertion that params.Time equals the expected
time for 1609459200000ms (e.g., the UTC instant corresponding to
2021-01-01T00:00:00Z using time.Unix or time.UnixMilli), referencing params.Time
and the parseArrivalsAndDeparturesForLocationParams call to locate the check.
- Around line 480-494: The test currently asserts entry["limitExceeded"] == true
without first ensuring more than maxCount arrivals exist; update the test to
first fetch the unbounded result (call serveApiAndRetrieveEndpoint with same
params but without maxCount or with a much larger maxCount) and count the total
arrivals, assert that total > 1 (the maxCount), then assert that the truncated
response (the existing request with maxCount=1) has len(ads) <= 1 and
entry["limitExceeded"] == true; reference the existing helper
serveApiAndRetrieveEndpoint and local variables ads and entry to locate where to
add the extra request and precondition check.
In `@internal/restapi/arrivals_and_departures_for_location_handler.go`:
- Around line 953-954: The code currently ignores errors from
api.GtfsManager.GtfsDB.Queries.GetStopsByIDs and GetRoutesForStops (assigned to
batchStops and batchRoutesForStops) which can produce an incomplete
references.stops block; update the handler to check both returned errors and
handle them explicitly (e.g., log the error and return the appropriate API error
response/status per the OpenAPI spec) instead of discarding them—ensure you use
the error values from the two calls to decide whether to abort/respond with an
error and only proceed to build references.stops when both calls succeed.
- Around line 348-350: The handler is swallowing failures from
batchFetchLocationRoutesAndTrips via collectArrivalsForLocationStop and
returning silently; change the flow so that collectArrivalsForLocationStop
returns the error up to the caller instead of returning nil, and have the caller
inspect that error and send the appropriate HTTP error response (e.g., 500) per
the OpenAPI behavior; locate the call site of collectArrivalsForLocationStop and
the implementation of
batchFetchLocationRoutesAndTrips/collectArrivalsForLocationStop and propagate
and handle errors rather than using a bare return to avoid silent partial
responses.
In `@internal/restapi/arrivals_and_departures_for_stop_handler.go`:
- Around line 421-423: The dedupe key uses bare alert.ID which is only
feed-scoped; change deduplication and situation IDs to include the feed/source
namespace (e.g., fmt.Sprintf("%s:%s", feedNameOrSource, alert.ID)) wherever
collectedAlerts, situationIDs, and models.Situation.ID are built. Specifically,
update the logic in arrivals_and_departures_for_stop_handler.go around
collectedAlerts and situationIDs (the append and map key usage for alert.ID) to
use the namespaced key, and update BuildSituationReferences in
reference_utils.go to accept or construct the same namespaced ID so
models.Situation.ID and the situationIds array consistently use the feed-scoped
namespace and avoid cross-feed collisions.
In `@internal/restapi/context_cancellation_test.go`:
- Around line 84-93: The test's status allowlist in the assert.True call is too
permissive: remove http.StatusUnauthorized and stop allowing a blanket
http.StatusBadRequest; instead restrict accepted codes to only those that
represent successful completion or cancellation/error paths (e.g.,
http.StatusOK, http.StatusInternalServerError, http.StatusRequestTimeout,
http.StatusGatewayTimeout, http.StatusNotFound) and if a 400 is legitimately
expected for a specific endpoint, check that explicitly for that route (or
assert the response body contains the cancellation-specific error) rather than
using http.StatusBadRequest in the general assert on statusCode; update the
assert.True usage that examines statusCode accordingly in
context_cancellation_test.go.
In `@internal/restapi/reference_utils.go`:
- Around line 116-126: The code currently calls utils.FormCombinedID(agencyID,
rawRouteID/rawStopID) even when agencyID is empty, producing malformed combined
IDs; update the logic in the block handling rawRouteID and rawStopID (and the
similar block at 137-139) to only call utils.FormCombinedID when both agencyID
!= "" and the raw ID != "" (i.e., check agencyID non-empty before forming the
combined ID), otherwise leave the rawRouteID/rawStopID unchanged after
getStringValue.
---
Outside diff comments:
In `@internal/restapi/arrival_and_departure_for_stop_handler.go`:
- Around line 717-743: Compute the vehicle's activeTripID (via
GetVehicleActiveTripID(vehicle) with fallback to targetTripID) before calling
inferStopSequenceFromPosition and pass that activeTripID into
inferStopSequenceFromPosition instead of always using targetTripID; this ensures
currentVehicleStopSequence is inferred for the trip the vehicle is actually on.
Update the code paths referencing activeTripID/currentVehicleStopSequence
(including the later call to getBlockSequenceForStopSequence that assigns
vehicleGlobalSeq) to use the same activeTripID variable.
---
Nitpick comments:
In `@internal/restapi/arrival_and_departure_for_stop_handler.go`:
- Around line 756-767: The fallback sequence inference currently issues
GetStopTimesForTrip and GetStopsByIDs on every call, causing repeated DB
queries; add a cache for trip stop geometry (e.g., a TripStopGeometryCache on
GtfsManager) and change the handler to query that cache first and only call
GetStopTimesForTrip/GetStopsByIDs on cache miss, storing the resolved stop
geometry (stopIDs -> stops) with a sensible TTL/eviction; update the code paths
in arrival_and_departure_for_stop_handler.go that call GetStopTimesForTrip and
GetStopsByIDs to use the new cache accessors on GtfsManager (e.g.,
GetCachedTripStops/GetOrLoadTripStops) and keep current_stop_sequence logic
unchanged.
🪄 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: 31caf61a-e3df-47b9-9fea-bb938123725d
📒 Files selected for processing (18)
gtfsdb/helpers.gogtfsdb/query.sqlgtfsdb/query.sql.gointernal/models/arrival_and_departure.gointernal/models/response.gointernal/models/situation.gointernal/models/stops.gointernal/restapi/arrival_and_departure_for_stop_handler.gointernal/restapi/arrivals_and_departures_for_location_handler.gointernal/restapi/arrivals_and_departures_for_location_handler_test.gointernal/restapi/arrivals_and_departures_for_stop_handler.gointernal/restapi/context_cancellation_test.gointernal/restapi/coverage_test.gointernal/restapi/input_validation_integration_test.gointernal/restapi/reference_utils.gointernal/restapi/routes.gointernal/restapi/trips_helper.gointernal/restapi/trips_helper_test.go
💤 Files with no reviewable changes (1)
- internal/restapi/trips_helper_test.go
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Member
|
Great work, @ARCoder181105! I'm reviewing and testing the endpoint right now. |
Collaborator
Author
|
Hi @Ahmedhossamdev, Thanks for the review! Here is the parameter documentation for the Required Parameters
Optional Parameters
(Note: While putting this together, I noticed two minor bugs: non-frequency trips weren't being correctly filtered back down to |
|
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
3 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.
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