🐛 bug: guard nil request in adaptor LocalContextFromHTTPRequest#4097
Conversation
Summary of ChangesHello @gaby, 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 addresses a potential panic in the 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
|
WalkthroughAdded defensive nil checks to two helper functions in the adaptor to avoid panics on nil inputs; expanded unit tests to cover nil request, missing context keys, and various context-copying scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Pull request overview
Prevents a panic in the adaptor layer by making LocalContextFromHTTPRequest safe to call with a nil *http.Request, while preserving existing behavior for non-nil requests so adapted handlers can still retrieve the stored Fiber context when present.
Changes:
- Add an early
nilguard inLocalContextFromHTTPRequestreturning(nil, false)whenr == nil. - Add unit tests covering nil requests, missing context key, and present context key cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/adaptor/adaptor.go | Adds a nil check to avoid panics when extracting local context from a nil request. |
| middleware/adaptor/adaptor_test.go | Adds focused tests validating LocalContextFromHTTPRequest behavior across key scenarios. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4097 +/- ##
=======================================
Coverage 91.03% 91.03%
=======================================
Files 119 119
Lines 11320 11324 +4
=======================================
+ Hits 10305 10309 +4
Misses 643 643
Partials 372 372
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.
Code Review
The pull request correctly implements a nil check in LocalContextFromHTTPRequest to prevent panics when a nil request is passed. The added unit tests are well-structured and cover the relevant edge cases, including nil requests and missing context keys. The changes improve the stability of the adaptor middleware without introducing any regressions.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
middleware/adaptor/adaptor_test.go (1)
879-902: Optional: replace vacuousassert.NotNil(t, &fctx)with a meaningful assertion.
assert.NotNil(t, &fctx)takes the address of a local variable — this is always non-nil and provides zero test coverage value. Now that both subtests are parallel, consider either removing the assertion entirely (the panic-safety is proven by the test not panicking) or replacing it with an assertion on observable side-effects (e.g., that no user values were written).🧹 Suggested clean-up (both affected subtests)
t.Run("invalid src", func(t *testing.T) { t.Parallel() var fctx fasthttp.RequestCtx CopyContextToFiberContext(nil, &fctx) - // Add assertion to ensure no panic and coverage is detected - assert.NotNil(t, &fctx) })t.Run("nil pointer", func(t *testing.T) { t.Parallel() var nilPtr *context.Context var fctx fasthttp.RequestCtx CopyContextToFiberContext(nilPtr, &fctx) - // Add assertion to ensure no panic and coverage is detected - assert.NotNil(t, &fctx) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/adaptor/adaptor_test.go` around lines 879 - 902, Replace the vacuous assert.NotNil(t, &fctx) checks in the "invalid src" and "nil pointer" subtests with assertions that verify observable side-effects of CopyContextToFiberContext: call fctx.UserValue("nil-request-context") (or the expected key used in the ctx) and assert it is nil (or not set), and keep the require.NotPanics where present; this uses the function CopyContextToFiberContext as the target and ensures the test actually verifies that no user values were written instead of asserting the address of a local variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@middleware/adaptor/adaptor_test.go`:
- Around line 879-902: Replace the vacuous assert.NotNil(t, &fctx) checks in the
"invalid src" and "nil pointer" subtests with assertions that verify observable
side-effects of CopyContextToFiberContext: call
fctx.UserValue("nil-request-context") (or the expected key used in the ctx) and
assert it is nil (or not set), and keep the require.NotPanics where present;
this uses the function CopyContextToFiberContext as the target and ensures the
test actually verifies that no user values were written instead of asserting the
address of a local variable.
Motivation
LocalContextFromHTTPRequestis called with a nil*http.Requestby returning(nil, false)early.Description
LocalContextFromHTTPRequestthat returnsnil, falseifr == nil.middleware/adaptor(Test_LocalContextFromHTTPRequest) that verify nil request handling, requests without the stored context key, and requests with the stored context value underlocalContextKey.middleware/adaptor/adaptor.goandmiddleware/adaptor/adaptor_test.goto include the change and tests.