🔥 feat: Add Session Access via Go Context #3339
Conversation
WalkthroughThe pull request integrates Go’s context support into session management. The middleware creates a Go context incorporating session data using Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FiberApp
participant Middleware
participant Handler
Client->>FiberApp: HTTP Request
FiberApp->>Middleware: Process request with session middleware
Middleware->>Middleware: Create Go Context with session (context.WithValue)
Middleware->>FiberApp: Set context in Fiber (c.SetContext)
FiberApp->>Handler: Forward request for processing
Handler->>Middleware: Retrieve session using FromGoContext
Middleware-->>Handler: Return session data
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3339 +/- ##
==========================================
- Coverage 83.65% 83.61% -0.05%
==========================================
Files 118 118
Lines 11707 11724 +17
==========================================
+ Hits 9794 9803 +9
- Misses 1485 1491 +6
- Partials 428 430 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
middleware/session/middleware_test.go (1)
457-502: Well-structured test for Go context integration.The test verifies that sessions can be accessed from both Fiber context and Go context, confirming that the implementation works correctly. It properly tests the "happy path" where a session exists and can be retrieved.
However, consider adding test cases for edge conditions:
Add tests for:
- When the provided context is nil
- When the context doesn't contain a session
This would improve test coverage for the
FromGoContextfunction.+func Test_Session_GoContext_EdgeCases(t *testing.T) { + t.Parallel() + + // Test with nil context + sess := FromGoContext(nil) + require.Nil(t, sess, "Session should be nil with nil context") + + // Test with context that doesn't have a session + ctx := context.Background() + sess = FromGoContext(ctx) + require.Nil(t, sess, "Session should be nil when not in context") +}middleware/session/store_test.go (1)
229-279: Well-structured test for the new Go context session functionality.This test properly validates the new functionality for accessing session data via Go context. It covers the main flow by:
- Getting a session directly from the store
- Setting data in the session
- Retrieving and validating the same session from Go context
- Verifying data consistency between contexts
- Testing end-to-end with a real HTTP request
Consider adding test cases for edge scenarios, such as:
- Behavior when a session hasn't been initialized before calling
FromGoContext- Error handling when session operations fail
For example:
t.Run("no session in context", func(t *testing.T) { app := fiber.New() c := app.AcquireCtx(&fasthttp.RequestCtx{}) defer app.ReleaseCtx(c) // No session has been created yet goCtxSess := FromGoContext(c.Context()) require.Nil(t, goCtxSess) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
middleware/session/middleware.go(2 hunks)middleware/session/middleware_test.go(2 hunks)middleware/session/session.go(3 hunks)middleware/session/store.go(2 hunks)middleware/session/store_test.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
middleware/session/session.go
[warning] 529-530: middleware/session/session.go#L529-L530
Added lines #L529 - L530 were not covered by tests
[warning] 534-534: middleware/session/session.go#L534
Added line #L534 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (8)
middleware/session/middleware.go (2)
6-6: Context package import added correctly.This import is needed for integrating Go's standard context capabilities with Fiber's session management.
94-97: Well-implemented session integration with Go context.The implementation properly adds the session to Go's context using the standard
context.WithValuepattern and then updates the Fiber context. This enables retrieving the session via Go's context package, which aligns with modern Go practices.middleware/session/session.go (2)
5-5: Context package import added correctly.This import is needed for the new
FromGoContextfunction.
34-41: Well-designed context key implementation.Using an unexported type for context keys is a best practice in Go as it prevents key collisions with other packages. The implementation follows the recommendations from the Go context package documentation.
middleware/session/store.go (2)
4-4: Context package import added correctly.This import is needed for the enhanced
Getmethod implementation.
104-113: Robust error handling and context integration.The enhanced
Getmethod now properly:
- Explicitly handles errors from
getSession- Sets the session in the Go context
- Updates the Fiber context with the Go context
This provides a consistent way to access sessions from both Fiber and Go contexts.
middleware/session/middleware_test.go (1)
4-5: Appropriate imports for testing.The added imports for
ioandnet/http/httptestare needed for reading response bodies and creating test HTTP requests.middleware/session/store_test.go (1)
5-6: LGTM: New imports support the new test functionality.The added imports (
ioandnet/http/httptest) are appropriate for the new test function that requires reading response bodies and simulating HTTP requests.
|
This unit test I ran locally reported no errors and I didn't change it. Anything else to note here? @ReneWerner87 @gaby
|
middleware/session/store.go
Outdated
| } | ||
|
|
||
| // Add session to Go context | ||
| ctx := context.WithValue(c.Context(), sessionContextKey, sess) |
There was a problem hiding this comment.
we should not add or change anything in a get function, this does not meet the expectations of the user
There was a problem hiding this comment.
@ReneWerner87 I think we have to have this, look at this code below，
app.Post("/process-data", func(c fiber.Ctx) error {
sess, err := store.Get(c)
if err != nil {
return err
}
go processDataInBackground(c.Context(), c.Body())
return c.SendStatus(fiber.StatusAccepted)
})
func processDataInBackground(ctx context.Context, data []byte) {
// Getting sessions from Go context
sess := session.FromGoContext(ctx)
if sess == nil {
log.Error("Session not found in context")
return
}
userID := sess.Get("userID").(string)
}There was a problem hiding this comment.
please do not write anything in a read context method
the value in the context should work the same as the value in the locals, i.e. at every place where we write to the locals, we should also write to the context
There was a problem hiding this comment.
you are right, the value in the context should work the same as the value in the locals
There was a problem hiding this comment.
Then it would make the most sense to create a new method and set the value in context and locals in this method
This method can then be used in all places where the locals are currently set
|
@sixcolors can you check this |
…port' of github.com:JIeJaitt/fiber into JIeJaitt-jiejaitt-feature/add-sessionMD-userContext-support
|
@ReneWerner87 How can I avoid lint error , I used the following comment but it doesn't seem to have any effect, he still reports lint error: |
In order to maintain the consistency of the fiber middleware, this pr is proposed to add the corresponding function mentioned in #3212 to the session middleware.