fix: Context method #126
fix: Context method #126
Conversation
…ontext(ctx).Info()
|
Warning Rate limit exceeded@hwbrzzl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces enhancements to context management in the Goravel framework. The changes focus on improving how context values are stored and retrieved by introducing a new map-based approach in Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
context.go (2)
64-79: Optimize internal key lookupThe current implementation uses a nested loop to check for internal keys. This can be optimized using a map for O(1) lookup.
Consider this optimization:
+var internalContextKeysMap = make(map[any]bool) + +func init() { + for _, key := range internalContextKeys { + internalContextKeysMap[key] = true + } +} func (c *Context) Context() context.Context { ctx := c.instance.UserContext() values := c.getGoravelContextValues() for key, value := range values { - skip := false - for _, internalContextKey := range internalContextKeys { - if key == internalContextKey { - skip = true - } - } - - if !skip { + if !internalContextKeysMap[key] { ctx = context.WithValue(ctx, key, value) } } return ctx }
107-114: Add documentation for getGoravelContextValuesThe helper method would benefit from documentation explaining its purpose and behavior.
Add this documentation:
+// getGoravelContextValues retrieves the map of context values stored under contextKey. +// Returns an empty map if no values are found or if type assertion fails. func (c *Context) getGoravelContextValues() map[any]any { value := c.instance.UserContext().Value(contextKey) if goravelCtxVal, ok := value.(map[any]any); ok { return goravelCtxVal } return make(map[any]any) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
context.go(4 hunks)context_request.go(2 hunks)context_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- context_test.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
context.go
55-55: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
🔇 Additional comments (2)
context.go (1)
95-99: LGTM: Value method implementation is correct
The method properly checks the values map first before falling back to the context's Value method.
context_request.go (1)
142-142: LGTM: Session handling changes are consistent
The changes correctly use the new sessionKey constant across all session-related methods, maintaining consistency with the changes in context.go.
Also applies to: 395-399, 403-404
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
context.go (2)
12-23:⚠️ Potential issueUse typed keys for context values
Using string constants as context keys is prone to collisions. Consider using custom types for context keys following Go's best practices.
+type userContextKeyType struct{} +type contextKeyType struct{} +type sessionKeyType struct{} const ( - userContextKey = "goravel_userContextKey" - contextKey = "goravel_contextKey" - sessionKey = "goravel_session" + userContextKey = userContextKeyType{} + contextKey = contextKeyType{} + sessionKey = sessionKeyType{} )
54-57:⚠️ Potential issueAdd nil check for values map
The current implementation assumes the map exists. Add a nil check to ensure the map is initialized.
func (c *Context) WithValue(key any, value any) { values := c.getGoravelContextValues() + if values == nil { + values = make(map[any]any) + } values[key] = value ctx := context.WithValue(c.instance.UserContext(), contextKey, values) c.instance.SetUserContext(ctx) }🧰 Tools
🪛 golangci-lint (1.62.2)
57-57: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
🧹 Nitpick comments (1)
context.go (1)
69-84: Optimize internal keys lookupThe current implementation uses nested loops to check for internal keys. Consider using a map for O(1) lookup performance.
var ( + internalContextKeysMap = map[any]struct{}{ + contextKey: {}, + sessionKey: {}, + } internalContextKeys = []any{ contextKey, sessionKey, } ) func (c *Context) Context() context.Context { ctx := c.getUserContext() values := c.getGoravelContextValues() for key, value := range values { - skip := false - for _, internalContextKey := range internalContextKeys { - if key == internalContextKey { - skip = true - } - } - - if !skip { + if _, isInternal := internalContextKeysMap[key]; !isInternal { ctx = context.WithValue(ctx, key, value) } } return ctx }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
context.go(3 hunks)context_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- context_test.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
context.go
57-57: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
63-63: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
64-64: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
🔇 Additional comments (3)
context.go (3)
62-64: LGTM! Good approach for preserving context.
The implementation correctly maintains both the original context and the values map.
🧰 Tools
🪛 golangci-lint (1.62.2)
63-63: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
64-64: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
100-104: LGTM! Clear and correct implementation.
The method correctly prioritizes the values map before falling back to the original context.
112-128: LGTM! Robust helper methods.
Both helper methods handle type assertions correctly and provide appropriate fallbacks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
context.go (1)
12-23:⚠️ Potential issueUse typed keys for context values instead of strings
Using string constants as context keys is prone to collisions. Following Go's best practices, consider using custom types for context keys.
Apply this change:
+type userContextKeyType struct{} +type contextKeyType struct{} +type sessionKeyType struct{} const ( - userContextKey = "goravel_userContextKey" - contextKey = "goravel_contextKey" - sessionKey = "goravel_session" + userContextKey = userContextKeyType{} + contextKey = contextKeyType{} + sessionKey = sessionKeyType{} )
🧹 Nitpick comments (2)
context.go (1)
71-87: Consider optimizing the internal keys checkThe current implementation iterates through internalContextKeys for each value. Consider using a map for O(1) lookup of internal keys.
var ( + internalContextKeysMap = map[any]struct{}{ + contextKey: {}, + sessionKey: {}, + } internalContextKeys = []any{ contextKey, sessionKey, } ) func (c *Context) Context() context.Context { ctx := c.getUserContext() values := c.getGoravelContextValues() for key, value := range values { - skip := false - for _, internalContextKey := range internalContextKeys { - if key == internalContextKey { - skip = true - } - } - - if !skip { + if _, isInternal := internalContextKeysMap[key]; !isInternal { ctx = context.WithValue(ctx, key, value) } } return ctx }context_test.go (1)
11-44: Consider adding edge case testsWhile the current tests cover the main functionality well, consider adding tests for:
- Handling of nil values
- Type assertion failures in getGoravelContextValues and getUserContext
- Concurrent access to context values
Example test cases:
func TestContextEdgeCases(t *testing.T) { httpCtx := Background() // Test nil value httpCtx.WithValue("key", nil) assert.Nil(t, httpCtx.Value("key")) // Test type assertion failure ctx := context.WithValue(context.Background(), contextKey, "invalid") httpCtx.WithContext(ctx) assert.NotNil(t, httpCtx.getGoravelContextValues()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
context.go(3 hunks)context_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
context.go
60-60: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
66-66: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
67-67: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
🔇 Additional comments (3)
context.go (3)
54-62: LGTM: Good implementation of value storage
The implementation correctly handles value storage using a map, includes proper nil checking, and maintains context integrity. The comment clearly explains the rationale.
🧰 Tools
🪛 golangci-lint (1.62.2)
60-60: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
64-69: LGTM: Good context preservation strategy
The implementation correctly preserves both the original context and values map, ensuring proper context inheritance.
🧰 Tools
🪛 golangci-lint (1.62.2)
66-66: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
67-67: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions
(staticcheck)
115-131: LGTM: Well-implemented helper methods
The helper methods properly handle type assertions and provide appropriate default values.
📑 Description
Previously, the context returned by the Context method can't get value normally, because we maintain a customized value.
Summary by CodeRabbit
New Features
Bug Fixes
Tests