feat: support yyyy-MM-dd_HH-mm-ss format for time parameter#1071
feat: support yyyy-MM-dd_HH-mm-ss format for time parameter#10713rabiii wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthrough
Changestrip-details time parameter datetime string support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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/trip_details_handler_test.go (1)
273-286: ⚡ Quick winAdd one non-UTC location test to cover the new agency-timezone parsing branch.
Line 276 calls
parseTripParams(req, true)withoutloc, so this only validates UTC fallback. Add a case passing a concrete location (for exampleAmerica/Los_Angeles) and assert the parsed instant matches that local wall-clock input.Suggested test addition
t.Run("time yyyy-MM-dd_HH-mm-ss format", func(t *testing.T) { req := httptest.NewRequest("GET", "/?time=2024-06-15_14-30-00", nil) params, errs := api.parseTripParams(req, true) assert.Nil(t, errs) require.NotNil(t, params.Time) assert.Equal(t, 2024, params.Time.Year()) assert.Equal(t, time.June, params.Time.Month()) assert.Equal(t, 15, params.Time.Day()) assert.Equal(t, 14, params.Time.Hour()) assert.Equal(t, 30, params.Time.Minute()) assert.Equal(t, 0, params.Time.Second()) }) + +t.Run("time yyyy-MM-dd_HH-mm-ss format with explicit location", func(t *testing.T) { + req := httptest.NewRequest("GET", "/?time=2024-06-15_14-30-00", nil) + loc, err := time.LoadLocation("America/Los_Angeles") + require.NoError(t, err) + + params, errs := api.parseTripParams(req, true, loc) + + assert.Nil(t, errs) + require.NotNil(t, params.Time) + assert.Equal(t, loc, params.Time.Location()) + assert.Equal(t, 14, params.Time.Hour()) +})🤖 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_details_handler_test.go` around lines 273 - 286, The current test case for the time yyyy-MM-dd_HH-mm-ss format only validates UTC fallback behavior by calling parseTripParams without a location parameter. Add a new test case that passes a concrete non-UTC location (such as America/Los_Angeles) to parseTripParams and verifies that the parsed time correctly reflects the local wall-clock values for that timezone. This will provide test coverage for the agency-timezone parsing branch that the current UTC-only test does not exercise.
🤖 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 72-87: The trip-details endpoint handler accepts both Unix
millisecond timestamps via strconv.ParseInt and datetime strings via
time.ParseInLocation, but the OpenAPI specification only documents the integer
format. Choose one approach: either remove the datetime string parsing logic
(the time.ParseInLocation call and associated error handling in the fieldErrors
validation) to restrict the implementation to integer-only, or update the
OpenAPI specification to document both the integer and string format support for
the time parameter. Ensure the implementation and OpenAPI spec remain
synchronized to prevent SDK generation and API conformance tool failures.
---
Nitpick comments:
In `@internal/restapi/trip_details_handler_test.go`:
- Around line 273-286: The current test case for the time yyyy-MM-dd_HH-mm-ss
format only validates UTC fallback behavior by calling parseTripParams without a
location parameter. Add a new test case that passes a concrete non-UTC location
(such as America/Los_Angeles) to parseTripParams and verifies that the parsed
time correctly reflects the local wall-clock values for that timezone. This will
provide test coverage for the agency-timezone parsing branch that the current
UTC-only test does not exercise.
🪄 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: e4ff93a2-6ade-4d40-8742-8afab5a2052c
📒 Files selected for processing (2)
internal/restapi/trip_details_handler.gointernal/restapi/trip_details_handler_test.go
| // Validate time — accepts either a Unix timestamp in milliseconds | ||
| // (e.g. "1718459400000") or a datetime in yyyy-MM-dd_HH-mm-ss format (e.g. "2024-06-15_14-30-00"). | ||
| if timeStr := r.URL.Query().Get("time"); timeStr != "" { | ||
| if timeMs, err := strconv.ParseInt(timeStr, 10, 64); err == nil { | ||
| timeParam := time.Unix(timeMs/1000, 0) | ||
| params.Time = &timeParam | ||
| } else { | ||
| fieldErrors["time"] = []string{"must be a valid Unix timestamp in milliseconds"} | ||
| timeLoc := time.UTC | ||
| if len(loc) > 0 && loc[0] != nil { | ||
| timeLoc = loc[0] | ||
| } | ||
| if timeParam, err := time.ParseInLocation("2006-01-02_15-04-05", timeStr, timeLoc); err == nil { | ||
| params.Time = &timeParam | ||
| } else { | ||
| fieldErrors["time"] = []string{"must be a valid Unix timestamp in milliseconds or a datetime in yyyy-MM-dd_HH-mm-ss format"} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify local OpenAPI contract shape for trip-details "time" query parameter.
# Read-only reconnaissance script.
set -euo pipefail
echo "== Locate OpenAPI files =="
fd -i 'openapi*.yml'
fd -i 'openapi*.yaml'
echo
echo "== Inspect trip-details time parameter in testdata/openapi.yml (if present) =="
if [ -f testdata/openapi.yml ]; then
rg -n -C4 'trip-details|name:\s*time|format:\s*int64|type:\s*integer' testdata/openapi.yml
else
echo "testdata/openapi.yml not found"
fi
echo
echo "== Inspect conformance tests referencing trip-details time param expectations =="
rg -n -C3 'trip-details|time.*parameter|int64|yyyy-MM-dd_HH-mm-ss' --type=goRepository: OneBusAway/maglev
Length of output: 50374
API contract mismatch: time parameter accepts string format not documented in OpenAPI spec.
The trip-details endpoint code (lines 72–87) accepts both Unix millisecond timestamps and yyyy-MM-dd_HH-mm-ss datetime strings, but the OpenAPI specification (testdata/openapi.yml, lines 752–757) defines time as type: integer, format: int64 only. This contract drift may cause SDK generation tools and API conformance checks to fail. Either restrict the implementation to integer-only or update the OpenAPI spec to document the string format support.
🤖 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_details_handler.go` around lines 72 - 87, The
trip-details endpoint handler accepts both Unix millisecond timestamps via
strconv.ParseInt and datetime strings via time.ParseInLocation, but the OpenAPI
specification only documents the integer format. Choose one approach: either
remove the datetime string parsing logic (the time.ParseInLocation call and
associated error handling in the fieldErrors validation) to restrict the
implementation to integer-only, or update the OpenAPI specification to document
both the integer and string format support for the time parameter. Ensure the
implementation and OpenAPI spec remain synchronized to prevent SDK generation
and API conformance tool failures.
Source: Coding guidelines
There was a problem hiding this comment.
Cool, but the correct workflow is to update the openapi.yml in the sdk repo first, so i will open an issue there to fix this
There was a problem hiding this comment.
@3rabiii, that makes sense — the SDK repo (OneBusAway/sdk-config) being the source of truth for the OpenAPI contract is the right approach. Updating the spec there first (or at least tracking it as a follow-up issue) keeps the contract and implementation in sync in the intended direction.
Would you like me to open a tracking issue in OneBusAway/sdk-config on your behalf? I can reference this PR as context and note that the time parameter for the trip-details endpoint should be updated from type: integer, format: int64 to also document the yyyy-MM-dd_HH-mm-ss string format.
🧠 Learnings used
Learnt from: ARCoder181105
Repo: OneBusAway/maglev PR: 787
File: internal/restapi/arrivals_and_departures_for_location.go:686-694
Timestamp: 2026-04-26T10:27:18.975Z
Learning: In OneBusAway/maglev `internal/restapi` Go handlers, it is acceptable (and intentional) to access `sql.NullString.String` and `sql.NullInt64.Int64` directly without checking `.Valid`. Treat Go zero values (`""` and `0`) as the correct wire-format defaults, matching the Java OBA output. Do not require `.Valid` guards for these fields in this layer; this should only be changed if the wire-format semantics are confirmed to differ.
1 participant
Description
This PR addresses by updating the shared
parseTripParamsfunction to accept thetimequery parameter as a calendar datetime string (yyyy-MM-dd_HH-mm-ss), as specified in the wiki.Changes Included:
timeparameter now natively accepts both Unix millisecond timestamps (preserving backwards compatibility) and human-readable datetime strings.time.ParseInLocationwith the agency's configured timezone. This ensures that a time requested as "14:30:00" is accurately evaluated in the agency's local time, not UTC.fieldErrorsmap to return a precise error message when an invalid format is provided.TestTripDetailsHandlerWithTimeParameterString) to verify the endpoint successfully returns HTTP 200 when using the new parameter format.Fixes: #1055
Summary by CodeRabbit
timeparameter now accepts datetime strings inyyyy-MM-dd_HH-mm-ssformat in addition to Unix timestamps in milliseconds.