🔥 feat: Add support for context.Context in keyauth middleware #3287
Conversation
pretty straightforward option to use context.Context instead of just fiber.Ctx, tests added accordingly.
|
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 |
WalkthroughThis pull request enhances API key middleware by refining context handling and token retrieval. The Changes
Sequence Diagram(s)sequenceDiagram
participant Req as Request
participant MW as Middleware
participant KA as KeyAuth Module
Req->>MW: Send request with API key
MW->>KA: Call New() to validate & store API key
KA-->>MW: Set token in Locals and Context
Note over MW,KA: Later, token is retrieved for authorization
MW->>KA: Call TokenFromContext(context)
alt context type is fiber.Ctx
KA-->>MW: Return token from Locals
else context type is context.Context
KA-->>MW: Return token from context.Value
else unsupported context type
KA-->>MW: Panic with error message
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3287 +/- ##
==========================================
- Coverage 83.66% 83.61% -0.06%
==========================================
Files 118 118
Lines 11714 11724 +10
==========================================
+ Hits 9801 9803 +2
- Misses 1485 1491 +6
- Partials 428 430 +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:
|
|
@vhespanha Are you done with the changes? Can we review it? If so, remove the "Draft" status. |
gaby
changed the title
gaby
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
![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
🧹 Nitpick comments (1)
middleware/keyauth/keyauth.go (1)
75-89: Well-structured type handling for multiple context types.The function has been updated to work with both
context.Contextandfiber.Ctxtypes through a type switch pattern, which is an idiomatic approach in Go. The panic for unsupported types is appropriate for programming errors.Consider adding documentation comments about the expected context types and panic behavior to help users understand how to properly use this function.
// TokenFromContext returns the bearer token from the request context. // returns an empty string if the token does not exist +// Accepts either fiber.Ctx or context.Context, and panics for other types func TokenFromContext(c any) string {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/keyauth/keyauth.go(3 hunks)middleware/keyauth/keyauth_test.go(1 hunks)
🔇 Additional comments (5)
middleware/keyauth/keyauth.go (2)
5-5: Added context import for Go's standard context package.Good addition necessary for the planned context support.
63-67: Good implementation for dual context support.This is an excellent approach that maintains backward compatibility while adding new functionality. The token is now stored in both
Locals(for backward compatibility) and the request context (for the new feature), ensuring a smooth transition.middleware/keyauth/keyauth_test.go (3)
507-531: Good test coverage for fiber.Ctx token retrieval.This first sub-test properly verifies that the
TokenFromContextfunction can retrieve the token from afiber.Ctxinstance. The test setup, execution, and assertions are clear and comprehensive.
533-560: Excellent test for context.Context support.This test is critical for confirming that the new functionality works as expected. It verifies that the token can be retrieved from a standard Go context using the same
TokenFromContextfunction, which demonstrates the dual nature of the enhanced API.
562-566: Good error handling test.Testing that the function properly panics for invalid context types is important for ensuring robust error handling. This test will prevent regressions where someone might mistakenly pass an invalid type to the function.
|
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 |
pretty straightforward option to use context.Context instead of just fiber.Ctx, tests added accordingly.
Description
as discussed in #3212 and #3175, some of the middleware would benefit from exposing support for raw context.Context, for usage outside of the HTTP layer. this is the first out of 3 pull requests for middlewares that are deemed necessary to support this as of now.
Type of change
Draft
i'm opening this as a draft PR as of now, if it aligns with what the issues requested, i'm happy to update the documentation and benchmarks, i feel like it's correct tho, since it basically mirrors the implementation and usage of the same feature for requestID (from #3200).