🔥 feat: Add All method to Bind #3373
Conversation
This commit introduces a new `All` method to the `Bind` struct, enabling the binding of request data from multiple sources (URI parameters, body, query parameters, headers, and cookies) into a single struct. The `All` method iterates through the available binding sources, applying them in a predefined precedence order. It merges the values from each source into the output struct, only updating fields that are currently unset. Changes: - Added `All` method to `Bind` struct. - Added `mergeStruct` helper function to merge struct values. - Added `isZero` helper function to check if a value is zero. - Added a test case for the `All` method in `bind_test.go` to validate its functionality.
WalkthroughA new method, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bind
participant URI
participant Body
participant Query
participant Header
participant Cookie
Client->>Bind: All(out)
Bind->>URI: Bind URI params
Bind->>Body: Bind body
Bind->>Query: Bind query params
Bind->>Header: Bind headers
Bind->>Cookie: Bind cookies
Bind-->>Client: Populated struct (with precedence)
Possibly related issues
Suggested labels
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
edvardsanta
changed the title
|
@ReneWerner87 please let me know if you have any feedback, suggestions, or improvements for the code. I’ll make any necessary adjustments and work on improving the tests when I have some time. |
bind_test.go
Outdated
| @@ -1885,3 +1885,52 @@ func Test_Bind_RepeatParserWithSameStruct(t *testing.T) { | |||
| testDecodeParser(MIMEApplicationForm, "body_param=body_param") | |||
| testDecodeParser(MIMEMultipartForm+`;boundary="b"`, "--b\r\nContent-Disposition: form-data; name=\"body_param\"\r\n\r\nbody_param\r\n--b--") | |||
| } | |||
|
|
|||
| func TestBind_All(t *testing.T) { | |||
The changes include: - Added `RequestConfig` struct to encapsulate request configuration (ContentType, Body, Headers, Cookies, Query). - Implemented `ApplyTo` method on `RequestConfig` to apply the configuration to the context. - Created multiple test cases for `Bind.All` covering successful binding, missing fields, overriding query parameters, and form binding. - Added a test case `Test_Bind_All_Uri_Precedence` to validate the precedence of URI parameters. - Added benchmark test `BenchmarkBind_All` to measure the performance of the `Bind.All` method. - Refactored the `TestBind_All` to use the new `RequestConfig` and assertion libraries.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
bind.go (2)
299-309: Remove the initial blank line inside theforblock to satisfyrevive/empty-lineslint rule
golangci‑lintfails withextra empty line at the start of a block.
A one‑line diff fixes it:for _, bindFunc := range sources { - + // attempt bind from the current source if err := bindFunc(tempStruct); err != nil { return err }🧰 Tools
🪛 GitHub Check: lint
[failure] 301-301:
empty-lines: extra empty line at the start of a block (revive)🪛 GitHub Actions: golangci-lint
[error] 301-301: golangci-lint (revive): extra empty line at the start of a block (empty-lines)
314-327:mergeStructcopies only top‑level zero fields – nested structs remain partially emptyBecause the function checks
isZeroon the destination field as a whole,
a nested struct such asAddress{City, Zip}will never receive theCity
coming from a later source once any field insideAddresshas a
non‑zero value.
If deep merging is important, consider recursing into nested structs or
requiring pointer fields.Not blocking, but worth documenting in godoc to avoid surprises.
bind_test.go (2)
1919-2041: Run each table‑driven sub‑test in parallel and userequirefor fatal assertions
- Add
t.Parallel()inside the sub‑test to comply with thetparallel
lint suggestion and speed up the suite.- Replace
assert.NoErrorwithrequire.NoErrorso the test aborts
immediately on failure, keeping subsequent checks safe.t.Run(tt.name, func(t *testing.T) { + t.Parallel() bind := newBind(app) @@ - err := bind.All(tt.out) - if tt.wantErr { - assert.Error(t, err) + err := bind.All(tt.out) + if tt.wantErr { + require.Error(t, err) return } - assert.NoError(t, err) + require.NoError(t, err)🧰 Tools
🪛 GitHub Check: lint
[failure] 2027-2027:
require-error: for error assertions use require (testifylint)
[failure] 1949-1949:
fieldalignment: struct with 48 pointer bytes could be 40 (govet)
[failure] 1921-1921:
fieldalignment: struct with 72 pointer bytes could be 64 (govet)
[failure] 1919-1919:
Test_Bind_All's subtests should call t.Parallel (tparallel)
1891-1917:RequestConfig.ApplyToleaves previous request state untouched – add a resetBecause the same
ctxis reused, fields that are not overridden by the
currentRequestConfig(query string, cookies, headers, …) survive from
earlier test cases, leading to flakiness (see wrong ID above).Insert a reset at the top of
ApplyTo:func (rc *RequestConfig) ApplyTo(ctx Ctx) { + ctx.Request().Reset() if rc.Body != nil {(This doesn’t affect production code – it’s only a test helper.)
🧰 Tools
🪛 GitHub Check: lint
[failure] 1891-1891:
fieldalignment: struct with 64 pointer bytes could be 56 (govet)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bind.go(2 hunks)bind_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
bind_test.go (2)
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
🪛 GitHub Check: lint
bind.go
[failure] 301-301:
empty-lines: extra empty line at the start of a block (revive)
bind_test.go
[failure] 2076-2076:
fieldalignment: struct with 72 pointer bytes could be 64 (govet)
[failure] 2046-2046:
fieldalignment: struct with 32 pointer bytes could be 24 (govet)
[failure] 2027-2027:
require-error: for error assertions use require (testifylint)
[failure] 1949-1949:
fieldalignment: struct with 48 pointer bytes could be 40 (govet)
[failure] 1921-1921:
fieldalignment: struct with 72 pointer bytes could be 64 (govet)
[failure] 1919-1919:
Test_Bind_All's subtests should call t.Parallel (tparallel)
[failure] 1891-1891:
fieldalignment: struct with 64 pointer bytes could be 56 (govet)
🪛 GitHub Actions: golangci-lint
bind.go
[error] 301-301: golangci-lint (revive): extra empty line at the start of a block (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Compare
- GitHub Check: repeated
- GitHub Check: Analyse
bind.go
Outdated
| func (b *Bind) All(out any) error { | ||
| outVal := reflect.ValueOf(out) | ||
| if outVal.Kind() != reflect.Ptr || outVal.Elem().Kind() != reflect.Struct { | ||
| return ErrUnprocessableEntity | ||
| } | ||
|
|
||
| outElem := outVal.Elem() | ||
|
|
||
| // Precedence: URL Params -> Body -> Query -> Headers -> Cookies | ||
| sources := []func(any) error{ | ||
| b.URI, | ||
| b.Body, | ||
| b.Query, | ||
| b.Header, | ||
| b.Cookie, | ||
| } | ||
|
|
||
| tempStruct := reflect.New(outElem.Type()).Interface() | ||
|
|
||
| // TODO: Support custom precedence with an optional binding_source tag | ||
| // TODO: Create WithOverrideEmptyValues | ||
| // Bind from each source, but only update unset fields | ||
| for _, bindFunc := range sources { | ||
|
|
||
| if err := bindFunc(tempStruct); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| tempStructVal := reflect.ValueOf(tempStruct).Elem() | ||
| mergeStruct(outElem, tempStructVal) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle “no body” and similar benign errors so All doesn’t abort early
b.Body() (and the other single‑source helpers) can legitimately return
ErrUnprocessableEntity, ErrEmptyBody, etc. when the incoming request
simply doesn’t contain data for that source.
All should treat those as non‑fatal and continue with the next source;
otherwise a perfectly valid request that, say, only has query params will
fail the whole bind.
- if err := bindFunc(tempStruct); err != nil {
- return err
- }
+ if err := bindFunc(tempStruct); err != nil {
+ // Skip “nothing to bind”‑style errors and continue.
+ if errors.Is(err, ErrUnprocessableEntity) ||
+ errors.Is(err, binder.ErrEmptyBody) {
+ continue
+ }
+ return err
+ }(You may need to import errors and, if available, the concrete “empty body”
error from the binder package.)
🧰 Tools
🪛 GitHub Check: lint
[failure] 301-301:
empty-lines: extra empty line at the start of a block (revive)
🪛 GitHub Actions: golangci-lint
[error] 301-301: golangci-lint (revive): extra empty line at the start of a block (empty-lines)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3373 +/- ##
==========================================
+ Coverage 83.87% 84.03% +0.16%
==========================================
Files 119 119
Lines 11892 11934 +42
==========================================
+ Hits 9974 10029 +55
+ Misses 1488 1475 -13
Partials 430 430
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:
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
bind_test.go (1)
2008-2008:⚠️ Potential issueTest expectation for Form binding needs correction.
When testing Form binding, with
id=2in the body, the expected ID should be 2, not 1. This appears to be a carryover from the previous test that had different data.- ID: 1, + ID: 2,This also highlights a potential issue with request context reuse between tests. It would be clearer to explicitly reset or create a new context for each test case.
🧹 Nitpick comments (4)
bind_test.go (4)
1891-1897: Well-designed RequestConfig struct for test configuration.This utility struct provides a clean way to configure request parameters for tests. The design is comprehensive, covering all the request components needed for the binding tests.
However, the struct could be optimized for memory efficiency:
type RequestConfig struct { - ContentType string - Body []byte - Headers map[string]string - Cookies map[string]string - Query string + Body []byte + Headers map[string]string + Cookies map[string]string + ContentType string + Query string }The static analysis indicates field alignment issues - reordering fields from largest to smallest can improve memory efficiency.
🧰 Tools
🪛 GitHub Check: lint
[failure] 1891-1891:
fieldalignment: struct with 64 pointer bytes could be 56 (govet)
2074-2112: Good benchmark implementation for the All method.The benchmark properly measures the performance of the
Allmethod with realistic data and all binding sources. This addresses the previous request for benchmarks.Similar to the
Userstruct in the tests, this one also has field alignment issues:type User struct { - ID int `param:"id" query:"id" json:"id" form:"id"` - Avatar *multipart.FileHeader `form:"avatar"` - Name string `query:"name" json:"name" form:"name"` - Email string `json:"email" form:"email"` - Role string `header:"x-user-role"` - SessionID string `json:"session_id" cookie:"session_id"` + Avatar *multipart.FileHeader `form:"avatar"` + ID int `param:"id" query:"id" json:"id" form:"id"` + Name string `query:"name" json:"name" form:"name"` + Email string `json:"email" form:"email"` + Role string `header:"x-user-role"` + SessionID string `json:"session_id" cookie:"session_id"` }🧰 Tools
🪛 GitHub Check: lint
[failure] 2076-2076:
fieldalignment: struct with 72 pointer bytes could be 64 (govet)
1921-1928: User struct efficiently captures binding from multiple sources.The struct correctly uses tags for different binding sources (param, query, json, form, header, cookie) which demonstrates the capability of the new
Allmethod.The field ordering could be optimized for memory efficiency (pointer fields first):
type User struct { - ID int `param:"id" query:"id" json:"id" form:"id"` - Avatar *multipart.FileHeader `form:"avatar"` - Name string `query:"name" json:"name" form:"name"` - Email string `json:"email" form:"email"` - Role string `header:"x-user-role"` - SessionID string `json:"session_id" cookie:"session_id"` + Avatar *multipart.FileHeader `form:"avatar"` + ID int `param:"id" query:"id" json:"id" form:"id"` + Name string `query:"name" json:"name" form:"name"` + Email string `json:"email" form:"email"` + Role string `header:"x-user-role"` + SessionID string `json:"session_id" cookie:"session_id"` }🧰 Tools
🪛 GitHub Check: lint
[failure] 1921-1921:
fieldalignment: struct with 72 pointer bytes could be 64 (govet)
2046-2050: User struct in URI precedence test is appropriately designed.The struct correctly includes tags for different binding sources to test precedence rules.
Similar to the other structs, field alignment could be optimized:
type User struct { - ID int `param:"id" json:"id" query:"id" form:"id"` - Name string `json:"name"` - Email string `json:"email"` + ID int `param:"id" json:"id" query:"id" form:"id"` + Name string `json:"name"` + Email string `json:"email"` }🧰 Tools
🪛 GitHub Check: lint
[failure] 2046-2046:
fieldalignment: struct with 32 pointer bytes could be 24 (govet)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bind_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
bind_test.go (2)
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
🪛 GitHub Check: lint
bind_test.go
[failure] 2076-2076:
fieldalignment: struct with 72 pointer bytes could be 64 (govet)
[failure] 2046-2046:
fieldalignment: struct with 32 pointer bytes could be 24 (govet)
[failure] 2027-2027:
require-error: for error assertions use require (testifylint)
[failure] 1949-1949:
fieldalignment: struct with 48 pointer bytes could be 40 (govet)
[failure] 1921-1921:
fieldalignment: struct with 72 pointer bytes could be 64 (govet)
[failure] 1919-1919:
Test_Bind_All's subtests should call t.Parallel (tparallel)
[failure] 1891-1891:
fieldalignment: struct with 64 pointer bytes could be 56 (govet)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (5)
bind_test.go (5)
13-13: Good addition of the required import.Adding the
stringspackage is necessary for the new test functionality, particularly for the request body creation inTest_Bind_All_Uri_Precedence.
19-19: Appropriate use of testify/assert package.Using the
assertpackage from testify aligns with the project's testing conventions as indicated in the retrieved learnings.
1899-1916: Clean implementation of ApplyTo method.The method correctly applies all configuration properties to the request context in a logical order. The null checks prevent errors when optional fields aren't provided.
2043-2072: Good test for URI parameter precedence.This test properly verifies that URI parameters take precedence over query parameters and JSON body when binding data, which is an important behavior of the new
Allmethod.The test correctly sets up the scenario:
- URI parameter:
id=111- Query parameter:
id=888- JSON body:
{"id": 999, ...}And it verifies that the URI parameter value (111) is chosen.
🧰 Tools
🪛 GitHub Check: lint
[failure] 2046-2046:
fieldalignment: struct with 32 pointer bytes could be 24 (govet)
1946-1946: Test query parameter setup could be improved.In the
defaultConfigfunction, the query ID duplicates the ID from the JSON body.Since the test is verifying binding behavior, consider making the IDs different to better validate the precedence rules:
- Query: "id=1&name=john", + Query: "id=2&name=john",This would make it clearer which source is being used when checking the bound value.
bind_test.go
Outdated
| assert.Error(t, err) | ||
| return | ||
| } | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use require for error assertions instead of assert.
The codebase consistently uses require for error assertions to stop test execution on failure.
- assert.Error(t, err)
+ require.Error(t, err)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: lint
[failure] 2027-2027:
require-error: for error assertions use require (testifylint)
- Reordered fields in `RequestConfig` and `User` structs for field alignment - Updated `Test_Bind_All` to use `require.NoError` for more robust error checking. - Corrected header key casing in `Test_Bind_All` to `X-User-Role` to match the struct tag. - Added `t.Parallel()` to the test function to enable parallel test execution.
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/fxamacker/cbor/v2" | ||
| "github.com/gofiber/fiber/v3/binder" | ||
| "github.com/stretchr/testify/assert" |
There was a problem hiding this comment.
Use testify.require() instead of assert()
| @@ -273,3 +275,57 @@ func (b *Bind) Body(out any) error { | |||
| // No suitable content type found | |||
| return ErrUnprocessableEntity | |||
| } | |||
|
|
|||
| func (b *Bind) All(out any) error { | |||
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an All method for the Bind struct to enable the merging of request data from multiple sources (URI, body, query, headers, and cookies), following a defined precedence order. Key changes include adding a new All method and mergeStruct helper in bind.go and comprehensive unit tests in bind_test.go covering various binding scenarios.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| bind_test.go | Adds tests for the new All method with various binding cases. |
| bind.go | Implements the All method and mergeStruct helper to combine data from multiple sources. |
Comments suppressed due to low confidence (1)
bind_test.go:1945
- [nitpick] Consider adding test cases where fields with valid zero values (such as numeric fields intentionally set to 0) are bound, to verify that the merge logic handles them correctly.
Query: "id=1&name=john",
| b.Cookie, | ||
| } | ||
|
|
||
| tempStruct := reflect.New(outElem.Type()).Interface() |
There was a problem hiding this comment.
The temporary struct (tempStruct) is created once outside the loop and then reused for each binding source. This may cause previous binding values to carry over into subsequent bindings. Consider reinitializing tempStruct inside the loop for each source to ensure isolated binding.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
The purpose of tempStruct is to act as a temporary holder for each source binding (such as URI, Body, Query, etc.) and merge it into the final outElem struct.
| srcField := src.Field(i) | ||
|
|
||
| // Skip if the destination field is already set | ||
| if isZero(dstField.Interface()) { |
There was a problem hiding this comment.
Using isZero to decide whether to update a field might unintentionally overwrite valid zero values (e.g., 0 for an int). Reevaluate the logic for what constitutes an 'unset' field, possibly by using pointer types or an explicit flag.
| if isZero(dstField.Interface()) { | |
| if (dstField.Kind() == reflect.Ptr && dstField.IsNil()) || (dstField.Kind() != reflect.Ptr && isZero(dstField.Interface())) { |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
This commit adds documentation for the `Bind.All` function to the API documentation. The documentation includes: - A description of the function's purpose and precedence order for binding data from different sources (URI, body, query, headers, cookies).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/api/bind.md (2)
446-451: Clarify merging semantics
The description and precedence list explain the order of sources but don’t explicitly state thatAllonly sets zero‑valued fields and preserves values from higher‑precedence binders. Consider adding a note, for example:“By design,
Allwill not override non‑zero fields set by earlier binders; it only populates fields that remain at their zero value.”
462-462: Fix code fence formatting
The example block is opened withgo(with a space). For consistency, it should bego(no space) to correctly highlight as Go code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/api/bind.md(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (1)
docs/api/bind.md (1)
28-28: Binder list updated to include All
The newAllbinder is now listed under the Binders section. Placing it after theURIbinder aligns with its broad, catch‑all scope.
| Name string `query:"name" json:"name" form:"name"` | ||
| Email string `json:"email" form:"email"` | ||
| Role string `header:"X-User-Role"` | ||
| SessionID string `json:"session_id" cookie:"session_id"` | ||
| ID int `param:"id" query:"id" json:"id" form:"id"` | ||
| } |
There was a problem hiding this comment.
Correct struct tag for URI parameters
The example struct uses param:"id", but the URI binder expects the uri tag (as shown in earlier sections). Update it to uri:"id" so that All will bind path parameters correctly.
Description
This pull request introduces a new
Allmethod in theBindstruct to unify data binding from multiple sources (URL parameters, body, query, headers, and cookies) into a single struct. It also includes corresponding documentation updates, test cases, and a benchmark for the new method.New
Allmethod inBindstruct:Allmethod: Added a newAllmethod in theBindstruct to bind data from multiple sources into a struct pointer. It processes the sources in a predefined precedence order and ensures that only unset fields are updated. Helper functionsmergeStructandisZerowere added to support this functionality.Testing and benchmarking:
Allmethod: Introduced comprehensive test cases for theAllmethod, including scenarios for invalid output types, partial JSON, precedence handling, and different content types (e.g., JSON, form data). Added aRequestConfighelper struct to simplify test setup.Allmethod: Added a benchmark test to measure the performance of theAllmethod under repeated binding operations.Documentation updates:
Allmethod: Updated the API documentation to include theAllmethod, detailing its usage, precedence order, and an example.These changes enhance the flexibility and usability of the
Bindstruct by consolidating multiple data sources into a single binding operation while maintaining clear documentation and robust testing.Fixes #3363
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md