[bugfix] correctly differentiate between nil / present-but-empty slices#4498
[bugfix] correctly differentiate between nil / present-but-empty slices#4498NyaaaWhatsUpDoc wants to merge 2 commits intogin-gonic:masterfrom
Conversation
| if cfTag == "" || cfTag == "multi" { | ||
| vs = strings.Split(opt.defaultValue, ",") | ||
| } else { | ||
| vs = []string{opt.defaultValue} |
There was a problem hiding this comment.
this isn't part of the fix (nor is it in the array part below), but it just means vs only needs to get written to once instead of twice in the case of cfTag == "" | cfTag == "multi"
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4498 +/- ##
==========================================
- Coverage 99.21% 99.01% -0.21%
==========================================
Files 42 46 +4
Lines 3182 3042 -140
==========================================
- Hits 3157 3012 -145
- Misses 17 20 +3
- Partials 8 10 +2
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:
|
|
@NyaaaWhatsUpDoc Please fix the conflicts. |
There was a problem hiding this comment.
Pull request overview
This PR fixes form binding so that slice fields correctly distinguish between "parameter not present" (nil slice) and "parameter present but empty" (non-nil, zero-length slice), including when defaults and collection_format are involved.
Changes:
- Adjusted
setByFormslice handling to treatvs == nil(no key) differently fromlen(vs) == 0(key present but empty), and to only apply defaults when the field is absent. - Updated default-value handling for slices and arrays with
collection_formattags to keep existing semantics while integrating the new nil vs empty behavior. - Expanded and updated
TestMappingEmptyValues(and related collection format tests) to assert the new behavior for slices with/without defaults and with collection formats.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
binding/form_mapping.go |
Refines slice and array form-binding logic to differentiate missing vs empty values, and correctly construct default vs based on collection_format. |
binding/form_mapping_test.go |
Updates expectations and adds coverage to verify nil vs empty slice behavior across defaulted, non-defaulted, and collection-format scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NyaaaWhatsUpDoc
force-pushed
the
bugfix/form-array-binding-regression
branch
from
f2443d1 to
47a2602
Compare
|
@appleboy merge conflicts fixed! |
| FileData []customPathUnmarshalText `form:"path,default=bar/foo;bar/foo/spam,parser=encoding.TextUnmarshaler" collection_format:"csv"` | ||
| } | ||
| err := mappingByPtr(&s, formSource{"path": {}}, "form") | ||
| err := mappingByPtr(&s, formSource{"path": nil}, "form") |
There was a problem hiding this comment.
previously this was relying on an empty (but not nil!) value slice to rely on default, but an empty slice isn't the zero value. a nil slice is.
| FileData []objectIDUnmarshalText `form:"ids,default=664a062ac74a8ad104e0e80e;664a062ac74a8ad104e0e80f,parser=encoding.TextUnmarshaler" collection_format:"csv"` | ||
| } | ||
| err := mappingByPtr(&s, formSource{"ids": {}}, "form") | ||
| err := mappingByPtr(&s, formSource{"ids": nil}, "form") |
There was a problem hiding this comment.
previously this was relying on an empty (but not nil!) value slice to rely on default, but an empty slice isn't the zero value. a nil slice is.
2 participants
This fixes the regression introduced in v1.11.0 which stopped differentiating between nil and zero length (but not nil) slices when binding form values.
Pull Request Checklist
Please ensure your pull request meets the following requirements:
masterbranch.docs/doc.md.Thank you for contributing!