fix: implement ageInSeconds strict staleness filter#1108
Conversation
Parse the ageInSeconds query parameter to filter out stale vehicles. Unlike the legacy Java system, ageInSeconds=0 now strictly filters for exact-match timestamps instead of silently disabling the filter. Absent or negative values bypass the filter to maintain expected logic. Updated MockVehicleOptions to accept custom timestamps and added tests covering positive, zero, negative, and absent parameter states.
|
Warning Review limit reached
More reviews will be available in 16 minutes and 27 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ChangesVehicle age filtering
Sequence Diagram(s)sequenceDiagram
participant Client
participant vehiclesForAgencyHandler
participant api.Clock
Client->>vehiclesForAgencyHandler: GET /vehicles-for-agency?ageInSeconds=...
vehiclesForAgencyHandler->>api.Clock: Now()
vehiclesForAgencyHandler->>vehiclesForAgencyHandler: parse ageInSeconds and filter stale vehicles
vehiclesForAgencyHandler-->>Client: filtered vehicles response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
internal/restapi/vehicles_for_agency_handler_test.go (1)
283-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an exact-cutoff inclusion assertion to the zero-second test.
This currently proves that stale vehicles are excluded, but it would still pass if
ageInSeconds=0incorrectly dropped vehicles updated exactly atapi.Clock.Now(), which is the key boundary in the PR objective.Proposed test addition
func TestVehiclesForAgencyHandler_AgeInSecondsZeroFiltersStrictly(t *testing.T) { api := createTestApiWithClock(t, clock.NewMockClock(ageFilterClock)) defer api.Shutdown() t.Cleanup(api.GtfsManager.MockResetRealTimeData) trip := mustGetTrip(t, api) + exactTS := ageFilterClock staleTS := ageFilterClock.Add(-10 * time.Minute) + api.GtfsManager.MockAddVehicleWithOptions("v_exact_zero", trip.ID, trip.RouteID, gtfs.MockVehicleOptions{ + Timestamp: &exactTS, + }) api.GtfsManager.MockAddVehicleWithOptions("v_stale_zero", trip.ID, trip.RouteID, gtfs.MockVehicleOptions{ Timestamp: &staleTS, }) params := url.Values{"ageInSeconds": {"0"}} _, model := callAPIHandler[VehiclesForAgencyResponse](t, api, vehiclesForAgencyURL(testdata.Raba.ID, params)) + assert.True(t, vehiclesForAgencyContainsID(model.Data.List, "v_exact_zero"), + "ageInSeconds=0 must retain vehicles updated at the exact reference time") assert.False(t, vehiclesForAgencyContainsID(model.Data.List, "v_stale_zero"), "ageInSeconds=0 must apply a strict cutoff and exclude stale vehicles") }🤖 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/vehicles_for_agency_handler_test.go` around lines 283 - 301, Update TestVehiclesForAgencyHandler_AgeInSecondsZeroFiltersStrictly in vehicles_for_agency_handler_test.go to also assert that a vehicle with a timestamp exactly equal to api.Clock.Now() is included when ageInSeconds is 0. Use createTestApiWithClock, api.GtfsManager.MockAddVehicleWithOptions, and vehiclesForAgencyContainsID to add a boundary-case vehicle alongside the stale one, then verify the exact-cutoff vehicle remains in the response while v_stale_zero is still excluded.
🤖 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/vehicles_for_agency_handler.go`:
- Around line 43-45: The age-based cutoff calculation in vehicles filtering can
overflow when converting a large parsed `ageInSeconds` into a `time.Duration`,
which can invert the cutoff and break request filtering. Update the logic in the
handler that parses the age parameter and computes `cutoff` to validate or bound
the parsed value before multiplying by `time.Second`, and only apply the filter
when the conversion is safe; use the existing `strconv.Atoi` parsing path and
the `referenceTime`/`cutoff` computation in `vehiclesForAgencyHandler` as the
place to fix it.
---
Nitpick comments:
In `@internal/restapi/vehicles_for_agency_handler_test.go`:
- Around line 283-301: Update
TestVehiclesForAgencyHandler_AgeInSecondsZeroFiltersStrictly in
vehicles_for_agency_handler_test.go to also assert that a vehicle with a
timestamp exactly equal to api.Clock.Now() is included when ageInSeconds is 0.
Use createTestApiWithClock, api.GtfsManager.MockAddVehicleWithOptions, and
vehiclesForAgencyContainsID to add a boundary-case vehicle alongside the stale
one, then verify the exact-cutoff vehicle remains in the response while
v_stale_zero is still excluded.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9001043f-29c4-48e2-9344-12839f58506e
📒 Files selected for processing (3)
internal/gtfs/gtfs_manager_mock.gointernal/restapi/vehicles_for_agency_handler.gointernal/restapi/vehicles_for_agency_handler_test.go
Calculating the time.Duration for the staleness cutoff can overflow if a client sends an excessively large ageInSeconds value. Switch to ParseInt and apply a maximum bound to prevent the duration from wrapping to a negative value. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
1 participant
Summary
Fixes a spec gap by implementing the
ageInSecondsstaleness filter for thevehicles-for-agencyendpoint.Problem
The handler previously ignored the
ageInSecondsparameter, returning all tracked vehicles regardless of age. Additionally, the legacy Java implementation contained a documented defect whereageInSeconds=0silently disabled the filter instead of applying a strict 0-second cutoff.Changes
ageInSecondsparameter.ageInSeconds=0now explicitly applies a 0-second staleness cutoff.TimestamptoMockVehicleOptionsingtfs_manager_mock.goto allow deterministic testing of vehicle age.Closes #1103
Summary by CodeRabbit
ageInSecondsfilter to the vehicles-for-agency response, letting results show only vehicles updated within the requested time window.