Fix: Support Unix millisecond timestamps for schedule-for-stop date parameter#1035
Open
ARCoder181105 wants to merge 6 commits into
Open
Fix: Support Unix millisecond timestamps for schedule-for-stop date parameter#1035ARCoder181105 wants to merge 6 commits into
ARCoder181105 wants to merge 6 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 49 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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)
📝 WalkthroughWalkthroughThe PR makes ChangesDate Format Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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: 3
🧹 Nitpick comments (2)
internal/utils/validation_test.go (1)
441-445: ⚡ Quick winAdd test cases for Unix timestamp edge cases.
While the valid Unix timestamp test is good, consider adding test cases for edge cases such as negative timestamps, extremely large values, and zero. This would ensure the validation properly handles out-of-range inputs.
📝 Suggested additional test cases
{ name: "valid unix timestamp in milliseconds", date: "1749686400000", wantErr: false, }, + { + name: "negative unix timestamp", + date: "-1000000000", + wantErr: true, + errMsg: expectedErrMsg, + }, + { + name: "unix timestamp far in future", + date: "9999999999999", + wantErr: true, + errMsg: expectedErrMsg, + }, { name: "empty date is valid", date: "", wantErr: false, },🤖 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/utils/validation_test.go` around lines 441 - 445, The existing test case "valid unix timestamp in milliseconds" in validation_test.go lacks edge coverage; add additional cases in the tests slice used by the Test... function (the table-driven tests around name/date/wantErr) that include negative timestamp (e.g., "-1000"), zero ("0"), and an extremely large value (e.g., a value beyond int64 range or a very large millisecond value) with appropriate expected wantErr booleans; update the test table entries so functions that validate Unix timestamps (the test cases referencing the date field in the table-driven test) exercise these edge cases to ensure out-of-range handling is covered.internal/restapi/schedule_for_stop_handler_test.go (1)
87-101: ⚡ Quick winStrengthen test to validate actual date value for YYYY-MM-DD format.
The current test only checks
assert.NotNil(entry["date"])but doesn't validate that the date field contains the expected Unix millisecond value. Validating the actual value would catch bugs like incorrect date parsing or formatting issues (such as the variable shadowing issue identified in the handler).📝 Proposed enhancement
t.Run("Valid date parameter in format YYYY-MM-DD", func(t *testing.T) { // NOTE: Hardcoded date 2025-06-12 used for test consistency with GTFS data validity endpoint := "/api/where/schedule-for-stop/" + stopID + ".json?key=TEST&date=2025-06-12" resp, model := serveApiAndRetrieveEndpoint(t, api, endpoint) assert.Equal(t, http.StatusOK, resp.StatusCode) assert.Equal(t, http.StatusOK, model.Code) assert.Equal(t, "OK", model.Text) data, ok := model.Data.(map[string]any) assert.True(t, ok) entry, ok := data["entry"].(map[string]any) assert.True(t, ok) assert.NotNil(t, entry["date"]) + // Validate that the date field contains the expected Unix millisecond timestamp + // 2025-06-12 00:00:00 in the agency timezone + assert.Equal(t, float64(1749686400000), entry["date"]) })🤖 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/schedule_for_stop_handler_test.go` around lines 87 - 101, Update the test to assert the actual Unix millisecond value for the date instead of only checking non-nil: parse "2025-06-12" with time.Parse("2006-01-02", "2025-06-12"), compute expectedMillis := parsedTime.UnixMilli(), extract the returned entry map from model.Data (the same entry variable used in this test) and compare entry["date"] (convert the JSON number to the appropriate numeric type) to expectedMillis using assert.Equal so the test fails if the handler returns an incorrectly parsed or formatted date.
🤖 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/schedule_for_stop_handler.go`:
- Line 66: Update the error message for the "date" field in the
schedule_for_stop_handler.go error map to exactly match the validation utility
string from utils.ValidateDate: "invalid date format, use YYYY-MM-DD or a Unix
millisecond integer" (note the lowercase 'i' and comma). Locate the map entry
that sets "date": {"Invalid date format. Use YYYY-MM-DD or a Unix millisecond
integer"} and replace its value with the exact text returned by
utils.ValidateDate so messages are consistent across ValidateDate and the
handler.
- Around line 54-76: The YYYY-MM-DD branch is shadowing parsedDate by using
"parsedDate, err := time.ParseInLocation(...)" which leaves the outer parsedDate
zero-valued; change that to assign to the existing variables (use "=" not ":=")
so the outer parsedDate and err are updated, ensuring subsequent uses
(targetDate = parsedDate.Format(...) and weekday =
strings.ToLower(parsedDate.Weekday().String())) use the correctly parsed date;
update the ParseInLocation call in schedule_for_stop_handler.go accordingly and
keep the existing handling of date and date = parsedDate.UnixMilli().
In `@internal/utils/validation.go`:
- Around line 110-113: The code currently accepts any int64 from
strconv.ParseInt(date, 10, 64) without range checks; change this to parse the
value into a variable (e.g., v, err := strconv.ParseInt(...)) then validate that
v is within reasonable Unix-millisecond bounds (for example minUnixMillis :=
int64(0) to disallow negative/pre-epoch values and maxUnixMillis :=
int64(32503680000000) which corresponds to year 3000) and return an error when v
< minUnixMillis or v > maxUnixMillis; add descriptive error text and consider
extracting the bounds as named constants to clarify intent.
---
Nitpick comments:
In `@internal/restapi/schedule_for_stop_handler_test.go`:
- Around line 87-101: Update the test to assert the actual Unix millisecond
value for the date instead of only checking non-nil: parse "2025-06-12" with
time.Parse("2006-01-02", "2025-06-12"), compute expectedMillis :=
parsedTime.UnixMilli(), extract the returned entry map from model.Data (the same
entry variable used in this test) and compare entry["date"] (convert the JSON
number to the appropriate numeric type) to expectedMillis using assert.Equal so
the test fails if the handler returns an incorrectly parsed or formatted date.
In `@internal/utils/validation_test.go`:
- Around line 441-445: The existing test case "valid unix timestamp in
milliseconds" in validation_test.go lacks edge coverage; add additional cases in
the tests slice used by the Test... function (the table-driven tests around
name/date/wantErr) that include negative timestamp (e.g., "-1000"), zero ("0"),
and an extremely large value (e.g., a value beyond int64 range or a very large
millisecond value) with appropriate expected wantErr booleans; update the test
table entries so functions that validate Unix timestamps (the test cases
referencing the date field in the table-driven test) exercise these edge cases
to ensure out-of-range handling is covered.
🪄 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: 5760160c-daa0-4e9a-ada5-0e95fe882817
📒 Files selected for processing (4)
internal/restapi/schedule_for_stop_handler.gointernal/restapi/schedule_for_stop_handler_test.gointernal/utils/validation.gointernal/utils/validation_test.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/restapi/schedule_for_stop_handler_test.go (1)
71-74: 💤 Low valueConsider handling errors from time parsing for clearer test failures.
Lines 71-72 ignore errors from
time.LoadLocationandtime.ParseInLocation. While the test expects these to succeed, handling errors would provide clearer diagnostics if the test setup is incorrect.🧪 Proposed improvement for error handling
- loc, _ := time.LoadLocation(agencies[0].Timezone) - parsedTime, _ := time.ParseInLocation("2006-01-02", "2025-06-12", loc) + loc, err := time.LoadLocation(agencies[0].Timezone) + assert.NoError(t, err, "Should load agency timezone") + parsedTime, err := time.ParseInLocation("2006-01-02", "2025-06-12", loc) + assert.NoError(t, err, "Should parse test date") expectedMillis := float64(parsedTime.UnixMilli()) assert.Equal(t, expectedMillis, entry["date"])🤖 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/schedule_for_stop_handler_test.go` around lines 71 - 74, The test currently ignores errors from time.LoadLocation and time.ParseInLocation (variables loc and parsedTime) which can hide setup failures; update the test to check and fail on those errors (e.g., use t.Fatalf or require.NoError) after calling time.LoadLocation and time.ParseInLocation, then compute expectedMillis from parsedTime and assert.Equal as before so failures show clear diagnostics.
🤖 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.
Nitpick comments:
In `@internal/restapi/schedule_for_stop_handler_test.go`:
- Around line 71-74: The test currently ignores errors from time.LoadLocation
and time.ParseInLocation (variables loc and parsedTime) which can hide setup
failures; update the test to check and fail on those errors (e.g., use t.Fatalf
or require.NoError) after calling time.LoadLocation and time.ParseInLocation,
then compute expectedMillis from parsedTime and assert.Equal as before so
failures show clear diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e035ee79-a2c4-447f-a486-aba5655da1bb
📒 Files selected for processing (4)
internal/restapi/schedule_for_stop_handler.gointernal/restapi/schedule_for_stop_handler_test.gointernal/utils/validation.gointernal/utils/validation_test.go
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Updates the
schedule-for-stopendpoint to accept Unix millisecond integers for thedateparameter, resolving an issue where onlyYYYY-MM-DDformatted strings were supported. This brings the endpoint into full compliance with the API specification.Changes Made
utils.ValidateDateto accept valid 64-bit integers (Unix milliseconds) in addition to the standardYYYY-MM-DDformat.scheduleForStopHandlerto parse the incoming integer timestamp, convert it to a timezone-aware date using the agency's location, and echo it back correctly in the response.validation_test.goandschedule_for_stop_handler_test.goto verify integer parsing, error handling for invalid strings, and correct JSON property mapping.Closes #1029
Summary by CodeRabbit
New Features
Bug Fixes / Validation