align reference stop IDs with schedule combined IDs

[3rabiii]

Description

This PR resolves a data inconsistency in the trips-for-route response where the Stop IDs provided in data.references.stops did not match the combined ID format (AgencyID_StopID) used within the trip schedules. This mismatch prevented successful client-side lookups.

Changes Made

• Translation Map Strategy: Updated collectStopIDsFromSchedule to build a map[string]string acting as a translation dictionary, mapping bare database IDs to their fully combined schedule IDs.
• Reference Serialization: Modified buildTripReferences to accept this translation map, applying the combined IDs when constructing the models.Stop references.
• Database Compatibility: Ensured that GetStopsByIDs queries continue to use bare IDs, maintaining compatibility with the database schema.
• Handler Signature: Updated the early return path in the main handler to pass nil for the new map parameter to satisfy the Go compiler.
• Test Coverage: Updated all relevant collectStopIDsFromSchedule unit tests to reflect the new map type. Enhanced the integration test (TestTripsForRouteHandler_DifferentRoutes) to strictly assert that reference stop IDs are generated in the combined format (verifying the presence of _).

Closes: #1237

Summary by CodeRabbit

• Bug Fixes
  • Corrected trip route references so stop entries retain their complete stop identifiers.
  • Improved handling of stop references across routes and schedules.
  • Added validation to ensure referenced stop IDs are returned in the expected format.

[coderabbitai]

Warning

Review limit reached

@3rabiii, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 7 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8cb13cad-d1e9-470a-b56d-47984222f10a

📥 Commits

Reviewing files that changed from the base of the PR and between 7054607 and 49e25c6.

📒 Files selected for processing (2)

• internal/restapi/trips_for_route_handler.go
• internal/restapi/trips_for_route_handler_test.go

📝 Walkthrough

Walkthrough

The trips-for-route handler now preserves combined stop IDs when collecting schedule stops and constructing response references. Tests verify combined reference IDs and the updated stop-ID mapping behavior.

Changes

Stop reference consistency

Layer / File(s)	Summary
Collect and wire combined stop IDs internal/restapi/trips_for_route_handler.go	Stop collection maps bare stop IDs to their original combined IDs, and reference-building calls receive the mapping.
Emit and validate combined references internal/restapi/trips_for_route_handler.go, internal/restapi/trips_for_route_handler_test.go	Constructed reference stops use combined IDs, with tests covering identifier format, populated mappings, malformed IDs, nil schedules, and empty stop times.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• OneBusAway/maglev#1218: Modifies related trips-for-route reference-building call sites and conditional logic.

Suggested reviewers: arcoder181105, ahmedhossamdev, fletcherw

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: aligning reference stop IDs with combined schedule IDs.
Linked Issues check	✅ Passed	The handler now serializes reference stops with combined IDs to match schedule stop times, satisfying #1237.
Out of Scope Changes check	✅ Passed	The changes stay within the stop-ID consistency fix and related test updates, with no unrelated behavior changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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_test.go`:
- Around line 194-197: Update the assertions in the route reference stops test
to compare the deduplicated refs.Stops ID set exactly with the deduplicated
schedule stop ID set from data.list[].schedule.stopTimes[].stopId. Remove the
underscore-only validation and assert equality in both contents and cardinality
so incorrect combined IDs cannot pass.

🪄 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: 8aa67315-261b-4143-a969-74d13987dd51

📥 Commits

Reviewing files that changed from the base of the PR and between e550ee2 and 7054607.

📒 Files selected for processing (2)

• internal/restapi/trips_for_route_handler.go
• internal/restapi/trips_for_route_handler_test.go

[aaronbrethorst]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

[aaronbrethorst]

Approved on the merits. This is a real bug and the fix is well-chosen.

The spec says data.list[].schedule.stopTimes[].stopId is a combined stop ID and that full detail for it appears in data.references.stops — which only works if the two strings match. They didn't: the schedule emitted utils.FormCombinedID(agencyID, stopTime.StopID) while references.stops[].id carried the bare feed ID straight off the DB row. So a client doing the lookup the spec describes found nothing.

What I like about the approach: you carry the exact combined string the schedule already produced through the map, rather than re-deriving it. stopID = combinedID is a plain assignment, not a re-wrap, so there's no way to double-prefix — which is the obvious failure mode for a change like this. And keeping the DB lookup keyed on bare IDs is right; GetStopsByIDs needs the feed ID.

I also checked this leaves the endpoint consistent with its siblings — stops_for_route_handler.go, reference_utils.go, and trips_for_location_handler.go all emit utils.FormCombinedID(agencyID, stop.ID) for reference stop IDs, so this brings trips-for-route in line rather than inventing a local convention. And references.stops[].routeIds were already combined via the SQL, so there's no residual mismatch left inside the stop object.

Tightening the integration test to exact set equality between references.stops[].id and the deduped schedule stop IDs is the right assertion — much better than checking for the presence of an underscore.

Blocked only on the merge conflict. main has since wrapped both buildTripReferences call sites in an includeReferences guard (from #1216), which shifts them. The resolution is mechanical and doesn't change behavior, so no re-review needed — merge main in, resolve, and I'll merge this.

Two small notes for later, neither blocking:

• collectStopIDsFromSchedule is first-wins per bare stop ID. If two trips in the same result set belong to different agencies and share a stop, only one combined ID lands in references and the other agency's stopTimes[].stopId still won't resolve. Blocks are agency-scoped in practice so this is rare, and it's strictly better than the status quo where neither resolved.
• The stopID := stop.ID fallback is unreachable — stops comes solely from GetStopsByIDs(bareIDs) where bareIDs are exactly the map's keys, so the ok branch always fires. Harmless, but it's dead.

Also: the PR description still describes the older assertion (checking for the presence of _) rather than the exact-set-equality one you actually landed in f694603c. Worth updating so the commit history reads accurately.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud