feat: fiber.Context implement context.Context#3382
feat: fiber.Context implement context.Context#3382ReneWerner87 merged 14 commits intogofiber:mainfrom
Conversation
WalkthroughThe changes remove the previous mechanism for storing and retrieving a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FiberCtx
participant Handler
User->>FiberCtx: Set Locals(key, value)
Handler->>FiberCtx: Value(key)
FiberCtx-->>Handler: value (from Locals)
Handler->>FiberCtx: Deadline()
FiberCtx-->>Handler: (zero, false)
Handler->>FiberCtx: Done()
FiberCtx-->>Handler: nil
Handler->>FiberCtx: Err()
FiberCtx-->>Handler: nil
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 #3382 +/- ##
==========================================
- Coverage 84.41% 84.39% -0.02%
==========================================
Files 120 120
Lines 12194 12190 -4
==========================================
- Hits 10293 10288 -5
Misses 1473 1473
- Partials 428 429 +1
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:
|
|
@pjebs Can you take a look at the merge conflicts and failing tests |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ctx.go (2)
43-43: Consider removing or clarifying the unused type.Currently,
contextKeyis declared but not referenced. If it's intended for future use, consider adding explanatory comments or referencing it to avoid confusion and lint warnings.
1820-1829: Clarify distinction from standard context.
Value(key any)delegates toc.fasthttp.UserValue, which differs from typicalcontext.Contextscoping rules. If the goal is standardcontext.Contextcompatibility, document any differences or add disclaimers to prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ctx.go(4 hunks)ctx_interface_gen.go(3 hunks)ctx_test.go(1 hunks)middleware/requestid/requestid.go(0 hunks)middleware/requestid/requestid_test.go(0 hunks)middleware/timeout/timeout.go(1 hunks)middleware/timeout/timeout_test.go(3 hunks)
💤 Files with no reviewable changes (2)
- middleware/requestid/requestid_test.go
- middleware/requestid/requestid.go
🔇 Additional comments (10)
ctx.go (3)
426-434: Confirm no-op behavior.
Deadline()intentionally returns no deadline (time.Time{}, false). Ensure this aligns with your design, as callers expecting realcontext.Contextbehavior might be surprised by the missing deadline support.
436-446: Confirm no-op behavior.
Done()intentionally returnsnil, which omits the usual cancellation signals in a standardcontext.Context. Verify this is acceptable so that dependent callers won't assume actual cancellation mechanics.
472-482: Confirm no-op behavior.
Err()unconditionally returnsnil, indicating no cancellation or deadline exceeded errors are signaled. Double-check that this matches your requirements for a partial context-like interface.ctx_test.go (1)
2190-2255: Well-implemented tests for the new context interface methods.The tests for the new context methods (
Deadline(),Done(),Err(), andValue()) look good. They follow a consistent pattern and thoroughly verify that these methods implement the standard Gocontext.Contextinterface with the appropriate no-operation behaviors. The test cases are clear, concise, and maintain the same structure as other tests in the file.ctx_interface_gen.go (5)
10-10: Import of "time" looks good
This import is presumably required to support the newly introduced context-deadline methods.
60-63: ValidateDeadline()implementation
The documentation mirrors standardcontext.Contextbehavior. Please ensure that the implementedDeadline()method truly reflects the described functionality, including concurrency safety and proper deadline reporting.Would you like a script to confirm the method’s actual implementation usage?
64-69: CheckDone()channel consistency
The doc states it may return nil if the context can’t be canceled. Verify that this channel is properly closed and tested wherever timeouts or cancellations occur.
75-80: ClarifyErr()behavior
Ensure that this reliably distinguishes betweenCanceledandDeadlineExceeded. If it’s effectively a no-op, clarify how and why.
331-337: Consider concurrency implications inValue(key any) any
Reading/writing local values across goroutines can cause data races. Confirm whether this method needs synchronization or is guaranteed to be single-threaded.middleware/timeout/timeout_test.go (1)
44-44: EnsuresleepWithContextreceives a validcontext.Context
Replacingc.Context()withcimplies thatfiber.Ctxmust implement the standardcontext.Context. Confirm that all tests pass and that no unexpected type assertions fail.Also applies to: 63-63, 84-84
| // Create a context with the specified timeout; any operation exceeding | ||
| // this deadline will be canceled automatically. | ||
| timeoutContext, cancel := context.WithTimeout(ctx.Context(), timeout) | ||
| timeoutContext, cancel := context.WithTimeout(ctx, timeout) |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Confirm fiber.Ctx implements context.Context
context.WithTimeout(ctx, timeout) expects a valid context.Context. If fiber.Ctx is intended to fully satisfy this interface, confirm at compile-time and runtime.
🏁 Script executed:
#!/bin/bash
# Attempt building the codebase to ensure no type mismatch errors.
go build ./...Length of output: 209
Below is a new verification script to ascertain at compile time that the provided ctx (of type fiber.Ctx) indeed implements the context.Context interface:
Review Update: Verify fiber.Ctx Compliance with context.Context
Please ensure that the ctx you pass to context.WithTimeout(ctx, timeout) (located in middleware/timeout/timeout.go at line 22) is a valid context.Context. The above script creates a compile-time check for *fiber.Ctx to assert that it implements context.Context. If the script fails to build, you’ll need to either modify the code or explicitly convert/extract a proper context.Context from fiber.Ctx.
🏁 Script executed:
#!/bin/bash
set -e
# Create a temporary directory for the compile-time check
mkdir -p temp_assert && cd temp_assert
# Write a small Go program that asserts fiber.Ctx implements context.Context.
cat > tmp_assert.go << 'EOF'
package main
import (
"context"
"fmt"
"github.com/gofiber/fiber/v2"
)
func main() {
// Compile-time assertion: if *fiber.Ctx doesn't implement context.Context,
// this line will cause a compilation error.
var _ context.Context = (*fiber.Ctx)(nil)
fmt.Println("fiber.Ctx implements context.Context")
}
EOF
# Initialize a temporary go module and fetch the fiber dependency.
go mod init temp_assert_module
go get github.com/gofiber/fiber/v2
# Attempt to build and run the temporary program.
go build tmp_assert.go && ./tmp_assertLength of output: 1681
Update Required: Invalid Context for Timeout
Our verification shows that *fiber.Ctx does not implement context.Context (it's missing the Deadline method). That means the call to context.WithTimeout(ctx, timeout) in middleware/timeout/timeout.go (line 22) is passing an invalid argument.
- In
middleware/timeout/timeout.go, update the code so that a validcontext.Contextis provided. If available, consider using a method likectx.Context()(if it returns a standard Go context) or use a fallback such ascontext.Background(). - Example revision (if
ctx.Context()yields a standard context):timeoutContext, cancel := context.WithTimeout(ctx.Context(), timeout)
Please ensure the revised context usage accurately reflects the intended behavior in your application.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ctx.go (2)
43-43: Unused type declaration needs explanation.The contextKey type is marked as unused with a nolint directive, with a comment that it's needed for future use. Since this was previously used for the Context() functionality that has been removed, consider adding more context about its future purpose or remove it entirely if it's no longer needed.
426-434: Implementation of context.Context interface: Deadline methodThis method implements the Deadline() method from the standard context.Context interface but returns no deadline. The comment explains that this is a no-op due to fasthttp limitations, which is useful information for users.
Consider adding a link to discussions or issues explaining the design decision to implement the context.Context interface with no-op methods, which would help users understand why this approach was chosen over other alternatives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx.go(4 hunks)ctx_interface_gen.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (6)
ctx.go (3)
436-446: Implementation of context.Context interface: Done methodThis method implements the Done() method from the standard context.Context interface but returns nil, meaning this context can never be canceled. The comment correctly explains the limitations.
Good job providing clear documentation explaining the fasthttp limitations. This helps users understand why cancellation signals aren't supported.
472-482: Implementation of context.Context interface: Err methodThis method implements the Err() method from the standard context.Context interface but always returns nil. The implementation aligns with the comment that this is a no-op due to fasthttp limitations.
The documentation is clear and matches the implementation.
1820-1824: Implementation of context.Context interface: Value methodThis method implements the Value() method from the standard context.Context interface by retrieving values from the fasthttp UserValue store. This approach provides a working implementation for storing request-scoped values.
Unlike the other context methods, this one actually provides useful functionality rather than being a no-op. The implementation is simple and efficient.
ctx_interface_gen.go (3)
10-10: Added time package for context.Context interfaceThe time package was added to support the Deadline() method from the context.Context interface. This change is necessary and correctly implemented.
60-80: Interface declaration for context.Context methodsThe Deadline(), Done(), and Err() methods have been added to the Ctx interface to match the standard context.Context interface. The documentation for these methods is comprehensive and follows the standard context package documentation.
These changes enable the Fiber Ctx interface to satisfy the standard context.Context interface, which improves compatibility with libraries expecting context.Context.
331-333: Interface declaration for Value methodThe Value() method has been added to complete the context.Context interface implementation. The documentation is clear and explains that this method retrieves values scoped to the request.
This method provides a way to access request-scoped values, which is a key part of the context.Context interface. The implementation works with the Locals() method but provides context.Context compatibility.
There was a problem hiding this comment.
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
middleware/timeout/timeout.go:21
- The removal of updating the Fiber context with the timeout-bound context may lead to unexpected behavior if downstream handlers depend on the updated context. Please confirm that this change is intentional and that all affected handlers are adjusted accordingly.
ctx.SetContext(timeoutContext)
middleware/requestid/requestid_test.go:73
- The removal of the 'From context.Context' test case reduces coverage of context-based request ID extraction. Please ensure that either this functionality is no longer supported or that alternative tests are provided to cover the desired behavior.
{ name: "From context.Context", args: { inputFunc: func(c fiber.Ctx) any { return c.Context() } } },
|
Please update the pull request title and description template. The documentation for context also needs to be updated |
Description
Makes fiber.Context implement context.Context
Fixes #3344
Changes introduced
fiber.Context now incorporates :
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.