🔥 feat: add BindError type with source and field metadata#4120
Conversation
|
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:
WalkthroughAdds a typed Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as Handler
participant Binder as Binder
participant Decoder as Decoder
Client->>Handler: HTTP request
Handler->>Binder: Bind().All(&target)
Binder->>Decoder: Decode URI
Decoder-->>Binder: success / error
alt URI error
Binder->>Binder: wrap -> BindError{Source: "uri", Field: ...}
Binder-->>Handler: *BindError
else URI success
Binder->>Decoder: Decode Body
Decoder-->>Binder: success / error
alt Body error
Binder->>Binder: wrap -> BindError{Source: "body", Field: ...}
Binder-->>Handler: *BindError
else Body success
Binder->>Decoder: Decode Query/Header/Cookie...
Decoder-->>Binder: success / error
alt any error
Binder->>Binder: wrap -> BindError{Source: "...", Field: ...}
Binder-->>Handler: *BindError
else
Binder-->>Handler: nil (bound)
end
end
end
Handler-->>Client: response (may inspect BindError via errors.As)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves error handling for data binding operations by introducing a new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement by adding the BindError type, which provides structured error information for binding failures. This change enhances error handling capabilities for developers using Fiber. The implementation is well-executed, with comprehensive test coverage and updated documentation. My main feedback is to consider refactoring the duplicated error-wrapping logic in the various Bind methods into a shared helper function to improve code maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
binder/mapping.go (1)
120-120: Avoid re-wrapping the decode error inparseToStruct.Line 120 wraps the same error with
fmt.Errorf("%w", err). Returningerrdirectly avoids an unnecessary wrapper and keeps the original error as the top-level value.♻️ Suggested change
- return fmt.Errorf("%w", err) + return err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@binder/mapping.go` at line 120, The function parseToStruct currently re-wraps the decode error by returning fmt.Errorf("%w", err); change this to return the original err directly (i.e., return err) so the original error value and its type are preserved; locate the return inside parseToStruct where fmt.Errorf("%w", err) is used and replace it with returning err without additional wrapping.bind.go (1)
220-225: Extract the repeated parse-error wrapping block into a helper.The same
errors.As(..., *Error)+newBindError(source, err)pattern is repeated across all bind methods. A shared helper would reduce duplication and keep behavior consistent.Also applies to: 239-245, 259-265, 278-284, 297-303, 316-322, 334-340, 355-361, 373-379, 392-398, 418-424
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bind.go` around lines 220 - 225, Extract the repeated pattern that checks errors.As(err, &Error) and returns either the original error or newBindError(source, err) into a helper method on the binder (e.g., func (b *binder) wrapParseError(err error, source BindSource) error) and replace the inline blocks (currently surrounding calls like b.returnErr(bind.Bind(b.ctx.Request(), out))) with calls to that helper; the helper should accept the parse error, use errors.As to detect *Error and return it unchanged, otherwise return newBindError(source, err), and update all occurrences that currently use BindSourceHeader and other BindSource constants to call this new method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bind_test.go`:
- Around line 114-121: The test handler registered with app.Get("/user/:id")
uses require.* assertions inside the route handler (calling require.Error,
require.ErrorAs, require.Equal), which the linter forbids; change those
in-handler assertions to use the testing.T methods (e.g., t.Fatalf/t.Errorf) or
capture the error/state to assert outside the handler: when calling
ctx.Bind().URI(new(U)) handle the returned err by using t.Fatalf/t.Errorf and
check error types (BindError, BindSourceURI, MultiError) via t.Errorf or
t.Fatalf inside the handler, or store err in a shared variable/channel and
perform require.* assertions after invoking the handler in the test body
instead.
- Around line 67-184: The failing tests call t.Parallel() inside subtests while
reusing the same app and Ctx (variables app and c obtained via
app.AcquireCtx(...)), causing races; fix by creating a fresh app and context per
subtest (move New() and app.AcquireCtx(...) into each t.Run closure) or remove
t.Parallel() from those subtests so each uses the shared c safely; update
references in Test_BindError_FieldExtraction and Test_BindError_Sources to
instantiate new app/ctx (or drop parallelization) before mutating c.Request(),
app.Get(...), or calling Bind().*.
In `@bind.go`:
- Around line 418-426: In Body(), after successfully calling the custom binder's
Parse via b.returnErr(customBinder.Parse(b.ctx, out)), run the same
StructValidator path used for built-in body binders: if b.StructValidator != nil
call b.StructValidator.Validate(out) (or the project's validator entrypoint) and
handle errors the same way (wrap/return as newBindError(BindSourceBody, err)
unless it's already a *Error via errors.As), instead of returning nil
immediately; this ensures customBinder.Parse(...) is followed by validation for
consistency with built-in binders.
In `@docs/api/bind.md`:
- Around line 689-690: Update the BindError paragraph to clarify that BindError
is returned only when manual handling is used and that in auto-handling mode
(WithAutoHandling) parse failures are converted to *fiber.Error (HTTP 400);
explicitly state that errors.As should be used to extract *BindError only when
not using WithAutoHandling, and that callers using automatic handling should
instead check for *fiber.Error or HTTP 400 status. Reference the BindError type
and the WithAutoHandling option in the updated sentence to make the distinction
explicit.
---
Nitpick comments:
In `@bind.go`:
- Around line 220-225: Extract the repeated pattern that checks errors.As(err,
&Error) and returns either the original error or newBindError(source, err) into
a helper method on the binder (e.g., func (b *binder) wrapParseError(err error,
source BindSource) error) and replace the inline blocks (currently surrounding
calls like b.returnErr(bind.Bind(b.ctx.Request(), out))) with calls to that
helper; the helper should accept the parse error, use errors.As to detect *Error
and return it unchanged, otherwise return newBindError(source, err), and update
all occurrences that currently use BindSourceHeader and other BindSource
constants to call this new method.
In `@binder/mapping.go`:
- Line 120: The function parseToStruct currently re-wraps the decode error by
returning fmt.Errorf("%w", err); change this to return the original err directly
(i.e., return err) so the original error value and its type are preserved;
locate the return inside parseToStruct where fmt.Errorf("%w", err) is used and
replace it with returning err without additional wrapping.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4120 +/- ##
==========================================
+ Coverage 91.04% 91.07% +0.03%
==========================================
Files 119 119
Lines 11344 11377 +33
==========================================
+ Hits 10328 10362 +34
- Misses 644 645 +1
+ Partials 372 370 -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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/api/bind.md (1)
705-717: Consider adding import context to the example.The example demonstrates
errors.Asusage correctly, but doesn't show the required imports (errors,github.com/google/uuid) or explicitly state it assumes manual handling mode (not usingWithAutoHandling). Consider adding a brief comment to clarify the handling mode.📝 Suggested enhancement
+// Example (assumes manual handling mode - not using WithAutoHandling) +// Required imports: "errors" and "github.com/google/uuid" var req struct { ID uuid.UUID `uri:"id"` Name string `json:"name"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api/bind.md` around lines 705 - 717, Update the example to show required imports and clarify manual handling: add mentions/imports for "errors" and "github.com/google/uuid" and a short comment above the snippet stating this example assumes manual error handling (not using WithAutoHandling). Also ensure the example references the same symbols used in the text (Bind().All, fiber.BindError, fiber.BindSourceURI) so readers can match the imports and handling mode to the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/api/bind.md`:
- Around line 687-722: The markdown in docs/api/bind.md needs to be linted; run
the project's markdown lint step by executing the Make target (run "make
markdown") locally and fix any reported issues in docs/api/bind.md (e.g.,
headings, code block fences, spacing) until the linter passes; then re-run "make
markdown" to confirm and update the commit.
---
Nitpick comments:
In `@docs/api/bind.md`:
- Around line 705-717: Update the example to show required imports and clarify
manual handling: add mentions/imports for "errors" and "github.com/google/uuid"
and a short comment above the snippet stating this example assumes manual error
handling (not using WithAutoHandling). Also ensure the example references the
same symbols used in the text (Bind().All, fiber.BindError, fiber.BindSourceURI)
so readers can match the imports and handling mode to the code.
|
@loderunner thx for the PR |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bind.go`:
- Around line 171-177: In returnBindErr, avoid using errors.As which matches
wrapped *Error values; instead perform a top-level type check (type assertion)
on retErr to see if it is *Error so wrapped fiber errors are not treated as raw
*Error in manual mode (WithoutAutoHandling). Also pass the transformed retErr
(not the original err) into newBindError when creating a *BindError. Update the
logic in returnBindErr to use a direct type assertion against *Error (and return
retErr if it truly is that concrete type) and call newBindError(source, retErr)
otherwise.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bind.go`:
- Around line 180-182: There is a stray block after the returnBindErr function
that adds an extra closing brace, a dangling "return nil", and another brace
which breaks compilation; remove the extraneous "}", "return nil", and final "}"
following the returnBindErr function so the function ends cleanly (locate
returnBindErr in bind.go and delete the stray block immediately after its
closing brace).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@ReneWerner87 I think I've adressed all AI comments. Good to merge? |
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
3 participants
Description
Implements #4118.
All
Bind()methods now return*BindErroron parse failure, giving callers the binding source and field that failed as structured data rather than an opaque error string.Fixes #4118
Changes introduced
What changed
New type and constants in
bind.go:All bind methods (
URI,Query,Header,Cookie,RespHeader,JSON,XML,CBOR,MsgPack,Form,Body, andAll) now return*BindErrorinstead of a raw error when parsing fails.BindErrorimplementsUnwrap, soerrors.Astraverses the full chain — you can extract either the*BindErroror the underlying error type directly:binder/mapping.go: Removed the"bind: "prefix thatparseToStructprepended to schema errors. The field/source context is now carried byBindErrorinstead.Gotchas
Validation errors are not bind errors. Errors from a registered
StructValidatorare returned as-is and not wrapped in*BindError.errors.As(err, &be)can be used to tell them apart — it succeeds only for parse failures.Auto-handling suppresses
BindError. WithWithAutoHandling(), parse failures are converted to an HTTP 400 response and the method returns*fiber.Error.BindErroris only returned in default (manual) mode.Breaking change: error message strings
Two error string formats changed:
binder.parseToStructlost the"bind: "prefix —"bind: name is empty"→"name is empty".bind "field" from source: underlying error.errors.Is/errors.Aschains are unaffected. Code that string-matcheserr.Error()will need updating.newBindErrorallocates only on error.docs/api/bind.mdupdated with aBindErrorsection covering the type, source constants, source-branching example, and validation vs binding error distinction.BindErrortype,BindSource*constants, structured error metadata on allBind()methods.location/pathfields.BindErrorimplementserrorandUnwrap, existing code unaffected.docs/api/bind.md.Type of change
Checklist
/docs/directory for Fiber's documentation.