Skip to content

fix: implement vehicleId query parameter in trip-details#1061

Open
3rabiii wants to merge 2 commits into
OneBusAway:mainfrom
3rabiii:fix-trip-details_issue_1
Open

fix: implement vehicleId query parameter in trip-details#1061
3rabiii wants to merge 2 commits into
OneBusAway:mainfrom
3rabiii:fix-trip-details_issue_1

Conversation

@3rabiii

@3rabiii 3rabiii commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Description

This PR addresses (spec gap) by implementing support for the vehicleId query parameter in the trip-details endpoint.

Previously, the endpoint ignored this parameter entirely. Per the wiki spec, when vehicleId is provided, the endpoint must filter the results to only consider the block instance currently assigned to that specific vehicle.

Changes Included:

  • Parameter Parsing: Updated TripParams and parseTripParams to explicitly extract the vehicleId from the URL query.
  • Vehicle Validation: Added logic in tripDetailsHandler to extract the raw entity ID and verify the vehicle's existence via GtfsManager.GetVehicleByID().
  • 404 Not Found Handling: If the provided vehicle ID is malformed, or if no block location currently exists for that vehicle, the handler immediately short-circuits and returns an HTTP 404.
  • Status Building: Injected the resolved *gtfs.Vehicle into BuildTripStatus instead of passing nil.
  • Unit Tests: Added comprehensive test coverage in trip_details_handler_test.go verifying valid, unknown, and malformed vehicleId inputs.

fixes: #1052

Summary by CodeRabbit

  • New Features

    • Trip details API accepts an optional vehicleId query parameter to produce vehicle-aware trip status.
  • Bug Fixes

    • Returns 404 for unknown, malformed, or cross-agency vehicle IDs; returns 200 when a valid matching vehicle is provided.
  • Tests

    • Added unit and end-to-end tests covering vehicleId parsing and trip-details handler behavior.

@coderabbitai

coderabbitai Bot commented Jun 11, 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: 7012d4e6-5b5e-459a-bd84-76ef64e65f0b

📥 Commits

Reviewing files that changed from the base of the PR and between d7df3cb and 850fae3.

📒 Files selected for processing (1)
  • internal/restapi/trip_details_handler.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/restapi/trip_details_handler.go

📝 Walkthrough

Walkthrough

Adds support for the vehicleId query parameter to trip-details: parses and stores vehicleId in TripParams, resolves and validates the agency-prefixed vehicle via GTFS when present (returning 404 on failure), and passes the resolved vehicle into BuildTripStatus when status is included.

Changes

Vehicle ID parameter support

Layer / File(s) Summary
Parameter handling and vehicle resolution
internal/restapi/trip_details_handler.go
Adds VehicleID to TripParams, reads vehicleId from the request query, validates agency prefix, looks up the vehicle with GetVehicleByID, returns 404 for parse/agency/lookup failures, and when IncludeStatus is true passes the resolved vehicle to BuildTripStatus instead of nil.
Test coverage for vehicleId parameter
internal/restapi/trip_details_handler_test.go
Adds unit subtests for parsing/defaulting vehicleId and an end-to-end test TestTripDetailsHandlerWithVehicleId that asserts 404 for unknown/malformed vehicleIds and 200 for a valid vehicleId with a matching GTFS mock vehicle.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

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

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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 The title clearly summarizes the main change: implementing the vehicleId query parameter in the trip-details endpoint.
Linked Issues check ✅ Passed Code changes meet all primary requirements: vehicleId parameter parsing, vehicle validation with agency matching, 404 handling for missing vehicles, and status building with resolved vehicle.
Out of Scope Changes check ✅ Passed All changes directly relate to implementing the vehicleId query parameter feature described in issue #1052; no unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_details_handler.go`:
- Around line 161-169: The code extracts agency and rawVehicleID from
params.VehicleID via utils.ExtractAgencyIDAndCodeID but never validates the
extracted agency; update the handling in the block that calls params.VehicleID
so that after extracting agencyID and rawVehicleID you compare the extracted
agencyID to the trip's agency (e.g. trip.AgencyID) and if they differ call
api.sendNotFound(w, r) and return; only when agencies match proceed to call
api.GtfsManager.GetVehicleByID(rawVehicleID) and the existing nil/error checks.
🪄 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: 40fd7c4d-3482-4535-9614-733f70cc416e

📥 Commits

Reviewing files that changed from the base of the PR and between 9898b8f and d7df3cb.

📒 Files selected for processing (2)
  • internal/restapi/trip_details_handler.go
  • internal/restapi/trip_details_handler_test.go

Comment thread internal/restapi/trip_details_handler.go
@sonarqubecloud

Copy link
Copy Markdown

@3rabiii 3rabiii requested a review from burma-shave June 14, 2026 22:30
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.

trip-details: implement vehicleId query parameter

1 participant