feat: fiber.Context implement context.Context #3382
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| @@ -19,12 +19,9 @@ func New(h fiber.Handler, timeout time.Duration, tErrs ...error) fiber.Handler { | |||
|
|
|||
| // 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR makes fiber.Ctx implement context.Context by adding the standard methods (Deadline, Done, Err, Value), updates the timeout middleware to use the new interface, removes the old Context/SetContext API, and refreshes tests and documentation accordingly.
- Implemented
Deadline,Done,Err, andValueonDefaultCtxand updated theCtxinterface. - Adjusted the timeout middleware and its tests to work with the new context methods.
- Removed legacy
Context/SetContextmethods and related tests; updated docs to reflect the new API.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/timeout/timeout.go | Use fiber.Ctx directly with context.WithTimeout, drop SetContext override |
| middleware/timeout/timeout_test.go | Updated tests to pass c (the Ctx) into sleepWithContext |
| docs/api/ctx.md | Rewrote the Context section to document Deadline, Done, Err, and Value |
| ctx_interface_gen.go | Removed Context/SetContext; added signatures for Deadline, Done, Err, Value |
| ctx.go | Deleted old context methods, added no-op implementations and Value method |
| ctx_test.go | Removed old Context tests, added tests for Deadline, Done, Err, and Value |
Comments suppressed due to low confidence (1)
ctx_test.go:2249
- [nitpick] Add a test case to verify that c.Value returns nil (or the zero value) when no value is set for the given key, ensuring full coverage of the new Value method's behavior for missing keys.
require.Equal(t, "doe", c.Value("john"))
| timeoutContext, cancel := context.WithTimeout(ctx, timeout) | ||
| defer cancel() | ||
|
|
There was a problem hiding this comment.
Passing the original fiber.Ctx to context.WithTimeout does not propagate the timeout to the handler because the fiber.Ctx methods (Done, Deadline, Err) are no-ops. The handler runs against the unmodified Ctx and cannot observe cancellation. Consider injecting the derived timeoutContext into the Ctx (e.g., via a wrapper or reinstating SetContext) so handler calls to context methods reflect the timeout.
| timeoutContext, cancel := context.WithTimeout(ctx, timeout) | |
| defer cancel() | |
| timeoutContext, cancel := context.WithTimeout(context.Background(), timeout) | |
| defer cancel() | |
| // Inject the derived timeoutContext into the fiber.Ctx. | |
| ctx.SetUserContext(timeoutContext) |
Copilot uses AI. Check for mistakes.
| // The contextKey type is unexported to prevent collisions with context keys defined in | ||
| // other packages. | ||
| type contextKey int | ||
|
|
||
| // userContextKey define the key name for storing context.Context in *fasthttp.RequestCtx | ||
| const userContextKey contextKey = 0 // __local_user_context__ | ||
| type contextKey int //nolint:unused // need for future (nolintlint) |
There was a problem hiding this comment.
The contextKey type is now unused since Context and SetContext were removed. Consider removing this orphan definition and its nolint directive to reduce confusion.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
middleware/timeout/timeout.go (1)
22-22: Context interface implementation verifiedThe change from
context.WithTimeout(ctx.Context(), timeout)tocontext.WithTimeout(ctx, timeout)is correct. Thefiber.Ctxnow implements thecontext.Contextinterface directly with the four required methods (Deadline,Done,Err,Value) as seen in thectx.gochanges.
🧹 Nitpick comments (2)
ctx.go (1)
43-43: Unused type should be cleaned upThe
contextKeytype is marked as unused but kept "for future". Consider removing it entirely to reduce code clutter, or add a comment explaining the specific future use case.-// The contextKey type is unexported to prevent collisions with context keys defined in -// other packages. -type contextKey int //nolint:unused // need for future (nolintlint)docs/api/ctx.md (1)
44-48: Grammar and clarity: add missing comma and pluralize 'no-ops'
The sentence here needs a comma after "However" and the term "nop" should be pluralized to "no-ops" for accuracy.
Proposed diff:- `Context` implements `context.Context`. However due to [current limitations in how fasthttp](https://github.com/valyala/fasthttp/issues/965#issuecomment-777268945) works, `Deadline()`, `Done()` and `Err()` operate as a nop. + `Context` implements `context.Context`. However, due to [current limitations in how fasthttp](https://github.com/valyala/fasthttp/issues/965#issuecomment-777268945) works, `Deadline()`, `Done()` and `Err()` operate as no-ops.🧰 Tools
🪛 LanguageTool
[uncategorized] ~46-~46: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...Contextimplementscontext.Context. However due to [current limitations in how fast...(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ctx.go(4 hunks)ctx_interface_gen.go(3 hunks)ctx_test.go(1 hunks)docs/api/ctx.md(1 hunks)middleware/timeout/timeout.go(1 hunks)middleware/timeout/timeout_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
docs/api/ctx.md (1)
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
🪛 LanguageTool
docs/api/ctx.md
[uncategorized] ~46-~46: A comma may be missing after the conjunctive/linking adverb ‘However’.
Context: ...Context implements context.Context. However due to [current limitations in how fast...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🔇 Additional comments (12)
middleware/timeout/timeout_test.go (1)
44-44: Test updates align with interface changesThe updates correctly pass the Fiber context
cdirectly tosleepWithContextinstead of extracting a standard context viac.Context(). This aligns with the new direct implementation ofcontext.Contextinterface onfiber.Ctx.Also applies to: 63-63, 84-84
ctx_interface_gen.go (1)
10-10: Import change supports new deadline functionalityCorrectly changed from
contexttotimepackage to support theDeadline() (time.Time, bool)method signature.ctx.go (4)
426-446: Context methods implemented with documented limitationsThe
Deadline()andDone()methods are correctly implemented as no-ops with clear documentation about fasthttp limitations. This is technically correct for thecontext.Contextinterface.
472-482: Error method correctly implemented as no-opThe
Err()method implementation is correct, returningnilas documented due to fasthttp limitations.
1820-1824: Value method provides access to fasthttp user valuesThe
Value()method correctly delegates tofasthttp.UserValue(), effectively exposing the existing locals mechanism through the context interface. This maintains consistency with the existingLocals()method behavior.
426-482:✅ Verification successful
Potential confusion due to limited context.Context implementation
While technically correct, the no-op implementations of
Deadline(),Done(), andErr()could lead to subtle bugs when users expect fullcontext.Contextbehavior (e.g., cancellation, timeouts). Consider adding runtime warnings or additional documentation.Consider enhancing the documentation or adding compile-time assertions to make limitations more obvious:
+// Note: This implementation has significant limitations compared to standard context.Context: +// - No cancellation support (Done() returns nil) +// - No deadline support (Deadline() returns zero time) +// - No error propagation (Err() returns nil) +// Use with caution when passing to libraries expecting full context.Context behavior. func (*DefaultCtx) Deadline() (time.Time, bool) { return time.Time{}, false }Verify that the timeout middleware and other context-dependent code still work correctly with these limitations:
🏁 Script executed:
#!/bin/bash # Search for potential context usage that might expect full functionality echo "Searching for context usage patterns that might be affected..." rg -A 3 -B 3 "context\.(WithCancel|WithDeadline|WithTimeout)" --type go # Check for any context.Done() usage echo "Checking for context.Done() usage..." rg "\.Done\(\)" --type go # Look for context error handling echo "Checking for context error handling..." rg "context\.(Canceled|DeadlineExceeded)" --type goLength of output: 8604
No changes needed for DefaultCtx’s context methods
The no-op behavior for Deadline(), Done(), and Err() is already clearly documented on each method. A search through the codebase shows that all timeout/cancellation logic uses standard context.Context (via context.Background/WithTimeout/etc.), not DefaultCtx, so there’s no unexpected breakage or hidden issues.
docs/api/ctx.md (2)
49-53: Documentation updated to reflect newcontext.Contextmethods
The signatures forDeadline,Done,Err, andValuecorrectly show thatfiber.Ctxnow implements thecontext.Contextinterface directly.
57-64: Example demonstrates passingfiber.Ctxascontext.Context
This snippet clearly illustrates howfiber.Ctxcan be used wherevercontext.Contextis required. No changes needed.ctx_test.go (4)
2190-2206: LGTM! Correct implementation test for Deadline().The test properly verifies that
Deadline()returns a zero time value andfalse, which is the expected behavior for a minimal context.Context implementation in an HTTP framework where deadlines are typically managed elsewhere.
2208-2222: LGTM! Correct implementation test for Done().The test properly verifies that
Done()returns anilchannel, which is appropriate for a minimal context.Context implementation. The type assertion syntax(<-chan struct{})(nil)correctly compares with the expected nil channel type.
2224-2238: LGTM! Correct implementation test for Err().The test properly verifies that
Err()returnsnil, indicating no context error. This is consistent with the minimal context.Context implementation approach for HTTP contexts where error handling is managed through other mechanisms.
2240-2255: LGTM! Excellent integration of Value() with Fiber's Locals mechanism.The test correctly verifies that
Value()retrieves values stored viac.Locals(). This is a smart implementation that leverages Fiber's existing local storage system while providing the standardcontext.Contextinterface, maintaining compatibility with existing Fiber patterns.
| // Deadline returns the time when work done on behalf of this context | ||
| // should be canceled. Deadline returns ok==false when no deadline is | ||
| // set. Successive calls to Deadline return the same results. | ||
| Deadline() (deadline time.Time, ok bool) | ||
| // Done returns a channel that's closed when work done on behalf of this | ||
| // context should be canceled. Done may return nil if this context can | ||
| // never be canceled. Successive calls to Done return the same value. | ||
| // The close of the Done channel may happen asynchronously, | ||
| // after the cancel function returns. | ||
| Done() <-chan struct{} | ||
| // Download transfers the file from path as an attachment. | ||
| // Typically, browsers will prompt the user for download. | ||
| // By default, the Content-Disposition header filename= parameter is the filepath (this typically appears in the browser dialog). | ||
| // Override this default with the filename parameter. | ||
| Download(file string, filename ...string) error | ||
| // If Done is not yet closed, Err returns nil. | ||
| // If Done is closed, Err returns a non-nil error explaining why: | ||
| // DeadlineExceeded if the context's deadline passed, | ||
| // or Canceled if the context was canceled for some other reason. | ||
| // After Err returns a non-nil error, successive calls to Err return the same error. | ||
| Err() error | ||
| // Request return the *fasthttp.Request object | ||
| // This allows you to use all fasthttp request methods | ||
| // https://godoc.org/github.com/valyala/fasthttp#Request |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Breaking change: Interface now implements context.Context directly
The interface correctly implements the four required methods for context.Context:
Deadline() (time.Time, bool)Done() <-chan struct{}Err() errorValue(key any) any
However, this is a breaking change as it removes the previous Context() and SetContext() methods that allowed users to store/retrieve custom standard contexts.
Verify that this breaking change is properly documented in migration guides:
Also applies to: 331-333
🏁 Script executed:
#!/bin/bash
# Check if breaking changes are documented
echo "Checking for migration documentation..."
fd -e md . | xargs rg -l -i "context|breaking|migration" || echo "No migration docs found"
# Check for any remaining usage of removed methods in docs
echo "Checking for outdated Context()/SetContext() usage in docs..."
fd -e md . | xargs rg "Context\(\)|SetContext\(" || echo "No outdated usage found"Length of output: 1598
Update documentation and migration guide for removed Context()/SetContext()
The generated ctx_interface_gen.go now implements context.Context directly and no longer provides Context() or SetContext() methods. This is a breaking change that needs to be clearly documented and all outdated references removed.
Please update the migration guide (e.g. docs/whats_new.md) and correct/remove references in the following files:
- docs/middleware/timeout.md
– usages ofc.Context()in text and code samples - docs/extra/faq.md
–host.Fiber.Handler()(c.Context()) - docs/client/request.md
–func (r *Request) Context()
–func (r *Request) SetContext(ctx context.Context) - docs/api/ctx.md
–func (c fiber.Ctx) SetContext(ctx context.Context)
– sample calls toc.SetContext(ctx)
Ensure the migration notes explain how users should now access the standard context.Context (via direct embedding) and remove or replace any old patterns.
🤖 Prompt for AI Agents
In ctx_interface_gen.go around lines 60 to 83, the interface now directly
implements context.Context by including Deadline, Done, Err, and Value methods,
removing the previous Context() and SetContext() methods. This is a breaking
change that requires updating the migration guide (e.g., docs/whats_new.md) to
document this removal and instruct users on the new usage pattern. Additionally,
remove or update all references to Context() and SetContext() in the
documentation files docs/middleware/timeout.md, docs/extra/faq.md,
docs/client/request.md, and docs/api/ctx.md to reflect this change and guide
users on accessing the standard context.Context directly.
|
@pjebs Are the concerns about timeout middleware valid? I believe they are. |
|
The concerns about timeout are not valid. timeoutContext will be a Also relevant: #3394 |
There was a problem hiding this comment.
- pls update whats new file
- use the https://pkg.go.dev/context#Context as golang composition or add an check to ensure that we meet the requirements of the interface at all times, we should make the same for https://pkg.go.dev/io#Writer https://github.com/gofiber/fiber/blob/main/ctx.go#L1808
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ctx.go (1)
47-47: Consider removing the unused contextKey type.This type appears to be orphaned from the previous context implementation and is no longer needed after removing the
Context()andSetContext()methods.
🧹 Nitpick comments (3)
docs/whats_new.md (3)
397-397: Use consistent formatting for the Ctx implementation bullet.The entry currently reads:
- Context now implements [context.Context].To match the style of other bullets and improve clarity, consider:
- Referencing
Ctxexplicitly (e.g., Ctx instead of Context)- Wrapping
context.Contextin backticks rather than link text- Using bold for the subject as in other lines
For example:
- - Context now implements [context.Context]. + - **Ctx** now implements `context.Context`.
402-404: Clarify no-op behavior for the new context methods.The methods
Deadline(),Done(), andErr()are implemented as no-ops due tofasthttplimitations, but the docs don’t currently mention this. To set proper expectations, append a short(no-op)note or equivalent. For example:- - **Deadline**: For implementing `context.Context`. - - **Done**: For implementing `context.Context`. - - **Err**: For implementing `context.Context`. + - **Deadline**: (no-op) For implementing `context.Context`. + - **Done**: (no-op) For implementing `context.Context`. + - **Err**: (no-op) For implementing `context.Context`.
414-414: Enhance the Value method description.To improve clarity and discovery:
- Wrap
Localsin backticks- Link to the API docs for
Value()- Clarify that it reads from
c.LocalsSuggested diff:
- - **Value**: For implementing `context.Context`. Returns request-scoped value from Locals. + - **Value**: Returns request-scoped values from `c.Locals`. See [API docs](./api/ctx.md#value) for details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx.go(4 hunks)docs/whats_new.md(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
🪛 GitHub Check: unit (1.24.x, macos-latest)
ctx.go
[failure] 43-43:
cannot use DefaultCtx{} (value of struct type DefaultCtx) as context.Context value in variable declaration: DefaultCtx does not implement context.Context (method Deadline has pointer receiver)
[failure] 42-42:
cannot use DefaultCtx{} (value of struct type DefaultCtx) as io.Writer value in variable declaration: DefaultCtx does not implement io.Writer (method Write has pointer receiver)
🪛 GitHub Check: unit (1.24.x, ubuntu-latest)
ctx.go
[failure] 43-43:
cannot use DefaultCtx{} (value of struct type DefaultCtx) as context.Context value in variable declaration: DefaultCtx does not implement context.Context (method Deadline has pointer receiver)
[failure] 42-42:
cannot use DefaultCtx{} (value of struct type DefaultCtx) as io.Writer value in variable declaration: DefaultCtx does not implement io.Writer (method Write has pointer receiver)
🪛 GitHub Check: unit (1.23.x, macos-latest)
ctx.go
[failure] 43-43:
cannot use DefaultCtx{} (value of type DefaultCtx) as context.Context value in variable declaration: DefaultCtx does not implement context.Context (method Deadline has pointer receiver)
[failure] 42-42:
cannot use DefaultCtx{} (value of type DefaultCtx) as io.Writer value in variable declaration: DefaultCtx does not implement io.Writer (method Write has pointer receiver)
[failure] 43-43:
cannot use DefaultCtx{} (value of type DefaultCtx) as context.Context value in variable declaration: DefaultCtx does not implement context.Context (method Deadline has pointer receiver)
[failure] 42-42:
cannot use DefaultCtx{} (value of type DefaultCtx) as io.Writer value in variable declaration: DefaultCtx does not implement io.Writer (method Write has pointer receiver)
🪛 GitHub Check: unit (1.23.x, ubuntu-latest)
ctx.go
[failure] 43-43:
cannot use DefaultCtx{} (value of type DefaultCtx) as context.Context value in variable declaration: DefaultCtx does not implement context.Context (method Deadline has pointer receiver)
[failure] 42-42:
cannot use DefaultCtx{} (value of type DefaultCtx) as io.Writer value in variable declaration: DefaultCtx does not implement io.Writer (method Write has pointer receiver)
[failure] 43-43:
cannot use DefaultCtx{} (value of type DefaultCtx) as context.Context value in variable declaration: DefaultCtx does not implement context.Context (method Deadline has pointer receiver)
[failure] 42-42:
cannot use DefaultCtx{} (value of type DefaultCtx) as io.Writer value in variable declaration: DefaultCtx does not implement io.Writer (method Write has pointer receiver)
🪛 GitHub Check: lint
ctx.go
[failure] 43-43:
cannot use DefaultCtx{} (value of type DefaultCtx) as context.Context value in variable declaration: DefaultCtx does not implement context.Context (method Deadline has pointer receiver) (typecheck)
[failure] 42-42:
cannot use DefaultCtx{} (value of type DefaultCtx) as io.Writer value in variable declaration: DefaultCtx does not implement io.Writer (method Write has pointer receiver)
🪛 GitHub Check: govulncheck-check
ctx.go
[failure] 43-43:
cannot use DefaultCtx{} (value of struct type DefaultCtx) as context.Context value in variable declaration: DefaultCtx does not implement context.Context (method Deadline has pointer receiver)
[failure] 42-42:
cannot use DefaultCtx{} (value of struct type DefaultCtx) as io.Writer value in variable declaration: DefaultCtx does not implement io.Writer (method Write has pointer receiver)
🪛 GitHub Actions: golangci-lint
ctx.go
[error] 42-42: cannot use DefaultCtx{} (value of type DefaultCtx) as io.Writer value in variable declaration: DefaultCtx does not implement io.Writer (method Write has pointer receiver)
🪛 GitHub Actions: Run govulncheck
ctx.go
[error] 42-42: cannot use DefaultCtx{} (value of struct type DefaultCtx) as io.Writer value in variable declaration: DefaultCtx does not implement io.Writer (method Write has pointer receiver)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (4)
ctx.go (4)
430-438: Well-designed no-op implementation for Deadline().The implementation correctly returns zero values to indicate no deadline support, which is appropriate given the fasthttp limitations documented in the comments.
440-450: Appropriate no-op implementation for Done().Returning
nilcorrectly indicates that this context cannot be canceled, which aligns with fasthttp's design constraints as documented.
476-486: Correct no-op implementation for Err().Returning
nilis appropriate since the context never enters an error state due to the no-op nature of cancellation in this implementation.
1836-1840: Excellent implementation of Value() method.This correctly exposes the existing Fiber locals mechanism through the standard context.Context interface, providing seamless integration with Go's context ecosystem.
|
tests are flaky |
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.