-
Notifications
You must be signed in to change notification settings - Fork 8.5k
feat(binding): add support for encoding.UnmarshalText in uri/query binding #4203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4203 +/- ##
==========================================
- Coverage 99.21% 98.82% -0.40%
==========================================
Files 42 44 +2
Lines 3182 3484 +302
==========================================
+ Hits 3157 3443 +286
- Misses 17 30 +13
- Partials 8 11 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@appleboy ready for review. I verified locally that all my new changes are covered with the new tests I added. The linting failure is unrelated to my changes since I did not add any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for encoding.TextUnmarshaler-based binding via a new parser=encoding.TextUnmarshaler tag option.
- Introduce a
parseroption inform_mapping.goand implementtrySetUsingParser - Update
setByFormand related functions to invokeUnmarshalTextwhenparseris set - Enhance documentation (
docs/doc.md) with usage examples and notes
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/doc.md | Add new “Bind custom unmarshaler” section and notes for parser tag |
| binding/form_mapping.go | Parse parser tag, implement trySetUsingParser, wire into setter |
Comments suppressed due to low confidence (2)
binding/form_mapping.go:199
- There are no unit tests covering the new
parser=encoding.TextUnmarshalerbehavior. Consider adding tests for simple values, slices, and default-value scenarios to prevent regressions.
func trySetUsingParser(val string, value reflect.Value, parser string) (isSet bool, err error) {
docs/doc.md:1054
- The JSON example is missing a closing
}at the end; add the trailing brace to make the output valid JSON.
{"Birthday":"2000/01/01","Birthdays":["2000/01/01","2000/01/02"],"BirthdaysDefault":["2020/09/01","2020/09/02"]
| if ok, err = trySetUsingParser(vs[0], value, opt.parser); ok { | ||
| return ok, err | ||
| } else if ok, err = trySetCustom(vs[0], value); ok { | ||
| return ok, err |
Copilot
AI
May 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The pattern of invoking trySetUsingParser followed by trySetCustom is repeated in multiple branches. Extracting this into a small helper would reduce duplication and improve readability.
| if ok, err = trySetUsingParser(vs[0], value, opt.parser); ok { | |
| return ok, err | |
| } else if ok, err = trySetCustom(vs[0], value); ok { | |
| return ok, err | |
| if ok, err = trySetValue(vs[0], value, opt); ok { | |
| return ok, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only repeated once. Extracting it to a function will make this code even more difficult to understand than it already is
|
Please rebase the master branch. @takanuva15 |
|
@appleboy done, just rebased it now |
|
@takanuva15 Please help to resolve the conflicts. |
d37dc58 to
ae06b4d
Compare
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for the parser=encoding.TextUnmarshaler tag in form/URI binding so users can opt in to encoding.TextUnmarshaler-based decoding.
- Updated docs with examples and behavior notes for the new
parsertag. - Extended
binding/form_mapping.goto parse aparseroption, addedtrySetUsingParser, and wired it into slice, array, and scalar binding. - Broadened the error message for unsupported
collection_formatto list all valid formats.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/doc.md | Added UnmarshalText examples, notes on parser behavior |
| binding/form_mapping.go | Introduced parser field in setOptions, trySetUsingParser, and updated binding logic |
Comments suppressed due to low confidence (2)
binding/form_mapping.go:199
- No unit tests cover the new
parser=encoding.TextUnmarshalerpath; please add tests for both successful and failure scenarios when the target type does and does not implementTextUnmarshaler.
func trySetUsingParser(val string, value reflect.Value, parser string) (isSet bool, err error) {
docs/doc.md:1106
- The JSON example is missing a closing
}. Please add the final brace to make the example valid JSON.
{"Birthday":"2000/01/01","Birthdays":["2000/01/01","2000/01/02"],"BirthdaysDefault":["2020/09/01","2020/09/02"]
| if ok, err = trySetUsingParser(vs[0], value, opt.parser); ok { | ||
| return ok, err | ||
| } else if ok, err = trySetCustom(vs[0], value); ok { | ||
| return ok, err |
Copilot
AI
May 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The pattern of calling trySetUsingParser and then trySetCustom is repeated in multiple branches. Consider unifying this sequence into a helper to reduce duplication and improve clarity.
| if ok, err = trySetUsingParser(vs[0], value, opt.parser); ok { | |
| return ok, err | |
| } else if ok, err = trySetCustom(vs[0], value); ok { | |
| return ok, err | |
| if ok, err = trySetValue(vs[0], value, opt.parser); ok { | |
| return ok, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only repeated once. Extracting it to a function will make this code even more difficult to understand than it already is.
Other comments;
- test coverage: form_mapping.go is at 100% test coverage, which is the only file I edited
- doc json mistake: fixed it and repushed
|
I will take it. |

With this PR, users can now specify
parser=encoding.TextUnmarshalerin their form/uri tag to enable automatic binding using theencoding.TextUnmarshalerinterface from the Golang standard library. Since this new parser tag is 100% opt-in only, this is fully backwards-compatible with previous versions of gin.Merging this PR will resolve a number of tickets regarding this functionality:
UnmarshalTextfrom Golangencoding.TextUnmarshalerfor URI and Query param binding #4177closes #4177
closes #4312 (please correct me if I'm wrong)
Sample code to run:
Test it with:
curl 'localhost:8088/test?birthday=2000-01-01&birthdays=2000-01-01,2000-01-02'Result
{"Birthday":"2000/01/01","Birthdays":["2000/01/01","2000/01/02"],"BirthdaysDefault":["2020/09/01","2020/09/02"]