🐛 fix: Nil pointer dereference with Must Bind binding #3171
Conversation
if err is nil err.Error() panics (eg. c.Bind().Must().JSON(...) successfully binds but panics
ItsMeSamey
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
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 |
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
ItsMeSamey
changed the title
|
@ItsMeSamey Anyway we could trigger this in a new unit-test? |
gaby
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3171 +/- ##
==========================================
- Coverage 82.36% 82.16% -0.21%
==========================================
Files 113 113
Lines 8474 11012 +2538
==========================================
+ Hits 6980 9048 +2068
- Misses 1089 1561 +472
+ Partials 405 403 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
make sure returnErr works with nil error
as in majority of cases we expect err to be nil, this should provide better short-cutting
|
@gaby I've added a test for this |
![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
🧹 Outside diff range and nitpick comments (1)
bind_test.go (1)
22-29: Approve the new test function with a suggestion.The new
Test_returnErrfunction is a good addition to test the fix for the nil pointer dereference issue. It correctly checks that no error is returned when a nil error is passed toreturnErr.To make the test more robust, consider adding an additional assertion to check the response status code:
err := c.Bind().Must().returnErr(nil) require.Equal(t, nil, err) +require.Equal(t, 200, c.Response().StatusCode())This will ensure that not only is no error returned, but also that the response status remains unchanged when a nil error is passed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- bind.go (1 hunks)
- bind_test.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
bind_test.go (1)
Line range hint
1-1183: Overall approval of the test file.The
bind_test.gofile appears to be well-structured and comprehensive, covering various binding scenarios including Query, Header, Body, URI, and Cookie bindings. The addition of theTest_returnErrfunction complements the existing test suite by addressing the specific issue mentioned in the PR.The file also includes benchmark functions, which are valuable for monitoring performance. No significant issues or necessary improvements are identified in the rest of the file.
bind.go (1)
44-45: Properly handling nil errors to prevent nil pointer dereferenceThe updated condition
if err == nil || b.shouldensures that the function returns early whenerrisnilorb.shouldistrue, preventing a nil pointer dereference when callingerr.Error()later in the code. This change effectively fixes the panic issue described in the pull request.
|
@gaby |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
bind.go (1)
43-43: Update documentation for Custom bindingThe comment "NOTE: Should/Must is still valid for Custom binders" above the
Custommethod is now outdated sinceShould/Mustmethods have been removed.Remove or update the comment to reflect the new error handling approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- bind.go (11 hunks)
- bind_test.go (0 hunks)
- ctx.go (0 hunks)
💤 Files with no reviewable changes (2)
- bind_test.go
- ctx.go
🧰 Additional context used
🔇 Additional comments (3)
bind.go (3)
52-52: LGTM: Consistent error handling pattern across all binding methodsThe binding methods now follow a consistent and clean pattern:
- Call the appropriate binder
- Return any binding errors directly
- Validate the struct if binding succeeds
This simplification makes the error handling more predictable and allows users to handle status codes according to their needs.
Also applies to: 61-61, 71-71, 80-80, 89-89, 98-98, 107-107, 116-116, 125-125
147-147: LGTM: Clean error handling in Body bindingThe Body binding method maintains a clean separation of concerns while consistently handling errors from custom binders. The direct error return aligns with the new error handling pattern.
22-22: Breaking Change: Removal of Must/Should chaining capabilityThe removal of the
shouldfield from theBindstruct is a breaking change that will affect existing code using theMust()/Should()chaining pattern. This change is necessary to fix the nil pointer dereference issue, but requires careful migration planning.Consider documenting the migration path for users in the changelog, suggesting alternative error handling patterns such as:
if err := c.Bind().JSON(&data); err != nil { return err }
|
we will discuss this again internally and then let you know which way we want to go, thank you for your work and help |
|
here the last statement we want to rename the feature so that there is no confusion with the golang concept of MUST for panics because the methods always return an error |
|
give us a little time, we are currently discussing what this could look like |
if err is nil err.Error() panics
(eg. c.Bind().Must().JSON(...) successfully binds but panics