🔥 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)
Assessment against linked issues
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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. |
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3373 +/- ##
==========================================
+ Coverage 84.00% 84.16% +0.16%
==========================================
Files 119 119
Lines 11897 11939 +42
==========================================
+ Hits 9994 10049 +55
+ Misses 1475 1462 -13
Partials 428 428
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.
- 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.
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",
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.
|
@edvardsanta PR is ready for review or ? can I switch from draft to normal? |
|
then I would take a look at it tomorrow |
|
Sure, I can move it from draft to open. Just a heads-up that there are a few technical debts in the current implementation: What's implemented
Technical Debts
If you're okay with that, i will openn the PR now. Feel free to create a follow-up issue for addressing these debts. I'd like to improve it later on, but since I just started at a new company, I’m a bit stretched for time right now. PS: Just realized I wrote this in the issue instead of the PR, i thought I was already commenting on it |
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new All method for the Bind struct that unifies data binding from multiple sources into a single struct, along with accompanying documentation, tests, and benchmarks.
- Added the All method in Bind to process URL parameters, body, query, headers, and cookies in a defined precedence order.
- Updated API docs with usage examples and added comprehensive tests and a benchmark for the new binding functionality.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/api/bind.md | Added documentation for the new All method with clear examples. |
| bind_test.go | Introduced test cases and benchmarks to validate the new All method. |
| bind.go | Implemented the All method along with helper functions mergeStruct and isZero. |
Comments suppressed due to low confidence (1)
bind.go:296
- [nitpick] Consider renaming 'tempStruct' to 'sourceStruct' or a similarly descriptive name to clarify its role in holding the temporary binding results.
tempStruct := reflect.New(outElem.Type()).Interface()
* feat: Add All method to Bind 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. * feat: Enhance Bind.All with comprehensive testing and configuration 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. * fix: Correct form binding in Test_Bind_All * refactor: Improve Bind.All test and struct field ordering - 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. * feat: Document Bind.All function in API documentation 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). * docs: lint Bind.All documentation * fix: Update parameter tags from 'param' to 'uri' in bind_test.go * fix: Update parameter tags from 'param' to 'uri' in bind.md * test: Replace assert with require in bind_test.go * feat: Add support for unified binding with defined precedence order in whats_new.md --------- Co-authored-by: RW <[email protected]>
* main: Update AGENTS.md Create AGENTS.md 🐛 bug: fix redirection flash messages violate cookie structure (gofiber#3457) build(deps): bump codecov/codecov-action from 5.4.2 to 5.4.3 🚀 Improve routing treeBuild flow (gofiber#3456) build(deps): bump github.com/tinylib/msgp from 1.2.5 to 1.3.0 (gofiber#3447) build(deps): bump DavidAnson/markdownlint-cli2-action from 19 to 20 🔥 feat: Add All method to Bind (gofiber#3373) 📝 docs: Document usage of Custom Tags in Logger middleware (gofiber#3446)
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