Make situation references resolve on every endpoint - #1313
Merged
burma-shave merged 18 commits intoAug 14, 2026
Merged
Conversation
Every list entry emitted situationIds, but references.situations was always left as the empty slice from NewEmptyReferences, so clients got IDs that resolved to nothing. Collect the alerts matched while building entries and convert them through the existing BuildSituationReferences helper. The alerts are now looked up with GetAlertsByIDs directly, using the route and agency the entry builder has already resolved, rather than through GetSituationIDsForTrip, which re-queried both per trip and discarded the alerts needed here. Situation IDs are stamped in combined agencyId_situationId form so the references match the situationIds the same response emits.
Contributor
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (11)
📝 WalkthroughWalkthroughThe REST API now collects situation references with emitted situation IDs across trip, stop, location, and route responses. Shared logic filters, qualifies, deduplicates, and converts alerts. Tests verify that emitted IDs resolve to ChangesSituation reference resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RESTAPI
participant TripSituations
participant References
Client->>RESTAPI: request trip or stop data
RESTAPI->>TripSituations: collect situation IDs and references
TripSituations-->>RESTAPI: return resolved situations
RESTAPI->>References: include situation references
RESTAPI-->>Client: return entries and references.situations
Possibly related PRs
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 |
This was referenced Aug 7, 2026
Collaborator
Author
|
@coderabbitai full review |
Contributor
|
trips-for-route had the same defect as trips-for-location: entries carried situationIds while references.situations was left empty, so the IDs resolved to nothing. Extract situationRefsForTrip, which returns the alerts alongside their situation IDs, and have GetSituationIDsForTrip delegate to it. Both endpoints now emit entry IDs and situation references from a single alert lookup per trip.
ARCoder181105
changed the title
Adding the situations argument pushed buildTripReferences to eight positional parameters, over the seven SonarCloud allows, and the call sites had become a row of same-typed values with no labels. Take a tripReferenceParams struct instead, and make it a method so the api receiver stops occupying a parameter slot. This matches the shape BuildReference already uses in the trips-for-location handler. No behaviour change: every use of the stops argument is a len or range, so the empty call site passing nil is equivalent to the empty slice it passed before.
buildTripReferences carried a cognitive complexity of 93 against the 15 SonarCloud allows: five unrelated phases inlined into one body, with the trip-to-model conversion written out twice. Introduce tripReferenceSets to hold the accumulating trips, routes and agencies, and give each phase its own function — collecting IDs from entries, filling in trips that were only referenced by ID, resolving routes and their agencies, and building the stop and trip lists. The entry point now reads as the sequence of those stages. Also restores a log message mangled while renaming the parameters in the previous commit, and drops a trip ID from the reference set when its combined form cannot be parsed, matching what the next/previous/active ID paths in the same function already did.
Six endpoints emit situationIds. Two never populated references.situations at all, and three populated it with raw alert IDs while emitting combined ones, so their references were unreachable either way. The wiki specs require resolution: trip-details calls them "combined IDs ... Full alert records are in references.situations", and arrival-and-departure-for-stop asks for records "for all situation IDs listed in entry.situationIds and entry.tripStatus.situationIds". Move the situation helpers out of the two handler files they were living in and into reference_utils.go, next to BuildSituationReferences, and add TripSituations, which returns a trip's IDs and their references together so the two cannot disagree. Convert trip-details, arrival-and-departure-for-stop and trip-for-vehicle onto it, and arrivals-and-departures-for-stop onto the collector, which also removes its second, separately derived top-level ID list. trip-details and arrival-and-departure-for-stop were each resolving the same trip twice, once for the IDs and once for the alerts; they now do it once. The status.SituationIDs branch is dropped where BuildTripStatus was given the same trip ID, which is both call sites. This settles only that the two sides agree. A feed whose producer already prefixes its alert IDs still yields a doubled prefix, consistently.
ARCoder181105
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/reference_utils.go`:
- Around line 515-530: Update situationReferences to validate that
BuildSituationReferences returns the same number of situations as refs before
the index-based ID assignment; if lengths differ, fail visibly rather than
pairing or indexing mismatched entries. Preserve the existing ordered ID
assignment when the lengths match.
In `@internal/restapi/situation_references_test.go`:
- Around line 106-109: Add a sawSituationID boolean to the situation reference
test, set it when iterating emitted IDs in the existing assertion loop, and
assert it after the loop so the test fails when no situation ID is emitted.
Follow the sibling test pattern in trips_for_location_handler_test.go and
trips_for_route_handler_test.go.
In `@internal/restapi/trips_for_location_handler_test.go`:
- Line 629: Replace the fixed time.Sleep after createTestApiWithRealTimeData
with a deterministic condition-based wait for the required real-time data state,
documenting the condition and preserving coverage for the load-delay behavior;
if createTestApiWithRealTimeData already guarantees synchronization, remove the
sleep instead.
In `@internal/restapi/trips_for_route_handler.go`:
- Line 405: Replace the per-trip situationRefsForTrip calls at both affected
sites with the existing bulk alert-loading flow used by
trips_for_location_handler.go. Resolve the entry trip from fetchedTrips using
entryTripID, derive its route and agency through tripAgencyMap/routeAgencyMap,
then pass the resolved IDs to GetAlertsByIDs and provide the results to
situations.add. Preserve interlined-block handling by sourcing the route from
the resolved entry trip rather than fetchedTrip unconditionally.
- Around line 896-899: Update the GetRouteIDsForStops error path in
routeIDsForStops to log the query failure with logging.LogError before returning
routeIDsByStop, matching the error-handling pattern used by fillMissingTrips and
fillRoutesAndAgencies.
- Around line 908-935: Update tripReferenceList to skip trips whose resolved
route has an empty AgencyID, rather than relying on the map lookup’s ok value;
preserve the existing skip for missing keys and only append references when the
route resolution provides a non-empty agency identifier.
🪄 Autofix
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: a692a796-9d77-4fb1-a464-7b8d64238790
📒 Files selected for processing (11)
internal/restapi/arrival_and_departure_for_stop_handler.gointernal/restapi/arrivals_and_departures_for_stop_handler.gointernal/restapi/reference_utils.gointernal/restapi/situation_references_test.gointernal/restapi/trip_details_handler.gointernal/restapi/trip_for_vehicle_handler.gointernal/restapi/trips_for_location_handler.gointernal/restapi/trips_for_location_handler_test.gointernal/restapi/trips_for_route_handler.gointernal/restapi/trips_for_route_handler_test.gointernal/restapi/trips_helper.go
Alert IDs were prefixed with the agency unconditionally. A GTFS-RT feed
exported by an OBA instance already carries that prefix — Puget Sound
ships "1_92239" — so maglev published "1_92239" as "1_1_92239", an ID no
client has ever seen.
Upstream reports "1_92239" for situationIds and references.situations
alike:
$ curl ".../arrivals-and-departures-for-stop/1_46766.json?key=..."
{"topLevel":["1_92239"],"perArrival":["1_92239"],
"referencedIDs":["1_92239"]}
Make the prefixing idempotent: leave an ID alone when it already starts
with this agency's prefix. A different agency's prefix is not treated as
ours, so a genuinely namespaced ID from another agency still gets one.
All six endpoints form situation IDs through one function now, so this
corrects every one of them.
- situationReferences pairs IDs with situations by position, so it now checks the two lengths agree and logs rather than mislabelling or indexing past the end if BuildSituationReferences ever starts skipping alerts. - tripReferenceList skips a trip whose route resolved to a zero value. The route map holds placeholders until the routes are fetched, so a lookup could succeed with an empty agency and emit references whose every combined ID was the empty string. - routeIDsForStops logs its query failure instead of silently returning no routes, matching the other loaders in the same file. - trips-for-route resolves a trip's alerts from the trips and routes it has already loaded, dropping two queries per entry. The entry trip is looked up by its own ID so interlined blocks still take their route from the entry rather than the active trip, and an unfetched trip falls back to the per-trip lookup. - The cross-endpoint situation test asserts that an ID was actually emitted. It was not: the arrivals case queried under the real clock, outside the fixture's calendar, so the stop had no arrivals and the case asserted nothing. It is now pinned to a service day. - Replaced a fixed sleep with a wait on the condition it stood for, the first real-time poll landing.
Collaborator
Author
|
Found the issue to be a global , I tried to make the changes complete for other endpoint as well so that a final PR fix be made , the reason the PR went this large... |
Member
|
Verified the premise of the prefix fix against real data rather than taking it on trust: pulled the live Puget Sound alerts export (/api/gtfs_realtime/alerts-for-agency/1.pb) and protoc --decode_raw confirms FeedEntity.id already carries the agency prefix (1_86736, 1_89529), while the informed entities inside carry raw IDs. So 1_1_86736 was real and 1_86736 is the right output. The one-source approach in TripSituations also matches BeanFactoryV2.getTripDetails, which builds situationIds and references.situations from the same bean list. One thing to fix: situationCollector dedupes on the agency-scoped ID rather than the alert ID, so at a stop served by a route from another agency the same alert can be emitted twice under two IDs. Details inline. The singular arrival-and-departure-for-stop also changed without a test, and it is one more row in the table you already wrote. |
An alert is reachable by more than one path. At a stop served by another agency's route, the same alert matches through both the route and the stop, and each path scoped its situation ID to the agency it happened to know. The alert then appeared in references.situations twice under two IDs, and an entry's situationIds resolved to only one of them. Take the agency from the alert's own informed entity, which does not vary by lookup path, and fall back to the caller's agency only for alerts that name none.
It was the only endpoint changed here with no case asserting that its situationIds resolve. Being the singular form, it needs a trip and a service date alongside the stop, so it takes the trip the stop is already drawn from and the pinned fixture date.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/restapi/situation_references_test.go (1)
66-78: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWait for the initial real-time feed before selecting the vehicle.
createTestApiWithRealTimeDatareturns before the first feed poll.anyRealTimeVehicleID(t, api)runs immediately on Line 78 and can fail before the endpoint checks. Wait until real-time vehicles are available beforeAddAlertForTestand vehicle selection, as ininternal/restapi/trips_for_location_handler_test.goLines 629-633.🤖 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/situation_references_test.go` around lines 66 - 78, Wait for the initial real-time feed to populate vehicles immediately after createTestApiWithRealTimeData and before AddAlertForTest and anyRealTimeVehicleID. Reuse the established wait pattern from trips_for_location_handler_test.go so vehicle selection only occurs once real-time data is available.
🤖 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/situation_references_test.go`:
- Around line 161-205: The test only validates each agency lookup independently
and does not verify deduplication when both results are combined. Extend
TestSituationRefsFromAlertsAgencyScope or add an endpoint-level assertion that
collects references from both caller agencies, then require exactly one
references.situations entry whose ID is the alert’s agency-qualified ID.
- Around line 120-126: Strengthen both resolution tests to assert the seeded
alert’s specific ID, not merely that any situation ID was emitted. In
internal/restapi/situation_references_test.go:120-126, require
25_situation-resolution-alert in emitted; in
internal/restapi/trips_for_location_handler_test.go:653-660, require
25_test-alert-trips-for-location among the collected entry situation IDs.
In `@internal/restapi/trips_for_route_handler.go`:
- Around line 337-340: Update the selected-trip handling around
resolveInterlinedEntryTripID so every resolution.SelectedTrip appended to
fetchedTrips is also indexed in tripsByID. Ensure its route agency is added to
routeAgencyMap when absent, preserving the prefetched lookup path used by
tripSituationRefs and avoiding fallback database queries.
---
Outside diff comments:
In `@internal/restapi/situation_references_test.go`:
- Around line 66-78: Wait for the initial real-time feed to populate vehicles
immediately after createTestApiWithRealTimeData and before AddAlertForTest and
anyRealTimeVehicleID. Reuse the established wait pattern from
trips_for_location_handler_test.go so vehicle selection only occurs once
real-time data is available.
🪄 Autofix
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: 4b0f5e79-504e-4645-acf2-8154daaa93e5
📒 Files selected for processing (4)
internal/restapi/reference_utils.gointernal/restapi/situation_references_test.gointernal/restapi/trips_for_location_handler_test.gointernal/restapi/trips_for_route_handler.go
The trip selected for an interlined entry was appended to the fetched trips but never added to the by-ID index, so looking up that entry's situations missed and fell back to querying the trip and its route again — for rows already in memory.
Asserting only that some situationId was emitted let an unrelated ID satisfy the test while the seeded alert went missing. Name the ID each test expects instead, and cover the case the agency scoping exists for: one alert reaching a collector down two paths must be recorded once. Also wait for the first real-time poll before picking a vehicle, which the sibling tests already do.
Truncating a UTC time to a day gives UTC midnight, which the handler reads in the agency's timezone as the previous calendar day. Build the date in the agency's own timezone so the case asks about the day it names.
burma-shave
reviewed
Aug 12, 2026
burma-shave
left a comment
burma-shave
left a comment
Collaborator
There was a problem hiding this comment.
Stage 2 code-quality pass on top of the situation-references fix. Three follow-ups on the redundant-lookup theme this PR already tackles, plus one narrow edge case to flag for awareness.
Stage 1 (spec-conformance) is settled separately: the idempotent agency-prefixing, the informed-entity agency-scoping, and the pre-existing situationIds absent-vs-empty-array behavior were all reviewed and are being kept as-is, documented in maglev.wiki's Implementation Decisions sections rather than changed here.
burma-shave
requested changes
Aug 12, 2026
Member
|
Hey @ARCoder181105 |
Four handlers changed on both sides: trip-for-vehicle now gates its references block on includeReferences, so the situation references are assigned inside that block, matching trip-details. buildTripsForLocationEntries took main's parameter list and its service-date-per-agency-timezone handling, and keeps returning the situations it collected. On a cancelled context it now sends the canceled response and returns no entries, as main does, rather than returning what it had built so far. trips-for-route keeps this branch's split of the reference builder into tripReferenceSets stages, and carries main's explicit tracking of trips that still need fetching into it: the set records the IDs directly instead of the builder inferring them from a zero-valued reference. The trips-for-route tests from both sides are kept, with main's buildTripReferences call updated to the tripReferenceParams signature.
BuildTripStatus resolved every trip's alerts to fill status.situationIds, then threw the resolved references away. Each handler that also needs the references for its own entry resolved the same trip a second time, so trip-details, trip-for-vehicle and arrival-and-departure-for-stop each ran two GetTrip+GetRoute pairs per request for one trip's situations. Return what was resolved alongside the status, in the same struct that now carries the block snapshot, and have those handlers reuse it. A handler that skipped the status has nothing to reuse and still resolves its own. GetSituationIDsForTrip existed only to strip the IDs off those references for the status; with the references reaching callers intact, the shared situationIDsFromRefs covers it and the second copy of that loop goes away.
The DUPLICATED loop probed for the base trip, discarded the record, looked up the trip's situations, and only then fetched the same trip again for the reference block. Because the trip was never indexed, every situation lookup fell through to a full route-and-agency query for a record the handler was about to hold anyway. Keep the record the probe already returned and index it before the lookup, the way the interlined path above does, which drops the second fetch.
A trip present in the index whose route was absent from routeAgencyMap scoped its situations to an empty agency, emitting the bare alert ID where every other ID in the response is combined-form — no error, just an ID a client cannot match against references.situations. Take the same fallback the unindexed branch already takes, which resolves the route and agency itself, rather than proceeding without an agency.
Collaborator
Author
|
@Ahmedhossamdev done resolved the merge conflict also addressed the requested changes ready for review |
ARCoder181105
added a commit
to ARCoder181105/maglev
that referenced
this pull request
Aug 14, 2026
Two resolutions worth noting. main now stamps each entry with agency-local midnight of the query day, which is the base this branch's resolver refines. Both halves of a resolver — the query day's midnight and the wall clock elapsed since it — are read in the agency's timezone, so trips-for-location now builds one resolver per agency rather than one per request. The DUPLICATED trip path collided with OneBusAway#1313's indexing fix, which landed the same lookup inline. Keep this branch's resolveDuplicatedBaseTrip extraction and index through it, so the situation lookup still reuses the record already in hand.
|
burma-shave
approved these changes
Aug 14, 2026
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.
Closes #1307.
situationIdsresolved to nothing on every endpoint that emits them. Three never populatedreferences.situationsat all; three populated it with raw alert IDs while emitting combined ones. The IDs were also double-prefixed — the feed ships1_92239, maglev published1_1_92239.Changes
TripSituationstoreference_utils.go: a trip's situation IDs and their references from one lookup, so the two cannot disagree.references.situationsontrips-for-location,trips-for-routeandtrip-for-vehicle, which never did; point the other three endpoints at the shared helper.1_92239, matching upstream.BuildTripStatusalready resolved, sotrip-details,trip-for-vehicleandarrival-and-departure-for-stopreuse them instead of resolving the same trip twice.GetSituationIDsForTripgoes with its last caller.trips-for-route: index a DUPLICATED trip before its situation lookup, and resolve the agency when a trip's route is absent fromrouteAgencyMaprather than emitting an unprefixed ID.Handler consistency
trip-for-vehiclegainedincludeReferenceshandling on main while this branch was open. Its situation references are assigned inside that gate, soincludeReferences=falsenow returns an emptysituationsarray along with the rest of the block — matchingtrip-details. The entry'ssituationIdsare unaffected.Deviation from Java
Java prefixes unconditionally in
GtfsRealtimeSource.createId, and every other ID in maglev goes throughFormCombinedID. The idempotent check here is deliberate — it is what makes maglev's output match upstream on the feeds that exist, which export alert IDs already prefixed. It has a known edge: an alert whose raw ID happens to begin with the agency ID plus an underscore is left unprefixed, a plausible shape for a numeric agency.Verification
Live against the King County feed with a Puget Sound alerts feed: every emitted
situationIdresolves on all six endpoints, and the IDs match upstream exactly (1_92239).TestSituationIDsResolveToReferencesasserts resolution across all six endpoints, including withincludeStatus=false;TestSituationRefsFromAlertsAgencyScopecovers the cross-agency ID;TestTripSituationRefsAgencyFallbackcovers the unmapped-route fallback.go vet(both tag sets) andmake testpass.Summary by CodeRabbit