🔥 feat: Add Context Support to RequestID Middleware #3200
Conversation
WalkthroughThe changes introduce enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
gaby
changed the title
JIeJaitt
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: 5
🧹 Outside diff range and nitpick comments (24)
middleware/timeout/timeout.go (1)
Line range hint
11-28: Consider adding documentation about timeout behavior.While the implementation is solid, it would be helpful to add documentation comments explaining:
- The timeout behavior and its impact on request processing
- How custom timeout errors are handled
- Examples of common use cases
Example documentation:
+// New creates a timeout middleware that cancels the request processing if it takes +// longer than the specified duration. It returns fiber.ErrRequestTimeout when: +// - The context deadline is exceeded +// - Any of the specified custom errors (tErrs) occur +// +// Example: +// +// app.Use(timeout.New(handler, 5*time.Second)) +// +// // With custom timeout errors +// app.Use(timeout.New(handler, 5*time.Second, ErrSlowDB, ErrSlowAPI)) func New(h fiber.Handler, t time.Duration, tErrs ...error) fiber.Handler {middleware/expvar/expvar.go (1)
Line range hint
19-30: Consider adding security recommendations for debug endpoints.The
/debug/varsendpoint exposes runtime statistics which could be sensitive in production environments.Consider:
- Adding a configuration option to disable this middleware in production
- Documenting security best practices (e.g., IP restrictions, authentication) for protecting this endpoint
- Adding a warning log when this middleware is enabled in production
Would you like me to help draft the security documentation and implementation?
middleware/redirect/redirect.go (1)
33-36: Consider documenting query string security implicationsWhile the query string handling is implemented correctly, it might be worth adding a comment about potential security implications of preserving query parameters during redirects, especially for sensitive operations.
Consider adding a comment like:
+ // Preserve query string during redirect, ensure sensitive parameters are handled appropriately queryString := string(c.RequestCtx().QueryArgs().QueryString()) if queryString != "" { queryString = "?" + queryString }middleware/requestid/requestid_test.go (1)
54-84: LGTM! Well-structured table-driven tests.The test structure effectively covers both
fiber.Ctxandcontext.Contextscenarios, aligning well with the PR objective of adding context support.Consider updating to use
anyinstead ofinterface{}since the project uses Go 1.18+:type args struct { - inputFunc func(c fiber.Ctx) interface{} + inputFunc func(c fiber.Ctx) any }🧰 Tools
🪛 golangci-lint
[warning] 61-61: use-any: since GO 1.18 'interface{}' can be replaced by 'any'
(revive)
64-64: fieldalignment: struct with 24 pointer bytes could be 16
(govet)
[warning] 71-71: use-any: since GO 1.18 'interface{}' can be replaced by 'any'
(revive)
[warning] 79-79: use-any: since GO 1.18 'interface{}' can be replaced by 'any'
(revive)
middleware/pprof/pprof.go (1)
51-71: Consider security implications of exposed pprof endpoints.Since pprof endpoints expose sensitive runtime information about your application, ensure that:
- These endpoints are properly secured in production
- Access is restricted to authorized personnel
- Consider implementing additional middleware for authentication/authorization when accessing these endpoints
You might want to wrap this middleware with security checks or disable it completely in production unless explicitly needed for debugging.
middleware/etag/etag.go (1)
95-95: LGTM! Consider documenting the API changeThe change maintains consistency with the new context handling approach.
Consider adding a note in the framework's upgrade guide about the migration from
Context()toRequestCtx()to help users update their code.docs/middleware/timeout.md (3)
11-11: Enhance context propagation documentationWhile the explanation is accurate, it would be helpful to add more details about how the timeout context is propagated through the request lifecycle and its implications for downstream operations.
Consider adding:
-As a `fiber.Handler` wrapper, it creates a context with `context.WithTimeout` which is then used with `c.Context()`. +As a `fiber.Handler` wrapper, it creates a context with `context.WithTimeout` which is then used with `c.Context()`. This timeout context is propagated to all downstream operations, allowing them to respect the timeout constraint and gracefully handle cancellation.
87-87: Consider improving error variable namingWhile the implementation is correct, consider using a more descriptive error variable name.
-var ErrFooTimeOut = errors.New("foo context canceled") +var ErrOperationTimeout = errors.New("operation context canceled")
119-119: Improve DB transaction error handlingWhile the context usage is correct, the DB transaction example could be improved with better error handling and transaction management.
Consider updating the example to include transaction rollback:
handler := func(ctx fiber.Ctx) error { tran := db.WithContext(ctx.Context()).Begin() + // Ensure transaction rollback on panic + defer func() { + if r := recover(); r != nil { + tran.Rollback() + } + }() if tran = tran.Exec("SELECT pg_sleep(50)"); tran.Error != nil { + tran.Rollback() return tran.Error } if tran = tran.Commit(); tran.Error != nil { + tran.Rollback() return tran.Error } return nil }middleware/static/static.go (1)
Line range hint
117-147: Consider documenting context handling best practicesWhile the changes to use
RequestCtx()are correct, consider adding documentation about:
- The performance implications of context handling in static file serving
- Best practices for context propagation in middleware
- Guidelines for when to use
RequestCtx()vs other context methodsThis will help maintainers and contributors understand the architectural decisions behind these changes.
middleware/adaptor/adaptor.go (1)
Line range hint
37-111: Architectural consistency in context handlingThe consistent transition from
Context()toRequestCtx()across the adaptor package strengthens the middleware's context handling capabilities. This change properly supports the PR's objective of adding context support for RequestID middleware while maintaining the integrity of the net/http to Fiber adaptation layer.Consider documenting these context handling patterns in the package documentation to help maintainers understand the correct usage of
RequestCtx()vsContext().bind.go (1)
Line range hint
98-166: Architecture Feedback: Good progress on context standardizationThe consistent update of context handling across all binding methods is a positive architectural change that:
- Standardizes the context access pattern
- Prepares the groundwork for RequestID middleware enhancement
- Maintains backward compatibility while improving the codebase
Consider documenting these context-related changes in the framework's migration guide to help users understand the standardized approach.
redirect.go (1)
144-146: Consider improving error handling in content type parsing.While the context change is correct, the content type parsing and binding operations silently ignore potential errors. Consider adding error logging or handling for better debugging and reliability.
Example improvement:
- ctype := utils.ToLower(utils.UnsafeString(r.c.RequestCtx().Request.Header.ContentType())) - ctype = binder.FilterFlags(utils.ParseVendorSpecificContentType(ctype)) + ctype := utils.ToLower(utils.UnsafeString(r.c.RequestCtx().Request.Header.ContentType())) + if ctype == "" { + // Log or handle empty content type + ctype = MIMEApplicationForm // or appropriate default + } + ctype = binder.FilterFlags(utils.ParseVendorSpecificContentType(ctype))ctx_interface_gen.go (1)
54-55: LGTM: Consistent method namingThe renaming from
SetUserContext()toSetContext()maintains consistency with theContext()getter method and follows Go's standard naming patterns.Consider adding a context key type and constants for request ID to prevent key collisions:
type contextKey string const ( RequestIDKey contextKey = "requestID" )docs/whats_new.md (1)
232-234: Improve clarity of context-related changesThe sentence structure could be improved to better convey these important breaking changes. Consider adding commas and restructuring for clarity.
- Context has been renamed to RequestCtx which corresponds to the FastHTTP Request Context. - UserContext has been renamed to Context which returns a context.Context object. - SetUserContext has been renamed to SetContext. + Context has been renamed to RequestCtx, which corresponds to the FastHTTP Request Context. + UserContext has been renamed to Context, which returns a context.Context object. + SetUserContext has been renamed to SetContext.🧰 Tools
🪛 LanguageTool
[uncategorized] ~233-~233: Possible missing comma found.
Context: ...text. - UserContext has been renamed to Context which returns a context.Context object....(AI_HYDRA_LEO_MISSING_COMMA)
redirect_test.go (1)
Incomplete Test Coverage for Context Handling
A pending TODO in
middleware/csrf/csrf_test.goindicates that not all context-related edge cases are currently tested.
middleware/csrf/csrf_test.gocontains a TODO for a specific context test case.🔗 Analysis chain
Line range hint
1-642: Verify test coverage for context handling.The test suite appears comprehensive, covering various redirect scenarios. However, let's verify that all context-related edge cases are properly tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for context-related functionality # Look for potential test gaps in context handling # Search for context-related methods in the codebase echo "Searching for context-related methods that might need testing..." rg -l "RequestCtx|Context" --type go # Search for existing test coverage echo "Analyzing existing test coverage..." rg -l "Test.*Context|Test.*Ctx" --type go # Look for TODOs related to context testing echo "Checking for pending test TODOs..." rg -i "todo.*context|todo.*test" --type goLength of output: 2630
middleware/logger/logger_test.go (1)
Line range hint
635-648: Consider handling Fprintf errors in streaming testsWhile the implementation is functionally correct, consider handling the
fmt.Fprintferrors instead of ignoring them with//nolint:errcheck. This would make the tests more robust by ensuring all error conditions are properly handled.Example improvement:
-fmt.Fprintf(w, "data: Message: %s\n\n", msg) //nolint:errcheck // ignore error +if _, err := fmt.Fprintf(w, "data: Message: %s\n\n", msg); err != nil { + break +}Also applies to: 806-819, 961-974
docs/api/ctx.md (2)
1497-1508: Add usage example for RequestCtx()The documentation would benefit from including a practical example showing how to use the RequestCtx() method, particularly demonstrating its compatibility with the context.Context interface.
Consider adding this example:
+```go title="Example" +app.Get("/", func(c fiber.Ctx) error { + // Get the raw RequestCtx + reqCtx := c.RequestCtx() + + // Use it with context.Context compatible APIs + ctx := reqCtx.UserValue("context").(context.Context) + + // Or set a deadline + deadline := time.Now().Add(1 * time.Second) + reqCtx.SetDeadline(deadline) + + return nil +}) +``` <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [typographical] ~1505-~1505: Consider adding a comma here. Context: ...tCtx() *fasthttp.RequestCtx ``` :::info Please read the [Fasthttp Documentation](https... (PLEASE_COMMA) </details> </details> --- `1911-1922`: **Enhance SetContext example with practical use case** While the current example shows the basic usage, it would be more helpful to demonstrate a practical use case with context values and cancellation. Consider expanding the example: ```diff ```go title="Example" app.Get("/", func(c fiber.Ctx) error { - ctx := context.Background() - c.SetContext(ctx) - // Here ctx could be any context implementation + // Create a context with cancellation + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Add values to context + ctx = context.WithValue(ctx, "userID", "123") + + // Set the enhanced context + c.SetContext(ctx) + + // Use the context values in your handler + if userID, ok := ctx.Value("userID").(string); ok { + return c.SendString("Processing request for user: " + userID) + } // ... })ctx.go (2)
391-397: Consider adding error logging for type assertion.The implementation is solid but could benefit from debug-level logging when type assertion fails, helping with troubleshooting in production.
func (c *DefaultCtx) Context() context.Context { ctx, ok := c.fasthttp.UserValue(userContextKey).(context.Context) if !ok { + if ctx != nil { + c.app.config.Logger.Debugf("Failed to assert stored context value to context.Context") + } ctx = context.Background() c.SetContext(ctx) } return ctx }
403-406: Add nil check for context parameter.While the current implementation works, adding a nil check would prevent potential issues and provide better error feedback.
func (c *DefaultCtx) SetContext(ctx context.Context) { + if ctx == nil { + panic("nil context provided to SetContext") + } c.fasthttp.SetUserValue(userContextKey, ctx) }ctx_test.go (2)
Line range hint
855-874: Consider adding error cases to Context() testsWhile the test covers the basic functionality well, consider adding test cases for:
- Context cancellation scenarios
- Timeout scenarios
- Error propagation through context chain
t.Run("CancelledContext", func(t *testing.T) { t.Parallel() ctx, cancel := context.WithCancel(context.Background()) cancel() c.SetContext(ctx) require.Error(t, ctx.Err()) })
3223-3225: Fix indentation in test assertionThe indentation is inconsistent with the rest of the file.
- require.Equal(t, "public, max-age=100", string(c.RequestCtx().Response.Header.Peek(HeaderCacheControl)), "CacheControl Control") - require.Equal(t, StatusOK, c.Response().StatusCode()) + require.Equal(t, "public, max-age=100", string(c.RequestCtx().Response.Header.Peek(HeaderCacheControl)), "CacheControl Control") + require.Equal(t, StatusOK, c.Response().StatusCode())middleware/requestid/requestid.go (1)
64-64: Avoid logging errors in utility functionsLogging errors within utility functions can lead to unnecessary log output and might not be appropriate at this abstraction level. Consider handling the unsupported type without logging.
Modify the default case to handle unsupported context types gracefully:
default: - log.Errorf("Unsupported context type: %T", c) + // Unsupported context type; return empty string }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
bind.go(5 hunks)client/client_test.go(1 hunks)ctx.go(4 hunks)ctx_interface_gen.go(1 hunks)ctx_test.go(4 hunks)docs/api/ctx.md(3 hunks)docs/middleware/timeout.md(4 hunks)docs/whats_new.md(1 hunks)middleware/adaptor/adaptor.go(3 hunks)middleware/adaptor/adaptor_test.go(3 hunks)middleware/cache/cache_test.go(1 hunks)middleware/compress/compress.go(1 hunks)middleware/etag/etag.go(2 hunks)middleware/expvar/expvar.go(1 hunks)middleware/idempotency/idempotency.go(1 hunks)middleware/logger/logger_test.go(3 hunks)middleware/pprof/pprof.go(1 hunks)middleware/redirect/redirect.go(1 hunks)middleware/requestid/requestid.go(2 hunks)middleware/requestid/requestid_test.go(1 hunks)middleware/static/static.go(2 hunks)middleware/timeout/timeout.go(1 hunks)middleware/timeout/timeout_test.go(2 hunks)redirect.go(1 hunks)redirect_test.go(8 hunks)
🧰 Additional context used
📓 Learnings (2)
middleware/adaptor/adaptor.go (1)
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-12T14:10:51.810Z
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`.
middleware/adaptor/adaptor_test.go (1)
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-12T14:10:51.810Z
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`.
🪛 LanguageTool
docs/api/ctx.md
[typographical] ~1505-~1505: Consider adding a comma here.
Context: ...tCtx() *fasthttp.RequestCtx ``` :::info Please read the [Fasthttp Documentation](https...
(PLEASE_COMMA)
docs/whats_new.md
[uncategorized] ~233-~233: Possible missing comma found.
Context: ...text. - UserContext has been renamed to Context which returns a context.Context object....
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 golangci-lint
middleware/requestid/requestid.go
[warning] 53-53: use-any: since GO 1.18 'interface{}' can be replaced by 'any'
(revive)
middleware/requestid/requestid_test.go
[warning] 61-61: use-any: since GO 1.18 'interface{}' can be replaced by 'any'
(revive)
64-64: fieldalignment: struct with 24 pointer bytes could be 16
(govet)
[warning] 71-71: use-any: since GO 1.18 'interface{}' can be replaced by 'any'
(revive)
[warning] 79-79: use-any: since GO 1.18 'interface{}' can be replaced by 'any'
(revive)
87-87: The copy of the 'for' variable "tt" can be deleted (Go 1.22+)
(copyloopvar)
🔇 Additional comments (33)
middleware/redirect/redirect.go (1)
33-33: LGTM! Note Fiber v3 requirement
The change from Context() to RequestCtx() aligns with the framework-wide standardization of context handling. This is a breaking change that requires Fiber v3.
middleware/requestid/requestid_test.go (1)
86-109: LGTM! Thread-safe parallel test implementation.
The implementation correctly:
- Rebinds the loop variable for parallel test safety
- Validates context propagation across middleware
- Uses clear assertions to verify the request ID flow
🧰 Tools
🪛 golangci-lint
87-87: The copy of the 'for' variable "tt" can be deleted (Go 1.22+)
(copyloopvar)
middleware/pprof/pprof.go (1)
51-71: LGTM! Context handling update aligns with framework standardization.
The consistent replacement of Context() with RequestCtx() across all pprof handlers aligns with the framework-wide context handling improvements. This change supports better context propagation while maintaining the core profiling functionality.
Let's verify that the pprof handlers can properly handle the RequestCtx:
✅ Verification successful
Verified: pprof handlers are compatible with RequestCtx.
No issues found regarding the changes from Context() to RequestCtx() in pprof handlers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that pprof handlers are compatible with RequestCtx
# Search for any special handling of fasthttp.RequestCtx in the pprof adaptor
rg -A 5 'RequestCtx' ./vendor/github.com/valyala/fasthttp/fasthttpadaptor
# Check if there are any existing issues or discussions about RequestCtx compatibility
gh issue list --search "pprof RequestCtx"
gh pr list --search "pprof RequestCtx"
Length of output: 440
middleware/timeout/timeout_test.go (2)
23-26: LGTM! Robust error handling pattern.
The context change aligns with the framework's standardization efforts. The multi-level error wrapping provides good debugging context while preserving the original error.
55-58: LGTM! Good practice with custom error testing.
The test properly validates timeout behavior with custom errors, ensuring middleware flexibility. The error wrapping maintains consistency with the standard timeout case.
middleware/etag/etag.go (1)
83-83: Verify consistent usage of RequestCtx() across the codebase
The change from Context() to RequestCtx() aligns with the framework's standardization of context handling.
Let's verify this change is consistently applied across the codebase:
✅ Verification successful
Consistent usage of RequestCtx() verified across the codebase
No remaining instances of Context().ResetBody() found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of Context().ResetBody()
# to ensure complete migration to RequestCtx().ResetBody()
# Search for any remaining instances of the old pattern
echo "Checking for remaining Context().ResetBody() calls:"
rg "Context\(\)\.ResetBody\(\)"
# Search for the new pattern to confirm consistent usage
echo -e "\nConfirming RequestCtx().ResetBody() usage:"
rg "RequestCtx\(\)\.ResetBody\(\)"
Length of output: 393
docs/middleware/timeout.md (1)
41-41: LGTM! Well-structured example with proper context handling
The example correctly demonstrates the usage of context for timeout handling, including proper cleanup of resources.
middleware/idempotency/idempotency.go (1)
54-54: LGTM! Consistent with framework's context standardization.
The change from Context() to RequestCtx() aligns with the framework-wide standardization of context handling.
Let's verify the consistency of context usage across this file:
middleware/static/static.go (3)
125-129: LGTM: Consistent context usage for response handling
The changes correctly use RequestCtx() for both status code retrieval and header manipulation, maintaining consistency with the new context handling approach.
145-147: Verify resolution of Issue #420
The response reset operations are correctly using RequestCtx(). However, let's verify if these changes fully address Issue #420.
#!/bin/bash
# Check if Issue #420 is resolved and if there are any related issues
gh issue view 420 2>/dev/null || echo "Issue not found or private"
# Look for any related test cases
rg -l "Issue #420" --type=go117-117: LGTM: Correct context method usage
The change to c.RequestCtx() is correct as it provides the raw fasthttp context needed by the file handler.
middleware/adaptor/adaptor.go (2)
37-37: LGTM: Correct usage of RequestCtx()
The change from Context() to RequestCtx() is appropriate here as it properly provides the FastHTTP request context to the handler.
46-46: LGTM: Proper context conversion
The change correctly uses RequestCtx() for converting the Fiber context to an http.Request, maintaining proper compatibility with the FastHTTP adapter.
bind.go (4)
98-98: LGTM: Context handling update is correct
The change from Context() to RequestCtx() aligns with the framework's standardized context handling approach.
107-107: LGTM: Consistent context handling update
The context handling change maintains consistency with other binding methods.
134-134: LGTM: Form binding context updates are consistent
Both form-related methods now correctly use RequestCtx(), maintaining consistency in context handling across different form binding scenarios.
Also applies to: 152-152
166-166: Verify content type handling with the new context
While the change to RequestCtx() is correct, let's verify that content type detection works consistently across different scenarios.
✅ Verification successful
Content type handling is correctly implemented and consistent
All content types utilize defined constants, ensuring uniform handling across different scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify content type handling across test files
# Look for test cases that verify different content types
# Search for content type related tests
rg -l "ContentType|MIMEApplication|MIMEText" --type go
# Search for specific content type constants usage
ast-grep --pattern 'const (
$$$
MIME$_ = $_
$$$
)'
Length of output: 704
Script:
#!/bin/bash
# Description: Search for MIME type definitions using alternative patterns
# Search for MIME type strings directly in the code
rg -F "application/" --type go
rg -F "text/" --type go
# Search for MIME type usage in functions or variables
rg "ContentType\(\)|SetContentType" --type go
# Use ast-grep to find functions that set or get content types
ast-grep --pattern 'func $_($$$) {
$$$
SetContentType($_)
$$$
}'
Length of output: 26172
redirect.go (1)
144-144: LGTM! Context handling change aligns with framework standardization.
The change from Context() to RequestCtx() is correct and consistent with the PR's objective of standardizing context handling across the framework. This modification maintains the same functionality while following the new context access pattern.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
Verified: Context handling is consistently standardized with RequestCtx().
All instances of Context() accessing Request.Header have been successfully replaced with RequestCtx(), ensuring uniform context handling across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of RequestCtx() vs Context() in the codebase
# This helps ensure the standardization is applied uniformly
# Check for any remaining usage of Context() method for request header access
rg -A 2 "Context\(\).*Request\.Header"
# Check for the new pattern usage
rg -A 2 "RequestCtx\(\).*Request\.Header"
Length of output: 2608
middleware/adaptor/adaptor_test.go (3)
165-173: LGTM! Context value access updated correctly.
The changes properly transition from Context() to RequestCtx() for accessing context values, maintaining consistency with the framework's context handling approach.
319-320: LGTM! Test assertions updated appropriately.
The changes correctly update the test assertions to use RequestCtx() for accessing request properties, maintaining consistency with the framework's context handling approach.
Also applies to: 324-324
395-396: LGTM! Nil body test case updated correctly.
The changes properly update the nil body test case to use RequestCtx(), maintaining consistency with the framework's context handling approach.
ctx_interface_gen.go (2)
51-53: LGTM: Method rename follows Go conventions
The renaming from UserContext() to Context() better aligns with Go's standard context package conventions. This change makes the API more intuitive for Go developers.
48-50: LGTM: Method rename improves clarity
The renaming from Context() to RequestCtx() better reflects its purpose of returning the underlying FastHTTP request context. This change aligns with the PR's objective of improving context handling.
Let's verify the impact on existing code:
docs/whats_new.md (1)
Line range hint 1-24: Complete draft sections in documentation
Several sections are marked as "DRAFT" and need to be completed:
- Context section
- Binding section
- Redirect section
- Generic functions section
Consider prioritizing the completion of these sections to ensure comprehensive documentation for v3 release.
✅ Verification successful
All draft sections have been completed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for remaining draft sections in the documentation
echo "Checking for draft sections..."
rg -i "draft" docs/whats_new.md
Length of output: 204
Script:
#!/bin/bash
# List all sections and indicate if they are marked as draft
echo "Listing sections and their draft status..."
rg -i "^## " docs/whats_new.md | while read -r line; do
section=$(echo "$line" | sed -n 's/^## //p')
if echo "$section" | grep -iq "draft"; then
status="DRAFT"
else
status="COMPLETE"
fi
echo "$section: $status"
done
Length of output: 2427
🧰 Tools
🪛 LanguageTool
[uncategorized] ~233-~233: Possible missing comma found.
Context: ...text. - UserContext has been renamed to Context which returns a context.Context object....
(AI_HYDRA_LEO_MISSING_COMMA)
redirect_test.go (1)
45-45: LGTM! Consistent updates to context handling in tests.
The changes consistently update the request context handling across all test cases, properly simulating cookie handling for testing purposes. The modifications align well with the PR's objective of enhancing context support.
Also applies to: 188-188, 239-239, 276-276, 312-312, 356-356, 541-541, 632-632
middleware/logger/logger_test.go (1)
635-635: LGTM! Consistent update to use RequestCtx()
The changes consistently update the method calls from Context() to RequestCtx() across all streaming test cases, aligning with the PR's objective to enhance context support.
Also applies to: 806-806, 961-961
middleware/cache/cache_test.go (1)
897-897: LGTM! Context method change aligns with PR objectives.
The change from Context() to RequestCtx() is correct and consistent with the PR's goal of enhancing context support in the Fiber framework. The test functionality remains unchanged as both methods provide access to the URI information needed for the test case.
client/client_test.go (1)
1575-1575: LGTM: Context method change aligns with standardization.
The change from Context() to RequestCtx() is consistent with the PR's objective of standardizing context handling across the framework.
Let's verify that the proxy functionality remains intact after this change:
ctx.go (2)
385-387: LGTM! Clear and concise implementation.
The method provides clean access to the underlying fasthttp context with appropriate documentation.
1192-1193: LGTM! Good refactoring for consistency.
The change to use RequestCtx() aligns with the new context access pattern while maintaining the same efficient implementation.
ctx_test.go (3)
846-853: LGTM: Test coverage for RequestCtx() is adequate
The test verifies the basic functionality of RequestCtx() by checking the returned type. The test is simple but sufficient for this getter method.
875-886: LGTM: SetContext test provides good coverage
The test properly verifies that context values are maintained after setting a new context. Good use of test utilities and assertions.
Line range hint 888-923: Excellent concurrent request testing
The test thoroughly verifies context isolation between multiple concurrent requests using parallel subtests. Good practices:
- Uses t.Parallel() for concurrent testing
- Verifies proper status codes
- Checks response body content
- Tests multiple requests (1-10)
middleware/timeout/timeout.go
Outdated
| @@ -11,9 +11,9 @@ import ( | |||
| // New implementation of timeout middleware. Set custom errors(context.DeadlineExceeded vs) for get fiber.ErrRequestTimeout response. | |||
| func New(h fiber.Handler, t time.Duration, tErrs ...error) fiber.Handler { | |||
| return func(ctx fiber.Ctx) error { | |||
| timeoutContext, cancel := context.WithTimeout(ctx.UserContext(), t) | |||
| timeoutContext, cancel := context.WithTimeout(ctx.Context(), t) | |||
There was a problem hiding this comment.
💡 Codebase verification
Remaining instances of UserContext and SetUserContext found.
Please remove or update the following instances to ensure consistent context handling:
middleware/requestid/requestid.godocs/whats_new.md
🔗 Analysis chain
LGTM! Context handling changes look correct.
The changes to use ctx.Context() and SetContext() align well with the PR objectives and maintain proper context propagation. The implementation correctly handles context cancellation and cleanup.
Let's verify the context usage pattern across the codebase:
Also applies to: 16-16
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent context handling patterns
# Expected: All context-related calls should use the new pattern
# Check for any remaining instances of UserContext
rg "UserContext|SetUserContext" -l
# Verify consistent timeout context usage pattern
ast-grep --pattern 'context.WithTimeout($ctx.Context(), $_)'
Length of output: 151
middleware/expvar/expvar.go
Outdated
| @@ -25,7 +25,7 @@ func New(config ...Config) fiber.Handler { | |||
| return c.Next() | |||
| } | |||
| if path == "/debug/vars" { | |||
| expvarhandler.ExpvarHandler(c.Context()) | |||
| expvarhandler.ExpvarHandler(c.RequestCtx()) | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent context usage detected across middleware. Please update the following files to use RequestCtx():
middleware/timeout/timeout.gomiddleware/requestid/requestid.gomiddleware/adaptor/adaptor.go
🔗 Analysis chain
LGTM! Verify consistency of context usage.
The change from Context() to RequestCtx() aligns with the framework-wide context handling improvements.
Let's verify the consistency of this change across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that Context() is not used where RequestCtx() should be used
# Expected: No occurrences of Context() in middleware handlers
# Search for any remaining Context() calls in middleware
rg -g '!*.md' -g '!*.txt' -g '!*_test.go' 'Context\(\)' ./middleware/
Length of output: 354
middleware/compress/compress.go
Outdated
| @@ -56,7 +56,7 @@ func New(config ...Config) fiber.Handler { | |||
| } | |||
|
|
|||
| // Compress response | |||
| compressor(c.Context()) | |||
| compressor(c.RequestCtx()) | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of context detected.
Usage of c.Context() found in:
middleware/requestid/requestid.go: ctx := context.WithValue(c.Context(), requestIDKey, rid)
🔗 Analysis chain
LGTM! Consistent with context standardization.
The change from Context() to RequestCtx() aligns with the framework's context handling standardization and maintains type safety with fasthttp's compressor.
Let's verify the consistency of this pattern across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistent usage of RequestCtx() instead of Context()
# across the middleware packages
# Check for any remaining usage of Context() method
rg -g '!*.md' -g '!*_test.go' 'c\.Context\(\)' ./middleware
# Verify the adoption of RequestCtx()
rg -g '!*.md' -g '!*_test.go' 'RequestCtx\(\)' ./middleware
Length of output: 2082
middleware/adaptor/adaptor.go
Outdated
| @@ -108,7 +108,7 @@ func HTTPMiddleware(mw func(http.Handler) http.Handler) fiber.Handler { | |||
| c.Request().Header.Set(key, v) | |||
| } | |||
| } | |||
| CopyContextToFiberContext(r.Context(), c.Context()) | |||
| CopyContextToFiberContext(r.Context(), c.RequestCtx()) | |||
There was a problem hiding this comment.
💡 Codebase verification
Insufficient test coverage for context propagation across middleware chains
The current tests do not cover context propagation across multiple middleware components. Please add comprehensive tests to ensure context values are properly preserved throughout the middleware chain.
🔗 Analysis chain
Verify context propagation in middleware chain
The change to use RequestCtx() is correct, but let's verify that context values are properly preserved across the middleware chain, especially for the RequestID middleware which is the focus of this PR.
Let's verify the context propagation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper context value handling in the middleware chain
# Look for RequestID middleware usage and context handling
echo "Checking RequestID middleware implementation and context usage:"
rg -A 5 "requestID.*Context|Context.*requestID"
# Look for potential test cases verifying context propagation
echo "Checking test coverage for context propagation:"
rg -A 5 "Test.*Context.*RequestID|Test.*RequestID.*Context"
Length of output: 1107
Script:
#!/bin/bash
# Description: Verify context propagation across all middleware components
# Check all middleware files for context handling
echo "Checking all middleware for context handling and propagation:"
rg -A 5 "context\.WithValue|SetContext|CopyContextToFiberContext" middleware/
# Check for tests that cover multiple middleware components and context propagation
echo "Checking for comprehensive test cases covering context propagation across middleware:"
rg -A 5 "Test.*Middleware.*Context|Test.*Context.*Propagation" tests/
Length of output: 3072
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/middleware/requestid.md (2)
52-53: Enhance the context support documentationWhile the introduction of context support is clear, consider adding more details about:
- Why this change is beneficial (e.g., better integration with Go's standard context)
- Migration notes for users upgrading from v2
Consider expanding the text like this:
-In version v3, Fiber will inject `requestID` into the built-in `Context` of Go. +In version v3, Fiber injects `requestID` into Go's built-in `Context`, enabling better integration with standard Go packages and improving request tracing across service boundaries. If you're upgrading from v2, you'll need to update your code to use `c.Context()` instead of directly accessing the context.
54-60: Add error handling to the example codeThe example demonstrates the new context usage correctly, but it would be more complete with error handling for cases where the requestID might not be present in the context.
Consider updating the example like this:
func handler(c fiber.Ctx) error { id := requestid.FromContext(c.Context()) + if id == "" { + // Handle case where requestID is not found + log.Printf("Warning: No request ID found in context") + id = "unknown" + } log.Printf("Request ID: %s", id) return c.SendString("Hello, World!") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/middleware/requestid.md(1 hunks)
🔇 Additional comments (1)
docs/middleware/requestid.md (1)
Line range hint 8-11: Verify function signatures with implementation
Let's ensure the documented function signatures match the actual implementation.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3200 +/- ##
==========================================
- Coverage 82.79% 82.77% -0.03%
==========================================
Files 114 114
Lines 11151 11163 +12
==========================================
+ Hits 9233 9240 +7
- Misses 1519 1523 +4
- Partials 399 400 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
benchmark is ok, just the current problem with the same names @JIeJaitt can you check the lint hints and the not covered code lines |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
middleware/requestid/requestid.go (1)
53-64: Enhance documentation and error handlingWhile the implementation is solid, consider these improvements:
- Add documentation comments to describe supported context types
- Provide more descriptive error message with example usage
// FromContext returns the request ID from context. // If there is no request ID, an empty string is returned. +// Supported context types: +// - fiber.Ctx: Retrieves request ID from Locals +// - context.Context: Retrieves request ID from context values func FromContext(c any) string { switch ctx := c.(type) { case fiber.Ctx: if rid, ok := ctx.Locals(requestIDKey).(string); ok { return rid } case context.Context: if rid, ok := ctx.Value(requestIDKey).(string); ok { return rid } default: - log.Errorf("Unsupported context type: %T", c) + log.Errorf("Unsupported context type: %T. Expected fiber.Ctx or context.Context", c) } return "" }middleware/requestid/requestid_test.go (2)
64-109: Consider adding more test cases for better coverage.The current test cases effectively verify the basic functionality. Consider adding:
- A negative test case with nil context
- Verification of response headers to ensure the request ID is properly set
- A test case with a pre-existing request ID in the request header
Here's a suggested addition to the test cases:
tests := []struct { name string args args + checkHeader bool // whether to verify response header + inputHeader string // pre-existing request ID }{ // ... existing test cases ... + { + name: "With nil context", + args: args{ + inputFunc: func(c fiber.Ctx) any { + return nil + }, + }, + }, + { + name: "Verify response header", + args: args{ + inputFunc: func(c fiber.Ctx) any { + return c + }, + }, + checkHeader: true, + }, + { + name: "With existing request ID", + args: args{ + inputFunc: func(c fiber.Ctx) any { + return c + }, + }, + inputHeader: "existing-id", + }, }🧰 Tools
🪛 golangci-lint
64-64: fieldalignment: struct with 24 pointer bytes could be 16
(govet)
87-87: The copy of the 'for' variable "tt" can be deleted (Go 1.22+)
(copyloopvar)
87-87: Consider Go 1.22 compatibility for loop variable handling.The line
tt := tt // Re bind variablesis a common pattern to prevent issues with loop variable capture in goroutines. While this is still good practice, note that it becomes unnecessary in Go 1.22+ as the language handles this automatically.If you're targeting Go 1.22+, you can safely remove this line. However, if you need to support older versions, it's best to keep it.
🧰 Tools
🪛 golangci-lint
87-87: The copy of the 'for' variable "tt" can be deleted (Go 1.22+)
(copyloopvar)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
middleware/requestid/requestid.go(2 hunks)middleware/requestid/requestid_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint
middleware/requestid/requestid_test.go
64-64: fieldalignment: struct with 24 pointer bytes could be 16
(govet)
87-87: The copy of the 'for' variable "tt" can be deleted (Go 1.22+)
(copyloopvar)
🔇 Additional comments (3)
middleware/requestid/requestid.go (2)
4-7: LGTM: Required imports added for context support
The new imports are necessary for the enhanced context handling functionality.
42-44: Verify backward compatibility with existing code
The implementation correctly stores the request ID in both Locals and Context. However, we should verify that existing code relying on Locals continues to work as expected.
✅ Verification successful
Let me check the actual usage patterns to better understand how the request ID is being accessed.
Implementation is backward compatible and follows documented patterns
The implementation correctly maintains backward compatibility because:
- The
FromContextfunction handles both access patterns (Locals and Context) in its implementation - The documentation explicitly mentions the v3 Context support as a new feature while maintaining the existing patterns
- The request ID is stored in both Locals and Context, ensuring both old and new access patterns work
- The order of operations (setting Locals before Context) is correct and doesn't affect functionality
The changes align perfectly with the documented behavior and maintain compatibility with existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usage patterns of requestID in the codebase
# to ensure backward compatibility
# Look for direct Locals access for requestID
rg -l "Locals.*requestID"
# Look for FromContext usage
rg -l "FromContext.*Context"
Length of output: 121
Script:
#!/bin/bash
# Check the actual implementation details
# Show the content of requestid.go to see how Locals is used
rg -A 5 "Locals.*requestID" middleware/requestid/requestid.go
# Show the content of the documentation to understand the intended usage
cat docs/middleware/requestid.md
Length of output: 2758
middleware/requestid/requestid_test.go (1)
54-63: LGTM! Well-structured test setup.
The test structure follows Go best practices with:
- Parallel test execution
- Table-driven test pattern
- Clear type definitions
…/add-user-context-support
|
@ReneWerner87 Hi, I fixed all the lint errors on my local computer, I just committed my latest code on this branch.
|
|
|
|
@ReneWerner87 Sorry, I haven't submitted my code yet when I sent the message. There must be no problem now.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
middleware/requestid/requestid.go (2)
53-67: Consider improving error logging behaviorThe implementation correctly handles both context types, but consider making the error logging less verbose in production environments to prevent log spam.
Consider this improvement:
- log.Errorf("Unsupported context type: %T. Expected fiber.Ctx or context.Context", c) + if fiber.IsDebug() { + log.Errorf("Unsupported context type: %T. Expected fiber.Ctx or context.Context", c) + }
42-67: Well-designed context integrationThe implementation elegantly supports both traditional Fiber context and Go's built-in context, providing a smooth migration path for existing code while enabling modern context-based patterns. The dual storage (Locals and Context) ensures backward compatibility without compromising the new functionality.
Consider documenting this dual-storage pattern in the middleware's documentation to help users understand when to use each approach:
- Use Locals for legacy code and simple scenarios
- Use Context for proper context propagation across service boundaries
middleware/requestid/requestid_test.go (1)
60-84: Consider enhancing the test case structure.While the table-driven approach is good, consider these improvements:
- Add an
expectedfield to the test case struct to make assertions more explicit- Add negative test cases (e.g., nil context, invalid context type)
Here's a suggested improvement:
type args struct { inputFunc func(c fiber.Ctx) any } tests := []struct { name string args args + expected string + wantErr bool }{ { name: "From fiber.Ctx", args: args{ inputFunc: func(c fiber.Ctx) any { return c }, }, + expected: reqID, + wantErr: false, }, { name: "From context.Context", args: args{ inputFunc: func(c fiber.Ctx) any { return c.Context() }, }, + expected: reqID, + wantErr: false, }, + { + name: "With nil context", + args: args{ + inputFunc: func(c fiber.Ctx) any { + return nil + }, + }, + expected: "", + wantErr: true, + }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
middleware/requestid/requestid.go(2 hunks)middleware/requestid/requestid_test.go(1 hunks)
🔇 Additional comments (3)
middleware/requestid/requestid.go (2)
4-7: LGTM: Appropriate imports for context support
The added imports are necessary and correctly chosen for the new context functionality.
42-44: LGTM: Proper context propagation implementation
The implementation correctly adds the request ID to both Locals and Context, maintaining backward compatibility while adding new functionality.
Let's verify that the context is properly handled in downstream middleware:
✅ Verification successful
The previous search might have failed because we need to understand the broader usage pattern. Let's check for imports and direct references first, along with the middleware structure to better target our search.
From the previous results, we found that the logger middleware is using requestid, and we should examine how it's being used and tested. We also need to verify if there are any direct context value retrievals in the codebase.
From the test files, we can see that there's a FromContext function being used, but we need to verify its implementation and how it's being used in the actual middleware to ensure proper context handling.
Based on the code examination, I can now generate the final response:
Context implementation is properly handled and verified
The implementation correctly stores and retrieves the request ID in both ways:
- Uses unexported
contextKeytype to prevent collisions - Stores in both Locals and Context for compatibility
- Provides a type-safe
FromContextfunction that handles bothfiber.Ctxandcontext.Context - Test coverage confirms proper context propagation
The logger middleware demonstrates proper consumption of the request ID through locals, and the FromContext implementation ensures safe access across the middleware chain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any middleware that might be accessing the request ID through context
rg -l "c\.Context\(\)" --type go | xargs rg "requestid\." -A 5
Length of output: 64
Script:
#!/bin/bash
# Let's first check for any direct imports or usage of the requestid package
rg "requestid" --type go -B 2 -A 2
# Also search for any GetReqID or similar methods that might be accessing the request ID
rg "GetReq.*ID|GetRequestID" --type go -B 2 -A 2
# Check the middleware package structure and potential usage
fd -e go -p "middleware.*"
Length of output: 5270
Script:
#!/bin/bash
# Let's examine the logger middleware which we found is using requestid
cat middleware/logger/logger_test.go
# Also check the requestid test file for context usage patterns
cat middleware/requestid/requestid_test.go
# Look for any context.Value usage with request ID
rg "context\.Value.*requestid" --type go -B 2 -A 2
Length of output: 27322
Script:
#!/bin/bash
# Let's check for any FromContext implementation and context key usage
cat middleware/requestid/requestid.go
# Also check if there are any other files using the request ID from context
rg "FromContext|GetRequestID" --type go -B 2 -A 2
Length of output: 16801
middleware/requestid/requestid_test.go (1)
54-59: LGTM! Well-structured test setup.
The test function follows Go's testing best practices with parallel execution enabled and clear test data setup.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| app := fiber.New() | ||
| app.Use(New(Config{ | ||
| Generator: func() string { | ||
| return reqID | ||
| }, | ||
| })) | ||
|
|
||
| var ctxVal string | ||
|
|
||
| app.Use(func(c fiber.Ctx) error { | ||
| ctxVal = FromContext(tt.args.inputFunc(c)) | ||
| return c.Next() | ||
| }) | ||
|
|
||
| _, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil)) | ||
| require.NoError(t, err) | ||
| require.Equal(t, reqID, ctxVal) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fix potential race condition and add cleanup.
There are several issues in the test execution:
- The
ctxValvariable is shared between parallel test executions, which could lead to race conditions - The Fiber app should be cleaned up after tests
- The response status code should be verified
Here's how to fix these issues:
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
app := fiber.New()
+ t.Cleanup(func() {
+ _ = app.Shutdown()
+ })
app.Use(New(Config{
Generator: func() string {
return reqID
},
}))
- var ctxVal string
+ var (
+ ctxVal string
+ err error
+ )
app.Use(func(c fiber.Ctx) error {
- ctxVal = FromContext(tt.args.inputFunc(c))
+ defer func() {
+ if r := recover(); r != nil && tt.wantErr {
+ err = fiber.ErrInternalServerError
+ }
+ }()
+ ctxVal = FromContext(tt.args.inputFunc(c))
return c.Next()
})
- _, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil))
- require.NoError(t, err)
- require.Equal(t, reqID, ctxVal)
+ resp, testErr := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil))
+ if tt.wantErr {
+ require.Error(t, err)
+ return
+ }
+ require.NoError(t, testErr)
+ require.Equal(t, fiber.StatusNotFound, resp.StatusCode)
+ require.Equal(t, tt.expected, ctxVal)
})
}Committable suggestion skipped: line range outside the PR's diff.
|
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 |
Description
In most business project scenarios of Go, it is a simple process from router to controller to service layer. Fiber is the operation of the routing layer, and we will convert Fiber's ctx into Go's built-in
Contextat the controller layer. Moreover, GO's built-inContextis more concurrent and secure at the service layer.To ensure link tracing of service invocations in the service layer, the
requestIDneeds to be passed to the service layer. Therefore, here we optimize Fiber'sUserContextto inject therequestIDinto itFixes #3175