fix(time): binding time with empty value#4103
Merged
Merged
Conversation
appleboy
approved these changes
May 21, 2025
appleboy
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR prevents ParseInt errors by filling empty time values with zero time before parsing and adds tests covering the new behavior for UNIX timestamps.
- Added an early check in
setTimeFieldto handle empty strings by settingtime.Time{}. - Removed the redundant post-switch empty-value block.
- Extended
TestMappingTimewith two new struct fields and assertions forunixandunixnano.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| binding/form_mapping.go | Inserted an early empty-string check and removed the lower duplicate |
| binding/form_mapping_test.go | Added ZeroUnixTime/ZeroUnixNanoTime fields and assertions |
Comments suppressed due to low confidence (2)
binding/form_mapping_test.go:189
- Add a test case for an empty string with a non-
unixformat (e.g.,time_format:"2006-01-02") to ensure zero-value behavior is consistent across all formats.
ZeroUnixTime time.Time `time_format:"unix"`
binding/form_mapping.go:400
- [nitpick] Consider trimming whitespace (e.g. strings.TrimSpace) before checking for empty to avoid treating " " as a valid timestamp.
if val == "" {
Comment on lines
+401
to
+402
| value.Set(reflect.ValueOf(time.Time{})) | ||
| return nil |
There was a problem hiding this comment.
The early return for empty values bypasses any time_location/time_utc tag processing; if users expect zero values in a specific time zone, consider applying tag-based location adjustments after setting zero time.
Suggested change
| value.Set(reflect.ValueOf(time.Time{})) | |
| return nil | |
| // Set to zero time after applying tag-based adjustments | |
| val = "0001-01-01T00:00:00Z" // Default zero time in RFC3339 format |
Member
|
testing fail so revert #4245 |
thinkerou
pushed a commit
that referenced
this pull request
May 22, 2025
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
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.
Fixes: #4098
In
setTimeField, when the stringvalis empty and thetimeFormatisunixorunixnano, an error occurs due tostrconv.ParseInthandling empty values. We can handle this by filling it with a zero value before parsing.master