Fix: includeTrip / includeStatus interaction now matches the specification - #1277
Fix: includeTrip / includeStatus interaction now matches the specification#1277Ahmedhossamdev wants to merge 4 commits into
includeTrip / includeStatus interaction now matches the specification#1277Conversation
setupTestApiWithMockVehicle called MockAddTrip with the ID of a trip it had just read out of the fixture database. MockAddTrip issues an INSERT OR REPLACE, so the call replaced a real trip row with a stub, wiping its block_id, shape_id and service_id. The test database is shared across the whole package via testDbSetupOnce, so the damage persisted for every test that ran afterwards, and it grew with each additional caller of the helper: the replace moves the row to a new rowid, so the next caller read and clobbered a different trip. Two callers were enough to break TestGetFirstStopOfNextTripInBlock_WithBlockContinuation, which needs a trip whose block still has a second trip in it. The call was redundant to begin with: the trip already exists in the fixture database, which is where the helper read it from.
The spec adds the active trip to data.references from the status path, so it is present whenever the status block is; includeTrip only adds it a second time and therefore matters solely when includeStatus=false. Maglev populated references.trips only under includeTrip, and compensated by defaulting includeTrip to true where the spec defaults it to false. The two errors cancelled at default settings and diverged as soon as either flag was set: includeTrip=false&includeStatus=true returned no trip reference at all, leaving entry.status.activeTripId undereferenceable. Add the trip and its route to references whenever the status block is built, and give trip-for-vehicle its documented includeTrip=false default. parseTripParams took the per-endpoint default as a positional bool; a second one would have been unreadable at the call site, so the defaults now travel in a TripParamDefaults struct.
|
Warning Review limit reached
Next review available in: 42 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 (3)
📝 WalkthroughWalkthroughThe PR adds separate defaults for trip and schedule inclusion. The trip-for-vehicle handler now adds active trip and route references when status is present, resolves route references with error handling, and expands test coverage for these combinations. ChangesTrip reference handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant tripForVehicleHandler
participant parseTripParams
participant routeReferenceByID
Client->>tripForVehicleHandler: Send includeTrip and includeStatus
tripForVehicleHandler->>parseTripParams: Parse endpoint parameters
tripForVehicleHandler->>routeReferenceByID: Resolve the active trip route
routeReferenceByID-->>tripForVehicleHandler: Return route reference or error
tripForVehicleHandler-->>Client: Return trip and route references
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: 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/trip_for_vehicle_handler_test.go (1)
172-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the required route-reference contract.
The status-enabled test accepts any non-empty route list. Assert that
refs.Trips[0].RouteIDexists inrefs.Routes.The all-false and
includeStatus=false&includeTrip=falsecases assert only that trip references are empty. Assert that route references are also empty.As per coding guidelines: “Cover every new branch or condition with tests.”
Also applies to: 202-216
🤖 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/trip_for_vehicle_handler_test.go` around lines 172 - 187, The trip-for-vehicle handler tests need to verify route-reference behavior. In the status-enabled test, assert that the first trip reference’s RouteID exists in model.Data.References.Routes; in the all-false and includeStatus=false/includeTrip=false cases, assert model.Data.References.Routes is empty alongside the existing trip-reference assertions.Source: Coding guidelines
🤖 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_details_handler.go`:
- Around line 27-40: Reduce cognitive complexity in RestAPI.parseTripParams by
extracting its date/time parsing and boolean include-parameter parsing branches
into focused helper functions, while keeping endpoint-default initialization in
parseTripParams. Preserve the existing validation, defaults, and returned
TripParams/map behavior.
In `@internal/restapi/trip_for_vehicle_handler.go`:
- Around line 176-179: Correct the comment immediately above the condition in
the trip handler to state that the active trip is appended when IncludeTrip is
requested or when status is present, with IncludeTrip serving as the fallback
when no status block was created; remove the inaccurate claim that it adds a
second reference.
- Around line 192-196: Update the routeReferenceByID error handling in the trip
handler to call sendNotFound when errors.Is(err, sql.ErrNoRows), while retaining
serverErrorResponse for all other errors. Add a handler test covering the
missing-route lookup path and asserting a 404 response.
---
Outside diff comments:
In `@internal/restapi/trip_for_vehicle_handler_test.go`:
- Around line 172-187: The trip-for-vehicle handler tests need to verify
route-reference behavior. In the status-enabled test, assert that the first trip
reference’s RouteID exists in model.Data.References.Routes; in the all-false and
includeStatus=false/includeTrip=false cases, assert model.Data.References.Routes
is empty alongside the existing trip-reference assertions.
🪄 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: b3fad636-d68c-4b45-a552-b43ece104f81
📒 Files selected for processing (5)
internal/restapi/reference_utils.gointernal/restapi/trip_details_handler.gointernal/restapi/trip_details_handler_test.gointernal/restapi/trip_for_vehicle_handler.gointernal/restapi/trip_for_vehicle_handler_test.go
parseTripParams inlined two near-identical epoch-or-layout timestamp parses and three near-identical boolean parses, each two levels deep, which put its cognitive complexity at 43 against an allowed 15. Pull the repeated shapes into parseEpochOrLayoutTime, parseIncludeParam and localizeTripTimes, leaving parseTripParams to do endpoint-default initialization and field-error collection. The accepted formats, the error messages, the defaults and the returned TripParams are unchanged; in particular the parsed times are still only re-expressed in the agency's timezone when the caller supplies one.
The route lookup backing the active trip's reference sent every failure through serverErrorResponse, so a trip pointing at a route that is not in the database produced a 500. Elsewhere in this handler, and per the convention documented in CONTRIBUTING, sql.ErrNoRows is a 404 and only genuine query failures are 500s. Also correct the comment above the reference condition: it described the legacy behaviour of adding the trip twice, whereas this code appends it once, with includeTrip acting as the fallback when no status block was built.
|
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: #1276
Description
The
trip-for-vehicleendpoint did not follow the specification whenincludeTripandincludeStatuswere used together.According to Spec Step 4 and Extension 4b:
Maglev only populated
references.TripswhenincludeTrip=true:trip_for_vehicle_handler.go:169trip_details_handler.go:32To compensate, the endpoint defaulted
includeTrip=true, while the specification defines the default asfalse.These two incorrect behaviors cancelled each other out with the default request, but diverged as soon as either parameter was explicitly set.
Current behavior
Request:
data.referencesJava verification
Verified in
TripStatusBeanServiceImpl:data.references.includeTripBeanflag.Changes
status != nil, the active trip and its route are added todata.references.includeTripis nowfalse, matching the specification.parseTripParamstook its per-endpoint default as a positional bool. A second one would have been unreadable at the call site, so the defaults now travel in aTripParamDefaultsstruct:The new
routeReferenceByIDhelper lives inreference_utils.goand reuses the existingbuildRouteModels.Resulting behavior
data.referencesincludeStatus=true)includeTrip=false&includeStatus=trueincludeTrip=true&includeStatus=falseincludeTrip=false&includeStatus=falseTests
TestTripForVehicleHandler_IncludeToggles:includeTrip=false omits trip referencestest, since that behavior was incorrect.New
TestTripForVehicleHandler_TripReferencescovers the three-way interaction:includeTrip=falsestill yields trip refs via the status path.includeTrip=false&includeStatus=falseyields no trip or route references, matching Extension 4a.includeTrip=true&includeStatus=falseyields trip refs, which is the only case where the flag matters.These live in a separate test function rather than as extra subtests:
createTestApisetsRateLimit: 5, and six requests against one API instance returns429.Unrelated fix included
The first commit removes a
MockAddTripcall fromsetupTestApiWithMockVehicle.MockAddTripissues anINSERT OR REPLACE, and the helper passed it the ID of a trip it had just read out of the fixture database — replacing a real trip row with a stub and wiping itsblock_id,shape_idandservice_id. The test database is shared across the package viatestDbSetupOnce, so the damage persisted for every test that ran afterwards, and it grew with each caller: the replace moves the row to a new rowid, so the next caller read and clobbered a different trip.Two callers were enough to break
TestGetFirstStopOfNextTripInBlock_WithBlockContinuation, which needs a trip whose block still has a second trip in it. The call was redundant to begin with — the trip already exists in the fixture database, which is where the helper read it from.It is in this PR only because the new test function cannot pass without it.
Handler consistency
parseTripParamsis shared withtrip-details, whose spec does defaultincludeTriptotrue. That default is preserved and onlytrip-for-vehiclechanges, but the shared helper is worth a second look during review.Summary by CodeRabbit