🔥 feat: Added support for custom binding precedence#4544
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesCustom binding precedence
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RequestType
participant BindAll
participant BindingSources
RequestType->>BindAll: binding_source precedence tag
BindAll->>BindingSources: invoke sources in configured order
BindingSources-->>BindAll: populated destination fields
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
aryan262
changed the title
|
@aryan262 Sounds like a cool feature, but we definitely need documentation; otherwise, no one will know about it and therefore no one will use it. |
|
@ReneWerner87 I am new to this repo, how should I add documentation for this PR ? |
|
here is the location of the docs files and this is the result https://docs.gofiber.io/ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4544 +/- ##
==========================================
- Coverage 93.23% 93.20% -0.03%
==========================================
Files 140 140
Lines 14567 14607 +40
==========================================
+ Hits 13581 13615 +34
- Misses 614 619 +5
- Partials 372 373 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gaby
changed the title
gaby
changed the title
|
@aryan262 The linter workflow is failing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/api/bind.md (1)
74-98: 📐 Maintainability & Code Quality | 🔵 TrivialRun
make markdownbefore merge. This repository has amarkdowntarget that checks all**/*.mdfiles, includingdocs/api/bind.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api/bind.md` around lines 74 - 98, Run the repository’s markdown formatting or validation step with make markdown after updating the bind documentation, and resolve any reported formatting issues in docs/api/bind.md before merging.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api/bind.md`:
- Line 97: Update the Fiber precedence-cache note in Bind.All documentation to
remove the claim of zero-allocation efficiency. State only that precedence
resolution is cached per reflect.Type to reduce repeated parsing overhead.
---
Nitpick comments:
In `@docs/api/bind.md`:
- Around line 74-98: Run the repository’s markdown formatting or validation step
with make markdown after updating the bind documentation, and resolve any
reported formatting issues in docs/api/bind.md before merging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
aryan262
changed the title
|
@gaby can you check now if it's failing. I'll check |
|
Error: bind_test.go:2952:1: File is not properly formatted (gofmt) |
|
@gaby I think it was because of an extra line expected in that file. Fixed it |
gaby
left a comment
gaby
left a comment
There was a problem hiding this comment.
Thanks for picking up the binding_source TODO — the feature is legitimately useful and the per-reflect.Type caching is the right design: tag parsing never runs on the hot path after the first request per type, All() already guarantees a pointer-to-struct before getBindingPrecedence runs (so NumField() can't panic), and the cached slice is never mutated after store, so sharing it is safe.
That said, requesting changes for a few correctness/usability gaps before this merges:
Main issues (details inline):
- Invalid tag tokens are silently dropped — a typo silently changes which sources get bound, and an all-invalid tag silently falls back to the default order. This should fail loudly.
- Undocumented/untested semantics — omitted sources are never bound, duplicates bind twice, and first-tag-wins is field-order dependent when multiple fields carry the tag.
- Docs example leaks the marker field —
c.JSON(req)will emit"BindingSource":{}; needsjson:"-"at minimum. - Lint/style — the C-style
forloop will trip themodernizelinter, and the docs note should use a:::infoadmonition like the rest of the file. - Benchmarks — the PR checks the Benchmarks box but adds none; every existing
All()call now pays async.Map.Load, so it'd be good to show the default path didn't regress and add one for the custom path.
None of these are structural — the core approach can stay as-is. 👍
Generated by Claude Code
| switch strings.TrimSpace(p) { | ||
| case "uri": | ||
| precedence = append(precedence, sourceURI) | ||
| case "body": | ||
| precedence = append(precedence, sourceBody) | ||
| case "query": | ||
| precedence = append(precedence, sourceQuery) | ||
| case "header": | ||
| precedence = append(precedence, sourceHeader) | ||
| case "cookie": | ||
| precedence = append(precedence, sourceCookie) | ||
| } |
There was a problem hiding this comment.
Unknown source names are silently dropped here. A typo like binding_source:"qurey,header" will silently bind header-only, and if all tokens are invalid, precedence stays empty and All() silently falls back to the default order. Since this parse runs once per type (not on the hot path), it can afford to fail loudly — please add a default: case that surfaces an error for unrecognized tokens rather than misconfiguring request binding silently.
Related edge cases worth deciding explicitly (and pinning with tests):
- Omitted sources —
binding_source:"query"means body/header/cookie/URI are never bound at all. Plausibly intended, but it's undocumented. - Duplicates —
"query,query"runs the query binder twice.
Generated by Claude Code
| } | ||
|
|
||
| var precedence []bindSource | ||
| for i := 0; i < t.NumField(); i++ { |
There was a problem hiding this comment.
The modernize linter (enabled in .golangci.yml) will likely flag this C-style loop — repo style is range-over-int, as used in mergeStruct in this same file:
for i := range t.NumField() {Generated by Claude Code
| precedence = append(precedence, sourceCookie) | ||
| } | ||
| } | ||
| break |
There was a problem hiding this comment.
First-tag-wins is field-order dependent: if two fields declare conflicting binding_source tags, whichever field comes first silently wins. Consider erroring when multiple fields carry the tag. It's also worth documenting that only top-level fields are scanned — a tag inside an embedded struct's type is ignored.
Generated by Claude Code
| var sources []func(any) error | ||
| customPrecedence := getBindingPrecedence(outElem.Type()) | ||
|
|
||
| if len(customPrecedence) > 0 { | ||
| for _, source := range customPrecedence { | ||
| switch source { | ||
| case sourceURI: | ||
| sources = append(sources, b.URI) | ||
| case sourceBody: | ||
| if len(b.ctx.Request().Body()) > 0 && len(b.ctx.RequestCtx().Request.Header.ContentType()) > 0 { | ||
| sources = append(sources, b.Body) | ||
| } | ||
| case sourceQuery: | ||
| sources = append(sources, b.Query) | ||
| case sourceHeader: | ||
| sources = append(sources, b.Header) | ||
| case sourceCookie: | ||
| sources = append(sources, b.Cookie) | ||
| } | ||
| } | ||
| } else { | ||
| // Precedence: URL Params -> Body -> Query -> Headers -> Cookies | ||
| sources = append(sources, b.URI) | ||
|
|
||
| // Check if both Body and Content-Type are set | ||
| if len(b.ctx.Request().Body()) > 0 && len(b.ctx.RequestCtx().Request.Header.ContentType()) > 0 { | ||
| sources = append(sources, b.Body) | ||
| // Check if both Body and Content-Type are set | ||
| if len(b.ctx.Request().Body()) > 0 && len(b.ctx.RequestCtx().Request.Header.ContentType()) > 0 { | ||
| sources = append(sources, b.Body) | ||
| } | ||
| sources = append(sources, b.Query, b.Header, b.Cookie) | ||
| } |
There was a problem hiding this comment.
Two small things here:
- The default path lost its pre-sized slice literal (
[]func(any) error{b.URI}→var sources []func(any) error+ appends).sources := make([]func(any) error, 0, 5)above the branch would avoid growth reallocations in both paths. - The body-availability check (
len(b.ctx.Request().Body()) > 0 && len(...ContentType()) > 0) is now duplicated in both branches — extract it into a variable above theif.
Also note every existing All() caller now pays a sync.Map.Load + type assertion even for untagged structs. That's cheap, but since the PR description checks the Benchmarks box, it would be good to add a BenchmarkBind_All_CustomPrecedence and confirm the existing BenchmarkBind_All didn't regress.
Generated by Claude Code
| type CustomPrecedenceReq struct { | ||
| // Specify the priority for binding using the binding_source tag. | ||
| // In this example, query parameters take highest priority, followed by headers. | ||
| BindingSource struct{} `binding_source:"query,header,cookie,body,uri"` |
There was a problem hiding this comment.
The marker field leaks into serialization: this example ends with c.JSON(req), so the response will include "BindingSource":{}. The example should at least tag it json:"-":
BindingSource struct{} `binding_source:"query,header,cookie,body,uri" json:"-"`More broadly, requiring users to add a meaningless exported field to their request structs is an awkward shape for a public API — the implementation actually accepts the tag on any field (first one found wins), so it may be worth documenting that it can live on an existing bound field instead of a dedicated marker.
Generated by Claude Code
| }) | ||
| ``` | ||
|
|
||
| > **Note:** For maximum performance, Fiber caches the precedence resolution per `reflect.Type` to reduce repeated parsing overhead. |
There was a problem hiding this comment.
This file uses Docusaurus admonitions rather than blockquotes — please switch to:
:::info
For maximum performance, Fiber caches the precedence resolution per `reflect.Type` to reduce repeated parsing overhead.
:::This section should also spell out the semantics of partial lists (sources omitted from the tag are not bound at all) and what happens with unrecognized source names.
Generated by Claude Code
| } | ||
|
|
||
| // go test -run Test_Bind_All_CustomPrecedence | ||
| func Test_Bind_All_CustomPrecedence(t *testing.T) { |
There was a problem hiding this comment.
Good happy-path coverage (query beats header beats body, plus cache reuse via the second request). Missing cases worth adding:
uriandcookiepositions in a custom order — those two sources are never exercised.- A tag that omits sources, pinning the "omitted sources are skipped" behavior.
- Invalid/unknown tokens (currently a silent fallback to default order — whatever the resolution, it needs a test pinning it).
Generated by Claude Code
gaby
left a comment
gaby
left a comment
There was a problem hiding this comment.
Re-reviewed at 0370b26 — this update addresses all of the previous round's feedback: invalid tokens now error (with negative-result caching, nice touch), multiple tags error instead of being field-order dependent, duplicates are deduped, the modernize loop style is fixed, the slice is pre-sized and hasBody extracted, the docs example no longer leaks a marker field into JSON, the note uses :::info with the partial-list/unknown-source semantics spelled out, and the test/benchmark coverage is now thorough. 👏
I also verified locally (fork CI hasn't run yet):
- ✅ All
Test_Bind_All*tests pass. - ✅ No regression on the default path —
BenchmarkBind_Allactually improved slightly vsmain(59 → 57 allocs/op, ~2056 → ~1991 B/op), thanks to the pre-sized slice. - ❌ Lint will fail CI — golangci-lint v2.12.2 (the version pinned in
.github/workflows/lint.yml) reports 6 issues, all inbind_test.go: 5×errcheckon the bare.(*DefaultCtx)assertions, and 1×gofmton the benchmark'sUserstruct alignment. Both are mechanical fixes — details inline.
Requesting changes only for the two lint items; the remaining inline comments are optional nits (stale cache comment, empty-token error message, require.EqualError, and a non-blocking note about the error path skipping returnErr). Once lint is green this looks good to me.
Generated by Claude Code
| Name string `binding_source:"query,header,cookie,body,uri" query:"name" header:"x-name" cookie:"c-name" json:"name" uri:"name"` | ||
| } | ||
|
|
||
| ctx := app.AcquireCtx(&fasthttp.RequestCtx{}).(*DefaultCtx) |
There was a problem hiding this comment.
These bare type assertions fail the linter — errcheck runs with check-type-assertions: true and _test.go files aren't excluded from it. Verified with the CI-pinned golangci-lint v2.12.2:
bind_test.go:2924:9: Error return value is not checked (errcheck)
ctx := app.AcquireCtx(&fasthttp.RequestCtx{}).(*DefaultCtx)
Same finding on lines 2942, 2956, 2969, and 2981. The existing pattern in this file (e.g. line 1830) is:
ctx := app.AcquireCtx(&fasthttp.RequestCtx{}).(*DefaultCtx) //nolint:errcheck,forcetypeassert // not neededGenerated by Claude Code
| BindingSource struct{} `binding_source:"query,header,cookie,body,uri"` | ||
| SessionID string `json:"session_id" cookie:"session_id"` | ||
| Name string `query:"name" json:"name" form:"name"` | ||
| Email string `json:"email" form:"email"` | ||
| Role string `header:"X-User-Role"` | ||
| ID int `uri:"id" query:"id" json:"id" form:"id"` | ||
| } |
There was a problem hiding this comment.
This struct isn't gofmt-formatted (adding the longer BindingSource field changes the alignment of the whole block) — the gofmt formatter check in CI will fail on it:
BindingSource struct{} `binding_source:"query,header,cookie,body,uri"`
SessionID string `json:"session_id" cookie:"session_id"`
Name string `query:"name" json:"name" form:"name"`
Email string `json:"email" form:"email"`
Role string `header:"X-User-Role"`
ID int `uri:"id" query:"id" json:"id" form:"id"`Running gofmt -w bind_test.go fixes it.
Generated by Claude Code
| sources []bindSource | ||
| } | ||
|
|
||
| var bindingPrecedenceCache sync.Map // map[reflect.Type][]bindSource |
There was a problem hiding this comment.
Nit: this comment is stale — the cache now stores cachedPrecedence, not []bindSource:
var bindingPrecedenceCache sync.Map // map[reflect.Type]cachedPrecedenceGenerated by Claude Code
| case "cookie": | ||
| source = sourceCookie | ||
| default: | ||
| err := fmt.Errorf("unknown binding_source %q", sourceName) |
There was a problem hiding this comment.
Nit: a trailing comma or double comma (binding_source:"query,") now produces the somewhat cryptic error unknown binding_source "". Consider either skipping empty tokens after TrimSpace or special-casing the message so the user can tell it's a stray comma rather than a bad source name.
Generated by Claude Code
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Note (non-blocking): this error path bypasses b.returnErr(...), so a bad binding_source tag surfaces as a raw error (→ 500 via the default error handler) even in auto-handling mode, unlike other binding failures which become 400s. Since a malformed tag is a programmer error rather than a client error, a 500 is arguably the correct behavior — just flagging it so it's a deliberate choice; might be worth a short comment here saying so.
Generated by Claude Code
| req := new(InvalidPrecedenceReq) | ||
| err := ctx.Bind().All(req) | ||
| require.Error(t, err) | ||
| require.Equal(t, "unknown binding_source \"invalid\"", err.Error()) |
There was a problem hiding this comment.
Nit: prefer the dedicated testify assertion here (and at line 3080):
require.EqualError(t, err, `unknown binding_source "invalid"`)Generated by Claude Code
4 participants
Description
Currently, Fiber's ctx.Bind().All() method binds values from request sources using a strict, hardcoded precedence (URI params -> Body -> Query -> Headers -> Cookies). If conflicting data exists across these sources, developers have no mechanism to override which source takes priority on a per-struct basis.
This PR introduces support for a new struct tag, binding_source, which allows developers to define a custom precedence order for data binding. For example: binding_source:"query,header,cookie,body,uri".
To maintain Fiber's strict zero-allocation and high-performance standards, this implementation avoids parsing struct tags or splitting strings on the hot path for every request. Instead, it utilizes a highly optimized internal caching layer (bindingPrecedenceCache via sync.Map) to process and store the precedence configuration exactly once per reflect.Type.
Fixes # (issue)
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