resolve interlining tripId mismatch in trips-for-route - #1256
Conversation
Update trips-for-route handler to correctly distinguish between the queried route's trip and the currently executing active trip for interlined blocks, strictly adhering to OBA spec requirements. - Added a batch DB query (GetTripsByBlockIDs) to fetch related trips for interlined blocks without introducing an N+1 query bottleneck. - Implemented an O(1) lookup map to resolve the queried route's trip ID for the outer envelope. - Retained the actual executing trip for status.activeTripId. - Added TestTripsForRouteHandler_InterlinedBlock with a custom in-memory GTFS fixture to guarantee strict spec compliance.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe trips-for-route handler resolves queried-route trip IDs for interlined blocks while retaining the vehicle’s active trip ID. Tests add reusable GTFS fixtures for service-day, overnight, looping-route, midpoint, and fallback scenarios. ChangesInterlined trips-for-route behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant tripsForRouteHandler
participant GTFSManager
Client->>tripsForRouteHandler: Request trips-for-route
tripsForRouteHandler->>GTFSManager: Load queried-route block trips
GTFSManager-->>tripsForRouteHandler: Return service-day candidates and time windows
tripsForRouteHandler->>tripsForRouteHandler: Resolve queried TripId
tripsForRouteHandler->>tripsForRouteHandler: Build status from active trip
tripsForRouteHandler-->>Client: Return schedule, TripId, and activeTripId
🚥 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: 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/trips_for_route_handler.go`:
- Around line 317-341: The interlined block-trip lookup currently loses
ServiceID and may select the wrong queried-route trip. In
internal/restapi/trips_for_route_handler.go lines 317-341, update
blockTripForRoute resolution to retain the trip selected for the requested
route, or key candidates by both block ID and service ID before resolving
fetchedTrip. In internal/restapi/trips_for_route_handler_test.go lines 505-524,
add an after-midnight fixture where current and previous service IDs share a
block and assert the outer trip ID matches the queried-route trip for the
correct service.
- Around line 330-336: Update the error branch following GetTripsByBlockIDs in
the trips-for-route handler to call serverErrorResponse with the lookup error
and immediately return. Do not log and continue processing, so interline
resolution failures produce an error response instead of returning a successful
response with an incorrect trip ID.
🪄 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: 1da74de7-3dc2-4faf-9687-28b9bf5bacf2
📒 Files selected for processing (2)
internal/restapi/trips_for_route_handler.gointernal/restapi/trips_for_route_handler_test.go
Refactored the `blockTripForRoute` mapping to use a composite struct (`blockServiceKey` containing `BlockID` and `ServiceID`) rather than just the block ID. This guarantees accurate trip resolution and prevents cross-day trip overwrites when block IDs are reused across different service days. - Reverted to a single, performant `GetTripsByBlockIDs` batch query combining current and previous service IDs. - Introduced `TestTripsForRouteHandler_OvernightInterlinedBlock` with a detailed after-midnight GTFS fixture to enforce strict spec compliance for active trips spanning multiple service days.
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/trips_for_route_handler_test.go`:
- Line 623: Add a Go doc comment immediately before
TestTripsForRouteHandler_OvernightInterlinedBlock, beginning exactly with that
function name and briefly describing the test.
In `@internal/restapi/trips_for_route_handler.go`:
- Around line 347-351: Preserve the queried trip identity when a block revisits
the route: update the blockTripForRoute handling in trips_for_route_handler.go
so the trip that caused the block to be selected is retained instead of always
keeping the first (BlockID, ServiceID) candidate. In
internal/restapi/trips_for_route_handler.go lines 347-351, apply this selection
consistently; in internal/restapi/trips_for_route_handler_test.go lines 623-645,
add a fixture with two queried-route trips in one block/service and assert the
outer tripId matches the selecting trip.
🪄 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: aa2b4695-31a1-4937-95ee-7c8b40d54898
📒 Files selected for processing (2)
internal/restapi/trips_for_route_handler.gointernal/restapi/trips_for_route_handler_test.go
Update blockTripForRoute to map block keys to a slice of trips with their respective time windows. Add time window overlap logic to find the exact queried-route trip overlapping with the active trip. Previously, the map stored only the first queried-route trip found. If a block visited the queried route, left for another route, and returned, the outer tripId would incorrectly default to the first match rather than the trip actually overlapping the active window. Add TestTripsForRouteHandler_LoopingRouteBlock fixture and missing doc comments for exported test functions.
Run go fmt on the handler file to fix failing CI formatting checks.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/trips_for_route_handler.go`:
- Around line 404-417: Update the candidate selection around blockTripForRoute
and entryTripID so a non-overlapping candidate is never implicitly chosen via
bestIdx’s default value. Preserve the trip that selected the block, or implement
an explicit deterministic fallback when no entry overlaps the active trip; add
an integration test covering a gap between multiple queried-route trips.
🪄 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: da665b6e-91bb-4431-ad9f-80afab2d13cc
📒 Files selected for processing (2)
internal/restapi/trips_for_route_handler.gointernal/restapi/trips_for_route_handler_test.go
Code reviewFound 2 issues:
maglev/internal/restapi/trips_for_route_handler.go Lines 399 to 420 in 3d57480
maglev/internal/restapi/trips_for_route_handler.go Lines 379 to 397 in 3d57480 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
You've identified the right problem and set up the right structure for it. The spec is explicit that these are two different things — data.list[].tripId is "the trip on the queried route that caused this block to be selected," while status.activeTripId is "the trip the vehicle is currently executing" — and before this PR we conflated them. Establishing that split, and keeping BuildTripStatus on the active trip, is correct.
The (BlockID, ServiceID) composite key is a genuine improvement too. Keying on block alone would collide across service days, and the overnight test shows you thought about that case rather than assuming it away.
Two things need fixing before I can merge it.
1. The overlap test can't succeed for the case it's meant to handle, and the fallback is silently wrong.
bestIdx := 0
activeMin := fetchedTrip.MinArrivalTime.Int64
activeMax := fetchedTrip.MaxDepartureTime.Int64
for i, e := range entries {
if e.MinArrivalTime <= activeMax && e.MaxDepartureTime >= activeMin {
bestIdx = i
break
}
}
entryTripID = entries[bestIdx].IDA block is one vehicle running trips in sequence, so two trips in the same block never overlap in time. The condition is a standard inclusive interval-overlap test, which means it can only be satisfied at exact contiguity — the queried-route trip ending at precisely the instant the active trip begins. Add any layover and it fails: a trip ending 10:25 against an active trip starting 10:30 gives 10:25 >= 10:30 → false.
We model layovers explicitly — there's a block_layover table in the schema, and this same handler calls GetActiveLayoverBlockIDsForRoute a couple hundred lines up — so this isn't a hypothetical.
When nothing matches, the loop never breaks and bestIdx stays at its initial 0. Since the query orders by t.block_id, t.min_arrival_time, t.id, entries[0] is the earliest queried-route trip in that block and service day. So the common case doesn't fall back to something neutral; it emits a specific wrong trip that can be hours from the one that actually caused block selection. And it does so silently.
TestTripsForRouteHandler_LoopingRouteBlock passes only because the fixture has tfr-loop-b ending and tfr-loop-c starting at exactly 10:30:00. Nudge one of those by a minute and the test fails — worth doing as a way to see the bug.
What you want is "the queried-route trip in this block whose window contains or is nearest to the query window" — nearest-by-time with a deterministic tie-break — not overlap. And there shouldn't be a silent fallback: if no candidate can be chosen, that's worth a log line at minimum.
I should flag that CodeRabbit raised this same bestIdx fallback on the last review round, and the only commit after it was Format trips for route handler. Easy for a bot comment to get lost in the noise, but this one was right.
2. tripId now disagrees with schedule and situationIds in the same entry.
schedule is built from tripID (the active trip) at the buildScheduleForTrip call, SituationIds likewise from tripID, but TripId is now entryTripID. When those differ — precisely the interlining case this PR exists for — the entry claims to be trip A while carrying trip B's stop times and alerts.
The spec calls schedule.stopTimes "scheduled stop times for this trip," meaning the entry's trip. It gets worse with schedule.previousTripId, defined as "the preceding trip in this vehicle's block": built from the active trip, that resolves to the queried-route trip — so in your own InterlinedBlock fixture the entry would list itself as its own predecessor.
Java derives tripId, schedule, and situationIds from a single BlockTripInstance and lets only status.activeTripId diverge. Moving all three onto entryTripID would match.
The test requests includeSchedule=true but never asserts on the schedule, which is why this didn't surface. An assertion that schedule.stopTimes belong to the trip named in tripId would catch it and guard the fix.
Smaller things, none blocking:
allServiceIDs := serviceIDsfollowed byappend(allServiceIDs, prevServiceIDs...)aliasesserviceIDs' backing array. Harmless today becauseserviceIDsisn't read afterward, but it's a trap for the next edit — an explicit copy costs nothing.- The
GetTripsByBlockIDsfailure both logs aWarnand returns a 500. Beyond the double reporting, it's the only DB lookup in this handler that hard-fails;layoverBlocks,prevServiceIDs,nullBlockTrips, andGetTripsInBlockall degrade gracefully. Worth being consistent one way or the other. entryAgencyID = agencyIDuses the agency parsed from the request path rather than the queried route's actualagency_id, bypassing therouteAgencyMapthe handler already built. Same answer in practice, but the map is right there.- The comments claiming the selected trip's window "overlaps with the active trip" describe something block trips can't do. Whatever selection rule you land on, please make the comments match it — these will mislead the next reader.
- The three fixture builders are near-identical ~50-line copies differing only in CSV payload and zip name; one parameterized helper would be easier to extend.
One pre-existing issue your new test happens to walk right past: ServiceDate is always todayMidnight.UnixMilli(), but the spec says trips extending past midnight take the previous calendar day. TestTripsForRouteHandler_OvernightInterlinedBlock exercises exactly that path and asserts nothing about serviceDate. Not this PR's job to fix, but since you've built the fixture, adding the assertion (even as a skipped/known-failing case) would be a gift to whoever picks it up.
The structure here is good and I'd like to land it. Fix the trip selection and the schedule/situationIds binding and I'll re-review quickly.
Replace the flawed overlap test for interlined trips with a nearest midpoint calculation, as sequential block trips cannot overlap. Add a warning log when no queried-route trip candidate is found. Bind schedule and situationIds to the resolved entryTripID rather than the active tripID to prevent mixing trip identities in the response. Retain status binding on the active trip per the specification. Refactor test fixtures into a parameterized helper, and add schedule assertions along with layover gap tests to guarantee accurate mapping.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/restapi/trips_for_route_handler.go (2)
354-359:⚠️ Potential issue | 🟠 MajorDo not return a successful response when queried-trip resolution fails.
GetTripsByBlockIDsis required to resolvedata.list[].tripIdto the queried-route trip. This branch only logs the database error. The empty-candidate branch also leavesentryTripIDequal to the active cross-route trip. The handler can return HTTP 200 with the wrong outer trip ID. Return the lookup error throughserverErrorResponseand do not emit an active-tripTripIdwhen no queried-route candidate exists.As per coding guidelines, “When a database lookup fails, return 404 via
sendNotFoundonly forerrors.Is(err, sql.ErrNoRows); route all other errors throughserverErrorResponseas 500.”Proposed fix
if err != nil { - // Degrade gracefully: skip interlining resolution for these blocks. - api.Logger.Warn("trips-for-route: failed to fetch block trips for interlining", "error", err) + api.serverErrorResponse(w, r, err) + return }Also applies to: 421-427
🤖 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/trips_for_route_handler.go` around lines 354 - 359, The interlining trip lookup must not degrade to a successful response: in the GetTripsByBlockIDs error branch, return sql.ErrNoRows through sendNotFound and all other errors through serverErrorResponse. In the empty-candidate handling around entryTripID, avoid retaining the active cross-route trip ID; only emit TripId when a queried-route candidate was resolved.Source: Coding guidelines
361-370: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winInclude the interlined trip in
fetchedTrips.
buildTripReferencespre-populatespresentTripsfromfetchedTrips, so interlined entries currently use the active trip’s reference instead of the quoted-route trip reference, and interlined schedules may use the active trip’s previous trip. AddfetchedTrips = append(fetchedTrips, selectedInterlineTrip)before building references, and add an interlinedincludeTrip=truetest.🤖 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/trips_for_route_handler.go` around lines 361 - 370, Append selectedInterlineTrip to fetchedTrips before invoking buildTripReferences so presentTrips uses the quoted-route trip reference and current interlined schedule. Add an interlined-trip test verifying includeTrip=true and the selected trip is included in fetchedTrips.
🤖 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.
Outside diff comments:
In `@internal/restapi/trips_for_route_handler.go`:
- Around line 354-359: The interlining trip lookup must not degrade to a
successful response: in the GetTripsByBlockIDs error branch, return
sql.ErrNoRows through sendNotFound and all other errors through
serverErrorResponse. In the empty-candidate handling around entryTripID, avoid
retaining the active cross-route trip ID; only emit TripId when a queried-route
candidate was resolved.
- Around line 361-370: Append selectedInterlineTrip to fetchedTrips before
invoking buildTripReferences so presentTrips uses the quoted-route trip
reference and current interlined schedule. Add an interlined-trip test verifying
includeTrip=true and the selected trip is included in fetchedTrips.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fd6e7f53-53e1-421d-94ca-cfab77de3bfa
📒 Files selected for processing (2)
internal/restapi/trips_for_route_handler.gointernal/restapi/trips_for_route_handler_test.go
Restore missing test cases to ensure proper coverage for block trip selection: - Restore TestTripsForRouteHandler_InterlinedBlock, OvernightInterlinedBlock, and LoopingRouteBlock. - Add a new GapCase test using the existing gapFiles fixture to test layovers. - Update all tests to use the standard createTestApiWithGTFSFixture helper. This resolves the undefined function compile error, eliminates dead fixture code, and fully covers the midpoint-selection logic.
8f02ea7 to
ddffa70
Compare
- Return 404 on sql.ErrNoRows and 500 on other DB errors instead of silent fallback. - Return 404 when no queried-route trip candidate is found. - Append the resolved interlined trip to fetchedTrips for accurate references. - Add test to verify interlined trip references.
# Conflicts: # internal/restapi/trips_for_route_handler.go
Two code paths returned 404 for the entire response whenever a single interlined block's queried-route trip couldn't be resolved: one after a real GetTripsByBlockIDs error, another when no matching trip was found for a block+service key. Both discarded every other correctly resolved entry in the response. The trips-for-route spec guarantees 200 OK even for unknown routes and out-of-service-area conditions, with no error or 404 case defined. Skip just the unresolvable entry instead, matching this handler's own pattern for other per-entry resolution failures (e.g. BuildTripStatus errors already log and continue rather than aborting). The GetTripsByBlockIDs error branch also had a dead sql.ErrNoRows check: it's a sqlc :many query, which returns an empty result with a nil error rather than ErrNoRows.
hms() and loopingRouteWithGapFiles() are leftover from earlier iterations of this test file's fixtures and are no longer called anywhere; gapFiles() supersedes the latter.
blockTripForRoute keyed candidates by (BlockID, ServiceID), requiring a block's active trip and its queried-route partner to share the exact same service_id string. GTFS allows more than one service_id to be active on a single calendar date, and nothing requires a block's trips to agree on which one they're tagged with, so a legitimately interlined block could fail to resolve and its entry would be silently dropped. Key on BlockID alone instead, matching how allLinkedBlocks already treats blocks earlier in this handler. Both queries feeding this lookup are already scoped to today's and yesterday's active service IDs, so this doesn't reopen the cross-day collision the service-scoped key was originally added to prevent.
tripsForRouteHandler had grown two new local types, a batched query, and a nested midpoint-distance search inlined directly into an already 300+ line function. Extract that into two named helpers: - buildBlockTripForRoute batch-fetches queried-route trips for the blocks fetchedTrips are interlined through. - resolveInterlinedEntryTripID picks, from those candidates, the one nearest a given active trip. No behavior change: this is a pure code-motion refactor so the handler reads as a sequence of named steps instead of mixing orchestration with the resolution mechanics inline.
resolveInterlinedEntryTripID picked the queried-route candidate whose time window was nearest the active trip's, searched across every candidate sharing the block ID regardless of service_id. An agency reusing a block ID across two unrelated service_ids (e.g. yesterday's and today's schedules) could therefore return a candidate from the wrong calendar day whenever that candidate's time-of-day happened to be numerically closer to the active trip's than the real match. Prefer candidates that share the active trip's exact service_id first: trips under one service_id recur together on every date it's active, so they can never be a cross-day collision. Only fall back to the broader nearest-midpoint search when no same-service_id candidate exists, preserving resolution for the legitimate case of two distinct service_ids both active on the same calendar day. Also skip candidates (and the active trip itself) whose cached MinArrivalTime/MaxDepartureTime are NULL, which happens for a trip with no stop_times rows. Reading these sql.NullInt64 fields via .Int64 without checking .Valid silently treated such a trip as starting at midnight, corrupting the nearest-midpoint comparison.
When no queried-route trip could be found anywhere in an interlined block, the handler skipped the entry entirely rather than emit one with a misleading tripId. That trades a documented guarantee (one entry per active block) for a stronger one no client actually needs: legacy OBA always reports the active trip's own ID here (tripId and status.activeTripId are the same field in the Java implementation), and this handler already defaults entryTripID to the active trip's ID before interlining resolution runs. Let that default stand when resolution fails instead of skipping the entry, matching legacy behavior for this one case while keeping the route-aware tripId this PR adds for every block that does resolve.
Schedule is built from entryTripID (the resolved queried-route trip), not the active trip, but no test asserted on schedule contents for an interlined block — only tripId and status.activeTripId were checked. Assert that schedule.stopTimes reflect tfr-trip-a's own 11:20/11:50 stop times rather than the active trip tfr-trip-b's 11:55/12:05, covering the schedule/tripId binding this PR introduces.
|
Comments have been addressed
3 participants
Description
This PR resolves a critical spec compliance issue in the
trips-for-routeendpoint regarding interlined blocks. Previously, the handler incorrectly assigned the currently executing trip to the outertripIdfield, even if that trip belonged to a different route due to interlining.According to the OBA specification, the outer
tripIdmust consistently reflect the queried route's trip, whilestatus.activeTripIdshould reflect the trip the vehicle is currently executing on the ground.Changes Made
GetTripsByBlockIDs), successfully avoiding an N+1 query problem.blockTripForRoute) to map block IDs to the correct queried-route trip ID. The outertripIdis now properly overridden using this map, whileBuildTripStatusremains untouched to accurately populatestatus.activeTripId.TestTripsForRouteHandler_InterlinedBlock. This test constructs an in-memory GTFS zip fixture featuring a single block spanning two distinct routes. By manipulating a mock clock, it enforces strict assertions to guarantee that the outertripIdmatches the queried route, while the inneractiveTripIdreflects the simulated active trip.Closes: #1254
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Tests