🔥 feat: Add Session Access via Go Context#3339
🔥 feat: Add Session Access via Go Context#3339JIeJaitt wants to merge 16 commits intogofiber:mainfrom
Conversation
WalkthroughThe changes focus on refining session management in the middleware. A new context key ( Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client/Request
participant M as Middleware
participant S as Session Store
C->>M: Incoming request
M->>S: Call GetAndSetInContext(c)
S->>S: Execute Get() with error handling
alt Session retrieved
S-->>M: Return session
else Error occurred
S-->>M: Return error
M->>M: Log error
end
M->>C: Proceed with updated context
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3339 +/- ##
==========================================
+ Coverage 83.61% 83.65% +0.04%
==========================================
Files 118 118
Lines 11727 11750 +23
==========================================
+ Hits 9806 9830 +24
Misses 1491 1491
+ Partials 430 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:
|
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
|
|
@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: |
gaby
left a comment
There was a problem hiding this comment.
One small comment, should we rename it? Also the documentation needs to be updated and the whats_new.md file.
Besides that it looks good to me.
Yes. Will do a full review this week. |
There was a problem hiding this comment.
session.Session and session.Middleware are distinct and have differing purposes. Storing them in the context with the same key is not correct.
See: https://github.com/gofiber/fiber/blob/main/docs/middleware/session.md#v2-to-v3
| m.initialize(c, cfg) | ||
|
|
||
| // Add session to Go context | ||
| ctx := context.WithValue(c.Context(), middlewareContextKey, m) |
There was a problem hiding this comment.
I don't understand the benefit of retaining the references in both c.Context and c.Locals. Given that the UserContext used is always linked to the ctx.
There was a problem hiding this comment.
I answered the question here: #3340 (comment)
At first I thought the same thing, but I think that since Middleware includes Session, why not get the Middleware when the user wants to use Session, and just fetch the Session from it? So this pr only considers storing Middleware into Fiber.Ctx and context.Context, but not Session. What do you think? @ReneWerner87 @gaby |
Session only exists in v3 for legacy use or for use without a context. Using Session methods is otherwise not recommended and Middleware methods would be preferred. See #2741 So I would reject the added GetAndSetInContext function. |
If that's the case, as I understand it Session is an already obsolete piece of code, and adding a new GetAndSetInContext function would encourage the use of an API that is no longer recommended. If my understanding is correct I would remove the GetAndSetInContext function. |
sixcolors
left a comment
There was a problem hiding this comment.
Thanks for your contribution! However, I do not support merging this change due to the reasons outlined in issue #3358.
Fiber is designed to be lightweight and high-performance, and modifying context.Context within middleware introduces unnecessary overhead.
If you need context.Context integration, you can implement a custom middleware to copy values into context.Context for your specific use case without affecting Fiber’s default behavior.
For more details, see: this comment.
I recommend closing this PR. Thanks again! 🚀
thinks，I've already understood the whole working process and I will close this pr |
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.