Fix/trip for vehicle include references - #1281
Conversation
The spec omits the references block when includeReferences=false, and the reference server returns every reference array empty for that request. trip-for-vehicle ignored the parameter and returned a fully populated block, so callers asking to skip references still paid for the stop, route and trip lookups behind them. Gate the reference building on the existing ShouldIncludeReferences helper, already used by stop and routes-for-agency, and follow the same convention of leaving the block present but empty rather than dropping the key. Extract the building itself into buildTripForVehicleReferences so the guard does not indent sixty lines of the handler, and so the handler reads as the sequence of steps it performs.
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (1)
📝 WalkthroughWalkthroughThe trip-for-vehicle handler now conditionally builds references. When ChangesTrip reference handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
![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
🤖 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/trip_for_vehicle_handler_test.go`:
- Around line 179-190: Update the “includeReferences=false empties the
references block” test to assert the handler responds with HTTP 200, then
require all reference collections—including Situations and StopTimes—to be
non-nil while remaining empty. Preserve the existing TripID assertion and checks
for Agencies, Routes, Trips, and Stops.
🪄 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: 1f6910ed-5bc8-441d-960c-89a32bcea679
📒 Files selected for processing (2)
internal/restapi/trip_for_vehicle_handler.gointernal/restapi/trip_for_vehicle_handler_test.go
The includeReferences=false test discarded the response, so a 429 from the shared rate limiter would have satisfied every assertion in it: the error envelope carries no references at all, and assert.Empty is happy with nil. Require a 200 first. Also require each collection to be non-nil as well as empty, and cover situations and stopTimes alongside the four the handler populates. The block is meant to stay present and merely empty, so these assertions pin the wire format to [] rather than null for every collection, including the two nothing in this handler touches.
|
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
1 participant
Fixes: #1280
Summary by CodeRabbit