Skip to content

Omit zero update times for vehicles for agency#1116

Open
3rabiii wants to merge 1 commit into
OneBusAway:mainfrom
3rabiii:fix-vehicles-for-agency-gap-8
Open

Omit zero update times for vehicles for agency#1116
3rabiii wants to merge 1 commit into
OneBusAway:mainfrom
3rabiii:fix-vehicles-for-agency-gap-8

Conversation

@3rabiii

@3rabiii 3rabiii commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR aligns the vehicles-for-agency endpoint with its spec by omitting lastUpdateTime and lastLocationUpdateTime when no real-time update has been received, rather than emitting a 0 or a fabricated server time.

Changes

  • Handler: Removed the fallback to api.Clock.Now() when a vehicle lacks a timestamp.
  • Outer Entry: Added omitzero tags to the exclusive VehicleStatus model fields.
  • Shared TripStatus: Implemented a custom MarshalJSON wrapper strictly within the vehicles-for-agency handler. This strips zero-valued time fields from the nested tripStatus for this endpoint only.

Why this approach?

As noted in the issue, applying a global omission tag to the shared models.TripStatus would break the API contract for other endpoints (e.g., trips-for-route) which explicitly require these fields to serialize as 0. This wrapper isolates the fix to vehicles-for-agency.

Testing

  • Added tests to assert raw JSON key absence for both the outer entry and the nested tripStatus.
  • Verified that existing tests for other endpoints sharing TripStatus remain unaffected.

Closes: #1115

Summary by CodeRabbit

  • Bug Fixes
    • Vehicle update timestamps are now omitted from API responses when no real GTFS-RT timestamp is available, instead of showing placeholder zero values.
    • The vehicle list response now handles timestamp fields consistently for both the main vehicle data and trip details.
    • Test coverage was expanded to verify timestamp fields appear only when present.

Per the vehicles-for-agency spec, lastUpdateTime and
lastLocationUpdateTime must be absent when no real-time data is
available. Previously, the handler forced these to the current
server time or emitted them as 0.

This commit does the following:
- Removes fallback clock times in the handler.
- Adds omitzero to the exclusive VehicleStatus fields.
- Uses a scoped custom MarshalJSON wrapper to omit zero times
  from the shared TripStatus exclusively for this endpoint.

This scoped approach guarantees zero impact on other endpoints
(like trips-for-route) that explicitly require a 0 value.
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 259f1939-f9ab-4426-aed0-ecb5fd43a20b

📥 Commits

Reviewing files that changed from the base of the PR and between 7db6201 and 28b7c3d.

📒 Files selected for processing (5)
  • internal/gtfs/gtfs_manager_mock.go
  • internal/models/vehicle.go
  • internal/models/vehicle_test.go
  • internal/restapi/vehicles_for_agency_handler.go
  • internal/restapi/vehicles_for_agency_handler_test.go

📝 Walkthrough

Walkthrough

Adds a NoTimestamp mock option for GTFS-RT vehicles, marks VehicleStatus time fields with omitzero JSON tags, and updates the vehicles-for-agency handler to omit lastUpdateTime/lastLocationUpdateTime (outer entry and nested tripStatus) when no real update timestamp exists, via a custom-marshaling response wrapper. Tests are updated and added accordingly.

Changes

Vehicles-for-agency timestamp omission

Layer / File(s) Summary
Mock vehicle NoTimestamp option
internal/gtfs/gtfs_manager_mock.go
Adds NoTimestamp field to MockVehicleOptions; MockAddVehicleWithOptions now sets Timestamp to nil when requested instead of always using current time.
VehicleStatus omitzero JSON tags
internal/models/vehicle.go, internal/models/vehicle_test.go
Adds omitzero to LastLocationUpdateTime/LastUpdateTime JSON tags; tests updated to expect field absence rather than zero values when unset.
Handler stops defaulting timestamps, wraps response marshal
internal/restapi/vehicles_for_agency_handler.go
Removes unconditional currentTime defaulting for outer and nested tripStatus time fields; introduces vehicleStatusResponse with custom MarshalJSON that strips zero-valued tripStatus time fields, wired via wrapVehicleStatusesForResponse.
Handler tests for timestamp presence/absence
internal/restapi/vehicles_for_agency_handler_test.go
Adds JSON helper functions and two tests confirming timestamps are omitted for NoTimestamp vehicles and present (non-zero) for vehicles with real timestamps, in both outer entries and tripStatus.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant VehiclesForAgencyHandler
  participant vehicleStatusResponse
  participant JSONEncoder

  Client->>VehiclesForAgencyHandler: GET /vehicles-for-agency
  VehiclesForAgencyHandler->>VehiclesForAgencyHandler: build VehicleStatus (skip currentTime default)
  VehiclesForAgencyHandler->>vehicleStatusResponse: wrapVehicleStatusesForResponse
  vehicleStatusResponse->>JSONEncoder: MarshalJSON
  JSONEncoder->>JSONEncoder: delete zero tripStatus time fields
  JSONEncoder-->>Client: response without zero timestamps
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • burma-shave
  • fletcherw
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title clearly states the main change: omitting zero update times for vehicles-for-agency.
Linked Issues check ✅ Passed The PR matches #1115: it omits both time fields when absent, keeps them present when set, and leaves other endpoints untouched.
Out of Scope Changes check ✅ Passed The changes are focused on the vehicles-for-agency timestamp behavior and its tests/mocks, with no unrelated feature work.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

@sonarqubecloud

Copy link
Copy Markdown

@3rabiii 3rabiii requested a review from burma-shave June 30, 2026 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[spec-gap] vehicles-for-agency: lastUpdateTime/lastLocationUpdateTime not "absent when zero"

1 participant