Propagate fiberCtx.UserContext() to huma.Context - Context() #659
Conversation
WalkthroughThe changes introduce a new type called Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
adapters/humafiber/humafiber.go (1)
62-64: LGTM: Clean adapter instantiationThe
Context()method correctly instantiates the newcontextAdapter, properly wrapping the original Fiber context. This implementation maintains a clean separation of concerns while providing the enhanced context handling capabilities.This adapter pattern implementation is a good example of how to extend functionality without modifying the underlying Fiber context, making it easier to maintain and update in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
adapters/humafiber/humafiber.go(2 hunks)
🔇 Additional comments (3)
adapters/humafiber/humafiber.go (3)
18-22: LGTM: Clean struct definition with interface compliance check
The contextAdapter struct is well-designed with proper interface assertion using the var _ pattern to ensure compile-time verification of context.Context interface implementation.
24-34: LGTM: Clean implementation of standard context methods
The Deadline, Done, and Err methods correctly delegate to the underlying Fiber context, maintaining the expected context.Context behavior.
36-42: LGTM: Value method correctly implements priority lookup
The implementation aligns with the PR objective by:
- First checking
UserContext().Value() - Falling back to
Context().Value()if no value is found
Let's verify the usage of UserContext in the codebase:
✅ Verification successful
Value method implementation is unique and correctly prioritizes context lookup
The implementation in adapters/humafiber/humafiber.go is the only Value method in the codebase that handles context values. The prioritized lookup (UserContext first, then falling back to Context) is unique to this adapter and doesn't conflict with any other implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other UserContext usages to ensure consistency
# and verify there are no potential conflicts with this new implementation
rg -A 2 "UserContext\(\)" --type go
Length of output: 213
Script:
#!/bin/bash
# Let's check for any other implementations of Value method in the codebase
# to ensure consistency with context handling
ast-grep --pattern 'func $_Value(key any) any {
$$$
}'
Length of output: 58
Script:
#!/bin/bash
# Let's check for any Value method implementations and context handling patterns
# using a broader search to catch potential variations
rg -A 5 "func.*Value.*any.*any" --type go
Length of output: 401
…ext()' for everything, except 'Value', 'Value' first tries to find value inside 'fiberCtx.UserContext()' and only if value is missed call original'fiberCtx.Context().Value()'
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
adapters/humafiber/humafiber.go (1)
65-69: Consider handling nil Done channel.While the implementation correctly initializes the
contextAdapter, consider handling the case whereContext().Done()returns nil. This is a valid case in thecontext.Contextinterface.Consider this safer initialization:
return &contextAdapter{ Ctx: c.orig, - done: c.orig.Context().Done(), + done: func() <-chan struct{} { + if done := c.orig.Context().Done(); done != nil { + return done + } + return nil + }(), user: c.orig.UserContext().Value, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
adapters/humafiber/humafiber.go(2 hunks)
🔇 Additional comments (3)
adapters/humafiber/humafiber.go (3)
18-23: LGTM! Good approach to prevent race conditions.
The contextAdapter type effectively prevents race conditions in fasthttp by caching both the Done() channel and the UserContext().Value function. The embedded *fiber.Ctx maintains access to other context functionality.
39-45: LGTM! Correct implementation of value priority.
The Value method correctly implements the PR objective by:
- First checking the user context via the cached function
- Falling back to the original context's Value method if no user value is found
Line range hint 18-69: Verify context propagation with Fiber middleware.
The changes look good, but let's verify the interaction with Fiber middleware and potential memory implications.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #659 +/- ##
=======================================
Coverage 93.04% 93.04%
=======================================
Files 22 22
Lines 4901 4901
=======================================
Hits 4560 4560
Misses 298 298
Partials 43 43 ☔ View full report in Codecov by Sentry. |
The problem: fiber uses UserContext() to hold user data, Huma invokes Context(). As the result, I could not propagate data inside context from Fiber. I have JWT/some other fiber middleware (this is right for us, we use fiber without Huma also), but Huma.Context.Context does not see these values
fix: adapters/humafiber - fixed Context() method - use 'fiberCtx.Context()' for everything, except 'Value', 'Value' first tries to find value inside 'fiberCtx.UserContext()' and only if value is missed call original'fiberCtx.Context().Value()'
Summary by CodeRabbit