Revert "🔥 feat: Add Context Support to RequestID Middleware" #3365
Conversation
This reverts commit f725ded.
sixcolors
requested review from
gaby,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe pull request simplifies the request ID retrieval in the Fiber middleware. The documentation now exclusively uses Changes
Sequence Diagram(s)sequenceDiagram
participant Handler
participant RequestID
Handler->>RequestID: Call FromContext(fiber.Ctx)
RequestID-->>Handler: Return request ID (or empty string)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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 #3365 +/- ##
==========================================
- Coverage 83.73% 83.67% -0.07%
==========================================
Files 118 118
Lines 11728 11716 -12
==========================================
- Hits 9821 9803 -18
- Misses 1481 1485 +4
- Partials 426 428 +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:
|
![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/requestid/requestid_test.go (1)
54-76: Test updated to match the simplified APIThe test has been appropriately updated to reflect the changes in the
FromContextfunction. The test now directly passes afiber.Ctxto the function instead of testing multiple input types.Minor improvements to note:
- The comment on line 54 mentions
Test_RequestID_Locals, but the function name at line 55 is stillTest_RequestID_FromContext. Consider renaming the function to match the comment for consistency.- The test is more straightforward now, focusing on the single use case which matches Fiber's intended pattern.
// go test -run Test_RequestID_Locals -func Test_RequestID_FromContext(t *testing.T) { +func Test_RequestID_Locals(t *testing.T) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/middleware/requestid.md(0 hunks)middleware/requestid/requestid.go(1 hunks)middleware/requestid/requestid_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- docs/middleware/requestid.md
🧰 Additional context used
🧬 Code Definitions (1)
middleware/requestid/requestid_test.go (2)
middleware/requestid/requestid.go (2) (2)
New(17-42)FromContext(46-51)middleware/requestid/config.go (1) (1)
Config(9-24)
🔇 Additional comments (1)
middleware/requestid/requestid.go (1)
46-49: Simplified and focused implementationThe function signature has been updated to explicitly accept
fiber.Ctxinstead of the previousanytype that handled bothfiber.Ctxandcontext.Context. This change aligns with the PR objective of removing unnecessary context modifications and maintaining Fiber's lightweight design.The simplified implementation improves:
- Type safety by using a specific type rather than
any- Performance by removing context conversion overhead
- Maintainability by following Fiber's pattern of using
c.Locals()for value storage
Reverts #3200
Reversion of Middleware Changes in Fiber v3
Following discussions in issue #3358, we have reverted changes that attempted to copy values into
context.Contextwithin various middlewares. These changes were removed to maintain Fiber’s lightweight design, improve performance, and avoid unnecessary complexity for users who do not requirecontext.Context.Why Were These Changes Reverted?
context.Contextfor every request introduces unnecessary overhead.c.Locals(), making context modifications redundant.context.Contextvalues can implement a trivial custom middleware without affecting default behavior.