feat!(middleware/session): re-write session middleware with handler #3016
Conversation
sixcolors
requested review from
gaby,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe recent changes focus on enhancing session management within the middleware by replacing the Changes
Assessment against linked issues
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 (
|
![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
Outside diff range and nitpick comments (3)
middleware/csrf/session_manager.go (1)
74-84: Consider adding logging for error scenarios in session deletion.if err != nil { log.Errorf("Failed to retrieve session for deletion: %v", err) }middleware/session/middleware.go (1)
51-94: Add detailed documentation for the middleware lifecycle.// NewMiddleware creates and manages the lifecycle of session middleware. // It retrieves a middleware instance from the pool, attaches it to the context, // and ensures it is properly released after the request is processed.middleware/session/session.go (1)
Line range hint
170-192: Improve error handling in theSavemethod to handle potential encoding failures more gracefully.if err != nil { log.Errorf("Session encoding failed: %v", err) return err }
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- middleware/csrf/session_manager.go (3 hunks)
- middleware/session/config.go (3 hunks)
- middleware/session/middleware.go (1 hunks)
- middleware/session/session.go (7 hunks)
Additional comments not posted (4)
middleware/csrf/session_manager.go (1)
57-68: Ensure thread safety when setting session tokens.Verification successful
The
Setmethod implementations found in the codebase indicate that thread safety measures are in place. Specifically:
- In
middleware/session/middleware.go, theSetmethod uses a mutex (m.mu.Lock()andm.mu.Unlock()) to ensure thread safety.- In
middleware/session/data.go, theSetmethod also uses a lock (d.Lock()andd.Unlock()) to ensure thread safety.These implementations confirm that the session management operations are thread-safe.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential data races when setting session tokens. # Command: Run your Go data race detector tool on this section.Length of output: 0
Script:
#!/bin/bash # Description: Check if the session management library used in the codebase provides thread-safe operations. # Command: Search for the session management library and inspect its documentation or implementation for thread safety. # Find the session management library used in the codebase session_lib=$(rg --files-with-matches 'session.FromContext' | head -n 1) # Check if the session management library provides thread-safe operations if [ -n "$session_lib" ]; then rg 'thread-safe' "$session_lib" else echo "Session management library not found." fiLength of output: 208
Script:
#!/bin/bash # Description: Extract and inspect the implementation of the `Set` method in the session management library to verify thread safety. # Find the implementation of the `Set` method in the session management library ast-grep --lang go --pattern 'func ($_ $_) Set($_, $_) $_' middleware/csrf/session_manager.goLength of output: 95
Script:
#!/bin/bash # Description: Search the entire codebase for the implementation of the `Set` method to verify thread safety. # Find the implementation of the `Set` method in the entire codebase ast-grep --lang go --pattern 'func ($_ $_) Set($_, $_) $_'Length of output: 1417
middleware/session/config.go (1)
13-15: RenamingExpirationtoIdleTimeoutclarifies the purpose and aligns with best practices.middleware/session/middleware.go (1)
31-37: Well-structuredMiddlewarestruct with appropriate use of synchronization for thread safety.middleware/session/session.go (1)
16-22: The addition ofidleTimeoutin theSessionstruct is a positive change for better session management.
Known IssuesStorage Adapter UPSERT BehaviorThere is an issue with the way Fiber's storage adapters handle setting keys. The current operation for setting a key is an UPSERT, which means it will insert a new key if it does not exist or update it if it does. This can lead to issues in scenarios with simultaneous requests altering the same session. For example:
If the session destruction completes first, the inconsequential change could recreate the session, potentially preventing actions like logging out and posing security risks. Since this PR changes the session behavior to save and update on every request, it amplifies the problem if not addressed. |
|
See: https://github.com/gofiber/recipes/tree/v3-csrf-with-session-PR-3016-update/csrf-with-session for usage example of this PR in current state. |
|
Those new sequence diagrams are awesome |
Co-authored-by: Renan Bastos <renanbastos.tec@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3016 +/- ##
==========================================
+ Coverage 80.11% 80.84% +0.72%
==========================================
Files 117 118 +1
Lines 9044 9162 +118
==========================================
+ Hits 7246 7407 +161
+ Misses 1364 1326 -38
+ Partials 434 429 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (32)
middleware/csrf/session_manager.go (4)
21-32: Improved session manager initializationThe changes to
newSessionManagerare well-implemented:
- Removing the
k stringparameter simplifies the function.- Registering
sessionKeyTypeandTokenwith the session store ensures proper serialization/deserialization.These changes align well with the new session key management approach.
However, consider adding error handling for the type registration:
if err := s.RegisterType(sessionKeyType(0)); err != nil { // Handle or log the error } if err := s.RegisterType(Token{}); err != nil { // Handle or log the error }This would make the function more robust against potential registration failures.
37-52: Improved session retrieval logic with suggestionsThe changes to
getRawmethod are well-implemented:
- Prioritizing context-based session retrieval can improve performance.
- Fallback to store retrieval ensures compatibility with various session management scenarios.
However, there are a few areas for improvement:
Error handling: The error from
m.session.Get(c)is silently ignored. Consider logging this error or returning it to the caller.Potential refactoring: The token retrieval logic is duplicated. Consider refactoring to reduce duplication:
func (m *sessionManager) getToken(s fiber.Session) (Token, bool) { token, ok := s.Get(sessionKey).(Token) return token, ok } // In getRaw: var token Token var ok bool if sess := session.FromContext(c); sess != nil { token, ok = m.getToken(sess) } else { storeSess, err := m.session.Get(c) if err != nil { log.Warn("csrf: failed to get session from store: ", err) return nil } token, ok = m.getToken(storeSess) }This refactoring would make the code more maintainable and reduce the risk of inconsistencies.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 48-48: middleware/csrf/session_manager.go#L48
Added line #L48 was not covered by tests
65-80: Improved session setting logic with suggestionsThe changes to
setRawmethod are well-implemented:
- Prioritizing context-based session aligns with the changes in
getRaw.- The use of
sessionKeyconstant improves consistency and type safety.However, there are areas for improvement:
Error handling: Errors from
m.session.Get(c)andstoreSess.Save()are logged but not returned. Consider propagating these errors to the caller for better error management.Potential refactoring: The token setting logic is duplicated. Consider refactoring to reduce duplication:
func (m *sessionManager) setToken(s fiber.Session, token Token) error { s.Set(sessionKey, token) if store, ok := s.(*session.Store); ok { return store.Save() } return nil } // In setRaw: token := Token{Key: key, Raw: raw, Expiration: time.Now().Add(exp)} var err error if sess := session.FromContext(c); sess != nil { err = m.setToken(sess, token) } else { storeSess, getErr := m.session.Get(c) if getErr != nil { return fmt.Errorf("csrf: failed to get session from store: %w", getErr) } err = m.setToken(storeSess, token) } if err != nil { log.Warn("csrf: failed to save session: ", err) }This refactoring would make the code more maintainable and reduce the risk of inconsistencies.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 74-74: middleware/csrf/session_manager.go#L74
Added line #L74 was not covered by tests
[warning] 78-78: middleware/csrf/session_manager.go#L78
Added line #L78 was not covered by tests
85-99: Improved session deletion logic with suggestionsThe changes to
delRawmethod are well-implemented:
- Prioritizing context-based session aligns with the changes in
getRawandsetRaw.- The use of
sessionKeyconstant improves consistency and type safety.However, there are areas for improvement:
Error handling: Errors from
m.session.Get(c)andstoreSess.Save()are logged but not returned. Consider propagating these errors to the caller for better error management.Potential refactoring: The token deletion logic is duplicated. Consider refactoring to reduce duplication:
func (m *sessionManager) deleteToken(s fiber.Session) error { s.Delete(sessionKey) if store, ok := s.(*session.Store); ok { return store.Save() } return nil } // In delRaw: var err error if sess := session.FromContext(c); sess != nil { err = m.deleteToken(sess) } else { storeSess, getErr := m.session.Get(c) if getErr != nil { return fmt.Errorf("csrf: failed to get session from store: %w", getErr) } err = m.deleteToken(storeSess) } if err != nil { log.Warn("csrf: failed to save session: ", err) }This refactoring would make the code more maintainable and reduce the risk of inconsistencies.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 87-87: middleware/csrf/session_manager.go#L87
Added line #L87 was not covered by tests
[warning] 93-93: middleware/csrf/session_manager.go#L93
Added line #L93 was not covered by tests
[warning] 97-97: middleware/csrf/session_manager.go#L97
Added line #L97 was not covered by testsmiddleware/session/config.go (2)
12-95: LGTM: Enhanced Config struct with improved session managementThe changes to the
Configstruct significantly improve session management capabilities:
- The addition of
Next,ErrorHandler,IdleTimeout, andAbsoluteTimeoutfields enhances configurability and error handling.- Replacing
ExpirationwithIdleTimeoutandAbsoluteTimeoutprovides more granular control over session expiration.- The improved documentation for all fields enhances code readability and usability.
These changes align well with the PR objectives of improving session management and addressing inconsistencies in timeout handling.
Consider adding a note to the
AbsoluteTimeoutdocumentation to clarify its relationship withIdleTimeout. For example:// AbsoluteTimeout defines the maximum duration of the session before it expires. // // If set to 0, the session will not have an absolute timeout, and will expire after the idle timeout. // Note: This value should be greater than or equal to IdleTimeout. // // Optional. Default: 0 AbsoluteTimeout time.DurationThis addition would help prevent potential configuration errors.
Line range hint
135-190: LGTM: Improved configDefault with enhanced error handlingThe updates to
configDefaultfunction are well-aligned with the changes made to theConfigstruct:
- Proper handling of new fields like
IdleTimeoutandAbsoluteTimeout.- Addition of a safeguard to ensure
AbsoluteTimeoutis always greater than or equal toIdleTimeout.- Improved error messages for better debuggability.
These changes contribute to more robust session configuration.
Consider using
log.Panicinstead ofpanicfor consistency with the error handling approach used elsewhere in the package. This would allow for better error logging and potential recovery in production environments. For example:if cfg.AbsoluteTimeout > 0 && cfg.AbsoluteTimeout < cfg.IdleTimeout { log.Panic("[session] AbsoluteTimeout must be greater than or equal to IdleTimeout") }if len(selectors) != numSelectors { log.Panic("[session] KeyLookup must be in the format '<source>:<name>'") }default: log.Panic("[session] unsupported source in KeyLookup")This change would provide more consistent error handling throughout the package.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 130-130: middleware/session/config.go#L130
Added line #L130 was not covered by tests
[warning] 162-162: middleware/session/config.go#L162
Added line #L162 was not covered by testsmiddleware/csrf/config.go (2)
125-125: ApproveIdleTimeoutinConfigDefault, suggest documenting rationale.The change from
ExpirationtoIdleTimeoutinConfigDefaultis consistent with the struct modification. The default value of 30 minutes is reasonable for CSRF token validity.Consider adding a comment explaining the rationale for choosing 30 minutes as the default
IdleTimeout. This would provide valuable context for developers using or maintaining the middleware.
150-151: ApproveIdleTimeoutcheck, suggest minor improvement.The change from
ExpirationtoIdleTimeoutin theconfigDefaultfunction is consistent with the previous modifications. The logic correctly ensures that a default value is set ifIdleTimeoutis not properly configured.Consider changing the condition to explicitly check for negative values:
-if cfg.IdleTimeout <= 0 { +if cfg.IdleTimeout < 0 {This change would allow users to set
IdleTimeoutto 0 if they want to disable the timeout feature. If this is not the intended behavior, please add a comment explaining why 0 is not a valid value.middleware/session/middleware.go (4)
12-39: LGTM: Well-structured Middleware type with proper concurrency considerations.The Middleware struct and related constants are well-designed. The use of sync.RWMutex for concurrency safety and sync.Pool for instance reuse are good practices.
Consider adding a brief comment explaining the purpose of the
destroyedfield in the Middleware struct for better clarity.
41-108: LGTM: Well-implemented initialization functions with proper session lifecycle management.The
NewandNewWithStorefunctions provide a flexible way to initialize the session middleware. The handler correctly manages the session lifecycle, including initialization and cleanup.Consider adding error handling for the case where
NewWithStorefails to create a store. Currently, ifNewStore(cfg)were to return an error (which it doesn't in the current implementation), it would be silently ignored.
235-289: LGTM: Well-implemented session management methods.The Destroy, Fresh, ID, and Reset methods provide necessary session management operations and are correctly implemented.
Consider adding a brief comment explaining the purpose of the
destroyedflag in the Destroy method, and how it interacts with other parts of the middleware (e.g., in the handler function).
1-301: Overall assessment: Well-implemented session middleware with room for minor improvements.This new session middleware implementation for Fiber is robust and well-structured. Key strengths include:
- Proper use of sync.Pool for performance optimization.
- Good concurrency safety with appropriate use of mutexes.
- Flexible configuration options.
- Comprehensive session management operations.
Areas for improvement:
- Error handling in the
initializeandacquireMiddlewaremethods could be more graceful to avoid potential panics in production.- Test coverage could be increased, particularly for error handling paths.
- Some additional documentation would improve code clarity, especially regarding the
destroyedflag and its implications.These improvements would further enhance the reliability and maintainability of the middleware. Overall, this is a solid implementation that provides a strong foundation for session management in Fiber applications.
Consider adding a middleware-specific logger or integrating with Fiber's logging system. This would allow for more granular control over log levels and could help with debugging in production environments without relying on panics for error reporting.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 117-117: middleware/session/middleware.go#L117
Added line #L117 was not covered by tests
[warning] 130-133: middleware/session/middleware.go#L130-L133
Added lines #L130 - L133 were not covered by tests
[warning] 144-144: middleware/session/middleware.go#L144
Added line #L144 was not covered by testsmiddleware/csrf/csrf.go (1)
Line range hint
1-353: Overall assessment: Successful implementation of CSRF middleware improvementsThe changes in this file successfully address the objectives outlined in the PR and issue #2741. Key improvements include:
- Simplified session manager initialization.
- Consistent replacement of
ExpirationwithIdleTimeoutthroughout the CSRF middleware.- Better alignment with NIST guidelines on distinguishing between timeout and expiration.
These modifications enhance the security and consistency of the CSRF protection mechanism. The code is now more maintainable and adheres to best practices for session management.
Consider the following for future improvements:
- Implement the absolute timeout mechanism mentioned in the PR comments.
- Update the documentation to reflect these changes, especially regarding the new
IdleTimeoutusage.- Ensure that all related tests are updated to use
IdleTimeoutinstead ofExpiration.middleware/session/session.go (3)
59-81: Approved: New Release method enhances session managementThe addition of the
Releasemethod is a valuable improvement for efficient session management. It aligns well with the PR objectives by providing a clear mechanism for returning sessions to the pool.Consider adding a note in the documentation about the thread-safety of this method. For example:
// Release releases the session back to the pool. +// +// This method is safe to call concurrently from multiple goroutines.This addition would provide clarity on the method's behavior in concurrent scenarios.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 78-78: middleware/session/session.go#L78
Added line #L78 was not covered by tests
Line range hint
295-326: Approved: New saveSession method enhances session managementThe addition of the
saveSessionmethod is a good improvement in code organization and aligns well with the PR objectives. It centralizes the logic for saving session data and ensures consistent handling of theidleTimeout.Consider adding more granular error handling:
func (s *Session) saveSession() error { if s.data == nil { return nil } s.mu.Lock() defer s.mu.Unlock() // Set idleTimeout if not already set if s.idleTimeout <= 0 { s.idleTimeout = s.config.IdleTimeout } // Update client cookie s.setSession() // Encode session data encCache := gob.NewEncoder(s.byteBuffer) s.data.RLock() err := encCache.Encode(&s.data.Data) s.data.RUnlock() if err != nil { return fmt.Errorf("failed to encode data: %w", err) } // Copy the data in buffer encodedBytes := make([]byte, s.byteBuffer.Len()) copy(encodedBytes, s.byteBuffer.Bytes()) // Pass copied bytes with session id to provider - return s.config.Storage.Set(s.id, encodedBytes, s.idleTimeout) + if err := s.config.Storage.Set(s.id, encodedBytes, s.idleTimeout); err != nil { + return fmt.Errorf("failed to store session data: %w", err) + } + return nil }This change would provide more context in case of a storage error, making debugging easier.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 286-286: middleware/session/session.go#L286
Added line #L286 was not covered by tests
[warning] 288-288: middleware/session/session.go#L288
Added line #L288 was not covered by tests
441-479: Approved: New methods for absolute session expirationThe addition of
absExpiration,isAbsExpired, andsetAbsExpirationmethods is a valuable improvement to the session management system. These methods provide an additional layer of control over session lifetimes, aligning well with the PR objectives.Consider adding a brief explanation in the package documentation about the difference between idle timeout and absolute expiration. For example:
/* Package session provides session management for Fiber applications. This package uses two types of session expiration: 1. Idle Timeout: The session expires after a period of inactivity. 2. Absolute Expiration: The session expires at a fixed point in time, regardless of activity. Idle Timeout is managed automatically, while Absolute Expiration can be set manually using the setAbsExpiration method. */ package sessionThis addition would provide clarity on the two expiration mechanisms and how they differ.
docs/middleware/session.md (5)
59-59: Consider clarifying IdleTimeout behavior in the legacy approachThe note about IdleTimeout behavior in the legacy approach is helpful. To further improve clarity, consider expanding this note to explain how the IdleTimeout behaves differently in the middleware handler approach. This will help users understand the key differences between the two approaches.
Consider adding the following information:
When using the legacy approach, the IdleTimeout will be updated when the session is saved. +When using the middleware handler approach, the IdleTimeout is automatically updated on each request.
132-148: Enhance Config struct documentationThe Config struct documentation is comprehensive, but it could benefit from additional context for the new timeout fields. This would help users understand the difference between IdleTimeout and AbsoluteTimeout and how they affect session behavior.
Consider adding brief descriptions to the IdleTimeout and AbsoluteTimeout fields:
type Config struct { // ... other fields ... - IdleTimeout time.Duration - AbsoluteTimeout time.Duration + IdleTimeout time.Duration // Maximum duration of inactivity before session expires + AbsoluteTimeout time.Duration // Maximum total duration of a session, regardless of activity // ... other fields ... }
207-218: Ensure consistent formatting for method signaturesThe method signatures in the Session Methods section are not consistently formatted. Some methods are on separate lines, while others are on the same line. Consistent formatting would improve readability.
Consider applying the following changes to ensure consistent formatting:
func (s *Session) Fresh() bool func (s *Session) ID() string -func (s *Session) Get(key string) any -func (s *Session) Set(key string, val any) -func (s *Session) Destroy() error -func (s *Session) Regenerate() error +func (s *Session) Get(key string) any +func (s *Session) Set(key string, val any) +func (s *Session) Destroy() error +func (s *Session) Regenerate() error func (s *Session) Release() func (s *Session) Reset() error -func (s *Session) Save() error -func (s *Session) Keys() []string +func (s *Session) Save() error +func (s *Session) Keys() []string func (s *Session) SetIdleTimeout(idleTimeout time.Duration)
397-418: Enhance custom types exampleThe example for registering custom types is helpful, but it could be more informative by demonstrating how to actually use the custom type in a session. This would provide a complete picture of how to work with custom types in sessions.
Consider expanding the example to show how to set and get a custom type in a session:
func main() { app := fiber.New() sessionMiddleware, sessionStore := session.NewWithStore() sessionStore.RegisterType(User{}) app.Use(sessionMiddleware) + app.Get("/", func(c fiber.Ctx) error { + sess := session.FromContext(c) + if sess == nil { + return c.SendStatus(fiber.StatusInternalServerError) + } + + // Set custom type in session + sess.Set("user", User{Name: "John Doe", Age: 30}) + + // Get custom type from session + if user, ok := sess.Get("user").(User); ok { + return c.SendString(fmt.Sprintf("User: %s, Age: %d", user.Name, user.Age)) + } + + return c.SendString("User not found in session") + }) app.Listen(":3000") }This enhancement demonstrates how to set and retrieve a custom type in a session, providing a more complete example for users.
423-437: Ensure consistent formatting for property descriptionsThe property descriptions in the Config table are not consistently formatted. Some end with periods, while others don't. Consistent formatting would improve readability and professionalism.
Consider applying the following changes to ensure consistent formatting:
-| **Storage** | `fiber.Storage` | Defines where session data is stored. | `nil` (in-memory storage) | -| **Next** | `func(c fiber.Ctx) bool` | Function to skip this middleware under certain conditions. | `nil` | -| **ErrorHandler** | `func(c fiber.Ctx, err error)` | Custom error handler for session middleware errors. | `nil` | -| **KeyGenerator** | `func() string` | Function to generate session IDs. | `UUID()` | -| **KeyLookup** | `string` | Key used to store session ID in cookie or header. | `"cookie:session_id"` | -| **CookieDomain** | `string` | The domain scope of the session cookie. | `""` | -| **CookiePath** | `string` | The path scope of the session cookie. | `"/"` | -| **CookieSameSite** | `string` | The SameSite attribute of the session cookie. | `"Lax"` | -| **IdleTimeout** | `time.Duration` | Maximum duration of inactivity before session expires. | `30 * time.Minute` | -| **AbsoluteTimeout** | `time.Duration` | Maximum duration before session expires. | `0` (no expiration) | -| **CookieSecure** | `bool` | Ensures session cookie is only sent over HTTPS. | `false` | -| **CookieHTTPOnly** | `bool` | Ensures session cookie is not accessible to JavaScript (HTTP only). | `true` | -| **CookieSessionOnly** | `bool` | Prevents session cookie from being saved after the session ends (cookie expires on close). | `false` | +| **Storage** | `fiber.Storage` | Defines where session data is stored | `nil` (in-memory storage) | +| **Next** | `func(c fiber.Ctx) bool` | Function to skip this middleware under certain conditions | `nil` | +| **ErrorHandler** | `func(c fiber.Ctx, err error)` | Custom error handler for session middleware errors | `nil` | +| **KeyGenerator** | `func() string` | Function to generate session IDs | `UUID()` | +| **KeyLookup** | `string` | Key used to store session ID in cookie or header | `"cookie:session_id"` | +| **CookieDomain** | `string` | The domain scope of the session cookie | `""` | +| **CookiePath** | `string` | The path scope of the session cookie | `"/"` | +| **CookieSameSite** | `string` | The SameSite attribute of the session cookie | `"Lax"` | +| **IdleTimeout** | `time.Duration` | Maximum duration of inactivity before session expires | `30 * time.Minute` | +| **AbsoluteTimeout** | `time.Duration` | Maximum duration before session expires | `0` (no expiration) | +| **CookieSecure** | `bool` | Ensures session cookie is only sent over HTTPS | `false` | +| **CookieHTTPOnly** | `bool` | Ensures session cookie is not accessible to JavaScript (HTTP only) | `true` | +| **CookieSessionOnly** | `bool` | Prevents session cookie from being saved after the session ends (cookie expires on close) | `false` |This change ensures all descriptions are consistently formatted without periods at the end.
🧰 Tools
🪛 LanguageTool
[style] ~436-~436: Consider using “inaccessible” to avoid wordiness.
Context: ... | Ensures session cookie is not accessible to JavaScript (HTTP only). ...(NOT_ABLE_PREMIUM)
docs/whats_new.md (7)
318-330: Consider enhancing the Session middleware documentation.The Session middleware section provides a good overview of the key changes in v3. However, consider the following improvements:
- Add a brief example demonstrating how to use the new middleware handler, including how to access the session store and manually release sessions.
- Clarify the behavior of
IdleTimeoutandAbsoluteTimeoutwhen both are set. Do they work in conjunction, or does one take precedence?- Explain the implications of manual session release on performance and potential memory leaks if not handled correctly.
These additions would provide a more comprehensive guide for users migrating from v2 to v3.
508-525: Address security implications of CSRF middleware changes.The CSRF middleware changes are well-documented. However, consider addressing the following points:
- Explain the security implications of reducing the default
IdleTimeoutfrom 1 hour to 30 minutes. How does this affect the balance between security and user experience?- Clarify whether the removal of the
SessionKeyfield affects existing implementations and if any migration steps are needed.- Provide guidance on best practices for setting the
IdleTimeoutvalue in different scenarios (e.g., for highly sensitive operations vs. general use cases).Adding this information would help users understand the security impact of these changes and how to implement them safely.
Line range hint
527-562: Enhance Filesystem to Static middleware migration guide.The migration instructions from Filesystem to Static middleware are clear. However, consider adding the following information:
- Explain the rationale behind removing the Filesystem middleware and consolidating its functionality into the Static middleware.
- Highlight any potential differences in behavior or performance between the old Filesystem middleware and the new Static middleware.
- Provide guidance on how to handle more complex scenarios that might have been using specific features of the Filesystem middleware.
This additional context would help users understand the reasons for the change and ensure a smooth migration process.
Line range hint
564-610: Expand on Healthcheck middleware changes and best practices.The Healthcheck middleware changes are well-documented. To further improve this section, consider:
- Explaining the benefits of separating the configuration for each health check endpoint (e.g., improved flexibility, easier maintenance).
- Providing guidance on when to use custom endpoints vs. default endpoints.
- Adding examples of common use cases for the new startup endpoint.
- Discussing best practices for implementing health check probes, including what to check and how to handle dependencies.
This additional information would help users leverage the full potential of the new Healthcheck middleware configuration.
Line range hint
63-148: Enhance Router changes documentation with additional context.The Router changes are well-documented with clear examples. To further improve this section, consider:
- Explaining the rationale behind the changes to the HTTP method registration signatures (e.g., improved type safety, clearer API).
- Discussing any performance implications of the new route chaining method compared to the old approach.
- Providing guidance on when to use the new route chaining method vs. the traditional approach.
- Adding a note about backwards compatibility and any migration steps for existing applications using the old Router methods.
This additional context would help users understand the benefits of the new Router interface and how to best utilize it in their applications.
Line range hint
150-196: Expand the Context section with detailed explanations and examples.The Context section provides a good overview of the changes, but it's currently marked as a draft. To improve this section:
- For each new feature (e.g., Partitioned cookies for CHIPS), provide a brief explanation of its purpose and a code example demonstrating its usage.
- For new methods, include short code snippets showing how to use them and when they might be beneficial.
- For removed methods, explain the rationale behind their removal and provide guidance on alternatives or migration paths.
- For changed methods, clearly explain how the changes affect existing code and provide migration examples.
Adding this level of detail would greatly enhance the usefulness of this section for developers migrating to v3.
Line range hint
612-614: Enhance the overall structure and completeness of the migration guide.The migration guide covers many important aspects of the transition from v2 to v3. To further improve its usefulness:
- Ensure consistency in the level of detail across all sections. Some sections (e.g., Router, Middlewares) are quite detailed, while others (e.g., Parser, Redirect) lack information.
- Consider adding a "Breaking Changes" section at the beginning, summarizing all major changes that might affect existing applications.
- Include a "Deprecations" section, if applicable, listing features that are deprecated in v3 and will be removed in future versions.
- Add a "Best Practices" section, providing guidance on how to best leverage the new features and changes in v3.
- Consider adding a "Troubleshooting" section, addressing common issues that users might encounter during migration.
These enhancements would provide a more comprehensive and user-friendly migration guide, helping developers transition smoothly from v2 to v3.
docs/middleware/csrf.md (2)
109-110: LGTM with a minor suggestionThe addition of
IdleTimeoutin the configuration table is correct. However, consider enhancing the description to provide more context:- IdleTimeout | `time.Duration` | IdleTimeout is the duration of inactivity before the CSRF token will expire. | 30 * time.Minute | + IdleTimeout | `time.Duration` | IdleTimeout is the duration of inactivity before the CSRF token will expire. The token's expiration is extended by this duration with each request. | 30 * time.Minute |This additional information helps users understand the dynamic nature of the token expiration.
304-304: LGTM with a minor suggestion for clarityThe explanation of the token lifecycle accurately reflects the new behavior introduced by the
IdleTimeoutconfiguration. However, consider a small adjustment for even better clarity:- Tokens are valid until they expire or until they are deleted. By default, tokens are valid for 30 minutes, and each subsequent request extends the expiration by the idle timeout. The token only expires if the user doesn't make a request for the duration of the idle timeout. + Tokens are valid until they expire or until they are deleted. By default, tokens have an idle timeout of 30 minutes. Each request resets this timeout, extending the token's validity. The token only expires if no requests are made for the duration of the idle timeout.This adjustment more explicitly ties the concept to the
IdleTimeoutconfiguration and clarifies the reset behavior.middleware/session/store_test.go (2)
128-129: Ensure middleware is properly initialized in the testLines 128-129 create a new
Middlewareinstance withmiddleware := &Middleware{}and set it in the context's locals. To accurately simulate the middleware's behavior, ensure that the middleware is properly configured as it would be in a real application. This might involve initializing any necessary fields or using a constructor function if available.
144-155: Include context release inTestStore_DeletesubtestsWithin the
TestStore_Deletefunction, contexts are not acquired, but if any are introduced in future modifications, remember to release them after use to maintain resource integrity during testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- docs/middleware/csrf.md (5 hunks)
- docs/middleware/session.md (1 hunks)
- docs/whats_new.md (3 hunks)
- middleware/csrf/config.go (3 hunks)
- middleware/csrf/csrf.go (3 hunks)
- middleware/csrf/csrf_test.go (5 hunks)
- middleware/csrf/session_manager.go (2 hunks)
- middleware/session/config.go (4 hunks)
- middleware/session/config_test.go (1 hunks)
- middleware/session/data.go (1 hunks)
- middleware/session/data_msgp.go (2 hunks)
- middleware/session/data_test.go (1 hunks)
- middleware/session/middleware.go (1 hunks)
- middleware/session/session.go (10 hunks)
- middleware/session/session_test.go (47 hunks)
- middleware/session/store.go (4 hunks)
- middleware/session/store_test.go (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- middleware/csrf/csrf_test.go
- middleware/session/config_test.go
- middleware/session/data_test.go
🧰 Additional context used
📓 Learnings (1)
middleware/session/config.go (1)
Learnt from: sixcolors PR: gofiber/fiber#3016 File: middleware/session/config.go:16-26 Timestamp: 2024-09-25T16:17:00.969Z Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
🪛 LanguageTool
docs/middleware/session.md
[style] ~436-~436: Consider using “inaccessible” to avoid wordiness.
Context: ... | Ensures session cookie is not accessible to JavaScript (HTTP only). ...(NOT_ABLE_PREMIUM)
🪛 GitHub Check: codecov/patch
middleware/csrf/session_manager.go
[warning] 48-48: middleware/csrf/session_manager.go#L48
Added line #L48 was not covered by tests
[warning] 74-74: middleware/csrf/session_manager.go#L74
Added line #L74 was not covered by tests
[warning] 78-78: middleware/csrf/session_manager.go#L78
Added line #L78 was not covered by tests
[warning] 87-87: middleware/csrf/session_manager.go#L87
Added line #L87 was not covered by tests
[warning] 93-93: middleware/csrf/session_manager.go#L93
Added line #L93 was not covered by tests
[warning] 97-97: middleware/csrf/session_manager.go#L97
Added line #L97 was not covered by testsmiddleware/session/config.go
[warning] 130-130: middleware/session/config.go#L130
Added line #L130 was not covered by tests
[warning] 162-162: middleware/session/config.go#L162
Added line #L162 was not covered by testsmiddleware/session/data.go
[warning] 37-37: middleware/session/data.go#L37
Added line #L37 was not covered by testsmiddleware/session/middleware.go
[warning] 117-117: middleware/session/middleware.go#L117
Added line #L117 was not covered by tests
[warning] 130-133: middleware/session/middleware.go#L130-L133
Added lines #L130 - L133 were not covered by tests
[warning] 144-144: middleware/session/middleware.go#L144
Added line #L144 was not covered by testsmiddleware/session/session.go
[warning] 78-78: middleware/session/session.go#L78
Added line #L78 was not covered by tests
[warning] 286-286: middleware/session/session.go#L286
Added line #L286 was not covered by tests
[warning] 288-288: middleware/session/session.go#L288
Added line #L288 was not covered by tests
[warning] 339-339: middleware/session/session.go#L339
Added line #L339 was not covered by testsmiddleware/session/store.go
[warning] 167-167: middleware/session/store.go#L167
Added line #L167 was not covered by tests
[warning] 177-178: middleware/session/store.go#L177-L178
Added lines #L177 - L178 were not covered by tests
[warning] 289-289: middleware/session/store.go#L289
Added line #L289 was not covered by tests
[warning] 306-307: middleware/session/store.go#L306-L307
Added lines #L306 - L307 were not covered by tests
[warning] 312-314: middleware/session/store.go#L312-L314
Added lines #L312 - L314 were not covered by tests
[warning] 316-316: middleware/session/store.go#L316
Added line #L316 was not covered by tests
🔇 Additional comments (37)
middleware/csrf/session_manager.go (1)
15-20: Improved type safety for session key managementThe introduction of
sessionKeyTypeand thesessionKeyconstant enhances type safety in session key management. This change aligns with Go best practices and helps prevent potential type-related errors.middleware/session/config.go (3)
8-8: LGTM: Added log package for error handlingThe addition of the
logpackage fromgitpro.ttaallkk.top/gofiber/fiber/v3/logis appropriate for the new error handling functionality introduced in this update.
108-115: LGTM: Updated ConfigDefault to align with new Config structureThe changes to
ConfigDefaultcorrectly reflect the updates made to theConfigstruct:
- Removal of the
Expirationfield.- Addition of
IdleTimeoutwith a sensible default of 30 minutes.- Explicit setting of
sourcetoSourceCookie.These changes ensure consistency with the new session management approach introduced in this update.
Line range hint
1-190: Summary: Comprehensive improvements to session middleware configurationThis update to
middleware/session/config.gosuccessfully addresses the objectives outlined in the PR and the linked GitHub issue:
- The session middleware has been re-written with improved configuration options, including
IdleTimeoutandAbsoluteTimeout, which provide more granular control over session expiration.- Error handling has been enhanced with the introduction of
DefaultErrorHandlerand improved error messages.- The changes align well with NIST guidelines on distinguishing between timeout and expiration.
The code quality is high, with thorough documentation and thoughtful error handling. The suggested minor improvements, if implemented, will further enhance the robustness and consistency of the package.
Overall, these changes significantly improve the session management capabilities of the Fiber framework.
middleware/csrf/config.go (1)
Line range hint
1-201: Summary of CSRF middleware configuration changesThe changes in this file align well with the PR objectives to address inconsistencies in session and CSRF middleware handling. Key modifications include:
- Replacing
ExpirationwithIdleTimeoutin theConfigstruct andConfigDefault.- Removing the
SessionKeyfield from theConfigstruct.- Updating the
configDefaultfunction to useIdleTimeout.These changes improve the consistency and clarity of the CSRF middleware configuration. However, they introduce breaking changes that may affect existing implementations, particularly the removal of
SessionKey.To ensure a smooth transition for users of this middleware, consider the following:
- Update the package documentation to highlight these breaking changes.
- Provide migration guidelines for users updating from the previous version.
- Run the following script to identify any internal usages that might need updating:
#!/bin/bash # Search for usages of Expiration and SessionKey in the codebase rg --type go '(Expiration|SessionKey)'This will help ensure that all related code is updated to reflect these changes.
middleware/session/middleware.go (7)
1-10: LGTM: Package declaration and imports are appropriate.The package name and imports are well-suited for a session middleware implementation in Fiber.
167-184: LGTM: Well-implemented FromContext function.The FromContext function correctly retrieves the Middleware from the Fiber context and gracefully handles the case where the middleware is not found.
186-233: LGTM: Well-implemented Set, Get, and Delete methods with proper concurrency safety.These methods provide CRUD operations for session data and use appropriate locking mechanisms to ensure thread safety.
291-301: LGTM: Correctly implemented Store method.The Store method simply returns the session store from the config, which is the expected behavior.
127-138: Increase test coverage for the saveSession method.The saveSession method has good error handling, but static analysis indicates that the error handling paths are not covered by tests.
Consider adding test cases that trigger the error handling paths:
- Test with a custom ErrorHandler
- Test with the DefaultErrorHandler
To verify the current test coverage and identify gaps, run:
#!/bin/bash # Check test coverage for the saveSession method go test -v -coverprofile=coverage.out ./middleware/session go tool cover -func=coverage.out | grep saveSessionThis will help identify which parts of the saveSession method need additional test coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 130-133: middleware/session/middleware.go#L130-L133
Added lines #L130 - L133 were not covered by tests
140-165:⚠️ Potential issueImprove error handling in acquireMiddleware and increase test coverage.
While using sync.Pool is good for performance, the panic in acquireMiddleware could lead to application crashes.
Consider handling the type assertion error more gracefully:
func acquireMiddleware() *Middleware { m, ok := middlewarePool.Get().(*Middleware) if !ok { - panic(ErrTypeAssertionFailed.Error()) + // Log the error and return a new instance + log.Printf("Type assertion failed in acquireMiddleware: %v", ErrTypeAssertionFailed) + return &Middleware{} } return m }Increase test coverage for both acquireMiddleware and releaseMiddleware, especially for error cases.
To check the current test coverage and identify gaps:
This will help ensure that all paths in these functions are properly tested.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 144-144: middleware/session/middleware.go#L144
Added line #L144 was not covered by tests
110-125:⚠️ Potential issueImprove error handling in the initialize method.
The current implementation panics if there's an error getting the session. This could lead to application crashes in production.
Consider handling the error more gracefully:
func (m *Middleware) initialize(c fiber.Ctx, cfg Config) { m.mu.Lock() defer m.mu.Unlock() session, err := cfg.Store.getSession(c) if err != nil { - panic(err) // handle or log this error appropriately in production + if cfg.ErrorHandler != nil { + cfg.ErrorHandler(c, err) + } else { + DefaultErrorHandler(c, err) + } + return } m.config = cfg m.Session = session m.ctx = c c.Locals(middlewareContextKey, m) }This change ensures that errors are handled consistently with the rest of the middleware.
To ensure this change doesn't introduce new issues, please run the following test:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 117-117: middleware/session/middleware.go#L117
Added line #L117 was not covered by testsmiddleware/csrf/csrf.go (3)
52-52: LGTM: Simplified session manager initializationThis change aligns with the PR objective of re-writing the session middleware. The simplified initialization improves code readability and maintainability.
237-237: LGTM: Consistent use of IdleTimeout in CSRF cookie updateThis change is consistent with the previous modification in
createOrExtendTokenInStorage. It further aligns the CSRF implementation with NIST guidelines and addresses issue #2741.
220-222: LGTM: Updated token storage with IdleTimeoutThis change addresses issue #2741 by replacing
ExpirationwithIdleTimeout, aligning with NIST guidelines on distinguishing between timeout and expiration.To ensure consistency, let's verify if this change has been applied throughout the codebase:
✅ Verification successful
To ensure that all instances of
Expirationhave been appropriately replaced withIdleTimeoutin the production codebase, let's verify that no such instances remain outside of test files:
Verified: 'Expiration' correctly replaced with 'IdleTimeout' in CSRF middleware
The change addresses issue #2741 by replacing
ExpirationwithIdleTimeoutwithin the CSRF middleware, aligning with NIST guidelines on distinguishing between timeout and expiration. Remaining instances ofExpirationin other components like caching and rate limiting are contextually appropriate and do not affect the CSRF functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of 'Expiration' related to session or CSRF # Test: Search for 'Expiration' in Go files. Expect: No occurrences related to session or CSRF timeout/expiration. rg --type go 'Expiration' -C 3Length of output: 27282
Script:
#!/bin/bash # Description: Check for any remaining instances of 'Expiration' in non-test Go files. # Test: Search for 'Expiration' in Go files excluding test files. Expect: No occurrences related to session or CSRF timeout/expiration. rg --type go 'Expiration' -g '!*.test.go' -C 3Length of output: 27298
middleware/session/session.go (4)
15-24: Approved: Session struct changes align with PR objectivesThe renaming of
exptoidleTimeoutand the addition of thefreshfield improve session management and align with the PR objectives. These changes address the inconsistencies in session handling and provide better clarity on the session state.
Line range hint
357-387: Approved: Improved setSession method with consistent timeout handlingThe changes in the
setSessionmethod align well with the PR objectives. The use ofidleTimeoutinstead ofExpirationprovides consistency with the rest of the session management logic. Setting bothMaxAgeandExpirefor the cookie is a good practice that ensures better compatibility across different browsers.These modifications contribute to a more robust and consistent session management system.
Line range hint
1-479: Summary: Significant improvements in session managementThis PR introduces substantial enhancements to the session middleware, effectively addressing the inconsistencies highlighted in issue #2741. The changes include:
- Renaming
exptoidleTimeoutfor clarity.- Adding a
freshfield to track new sessions.- Implementing a
Releasemethod for efficient session pool management.- Improving the
Savemethod to prevent unnecessary saves.- Adding absolute expiration functionality.
These modifications align well with NIST guidelines and provide a more robust session management system. The suggested improvements, if implemented, will further enhance the code quality and maintainability.
To fully realize the benefits of these changes:
- Consider implementing the suggested error handling improvements.
- Enhance the documentation as proposed to clarify the different expiration mechanisms.
- Improve test coverage for the identified uncovered lines.
Overall, this PR represents a significant step forward in addressing the session management inconsistencies and improving the overall quality of the middleware.
78-78: Consider improving test coverageStatic analysis has identified several lines that are not covered by tests:
- Line 78 in the
Releasemethod- Lines 286 and 288 in the
Savemethod- Line 339 in the
KeysmethodWhile 100% test coverage isn't always necessary or practical, consider adding tests for these cases if feasible. This could improve the robustness of the codebase and catch potential edge case bugs.
To help identify which tests might be missing, you can run the following command:
This will generate an HTML report of the test coverage, which can help visualize which parts of the code need additional testing.
Also applies to: 286-286, 288-288, 339-339
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 78-78: middleware/session/session.go#L78
Added line #L78 was not covered by testsdocs/middleware/session.md (1)
Line range hint
1-458: Overall documentation quality is excellent with minor improvement opportunitiesThe updated session middleware documentation is comprehensive, well-structured, and informative. It effectively covers the changes introduced in v3 and provides clear guidance for users upgrading from v2. The addition of a migration guide, detailed type descriptions, and various usage examples significantly enhances the documentation's value.
To further improve this already high-quality documentation, consider the following suggestions:
- Ensure consistent formatting throughout the document, particularly in method signatures and configuration property descriptions.
- Expand examples to demonstrate more real-world scenarios, such as using custom types in sessions.
- Add more context to new features like IdleTimeout and AbsoluteTimeout to help users understand their impact on session behavior.
- Review the document for any remaining inconsistencies in terminology or explanations.
Overall, this documentation provides an excellent resource for users of the Fiber session middleware.
🧰 Tools
🪛 LanguageTool
[style] ~436-~436: Consider using “inaccessible” to avoid wordiness.
Context: ... | Ensures session cookie is not accessible to JavaScript (HTTP only). ...(NOT_ABLE_PREMIUM)
docs/middleware/csrf.md (4)
37-37: LGTM: IdleTimeout configuration added correctlyThe addition of
IdleTimeoutset to 30 minutes is consistent with the PR objectives and accurately reflects the new configuration option for the CSRF middleware.
126-126: LGTM: Default configuration updated correctlyThe update to use
IdleTimeoutin the default configuration is consistent with the PR objectives and previous changes in the document. The 30-minute default value is correctly set.
145-145: LGTM: Recommended configuration updated correctlyThe update to use
IdleTimeoutin the recommended configuration is consistent with the PR objectives and previous changes in the document. The 30-minute value is correctly set.
Line range hint
1-324: Overall documentation update is comprehensive and accurateThe changes to this documentation file are consistent and align well with the PR objectives. Key points:
Expirationhas been replaced withIdleTimeoutthroughout the document.- The new default value of 30 minutes is correctly reflected in all relevant sections.
- The token lifecycle explanation has been updated to accurately describe the new behavior.
These updates provide clear and accurate information about the new
IdleTimeoutfeature in the CSRF middleware. The documentation now correctly reflects the changes in the middleware's behavior, which will help users understand and implement the CSRF protection effectively.middleware/session/session_test.go (10)
Line range hint
20-35: Improved session management and resource handlingThe changes in this segment enhance the test setup and teardown:
- Using
NewStore()instead ofNew()improves naming consistency.- Adding
sess.Release()ensures proper resource management.- Releasing and reacquiring the context simulates a new request scenario.
These modifications contribute to more robust and realistic testing of the session middleware.
Line range hint
49-74: Enhanced key type flexibility and consistent resource management
- The change from
[]string{}to[]any{}for thekeysexpectation indicates improved flexibility in key types supported by the session.- The addition of
sess.Release()at the end of the test ensures consistent resource cleanup.These changes improve the robustness of the tests and reflect enhancements in the session implementation.
Line range hint
84-100: Improved resource management and test documentationThe changes in this segment enhance the test in several ways:
- Adding
sess.Release()ensures proper cleanup of session resources.- Releasing and reacquiring the context helps simulate a fresh request scenario.
- The added comments improve the readability and clarity of the test flow.
These modifications contribute to more maintainable and understandable tests.
Line range hint
110-114: Enhanced error handling and test structureThe modifications in this segment improve the test robustness:
- Adding
defer sess.Release()ensures session cleanup even if the test fails.- Moving the freshness check after the error check improves the logical flow of the test.
These changes contribute to more reliable and structured testing of the session middleware.
Line range hint
121-195: Comprehensive improvements in session testingThis segment demonstrates several enhancements to the session type tests:
- Consistent use of
NewStore()aligns with earlier changes.- Multiple
sess.Release()calls ensure proper resource management throughout the test.- Repeated release and reacquire of the context simulate multiple request scenarios, improving test coverage.
- The change from
[]string{}to[]any{}forkeysexpectation reflects increased flexibility in key types.These modifications result in more thorough and realistic testing of the session middleware across various data types and scenarios.
330-503: Excellent addition of comprehensive key type testingThe new
Test_Session_KeyTypesfunction greatly enhances the test suite:
- It tests a wide range of key and value types, ensuring the session can handle various data types correctly.
- The inclusion of checks for unregistered key types verifies the type safety of the session implementation.
- The test covers both setting and retrieving values, ensuring consistency across operations.
This addition significantly improves the robustness and reliability of the session middleware by thoroughly testing its type handling capabilities.
Line range hint
557-616: Updated terminology and improved resource managementThis segment shows important updates to the session timeout testing:
- The function rename from
Test_Session_Save_ExpirationtoTest_Session_Save_IdleTimeoutreflects a change in terminology, likely aligning with updated session management concepts.- Replacing
SetExpirywithSetIdleTimeoutis consistent with this terminology change.- The addition of multiple
sess.Release()calls ensures proper resource cleanup throughout the test.These changes improve the clarity of the test's purpose and maintain consistent resource management practices.
617-681: Valuable addition of absolute timeout testingThe new
Test_Session_Save_AbsoluteTimeoutfunction is a significant enhancement to the test suite:
- It introduces testing for absolute timeout functionality, complementing the existing idle timeout tests.
- The test verifies that sessions expire correctly after the absolute timeout period.
- The inclusion of a check for retrieving an expired session by ID covers an important edge case.
This addition ensures more comprehensive coverage of session timeout behaviors, contributing to a more robust and reliable session management system.
Line range hint
690-1260: Consistent application of improved resource managementThe remaining changes in the file demonstrate a thorough and consistent approach to resource management:
- Multiple instances of
sess.Release()anddefer sess.Release()have been added across various test and benchmark functions.- The use of
deferin appropriate places ensures that sessions are properly released even if a test or benchmark fails.- These changes have been applied consistently throughout the file, including in complex scenarios like concurrency tests.
This systematic improvement in resource handling enhances the reliability and cleanliness of the test suite, reducing the risk of resource leaks and improving overall test stability.
Line range hint
1-1260: Comprehensive enhancements to session middleware testingThe changes in this file represent a significant improvement to the session middleware test suite:
- Consistent resource management: The addition of
sess.Release()calls throughout the file ensures proper cleanup of session resources.- Improved test coverage: New tests for various key types and absolute timeouts enhance the comprehensiveness of the suite.
- Updated terminology: The shift from "expiration" to "idle timeout" reflects evolving session management concepts.
- Enhanced error handling: The use of
deferfor session release improves the robustness of tests.- Better simulation of real-world scenarios: Releasing and reacquiring contexts mimics multiple request lifecycles.
These improvements contribute to a more reliable, thorough, and maintainable test suite, which in turn supports the development of a more robust session middleware.
middleware/session/data_msgp.go (3)
39-42: Ensure correct MessagePack encoding for empty maps.The
EncodeMsgmethod writes a fixed byte0x80, which represents an empty map in MessagePack format. Since thedatastruct appears to have no fields to serialize, this is appropriate. However, confirm that writing an empty map aligns with the expected behavior of the session serialization.Consider verifying that the
datastruct has no fields and that clients can correctly deserialize an empty map without errors.
50-50: Consistency in handling empty maps during marshaling.Similar to
EncodeMsg, theMarshalMsgmethod appends0x80to represent an empty map. Ensure that this approach is consistent with the rest of the codebase and that deserialization on the receiving end handles this correctly.Also applies to: 52-54
89-90: Confirm theMsgsizecalculation is accurate.The
Msgsizemethod now returns a constant values = 1, which corresponds to the size of an empty map in MessagePack (1 byte for0x80). Verify that this accurately reflects the serialized size of thedatastruct.
| storeSess, err := m.session.Get(c) | ||
| if err != nil { | ||
| // Handle error | ||
| return nil |
There was a problem hiding this comment.
Improve test coverage for error handling paths
The static analysis tool has identified several lines that are not covered by tests, primarily in error handling paths. This lack of coverage could lead to undetected issues in error scenarios. Consider adding test cases that exercise these error paths:
- Failure to get session from store (lines 48, 74, 93)
- Failure to save session (lines 78, 97)
- Session deletion (line 87)
Improving test coverage for these scenarios will enhance the reliability of the CSRF middleware and ensure that error handling behaves as expected in various situations.
Would you like assistance in generating test cases for these error handling paths?
Also applies to: 74-74, 78-78, 87-87, 93-93, 97-97
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 48-48: middleware/csrf/session_manager.go#L48
Added line #L48 was not covered by tests
middleware/session/store.go
Outdated
| if err := sess.Reset(); err != nil { | ||
| return nil, fmt.Errorf("failed to reset session: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add unit tests for session reset failure handling
The code handling errors during sess.Reset() is not covered by tests. If sess.Reset() fails, the function returns an error. Adding unit tests for this path will ensure that errors are correctly propagated and managed.
I can help create unit tests for this scenario if needed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 177-178: middleware/session/store.go#L177-L178
Added lines #L177 - L178 were not covered by tests
| return nil, err | ||
| } |
There was a problem hiding this comment.
Add unit tests for error handling during session retrieval
In the GetByID method, the error handling when s.Storage.Get(id) returns an error is not covered by tests. Ensuring this path is tested will improve the robustness of session retrieval logic.
Let me know if you'd like assistance in writing these tests.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 289-289: middleware/session/store.go#L289
Added line #L289 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
middleware/session/session.go (5)
33-43: Approved: Simplified sessionPool initializationThe change to
sessionPoolis a good optimization, directly returning a newSessioninstance. This simplifies the code and potentially improves performance by reducing the need for additional checks or initializations when acquiring a session from the pool.For consistency, consider updating the
byteBufferPoolin a similar manner:var byteBufferPool = sync.Pool{ New: func() any { - return new(bytes.Buffer) + return &bytes.Buffer{} }, }This change would make both pool initializations consistent in their approach.
46-61: Approved: Simplified acquireSession function with minor suggestionThe changes to the
acquireSessionfunction are appropriate and align well with the modifications to theSessionstruct. The removal of thebyteBuffercheck and settingfreshto true for newly acquired sessions are correct improvements.Consider improving error handling in the type assertion:
-s := sessionPool.Get().(*Session) //nolint:forcetypeassert,errcheck // We store nothing else in the pool +s, ok := sessionPool.Get().(*Session) +if !ok { + panic("invalid type in session pool") +}This change would make the code more robust against potential future modifications to the pool.
63-85: Approved: New Release method with clear documentationThe addition of the
Releasemethod is an excellent improvement for efficient resource management. The detailed documentation clearly explains its purpose and usage, which is crucial for preventing misuse.For consistency with other methods in the file, consider adding a usage example in the documentation:
// Usage: // // sess := session.Get(ctx) // defer sess.Release()This addition would make the documentation style consistent across all methods.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-82: middleware/session/session.go#L82
Added line #L82 was not covered by tests
292-318: Approved: Well-structured saveSession method with suggestionThe new
saveSessionmethod is a great addition, providing a clear separation of concerns from theSavemethod. It handles the core logic of saving the session, including setting theidleTimeoutand encoding the data, which aligns well with the PR objectives of improving session management.Consider enhancing error handling by wrapping the error returned from
s.config.Storage.Set:- return s.config.Storage.Set(s.id, encodedBytes, s.idleTimeout) + if err := s.config.Storage.Set(s.id, encodedBytes, s.idleTimeout); err != nil { + return fmt.Errorf("failed to save session: %w", err) + } + return nilThis change would provide more context in case of a storage error, making debugging easier.
463-501: Approved: Enhanced session expiration management with suggestionThe addition of
absExpiration,isAbsExpired, andsetAbsExpirationmethods is an excellent improvement to the session management capabilities. These methods allow for more granular control over session expiration, supporting both idle timeout and absolute (hard) expiration policies. This aligns well with the PR objectives and follows best practices in session management.For consistency with other methods in the file, consider renaming
setAbsExpirationtosetAbsoluteExpiration:-func (s *Session) setAbsExpiration(absExpiration time.Time) { +func (s *Session) setAbsoluteExpiration(absExpiration time.Time) {This change would make the method name more explicit and consistent with the full terminology used in the comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- middleware/session/session.go (7 hunks)
- middleware/session/session_test.go (48 hunks)
🧰 Additional context used
📓 Learnings (1)
middleware/session/session.go (1)
Learnt from: sixcolors PR: gofiber/fiber#3016 File: middleware/session/session.go:272-293 Timestamp: 2024-10-02T15:16:39.868Z Learning: In the `middleware/session/session.go` file, the `saveSession()` method returns either `nil` or an error, so additional error wrapping in the `Save()` method is unnecessary.
🪛 GitHub Check: codecov/patch
middleware/session/session.go
[warning] 82-82: middleware/session/session.go#L82
Added line #L82 was not covered by tests
[warning] 283-283: middleware/session/session.go#L283
Added line #L283 was not covered by tests
[warning] 285-285: middleware/session/session.go#L285
Added line #L285 was not covered by tests
[warning] 331-331: middleware/session/session.go#L331
Added line #L331 was not covered by tests
🪛 GitHub Check: lint
middleware/session/session_test.go
[failure] 1267-1267:
Duplicate words (app) found (dupword)
🔇 Additional comments (13)
middleware/session/session.go (4)
15-24: Approved: Session struct improvements align with PR objectivesThe changes to the
Sessionstruct are well-aligned with the PR objectives:
- Renaming
exptoidleTimeoutimproves clarity and consistency with NIST guidelines on session management.- The addition of the
freshfield allows for better tracking of newly created sessions.These modifications enhance the overall session management capabilities and readability of the code.
269-290: Approved: Improved Save method with handler-aware logicThe changes to the
Savemethod are well-thought-out and align with the PR objectives. The check for whether the session is being used in the handler prevents unnecessary saves, improving efficiency and potentially avoiding race conditions.The separation of concerns by introducing a
saveSessionmethod is a good practice, enhancing code readability and maintainability.Note: The static analysis tool indicates that lines 283 and 285 are not covered by tests. Consider adding test cases to cover these new code paths:
- When the session is being used in the handler.
- When the session is not being used in the handler.
This will ensure the new logic is thoroughly tested and maintain high code coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 283-283: middleware/session/session.go#L283
Added line #L283 was not covered by tests
[warning] 285-285: middleware/session/session.go#L285
Added line #L285 was not covered by tests
336-347: Approved: Consistent renaming of SetIdleTimeout methodThe renaming of
SetExpirytoSetIdleTimeoutis a positive change that aligns well with the PR objectives. This modification:
- Improves clarity by using more precise terminology.
- Enhances consistency with the new
idleTimeoutfield in theSessionstruct.- Better reflects the method's purpose in the context of session management.
The updated documentation accurately describes the method's new name and purpose, which is crucial for maintaining clear and usable APIs.
Line range hint
1-501: Summary: Comprehensive improvements to session managementThis PR introduces significant enhancements to the session management capabilities:
- Improved terminology consistency (e.g.,
idleTimeoutinstead ofexp).- Enhanced session lifecycle management with the new
Releasemethod.- More efficient saving mechanism that avoids unnecessary operations.
- Introduction of absolute expiration alongside idle timeout.
- Better alignment with NIST guidelines on session management.
These changes effectively address the issues raised in #2741 and improve the overall robustness of the session middleware.
Suggestions for minor improvements have been provided in individual comments, mainly focusing on consistency and error handling. Consider addressing these to further refine the implementation.
Great work on this comprehensive update to the session management system!
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-82: middleware/session/session.go#L82
Added line #L82 was not covered by testsmiddleware/session/session_test.go (9)
Line range hint
20-35: Improved session store initialization and resource managementThe changes in this segment look good. The use of
NewStore()aligns with the refactoring mentioned in the PR objectives. Additionally, the introduction ofsess.Release()andapp.ReleaseCtx(ctx)calls improves resource management by ensuring that resources are properly released after use.
Line range hint
49-74: Enhanced flexibility in session key typesThe changes in this segment improve the flexibility of the session middleware by allowing keys of any type, not just strings. This is evident from the change of
[]string{}to[]any{}for the empty keys slice. This enhancement aligns well with the PR objectives of improving session management.
Line range hint
84-110: Improved resource management and error handlingThe additions in this segment further enhance resource management. The use of
defer sess.Release()ensures that the session is properly released even if an error occurs during the test execution. This is a good practice that improves the robustness of the tests.
Line range hint
121-287: Consistent refactoring applied to test setupThe changes in this large segment are consistent with the overall refactoring of the session middleware. The use of
NewStore()and additionalRelease()calls maintain the improved resource management seen in earlier changes. The core test logic remains unchanged, which is good for preserving test coverage.
330-503: Comprehensive test added for session key typesExcellent addition of the
Test_Session_KeyTypesfunction. This new test significantly enhances the coverage of the session middleware by testing various key types, including both primitive and custom types. The inclusion of checks for unregistered key types is particularly valuable, as it verifies the robustness of the type handling system. This addition aligns well with the PR objectives of improving session management and enhances the overall quality of the test suite.
Line range hint
557-616: Aligned test with new IdleTimeout terminologyThe changes in this segment successfully update the test to use the new
IdleTimeoutterminology instead ofExpiration. This includes renaming the test function toTest_Session_Save_IdleTimeoutand usingSetIdleTimeoutinstead ofSetExpiry. These modifications accurately reflect the changes described in the PR objectives and ensure that the tests remain consistent with the updated session middleware implementation.
617-681: Crucial test added for AbsoluteTimeout functionalityThe addition of
Test_Session_Save_AbsoluteTimeoutis a valuable enhancement to the test suite. This new test function verifies the behavior of sessions with absolute timeout settings, which is an important aspect of session management. The test comprehensively checks for session expiration and ensures that expired sessions cannot be retrieved by ID. This addition significantly improves the test coverage for timeout-related functionality and aligns perfectly with the PR objectives of enhancing session management.
Line range hint
984-1060: Consistent refactoring and improved resource management in benchmarksThe changes in the benchmark functions are consistent with the refactoring seen throughout the file. The use of
NewStore()instead ofNew()aligns with earlier modifications. Importantly, the addition ofsess.Release()calls after each benchmark iteration improves resource management, ensuring that sessions are properly cleaned up. These changes contribute to more accurate and reliable benchmark results by preventing resource leaks during testing.
Line range hint
1157-1260: Enhanced concurrency testing with improved resource managementThe updates to the concurrency test are well-implemented and improve its robustness. The use of
NewStore()is consistent with earlier changes, while the addition ofsess.Release()calls ensures proper resource cleanup in concurrent scenarios. The introduction of a buffered channel for collecting errors is a smart improvement that prevents potential goroutine leaks. These changes collectively enhance the reliability of the concurrency test and align well with the overall improvements in resource management throughout the file.
|
Just a reminder: The change to the |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
middleware/session/session_test.go (2)
764-764: Improved session resource management and updated cookie format checkThe changes in the
Test_Session_Cookiefunction demonstrate continued improvements:
- Addition of
sess.Release()maintains consistent resource management.- The updated cookie format check using a regular expression allows for more flexible validation of the session cookie.
These modifications contribute to better resource handling and more robust cookie validation.
Consider making the regular expression more specific if possible. For example, you could use
[a-f0-9]{8}-[a-f0-9]{4}-4[a-f0-9]{3}-[89ab][a-f0-9]{3}-[a-f0-9]{12}for the UUID part if it follows the standard UUID v4 format. This would provide stricter validation while still allowing for flexibility in other parts of the cookie string.Also applies to: 776-777, 779-781
1263-1290: Valuable addition of error handling testThe new
Test_Session_StoreGetDecodeSessionDataErrorfunction is a welcome addition to the test suite:
- It verifies the middleware's behavior when encountering invalid or corrupt session data, which is crucial for robust error handling.
- The test uses the
uuidpackage to generate a fake session ID, consistent with other changes in the file.- It follows established patterns for store creation and context management.
This test enhances the overall reliability of the session middleware by ensuring proper handling of decode errors.
Consider adding a check to verify that the session was not created or retrieved despite the error. This could be done by attempting to get a value from the session after the error occurs, expecting it to fail or return nil. This would ensure that the middleware doesn't create a new session when it encounters invalid data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- middleware/session/session_test.go (49 hunks)
🔇 Additional comments (15)
middleware/session/session_test.go (15)
11-11: Improved session management and resource handlingThe changes in this section demonstrate better resource management practices:
- Use of
NewStore()instead ofNew()for creating stores, which likely provides more flexibility or features.- Addition of
sess.Release()calls ensures proper cleanup of session resources.- The import of the
uuidpackage suggests enhanced functionality related to unique identifiers.These modifications contribute to more robust and efficient session handling.
Also applies to: 21-21, 36-37, 89-89, 100-101
50-50: Enhanced type flexibility in session data handlingThe modifications in the
Test_Session_Typesfunction demonstrate improved type handling:
- Session keys are now managed using
[]anyinstead of[]string, allowing for more flexible data types in sessions.- Consistent with earlier changes,
sess.Release()calls have been added to ensure proper resource cleanup.These changes contribute to more versatile session data management and maintain the improved resource handling pattern.
Also applies to: 64-64, 75-75, 196-196
297-297: Consistent improvements in resource managementThe changes in the
Test_Session_Store_Resetfunction maintain the pattern of improvements seen throughout the file:
- Use of
NewStore()for store creation.- Addition of
sess.Release()for proper session cleanup.- Use of
defer app.ReleaseCtx(ctx)ensures context release even in case of panics or early returns.These modifications contribute to more robust and consistent resource management across different test scenarios.
Also applies to: 317-325
331-504: Comprehensive new test for session key typesThe addition of the
Test_Session_KeyTypesfunction significantly enhances the test suite:
- It verifies the behavior of various key types in session data, including standard types and custom structs.
- The test includes both positive cases (registered types) and negative cases (unregistered types), ensuring robust type handling.
- It follows the established patterns of using
NewStore(),sess.Release(), and deferred context release, maintaining consistency with other tests.This new test function greatly improves the coverage and reliability of the session middleware's type handling capabilities.
558-558: Improved session timeout handling and consistent resource managementThe changes in the
Test_Session_Save_IdleTimeoutfunction (previouslyTest_Session_Save_Expiration) demonstrate important improvements:
- The renaming of the function and the change from
SetExpirytoSetIdleTimeoutindicate a more precise conceptualization of session timeouts, likely aligning better with real-world usage patterns.- The addition of
sess.Release()calls maintains the consistent pattern of improved resource management seen throughout the file.These modifications contribute to more accurate session timeout testing and maintain the improved resource handling pattern.
Also applies to: 583-583, 589-603
618-682: Comprehensive new test for absolute session timeoutsThe addition of the
Test_Session_Save_AbsoluteTimeoutfunction significantly enhances the test suite:
- It verifies the behavior of sessions with absolute timeout settings, a crucial aspect of session management.
- The test covers both the validity of the session before expiration and the proper handling of expired sessions.
- It includes an important check for the inability to retrieve an expired session by ID, ensuring proper security measures.
- The function follows the established patterns of using
NewStore(),sess.Release(), and deferred context release, maintaining consistency with other tests.This new test function greatly improves the coverage and reliability of the session middleware's timeout handling capabilities, particularly for absolute timeouts.
691-691: Consistent improvement in session resource managementThe changes in the
Test_Session_Destroyfunction maintain the pattern of improved resource handling:
- Addition of
sess.Release()calls ensures proper cleanup of session resources.- Use of
defer sess.Release()in the second part of the test guarantees session cleanup even in case of test failures.These modifications contribute to more robust and consistent resource management, aligning with the improvements seen throughout the file.
Also applies to: 700-700, 730-730, 739-739
752-753: Consistent updates to session configuration testingThe changes in the
Test_Session_Custom_Configfunction align with updates seen elsewhere in the file:
- Use of
NewStore()instead ofNew()for creating session stores.- Testing of
IdleTimeoutinstead ofExpiration, reflecting the shift in timeout handling terminology and functionality.These modifications ensure that the custom configuration tests remain consistent with the updated session middleware API and concepts.
Also applies to: 756-757
788-788: Consistent improvement in session resource managementThe changes in the
Test_Session_Cookie_In_Middleware_Chainfunction maintain the pattern of improved resource handling:
- Addition of
sess.Release()calls ensures proper cleanup of session resources.- Use of
defer sess.Release()guarantees session cleanup even in case of test failures.These modifications contribute to more robust and consistent resource management, aligning with the improvements seen throughout the file.
Also applies to: 803-804, 807-807
820-820: Consistent improvement in session resource managementThe changes in the
Test_Session_Deletes_Single_Keyfunction maintain the pattern of improved resource handling:
- Addition of
sess.Release()calls ensures proper cleanup of session resources.- Use of
defer sess.Release()guarantees session cleanup even in case of test failures.These modifications contribute to more robust and consistent resource management, aligning with the improvements seen throughout the file.
Also applies to: 831-831, 841-841, 847-847
862-862: Consistent updates and improvements in session handlingThe changes in the
Test_Session_Resetfunction align with improvements seen throughout the file:
- Use of
NewStore()instead ofNew()for creating session stores.- Addition of
sess.Release()calls for proper resource management.- Updated check for empty keys using
[]any{}instead of[]string{}, consistent with the improved type flexibility in session data handling.These modifications ensure that the session reset tests remain consistent with the updated session middleware API and concepts, while also improving resource management.
Also applies to: 884-884, 906-906, 917-918
936-936: Consistent improvements in session handling and resource managementThe changes in the
Test_Session_Regeneratefunction align with improvements seen throughout the file:
- Use of
NewStore()instead ofNew()for creating session stores.- Addition of
sess.Release()calls for proper resource management.- Use of
defer sess.Release()ensures session cleanup even in case of test failures.These modifications contribute to more robust and consistent resource management, while keeping the test aligned with the updated session middleware API.
Also applies to: 952-953, 966-966
985-985: Improved resource management in benchmark functionsThe changes in all benchmark functions demonstrate a consistent improvement in resource handling:
- Addition of
sess.Release()calls ensures proper cleanup of session resources after each benchmark iteration.- Consistent use of
NewStore()instead ofNew()for creating session stores.These modifications are crucial for several reasons:
- They ensure that the benchmarks accurately reflect the true performance characteristics of the session middleware, including resource cleanup.
- They prevent potential resource leaks during benchmarking, which could skew results.
- They maintain consistency with the improvements made in the test functions, ensuring that benchmarks and tests use the middleware in the same way.
By implementing these changes, the benchmarks now provide a more accurate representation of the session middleware's performance and resource usage.
Also applies to: 996-997, 1003-1003, 1016-1017, 1025-1025, 1036-1038, 1046-1046, 1059-1061, 1071-1071, 1084-1084, 1090-1090, 1105-1105, 1113-1113, 1125-1125, 1133-1133, 1147-1147
Line range hint
1158-1262: Excellent addition of comprehensive concurrency testThe new
Test_Session_Concurrencyfunction is a significant and valuable addition to the test suite:
- It verifies the thread-safety of the session middleware by simulating concurrent access using goroutines.
- The test covers crucial aspects of session handling in a concurrent environment:
- Session creation and retrieval
- Data persistence across requests
- Proper handling of session freshness
- Correct functioning of session destruction
- It employs good concurrent programming practices:
- Use of sync.WaitGroup for synchronization
- Buffered channel for collecting errors from goroutines
- Proper resource management with deferred releases
This test greatly enhances the reliability of the session middleware by ensuring its correct behavior under concurrent access, which is crucial for real-world web applications.
Line range hint
1-1290: Summary of improvements in session middleware testsThe changes in this file represent a significant enhancement to the session middleware test suite:
Consistent improvement in resource management:
- Addition of
sess.Release()calls throughout all tests and benchmarks.- Use of
deferfor ensuring cleanup in various scenarios.API updates:
- Consistent use of
NewStore()instead ofNew()for store creation.- Shift from
ExpirationtoIdleTimeoutin configuration and testing.Enhanced type handling:
- Use of
[]anyinstead of[]stringfor session keys, allowing more flexible data types.New comprehensive tests:
- Addition of
Test_Session_KeyTypesfor thorough type handling verification.- Introduction of
Test_Session_Save_AbsoluteTimeoutfor testing absolute timeout behavior.- Implementation of
Test_Session_Concurrencyfor verifying thread-safety.- Addition of
Test_Session_StoreGetDecodeSessionDataErrorfor robust error handling checks.Improved benchmark accuracy:
- Inclusion of resource cleanup in benchmarks for more realistic performance measurements.
These changes collectively contribute to a more robust, reliable, and comprehensive test suite for the session middleware. The improvements in resource management and the addition of new test scenarios will likely lead to increased stability and accuracy in test results, as well as better coverage of real-world usage patterns and edge cases.
- Update absolute timeout handling in getSession function - Set absolute expiration time in getSession function - Delete expired session in GetByID function
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
middleware/session/store.go (4)
35-45: Improved function naming and documentationThe renaming of
NewtoNewStoreand the addition of detailed documentation enhance the clarity and usability of the function.Consider updating the usage example to reflect the new function name:
- store := session.New() + store := session.NewStore()
164-169: Improved error handling ingetSessionmethodThe addition of error handling for session data decoding enhances the robustness of the method. The mutex is properly unlocked before returning the error, which is a good practice.
Consider using
errors.Joinfor cleaner error wrapping:- return nil, fmt.Errorf("failed to decode session data: %w", err) + return nil, errors.Join(errors.New("failed to decode session data"), err)
172-182: Enhanced session expiration handlingThe addition of absolute session expiration handling improves the overall session management. The logic for resetting expired sessions and setting new expiration times is well-implemented.
Consider using
errors.Joinfor cleaner error wrapping in the session reset error:- return nil, fmt.Errorf("failed to reset session: %w", err) + return nil, errors.Join(errors.New("failed to reset session"), err)
256-324: NewGetByIDmethod with comprehensive implementationThe addition of the
GetByIDmethod provides a useful way to retrieve sessions directly by ID. The implementation includes proper error handling, session expiration checks, and cleanup of expired sessions. The detailed documentation clearly explains the method's behavior and potential pitfalls.Consider using
errors.Joinfor cleaner error wrapping in the session data decoding error:- return nil, fmt.Errorf("failed to decode session data: %w", err) + return nil, errors.Join(errors.New("failed to decode session data"), err)middleware/session/session_test.go (4)
331-504: LGTM: Comprehensive key type testing addedThe new
Test_Session_KeyTypesfunction significantly enhances the test coverage for session functionality. It thoroughly tests various key types, including both registered and unregistered types, which is crucial for ensuring type safety and correct behavior of the session store.The test function includes proper resource management with deferred releases, aligning with the improved practices seen throughout the file.
This addition aligns well with the PR objectives and greatly improves the robustness of the test suite.
Consider adding error checking when setting unregistered key types. For example:
err := sess.Set(unregisteredKey, "test") require.Error(t, err, "Expected an error when setting an unregistered key type")This would make the test even more thorough by explicitly checking that setting unregistered types results in an error.
Line range hint
558-700: LGTM: Enhanced timeout testing and consistent resource managementThe updates to
Test_Session_Save_IdleTimeout(renamed fromTest_Session_Save_Expiration) and the addition ofTest_Session_Save_AbsoluteTimeoutalign well with the PR objectives. These changes provide better coverage for the timeout functionality, including the new absolute timeout feature.The consistent use of
NewStore()for initialization and the addition of session releases improve resource management throughout these tests.For consistency, consider adding error checking for
sess.Save()calls in both functions. For example:err = sess.Save() require.NoError(t, err, "Failed to save session")This would make the error handling more robust and consistent across all test functions.
1281-1319: LGTM: Comprehensive error handling test addedThe new
Test_Session_StoreGetDecodeSessionDataErrorfunction significantly enhances the test coverage for error handling when decoding invalid session data. It properly tests both theGet()andGetByID()methods, ensuring that appropriate errors are returned when dealing with corrupted session data.This addition aligns well with the PR objectives of improving session management and error handling, contributing to the overall robustness of the session middleware.
For consistency with other tests in the file, consider using
require.ErrorIs()instead ofrequire.ErrorContains()when checking for specific errors. For example:require.ErrorIs(t, err, ErrFailedToDecodeSessionData, "Unexpected error")This assumes that there's a specific error type
ErrFailedToDecodeSessionDatadefined for this case. If not, the current implementation is appropriate.🧰 Tools
🪛 GitHub Check: lint
[failure] 1281-1281:
empty-lines: extra empty line at the end of a block (revive)
[failure] 1319-1319:
unnecessary trailing newline (whitespace)
1281-1281: Minor: Address linting issuesThere are two minor linting issues in the file:
- An extra empty line at the end of a block (line 1281).
- An unnecessary trailing newline (line 1319).
While these don't affect functionality, addressing them would improve code consistency and adhere to Go style guidelines.
Consider making the following changes:
- Remove the extra empty line at line 1281.
- Remove the trailing newline at line 1319.
These minor adjustments will resolve the linting warnings and improve the overall code style consistency.
Also applies to: 1319-1319
🧰 Tools
🪛 GitHub Check: lint
[failure] 1281-1281:
empty-lines: extra empty line at the end of a block (revive)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- middleware/session/session_test.go (49 hunks)
- middleware/session/store.go (4 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
middleware/session/session_test.go
[failure] 1281-1281:
empty-lines: extra empty line at the end of a block (revive)
[failure] 1319-1319:
unnecessary trailing newline (whitespace)
🔇 Additional comments (35)
middleware/session/store.go (8)
16-20: Improved error handling with new error variablesThe addition of these new error variables enhances the error handling capabilities of the package. The error messages are consistent in capitalization and terminology, following Go conventions.
53-62: EnhancedNewStorefunction with custom type registrationThe addition of custom type registration for
AbsoluteTimeoutimproves the flexibility of the session store. This change allows for better handling of absolute timeouts in sessions.
77-104: ImprovedGetmethod with better documentation and session loading checkThe
Getmethod has been enhanced with detailed documentation and a check to prevent duplicate session loading. This improvement helps prevent potential issues and improves the overall robustness of the session handling.
106-121: Added documentation forgetSessionmethodThe addition of detailed documentation for the
getSessionmethod improves the overall clarity of the codebase and helps developers understand the method's purpose and usage.
187-196: Added documentation forgetSessionIDmethodThe addition of detailed documentation for the
getSessionIDmethod enhances the overall clarity of the codebase and aids developers in understanding the method's purpose and usage.
221-230: Added documentation forResetmethodThe addition of detailed documentation for the
Resetmethod improves the overall clarity of the codebase and helps developers understand the method's purpose and potential error handling.
236-248: Added documentation forDeletemethodThe addition of detailed documentation for the
Deletemethod enhances the overall clarity of the codebase and aids developers in understanding the method's purpose, parameters, and potential error handling.
Line range hint
1-324: Overall assessment: Significant improvements to the session store implementationThe changes made to
middleware/session/store.gorepresent a substantial improvement to the session management functionality. Key enhancements include:
- Introduction of specific error variables for better error handling.
- Renaming of the
Newfunction to the more descriptiveNewStore.- Addition of comprehensive documentation for all methods.
- Implementation of absolute timeout handling for sessions.
- Introduction of the
GetByIDmethod for direct session retrieval.These changes collectively improve the package's usability, robustness, and maintainability. The code is well-structured, and the new features are implemented with careful consideration for error handling and edge cases.
middleware/session/session_test.go (27)
11-11: LGTM: Added UUID package importThe addition of the
github.com/google/uuidimport is appropriate for generating UUIDs in the tests. This is a reliable and widely-used package for this purpose.
21-21: LGTM: Updated store initializationThe change from
New()toNewStore()is consistent with the PR objectives mentioning a re-write of the session middleware. This new initialization method likely provides enhanced functionality or configuration options.
36-37: LGTM: Improved resource managementThe addition of
sess.Release()and the reacquisition of the context enhance resource management and ensure a clean state for subsequent test operations. This change aligns with best practices for handling sessions and contexts in tests.
50-50: LGTM: Enhanced type flexibility for session keysThe change from
[]string{}to[]any{}in the assertions forsess.Keys()indicates increased flexibility in the types of keys that can be stored in the session. This modification allows for a more versatile session storage system.Also applies to: 64-64
85-87: LGTM: Consistent resource cleanupThe addition of explicit
sess.Release()andapp.ReleaseCtx(ctx)calls ensures proper cleanup of resources after test operations. This change is consistent with earlier modifications and reinforces good resource management practices throughout the test suite.
100-101: LGTM: Consistent session releaseThe addition of
sess.Release()after checking for a fresh session ensures consistent resource cleanup, even in this specific test scenario. This change aligns with the improved resource management practices implemented throughout the test suite.
111-111: LGTM: Robust session cleanup with deferThe addition of a deferred
sess.Release()call ensures that the session is properly released even in case of early returns or panics. This is a robust Go practice for resource management and improves the overall reliability of the test cleanup.
122-122: LGTM: Consistent store initializationThe change from
New()toNewStore()in theTest_Session_Typesfunction maintains consistency with earlier modifications. This ensures that the new store initialization method is used uniformly across all tests in the suite.
196-198: LGTM: Consistent resource management in Test_Session_TypesThe addition of session release and context reacquisition in the
Test_Session_Typesfunction maintains consistency with the resource management improvements implemented throughout the test suite. These changes ensure a clean state between different parts of the test, enhancing test isolation and reliability.
288-289: LGTM: Final session cleanup in Test_Session_TypesThe addition of
sess.Release()at the end of theTest_Session_Typesfunction ensures proper cleanup of the session resource. This change completes the consistent resource management pattern implemented throughout the test suite.
297-297: LGTM: Consistent store initialization in Test_Session_Store_ResetThe change from
New()toNewStore()in theTest_Session_Store_Resetfunction maintains consistency with earlier modifications. This ensures that the new store initialization method is used uniformly across all tests in the suite.
317-325: LGTM: Enhanced resource management in Test_Session_Store_ResetThe additions of session release, context reacquisition, and deferred releases in the
Test_Session_Store_Resetfunction align with the improved resource management practices implemented throughout the test suite. The use ofdeferensures proper cleanup even in case of early returns or panics, enhancing overall test reliability.
513-513: LGTM: Consistent store initialization in Test_Session_SaveThe change from
New()toNewStore()in theTest_Session_Savefunction maintains consistency with earlier modifications. This ensures that the new store initialization method is used uniformly across all tests in the suite.
528-528: LGTM: Consistent session cleanup in Test_Session_SaveThe addition of
sess.Release()calls at the end of each subtest inTest_Session_Saveensures proper cleanup of session resources. These changes maintain consistency with the improved resource management practices implemented throughout the test suite.Also applies to: 554-554
709-709: LGTM: Consistent updates in Test_Session_DestroyThe changes in
Test_Session_Destroy, including the use ofNewStore()for initialization and the addition of session releases, align with the modifications seen throughout the file. These updates improve resource management and maintain consistency across the test suite.Also applies to: 730-730, 748-748, 757-757
770-775: LGTM: Improved configuration testingThe updates in
Test_Session_Custom_Configenhance the testing of configuration options. The use ofNewStore()with custom configuration and the checks for both custom and default values provide better coverage for the store's configuration capabilities.
782-782: LGTM: Enhanced cookie testing and consistent updatesThe changes in
Test_Session_Cookie, including the use ofNewStore(), addition of session release, and updated cookie checking logic, align with the improvements seen throughout the file. The more specific cookie validation enhances the test's effectiveness.Also applies to: 794-795, 797-799
806-806: LGTM: Consistent updates in middleware chain testThe changes in
Test_Session_Cookie_In_Middleware_Chain, including the use ofNewStore()and the addition of session releases, align with the improvements seen throughout the file. These updates enhance resource management and maintain consistency across the test suite.Also applies to: 821-822, 825-825
838-838: LGTM: Consistent resource management in single key deletion testThe updates in
Test_Session_Deletes_Single_Key, including the use ofNewStore()and the addition of session releases (including a deferred release), align with the improvements seen throughout the file. These changes enhance resource management and maintain consistency across the test suite.Also applies to: 849-849, 859-859, 865-865
880-880: LGTM: Consistent updates and improved type handling in reset testThe changes in
Test_Session_Reset, including the use ofNewStore(), addition of session releases, and updated type assertion, align with the improvements seen throughout the file. The change from[]stringto[]anyenhances type flexibility. These updates improve resource management and maintain consistency across the test suite.Also applies to: 902-902, 924-924, 935-936
954-954: LGTM: Consistent updates in regenerate testThe changes in
Test_Session_Regenerate, including the use ofNewStore()and the addition of session releases, align with the improvements seen throughout the file. These updates enhance resource management and maintain consistency across the test suite.Also applies to: 970-971, 984-984
Line range hint
1003-1015: LGTM: Improved resource management in benchmarksThe updates in
Benchmark_Session, including the use ofNewStore()and the addition of session releases within the benchmark loops, align with the improvements seen throughout the file. These changes ensure proper resource management even during performance testing, which is crucial for accurate benchmarking results.Also applies to: 1021-1035
Line range hint
1043-1056: LGTM: Consistent resource management in parallel benchmarksThe changes in
Benchmark_Session_Parallel, including the use ofNewStore()and the addition of session releases within the parallel benchmark loops, align with the improvements seen throughout the file. These updates ensure proper resource management even during parallel performance testing, which is essential for accurate and reliable benchmark results.Also applies to: 1064-1079
Line range hint
1089-1102: LGTM: Improved resource management in asserted benchmarksThe updates in
Benchmark_Session_Asserted, including the use ofNewStore()and the addition of session releases within the asserted benchmark loops, align with the improvements seen throughout the file. These changes ensure proper resource management even during asserted performance testing, contributing to more accurate and reliable benchmark results.Also applies to: 1108-1123
Line range hint
1131-1143: LGTM: Consistent resource management in parallel asserted benchmarksThe changes in
Benchmark_Session_Asserted_Parallel, including the use ofNewStore()and the addition of session releases within the parallel asserted benchmark loops, align with the improvements seen throughout the file. These updates ensure proper resource management even during parallel asserted performance testing, contributing to more accurate and reliable benchmark results.Also applies to: 1151-1165
1176-1176: LGTM: Enhanced resource management in concurrency testThe updates in
Test_Session_Concurrency, including the use ofNewStore()and the addition of session releases in concurrent test operations, align with the improvements seen throughout the file. These changes ensure proper resource management even during concurrent testing scenarios, which is crucial for detecting race conditions and ensuring thread-safety of the session handling.Also applies to: 1190-1190, 1214-1216, 1233-1233
Line range hint
1-1319: Overall: Significant improvements in session testing and resource managementThis update to the session middleware test file brings substantial enhancements:
- Consistent use of
NewStore()for initialization across all tests and benchmarks.- Significantly improved resource management with consistent session releases.
- Enhanced test coverage, including new tests for key types, absolute timeout, and error handling.
- Updated benchmarks to ensure proper resource management during performance testing.
- Improved concurrency testing to ensure thread-safety of session handling.
These changes align well with the PR objectives of re-writing the session middleware with improved management and error handling. The expanded test coverage, especially for new features like absolute timeout and various key types, contributes to the overall robustness of the session middleware.
The consistent updates across all test
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
middleware/session/store.go (3)
35-62: Improved NewStore function with enhanced documentation and featuresThe renaming of
NewtoNewStoreand the addition of detailed documentation significantly improve the function's clarity and usability. The new absolute timeout handling is a valuable addition to session management.Consider updating the usage example in the documentation to reflect the new function name:
- store := session.New() + store := session.NewStore()
Line range hint
121-182: Improved getSession method with enhanced session managementThe additions to handle session freshness, expiration, and absolute timeouts significantly improve the session management capabilities. The error handling for session data decoding has also been enhanced.
However, there's a potential issue with error variable shadowing in the data decoding error handling. Consider the following change:
- err := sess.decodeSessionData(rawData) + decodeErr := sess.decodeSessionData(rawData) sess.data.Unlock() - if err != nil { + if decodeErr != nil { sess.mu.Unlock() - return nil, fmt.Errorf("failed to decode session data: %w", err) + return nil, fmt.Errorf("failed to decode session data: %w", decodeErr) }This change prevents shadowing the outer
errvariable and ensures that the correct error is being wrapped and returned.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 178-178: middleware/session/store.go#L178
Added line #L178 was not covered by tests
178-178: Improve test coverage for error handling pathsThe static analysis tool has identified three lines that are not covered by tests:
- Line 178: Error handling for session reset in
getSession- Line 290: Error handling for storage retrieval in
GetByID- Line 315: Error handling for session destruction in
GetByIDConsider adding unit tests to cover these error scenarios. This will ensure that the error handling paths are working as expected and improve the overall robustness of the package.
Also applies to: 290-290, 315-315
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 178-178: middleware/session/store.go#L178
Added line #L178 was not covered by testsmiddleware/session/session.go (3)
46-61: Approved: acquireSession function improvements with suggestionThe changes to
acquireSessionfunction are appropriate. Settingfreshto true for newly acquired sessions is correct. The removal of thebyteBuffercheck is safe as it's always initialized in the pool's New function.However, consider improving error handling in the type assertion:
- s := sessionPool.Get().(*Session) //nolint:forcetypeassert,errcheck // We store nothing else in the pool + s, ok := sessionPool.Get().(*Session) + if !ok { + panic("invalid type in session pool") + }This change would make the code more robust against potential future modifications to the pool.
63-85: Approved: New Release method with suggestionThe addition of the
Releasemethod is an excellent improvement for resource management. The detailed documentation clearly explains its usage and limitations.Consider adding a check to prevent double release:
func (s *Session) Release() { if s == nil { return } + if s.data == nil { + // Session already released + return + } releaseSession(s) }This change would make the method more robust against potential misuse.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-82: middleware/session/session.go#L82
Added line #L82 was not covered by tests
475-513: Approved: New methods for absolute expiration managementThe addition of
absExpiration,isAbsExpired, andsetAbsExpirationmethods provides a robust mechanism for managing absolute expiration times for sessions. This aligns well with the PR objectives for improved session management.Consider adding a method to clear the absolute expiration:
// clearAbsExpiration removes the absolute expiration time from the session. func (s *Session) clearAbsExpiration() { s.Delete(absExpirationKey) }This would provide a convenient way to remove the absolute expiration if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- middleware/session/session.go (8 hunks)
- middleware/session/session_test.go (50 hunks)
- middleware/session/store.go (4 hunks)
- middleware/session/store_test.go (6 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
middleware/session/session.go
[warning] 82-82: middleware/session/session.go#L82
Added line #L82 was not covered by tests
[warning] 287-287: middleware/session/session.go#L287
Added line #L287 was not covered by tests
[warning] 289-289: middleware/session/session.go#L289
Added line #L289 was not covered by tests
[warning] 335-335: middleware/session/session.go#L335
Added line #L335 was not covered by testsmiddleware/session/store.go
[warning] 178-178: middleware/session/store.go#L178
Added line #L178 was not covered by tests
[warning] 290-290: middleware/session/store.go#L290
Added line #L290 was not covered by tests
[warning] 315-315: middleware/session/store.go#L315
Added line #L315 was not covered by tests
🔇 Additional comments (28)
middleware/session/store_test.go (7)
23-23: LGTM: Consistent renaming ofNew()toNewStore()The changes in the
Test_Store_getSessionIDfunction correctly implement the renaming ofNew()toNewStore(). This maintains consistency with the updated API and doesn't alter the logic of the test cases.Also applies to: 36-38, 51-53
76-76: LGTM: Consistent renaming inTest_Store_GetThe change in the
Test_Store_Getfunction correctly implements the renaming ofNew()toNewStore(). This maintains consistency with the updated API and doesn't alter the logic of the test case.
96-96: LGTM: Consistent renaming inTest_Store_DeleteSessionThe change in the
Test_Store_DeleteSessionfunction correctly implements the renaming ofNew()toNewStore(). This maintains consistency with the updated API and doesn't alter the logic of the test case.
120-140: Good addition, but consider usingNewStore()The new test case
TestStore_Get_SessionAlreadyLoadedis a valuable addition, covering an important error scenario. However, at line 132, consider usingNewStore()instead of&Store{}to ensure proper initialization and consistency with other tests:store := NewStore()This change will ensure that any initialization logic in
NewStore()is not bypassed.
142-156: LGTM: Comprehensive test cases forDeletemethodThe new
TestStore_Deletefunction adds valuable test coverage for theDeletemethod. It correctly tests two important scenarios:
- Attempting to delete with an empty session ID (expecting an error).
- Attempting to delete a non-existing session (expecting no error).
The use of
NewStore()is consistent with other tests, and the error checking is appropriate.
158-218: LGTM with a suggestion: Release the acquired contextThe new
Test_Store_GetByIDfunction is a great addition, providing comprehensive coverage for theGetByIDmethod across various scenarios. However, there's a minor improvement to be made:At line 182, a new context is acquired:
ctx := fiber.New().AcquireCtx(&fasthttp.RequestCtx{})To prevent potential resource leaks, especially in tests that may run in parallel, it's recommended to release this context after use. Add the following line at the end of the "valid session ID" subtest:
fiber.New().ReleaseCtx(ctx)This ensures proper cleanup of resources and aligns with best practices for context management in Fiber.
Line range hint
1-218: Overall assessment: Good improvements with minor suggestionsThe changes to
middleware/session/store_test.goare generally well-implemented and valuable:
- Consistent renaming of
New()toNewStore()throughout the file.- Addition of new test cases (
TestStore_Get_SessionAlreadyLoaded,TestStore_Delete, andTest_Store_GetByID) that improve coverage and test important scenarios.Minor suggestions for improvement:
- In
TestStore_Get_SessionAlreadyLoaded, consider usingNewStore()instead of&Store{}for consistency.- In the "valid session ID" subtest of
Test_Store_GetByID, remember to release the acquired context to prevent potential resource leaks.These changes significantly enhance the test suite for the session middleware, providing better coverage and aligning with best practices.
middleware/session/store.go (4)
16-20: Improved error handling with new error variablesThe addition of these new error variables enhances the error handling capabilities of the package. The error messages are clear, consistent, and follow Go conventions.
77-104: Enhanced Get method with duplicate loading preventionThe addition of a check for an already loaded session is a valuable safeguard against potential inconsistencies. The detailed documentation provides clear guidance on the method's behavior and possible errors.
236-254: Improved Delete method with input validationThe addition of detailed documentation and the check for an empty session ID enhance the method's usability and robustness. The empty ID check is a good safeguard against potential errors.
Line range hint
1-322: Overall assessment: Significant improvements to session managementThe changes made to
middleware/session/store.gorepresent a substantial enhancement to the session management capabilities of the package. Key improvements include:
- Better error handling with new, specific error variables
- Enhanced documentation across all methods
- Improved session retrieval and management logic
- New features such as absolute timeout handling
The identified issues are minor and can be easily addressed. Once these are resolved and test coverage is improved for the error handling paths, the changes will greatly benefit the overall quality and functionality of the session management system.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 290-290: middleware/session/store.go#L290
Added line #L290 was not covered by tests
[warning] 315-315: middleware/session/store.go#L315
Added line #L315 was not covered by testsmiddleware/session/session.go (8)
15-24: Approved: Session struct improvementsThe changes to the Session struct are well-aligned with the PR objectives. Renaming
exptoidleTimeoutimproves clarity, and the addition of thefreshfield enhances session management capabilities.
26-31: Approved: New constants for absolute expirationThe addition of
absExpirationKeyTypeandabsExpirationKeyprovides a structured way to manage absolute expiration times for sessions. This enhancement aligns well with the PR objectives for improved session management.
41-42: Approved: Improved sessionPool initializationThe change to return a pointer to a new Session in the sessionPool's New function ensures proper initialization of new sessions. This is a good practice for memory management and consistency.
296-322: Approved: New saveSession method for improved session managementThe addition of the
saveSessionmethod is a good improvement in code organization. It separates the concerns of saving session data from the mainSavemethod, enhancing maintainability. The automatic setting ofidleTimeoutensures consistent session management across the application.
340-351: Approved: Renamed SetIdleTimeout methodThe renaming of
SetExpirytoSetIdleTimeoutis a positive change that improves clarity and consistency in the API. This aligns well with the PR objectives to address inconsistencies in session handling. The updated documentation accurately reflects the method's purpose.
Line range hint
354-387: Approved: Updated setSession method using idleTimeoutThe changes in the
setSessionmethod correctly implement the use ofidleTimeoutfor setting cookie expiration. This is consistent with the earlier changes in the Session struct and ensures that session cookies are managed correctly based on the idle timeout.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 287-287: middleware/session/session.go#L287
Added line #L287 was not covered by tests
[warning] 289-289: middleware/session/session.go#L289
Added line #L289 was not covered by tests
[warning] 335-335: middleware/session/session.go#L335
Added line #L335 was not covered by tests
Line range hint
1-513: Overall assessment: Significant improvements in session managementThis PR introduces substantial enhancements to the session middleware:
- Improved clarity with the renaming of
exptoidleTimeout.- Enhanced resource management with the new
Releasemethod.- More efficient session saving with handler-aware checks.
- Better separation of concerns with the new
saveSessionmethod.- Introduction of absolute expiration management.
These changes align well with the PR objectives and address the issues raised in #2741. The code is generally well-structured and documented.
Some minor suggestions for improvement have been made:
- Enhancing error handling in
acquireSession.- Adding a check to prevent double release in the
Releasemethod.- Improving test coverage for the new code paths in the
Savemethod.- Adding a method to clear absolute expiration.
Overall, this is a solid improvement to the session middleware.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-82: middleware/session/session.go#L82
Added line #L82 was not covered by tests
269-294: Approved: Improved Save method with handler-aware logicThe changes to the
Savemethod are well-thought-out and align with the PR objectives. The check for whether the session is being used in the handler prevents unnecessary saves, improving efficiency.Note: The static analysis tool indicates that lines 287 and 289 are not covered by tests. Consider adding test cases to cover these new code paths to ensure robustness.
To verify the test coverage, you can run the following command:
This will help identify the specific lines that need additional test coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 287-287: middleware/session/session.go#L287
Added line #L287 was not covered by tests
[warning] 289-289: middleware/session/session.go#L289
Added line #L289 was not covered by testsmiddleware/session/session_test.go (9)
11-11: LGTM: Added import for uuid packageThe addition of the "github.com/google/uuid" import is appropriate for generating unique identifiers in session management.
21-21: Improved session management and flexibility
- Changed from
New()toNewStore()for store creation, which aligns with the new naming convention.- Added
sess.Release()andapp.ReleaseCtx(ctx)calls, improving resource management by explicitly releasing sessions and contexts.- Changed key type from
[]stringto[]any, allowing for more flexible key types in sessions.These changes enhance the overall robustness and flexibility of the session management system.
Also applies to: 36-37, 50-50, 64-64, 75-75, 85-87, 100-101
513-513: Enhanced session lifecycle management
- Consistently changed from
New()toNewStore()for store creation.- Added
sess.Release()calls, further improving resource management.- Changed from
SetExpiry()toSetIdleTimeout(), indicating a more precise approach to session timeout handling.- Added new verification code to ensure proper session expiration and creation of new sessions.
These changes significantly improve the session lifecycle management, ensuring proper resource cleanup and more accurate timeout handling.
Also applies to: 528-528, 534-534, 554-554, 566-566, 583-583, 589-589, 602-603, 608-615
558-558: Improved test coverage for session timeout mechanisms
- Renamed
Test_Session_Save_ExpirationtoTest_Session_Save_IdleTimeout, accurately reflecting the change in timeout mechanism.- Added new test function
Test_Session_Save_AbsoluteTimeout, which provides coverage for the newly implemented absolute timeout feature.These changes enhance the test suite by ensuring both idle timeout and absolute timeout mechanisms are properly tested. The new test thoroughly checks various aspects of the absolute timeout behavior, including:
- Correct timeout application
- Session data persistence and retrieval
- Proper session expiration and recreation
This addition significantly improves the robustness of the session management testing.
Also applies to: 618-698
709-709: Improved session management, flexibility, and verification
- Consistently changed from
New()toNewStore()for store creation throughout the file.- Added
sess.Release()calls in various places, ensuring proper resource management.- Introduced a custom key generator in the store configuration, allowing for more flexibility in session ID creation.
- Added cookie verification code to ensure that session cookies are properly set.
These changes enhance the overall robustness of the session management system by improving resource handling, allowing for custom session ID generation, and verifying proper cookie setting. The consistency in using
NewStore()andRelease()calls throughout the file is commendable.Also applies to: 718-718, 730-730, 748-748, 757-757, 770-771, 774-775, 782-782, 794-795, 797-799, 806-806, 821-822, 825-825, 838-838, 849-849, 859-859, 865-865, 880-880
902-902: Consistent improvement in resource managementThroughout these changes, there's a clear focus on improving and maintaining consistent resource management:
- Added
sess.Release()calls consistently across various test functions and benchmarks.- Included
app.ReleaseCtx(c)calls where appropriate to release context resources.- Continued the pattern of changing from
New()toNewStore()for store creation.These modifications significantly enhance the overall resource handling in the test suite. By explicitly releasing sessions and contexts, the code reduces the risk of resource leaks and improves overall performance, especially in benchmark scenarios.
The consistency in applying these changes across different parts of the file is commendable and contributes to a more robust and reliable test suite.
Also applies to: 924-924, 935-936, 954-954, 970-971, 984-984, 1003-1003, 1014-1015, 1021-1021, 1034-1035, 1043-1043, 1054-1056, 1064-1064, 1077-1079, 1089-1089, 1102-1102, 1108-1108, 1123-1123, 1131-1131, 1143-1143, 1151-1151, 1165-1165, 1176-1176
1280-1318: Excellent addition of error handling testThe new test function
Test_Session_StoreGetDecodeSessionDataErroris a valuable addition to the test suite. It covers an important edge case in session management:
- It simulates a scenario where invalid session data is stored.
- It tests both
store.Get()andstore.GetByID()methods with this invalid data.- It verifies that appropriate errors are returned and that the error messages are as expected.
This test enhances the robustness of the session management system by ensuring proper handling of corrupted or invalid session data. It's particularly important for maintaining data integrity and security in the session middleware.
The structure of the test is clear and thorough, making it easy to understand the scenario being tested and the expected outcomes.
Line range hint
1-1318: Summary: Significant improvements in session management and testingThe changes in this file represent a substantial enhancement to the session middleware and its testing suite:
- Consistent improvement in resource management through the addition of
Release()calls for both sessions and contexts.- Introduction of new timeout mechanisms, including the change from expiration to idle timeout and the addition of absolute timeout.
- Enhanced flexibility with the use of
[]anyfor session keys and the option for custom key generators.- Improved error handling and edge case coverage, particularly with the new test for decoding invalid session data.
- Better naming conventions and consistency in function calls (e.g.,
NewStore()instead ofNew()).These modifications significantly contribute to the robustness, efficiency, and maintainability of the session middleware. The expanded test coverage ensures that the new features and edge cases are properly handled, reducing the likelihood of bugs in production.
The changes align well with the stated objectives in the PR description, particularly in re-writing the session middleware with improved management and introducing breaking changes for better consistency and adherence to standards.
1190-1190: Improved session handling and resource management
- Changed from
store.Get(localCtx)tostore.getSession(localCtx), potentially using a new internal method for session retrieval.- Added
sess.Release()anddefer sess.Release()calls, further improving resource management.These changes continue to enhance the resource handling in the test suite. The use of
getSessioninstead ofGetmight indicate a more specific or optimized method for session retrieval in this context.To ensure the correctness of the
getSessionmethod, please run the following verification:Also applies to: 1214-1216, 1233-1233
✅ Verification successful
Session retrieval method verified
The
getSessionmethod exists and is utilized correctly within themiddleware/sessionpackage. The changes effectively enhance session handling and resource management as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and usage of the getSession method rg --type go "func.*getSession" ./middleware/session rg --type go "store\.getSession" ./middleware/sessionLength of output: 1112
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
middleware/session/store_test.go (2)
120-140: LGTM with suggestion: UseNewStore()for consistencyThe new
TestStore_Get_SessionAlreadyLoadedfunction is a valuable addition, enhancing test coverage for session management. It correctly checks for theErrSessionAlreadyLoadedByMiddlewareerror when a session is already loaded by middleware.However, for consistency with other tests and to ensure proper initialization:
- store := &Store{} + store := NewStore()This change aligns with the initialization pattern used in other tests and ensures any necessary setup in
NewStore()is not bypassed.
158-217: LGTM with suggestion: Release the acquired contextThe new
Test_Store_GetByIDfunction is an excellent addition to the test suite. It comprehensively covers three crucial scenarios:
- Attempting to get a session with an empty ID.
- Attempting to get a non-existent session.
- Creating, retrieving, and manipulating a valid session.
The test thoroughly exercises various session methods, enhancing the overall test coverage.
However, to prevent potential resource leaks, consider releasing the acquired context:
ctx := fiber.New().AcquireCtx(&fasthttp.RequestCtx{}) + defer fiber.New().ReleaseCtx(ctx) session, err := store.Get(ctx) defer session.Release()This ensures proper cleanup of resources after the test execution.
middleware/session/store.go (2)
Line range hint
121-183: LGTM with a minor suggestion: Improved getSession method with enhanced session managementThe additions for handling session freshness, expiration, and absolute timeout significantly improve session management. The enhanced error handling, particularly for session decoding, increases the method's robustness.
However, there's a minor issue in the error handling for session decoding:
if err != nil { sess.mu.Unlock() sess.Release() - return nil, fmt.Errorf("failed to decode session data: %w", err) + return nil, fmt.Errorf("failed to decode session data: %w", decodeErr) }This change ensures that the correct error (
decodeErr) is being wrapped and returned.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 179-179: middleware/session/store.go#L179
Added line #L179 was not covered by tests
179-179: Suggestion: Improve test coverage for error handling casesThe static analysis tool has identified a few lines that are not covered by tests:
- Line 179: Error handling for session reset failure
- Line 297: Error handling for storage retrieval failure in
GetByID- Lines 323-324: Error handling for session destruction failure in
GetByIDConsider adding test cases to cover these specific error scenarios. This will improve code coverage and ensure that the error handling logic works as expected in edge cases.
Would you like assistance in writing these additional test cases?
Also applies to: 297-297, 323-324
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 179-179: middleware/session/store.go#L179
Added line #L179 was not covered by testsdocs/middleware/session.md (4)
5-123: LGTM! Comprehensive introduction and migration guide.The expanded introduction and detailed migration guide provide excellent context for users, especially those upgrading from v2 to v3. The inclusion of both legacy and recommended approaches is particularly helpful.
Minor formatting improvements needed in the migration guide section.
To enhance readability and adhere to Markdown best practices, consider the following adjustments:
- Add blank lines around headings.
- Remove trailing colons from headings.
For example:
### v3 Example (Recommended Middleware Handler) + -##### Key Features: +##### Key Features + -##### Usage Considerations: +##### Usage Considerations + -##### Example Use Cases: +##### Example Use Cases +These changes will improve the document's structure and consistency.
177-229: LGTM! Comprehensive Signatures section.The Signatures section provides a thorough list of available methods for various components of the session middleware, including new and updated methods.
Consider adding brief descriptions for each method.
To enhance the usefulness of the Signatures section, consider adding brief descriptions for each method. This would help users understand the purpose of each method at a glance. For example:
// Session Methods func (s *Session) Fresh() bool // Returns true if the session is new func (s *Session) ID() string // Returns the session ID func (s *Session) Get(key string) any // Retrieves a value from the session // ... (add descriptions for other methods)This addition would make the documentation more informative and user-friendly.
230-439: LGTM! Comprehensive and informative Examples section.The Examples section provides a wide range of use cases, including the recommended middleware handler approach, custom storage implementation, session usage without middleware handler, and handling of custom types. The inclusion of security notices and middleware order information adds significant value.
Consider improving consistency of code comments across examples.
To enhance readability and maintain consistency, consider adding more inline comments to all example code snippets. This would help users understand the purpose of each code block more easily. For instance, in the "Middleware Handler (Recommended)" example:
// Initialize Fiber app app := fiber.New() // Create session middleware and store sessionMiddleware, sessionStore := session.NewWithStore() // Use session middleware app.Use(sessionMiddleware) // Use CSRF middleware with session store app.Use(csrf.New(csrf.Config{ Store: sessionStore, })) // Define route handler app.Get("/", func(c fiber.Ctx) error { // Get session from context sess := session.FromContext(c) if sess == nil { return c.SendStatus(fiber.StatusInternalServerError) } // ... (rest of the handler code) })Applying this level of commenting consistently across all examples would improve the overall documentation quality.
🧰 Tools
🪛 LanguageTool
[style] ~238-~238: This phrase is redundant. Consider using “outside”.
Context: ...method when you need to manage sessions outside of the standard HTTP workflow, such as in ...(OUTSIDE_OF)
🪛 Markdownlint
232-232: Expected: 1; Actual: 0; Above
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
236-236: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
240-240: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
245-245: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
236-236: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
240-240: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
245-245: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
237-237: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
241-241: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
246-246: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
247-247: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
🪛 GitHub Check: markdownlint
[failure] 232-232: Headings should be surrounded by blank lines
docs/middleware/session.md:232 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Above] [Context: "####GetByIDMethod"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 236-236: Headings should be surrounded by blank lines
docs/middleware/session.md:236 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "##### Key Features:"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 236-236: Trailing punctuation in heading
docs/middleware/session.md:236:19 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md026.md
[failure] 237-237: Lists should be surrounded by blank lines
docs/middleware/session.md:237 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Context Independence: Se..."] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md
[failure] 240-240: Headings should be surrounded by blank lines
docs/middleware/session.md:240 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "##### Usage Considerations:"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 240-240: Trailing punctuation in heading
docs/middleware/session.md:240:27 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md026.md
[failure] 241-241: Lists should be surrounded by blank lines
docs/middleware/session.md:241 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Manual Persistence: Sinc..."] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md
[failure] 245-245: Headings should be surrounded by blank lines
docs/middleware/session.md:245 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "##### Example Use Cases:"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 245-245: Trailing punctuation in heading
docs/middleware/session.md:245:24 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md026.md
[failure] 246-246: Lists should be surrounded by blank lines
docs/middleware/session.md:246 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Scheduled Jobs: Retrieve..."] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md
440-477: LGTM! Well-structured Config section.The Config section provides a comprehensive table of configuration options with descriptions and default values, followed by a code snippet showing the default configuration. This structure makes it easy for users to understand and customize the session middleware.
Consider clarifying some configuration option descriptions.
To enhance user understanding, consider refining the descriptions of some configuration options. For example:
- KeyLookup: Clarify that this is a string in the format "source:name" where source can be "header" or "cookie".
- CookieSameSite: Provide a brief explanation of what each possible value ("Lax", "Strict", "None") means.
- IdleTimeout and AbsoluteTimeout: Clarify the difference between these two timeout mechanisms.
For instance:
| **KeyLookup** | `string` | Key used to store session ID. Format: "source:name" (e.g., "cookie:session_id" or "header:X-Session-ID") | `"cookie:session_id"` | | **CookieSameSite** | `string` | SameSite attribute of the session cookie. Options: "Lax" (default, prevents CSRF in most cases), "Strict" (may break some functionalities), "None" (requires HTTPS) | `"Lax"` | | **IdleTimeout** | `time.Duration` | Maximum duration of inactivity before session expires. Resets on each request. | `30 * time.Minute` | | **AbsoluteTimeout** | `time.Duration` | Maximum total duration of a session, regardless of activity. Set to 0 for no expiration. | `0` (no expiration) |These clarifications would help users make more informed decisions when configuring the middleware.
🧰 Tools
🪛 LanguageTool
[style] ~455-~455: Consider using “inaccessible” to avoid wordiness.
Context: ... | Ensures session cookie is not accessible to JavaScript (HTTP only). ...(NOT_ABLE_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- docs/middleware/session.md (1 hunks)
- middleware/session/store.go (4 hunks)
- middleware/session/store_test.go (6 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/middleware/session.md
[style] ~238-~238: This phrase is redundant. Consider using “outside”.
Context: ...method when you need to manage sessions outside of the standard HTTP workflow, such as in ...(OUTSIDE_OF)
[style] ~455-~455: Consider using “inaccessible” to avoid wordiness.
Context: ... | Ensures session cookie is not accessible to JavaScript (HTTP only). ...(NOT_ABLE_PREMIUM)
🪛 Markdownlint
docs/middleware/session.md
232-232: Expected: 1; Actual: 0; Above
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
236-236: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
240-240: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
245-245: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
236-236: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
240-240: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
245-245: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
237-237: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
241-241: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
246-246: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
247-247: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
🪛 GitHub Check: markdownlint
docs/middleware/session.md
[failure] 232-232: Headings should be surrounded by blank lines
docs/middleware/session.md:232 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Above] [Context: "####GetByIDMethod"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 236-236: Headings should be surrounded by blank lines
docs/middleware/session.md:236 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "##### Key Features:"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 236-236: Trailing punctuation in heading
docs/middleware/session.md:236:19 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md026.md
[failure] 237-237: Lists should be surrounded by blank lines
docs/middleware/session.md:237 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Context Independence: Se..."] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md
[failure] 240-240: Headings should be surrounded by blank lines
docs/middleware/session.md:240 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "##### Usage Considerations:"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 240-240: Trailing punctuation in heading
docs/middleware/session.md:240:27 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md026.md
[failure] 241-241: Lists should be surrounded by blank lines
docs/middleware/session.md:241 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Manual Persistence: Sinc..."] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md
[failure] 245-245: Headings should be surrounded by blank lines
docs/middleware/session.md:245 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "##### Example Use Cases:"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 245-245: Trailing punctuation in heading
docs/middleware/session.md:245:24 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md026.md
[failure] 246-246: Lists should be surrounded by blank lines
docs/middleware/session.md:246 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Scheduled Jobs: Retrieve..."] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md
🪛 GitHub Check: codecov/patch
middleware/session/store.go
[warning] 179-179: middleware/session/store.go#L179
Added line #L179 was not covered by tests
[warning] 297-297: middleware/session/store.go#L297
Added line #L297 was not covered by tests
[warning] 323-324: middleware/session/store.go#L323-L324
Added lines #L323 - L324 were not covered by tests
🔇 Additional comments (11)
middleware/session/store_test.go (5)
23-23: LGTM: Consistent renaming ofNew()toNewStore()The changes in the
Test_Store_getSessionIDfunction correctly implement the renaming ofNew()toNewStore(). This modification is consistent across all test cases within the function, maintaining the intended functionality while updating the API.Also applies to: 36-38, 51-53
76-76: LGTM: Consistent renaming in Test_Store_GetThe change in the
Test_Store_Getfunction correctly implements the renaming ofNew()toNewStore(). This modification maintains consistency with the updated API.
96-96: LGTM: Consistent renaming in Test_Store_DeleteSessionThe change in the
Test_Store_DeleteSessionfunction correctly implements the renaming ofNew()toNewStore(). This modification maintains consistency with the updated API.
142-156: LGTM: Comprehensive test cases for Store.DeleteThe new
TestStore_Deletefunction is a valuable addition to the test suite. It covers two important scenarios:
- Attempting to delete a session with an empty ID, which correctly returns an
ErrEmptySessionIDerror.- Attempting to delete a non-existing session, which correctly doesn't return an error.
These test cases enhance the robustness of the session deletion functionality and provide good coverage for edge cases.
Line range hint
1-217: Overall assessment: Significant improvement in test coverageThe changes to
middleware/session/store_test.gorepresent a substantial enhancement to the test suite for session management. Key improvements include:
- Consistent renaming of
New()toNewStore()across all test cases.- Addition of new test functions that cover important scenarios:
TestStore_Get_SessionAlreadyLoadedTestStore_DeleteTest_Store_GetByIDThese additions significantly improve the test coverage for session creation, retrieval, manipulation, and deletion. They also address edge cases and error conditions, enhancing the overall robustness of the session management system.
The suggested minor improvements (using
NewStore()consistently and releasing acquired contexts) will further refine the test suite. Overall, these changes contribute to a more comprehensive and reliable testing framework for the session middleware.middleware/session/store.go (4)
16-20: LGTM: New error variables enhance error handlingThe introduction of
ErrSessionAlreadyLoadedByMiddlewareandErrSessionIDNotFoundInStore, along with the update toErrEmptySessionID, improves error handling specificity. The error messages are consistent and follow Go conventions.
35-62: LGTM: Improved NewStore function with enhanced documentation and functionalityThe renaming of
NewtoNewStoreimproves clarity. The added documentation, including parameters, return values, and usage examples, enhances developer understanding. The new absolute timeout handling aligns well with the PR objectives for improved session management.
77-104: LGTM: Enhanced Get method with improved error handlingThe addition of a check for sessions already loaded by middleware prevents potential issues with duplicate loading. The updated documentation clearly explains this new behavior, enhancing the method's usability and robustness.
237-255: LGTM: Enhanced Delete method with improved documentation and error handlingThe addition of detailed documentation, including parameters, return values, and usage examples, significantly improves the method's usability. The new error handling for empty session IDs enhances the robustness of the method.
docs/middleware/session.md (2)
125-176: LGTM! Well-structured Types section.The Types section clearly defines the Config, Middleware, Session, and Store structs, providing users with a comprehensive overview of the key components in the session middleware. The inclusion of code snippets for each type enhances understanding.
Line range hint
1-477: Excellent documentation update for the session middleware.This comprehensive update to the session middleware documentation significantly enhances its usefulness for Fiber users. Key strengths include:
- Detailed migration guide from v2 to v3.
- Clear explanations of new concepts like
IdleTimeoutandAbsoluteTimeout.- Comprehensive examples covering various use cases.
- Well-structured Config section with detailed explanations.
- Inclusion of important security notices and best practices.
The minor formatting and consistency issues identified earlier do not detract from the overall quality of the documentation. Once addressed, this will be an exemplary resource for developers using the Fiber framework.
🧰 Tools
🪛 LanguageTool
[style] ~238-~238: This phrase is redundant. Consider using “outside”.
Context: ...method when you need to manage sessions outside of the standard HTTP workflow, such as in ...(OUTSIDE_OF)
[style] ~455-~455: Consider using “inaccessible” to avoid wordiness.
Context: ... | Ensures session cookie is not accessible to JavaScript (HTTP only). ...(NOT_ABLE_PREMIUM)
🪛 Markdownlint
232-232: Expected: 1; Actual: 0; Above
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
236-236: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
240-240: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
245-245: Expected: 1; Actual: 0; Below
Headings should be surrounded by blank lines(MD022, blanks-around-headings)
236-236: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
240-240: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
245-245: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
237-237: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
241-241: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
246-246: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
247-247: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
🪛 GitHub Check: markdownlint
[failure] 232-232: Headings should be surrounded by blank lines
docs/middleware/session.md:232 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Above] [Context: "####GetByIDMethod"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 236-236: Headings should be surrounded by blank lines
docs/middleware/session.md:236 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "##### Key Features:"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 236-236: Trailing punctuation in heading
docs/middleware/session.md:236:19 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md026.md
[failure] 237-237: Lists should be surrounded by blank lines
docs/middleware/session.md:237 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Context Independence: Se..."] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md
[failure] 240-240: Headings should be surrounded by blank lines
docs/middleware/session.md:240 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "##### Usage Considerations:"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 240-240: Trailing punctuation in heading
docs/middleware/session.md:240:27 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md026.md
[failure] 241-241: Lists should be surrounded by blank lines
docs/middleware/session.md:241 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Manual Persistence: Sinc..."] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md
[failure] 245-245: Headings should be surrounded by blank lines
docs/middleware/session.md:245 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "##### Example Use Cases:"] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md022.md
[failure] 245-245: Trailing punctuation in heading
docs/middleware/session.md:245:24 MD026/no-trailing-punctuation Trailing punctuation in heading [Punctuation: ':'] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md026.md
[failure] 246-246: Lists should be surrounded by blank lines
docs/middleware/session.md:246 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Scheduled Jobs: Retrieve..."] https://github.com/DavidAnson/markdownlint/blob/v0.35.0/doc/md032.md
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
docs/middleware/session.md (7)
58-60: Consider clarifying IdleTimeout behavior in the legacy approachThe note about IdleTimeout behavior in the legacy approach is helpful. To further improve clarity, consider expanding this note to explain how the IdleTimeout behaves when using the recommended middleware handler approach. This will help users understand the differences between the two approaches more clearly.
:::note When using the legacy approach, the IdleTimeout will be updated when the session is saved. +When using the recommended middleware handler approach, the IdleTimeout is automatically updated on each request. :::
132-147: Consider adding brief descriptions for Config struct fieldsTo enhance the documentation, consider adding brief descriptions for each field in the Config struct. This will help users understand the purpose and impact of each configuration option more easily.
For example:
type Config struct { // Storage defines where session data is stored Storage fiber.Storage // Next defines a function to skip this middleware when returned true Next func(fiber.Ctx) bool // Add descriptions for other fields... }
180-228: Enhance consistency in method descriptionsTo improve the documentation, consider adding brief descriptions for each method in the Signatures section, similar to the detailed explanation provided for the GetByID method. This will help users understand the purpose and usage of each method more easily.
For example:
// New creates a new session middleware func New(config ...Config) *Middleware // NewWithStore creates a new session middleware and returns both the middleware and the store func NewWithStore(config ...Config) (fiber.Handler, *Store) // Add descriptions for other methods...
418-444: Enhance custom types exampleThe example for registering custom types is helpful, but it could be improved by showing how to actually use the custom type in a session. Consider adding a route that demonstrates setting and getting a custom type in the session.
Here's an example of how you could enhance the custom types example:
app.Get("/set-user", func(c fiber.Ctx) error { sess := session.FromContext(c) if sess == nil { return c.SendStatus(fiber.StatusInternalServerError) } user := User{Name: "John Doe", Age: 30} sess.Set("user", user) return c.SendString("User set in session") }) app.Get("/get-user", func(c fiber.Ctx) error { sess := session.FromContext(c) if sess == nil { return c.SendStatus(fiber.StatusInternalServerError) } user, ok := sess.Get("user").(User) if !ok { return c.SendString("User not found in session") } return c.JSON(user) })
445-482: Align Config table with Default Config codeTo improve consistency and clarity, consider aligning the order of fields in the Config table with the order in the Default Config code. This will make it easier for users to compare the default values with the available options.
Additionally, ensure that all fields in the Default Config are also present in the Config table. For example, the
Storefield is present in the Default Config but not in the Config table.🧰 Tools
🪛 LanguageTool
[style] ~460-~460: Consider using “inaccessible” to avoid wordiness.
Context: ... | Ensures session cookie is not accessible to JavaScript (HTTP only). ...(NOT_ABLE_PREMIUM)
240-240: Consider rephrasing for concisenessThe phrase "outside of" is redundant. Consider using just "outside" for improved conciseness.
-method when you need to manage sessions outside of the standard HTTP workflow, such as in +method when you need to manage sessions outside the standard HTTP workflow, such as in🧰 Tools
🪛 LanguageTool
[style] ~240-~240: This phrase is redundant. Consider using “outside”.
Context: ...method when you need to manage sessions outside of the standard HTTP workflow, such as in ...(OUTSIDE_OF)
460-460: Consider using "inaccessible" for clarityTo improve readability and avoid wordiness, consider using "inaccessible" instead of "not accessible".
-| Ensures session cookie is not accessible to JavaScript (HTTP only). +| Ensures session cookie is inaccessible to JavaScript (HTTP only).🧰 Tools
🪛 LanguageTool
[style] ~460-~460: Consider using “inaccessible” to avoid wordiness.
Context: ... | Ensures session cookie is not accessible to JavaScript (HTTP only). ...(NOT_ABLE_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- docs/middleware/session.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/middleware/session.md
[style] ~240-~240: This phrase is redundant. Consider using “outside”.
Context: ...method when you need to manage sessions outside of the standard HTTP workflow, such as in ...(OUTSIDE_OF)
[style] ~460-~460: Consider using “inaccessible” to avoid wordiness.
Context: ... | Ensures session cookie is not accessible to JavaScript (HTTP only). ...(NOT_ABLE_PREMIUM)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
middleware/session/config.go (3)
12-95: Approve changes to Config struct with a minor suggestionThe changes to the
Configstruct are well-documented and provide improved flexibility for session management. The addition ofNext,ErrorHandler,IdleTimeout, andAbsoluteTimeoutfields enhances the middleware's capabilities.Consider adding a brief example in the documentation for the
Nextfield to illustrate its usage, similar to how you've documented theDefaultErrorHandler. This would help developers understand how to implement conditional middleware skipping.
Line range hint
133-187: Approve changes to configDefault with a suggestionThe updates to
configDefaultimprove configuration validation and error handling. The addition of checks forIdleTimeoutandAbsoluteTimeoutensures more robust session management.Consider wrapping the panic calls with a custom function to allow for easier testing and potential future enhancements in error handling. For example:
func configError(message string) { panic("[session] " + message) } // Usage if len(selectors) != numSelectors { configError("KeyLookup must be in the format '<source>:<name>'") }This approach maintains the current behavior while providing flexibility for future modifications.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-129: middleware/session/config.go#L129
Added line #L129 was not covered by tests
[warning] 160-160: middleware/session/config.go#L160
Added line #L160 was not covered by tests
129-129: Improve test coverage for error handling pathsThe static analysis tool indicates that lines 129 and 160 are not covered by tests. These lines are part of error handling paths in the
DefaultErrorHandlerandconfigDefaultfunctions respectively.Consider adding test cases to cover these error scenarios:
- Test the
DefaultErrorHandlerwhenSendStatusfails.- Test
configDefaultwith anAbsoluteTimeoutless thanIdleTimeout.Improving test coverage for these edge cases will enhance the overall robustness of the session middleware.
Also applies to: 160-160
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-129: middleware/session/config.go#L129
Added line #L129 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- middleware/session/config.go (4 hunks)
🧰 Additional context used
📓 Learnings (1)
middleware/session/config.go (1)
Learnt from: sixcolors PR: gofiber/fiber#3016 File: middleware/session/config.go:16-26 Timestamp: 2024-09-25T16:17:00.969Z Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
🪛 GitHub Check: codecov/patch
middleware/session/config.go
[warning] 129-129: middleware/session/config.go#L129
Added line #L129 was not covered by tests
[warning] 160-160: middleware/session/config.go#L160
Added line #L160 was not covered by tests
🔇 Additional comments (1)
middleware/session/config.go (1)
108-115: Approve changes to ConfigDefaultThe updates to
ConfigDefaultcorrectly reflect the changes made to theConfigstruct. The replacement ofExpirationwithIdleTimeoutand its default value of 30 minutes is appropriate for general use cases.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
middleware/session/config.go (2)
117-131: LGTM with suggestion: DefaultErrorHandler implementationThe
DefaultErrorHandlerfunction provides a good basic error handling mechanism. However, consider enhancing the error logging to include more context, such as the request ID or path.Example improvement:
func DefaultErrorHandler(c fiber.Ctx, err error) { log.Errorf("session error on %s: %v", c.Path(), err) if sendErr := c.SendStatus(fiber.StatusInternalServerError); sendErr != nil { log.Errorf("session: failed to send error status: %v", sendErr) } }This change would provide more detailed logging, making it easier to trace and debug issues.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-129: middleware/session/config.go#L129
Added line #L129 was not covered by tests
129-129: Consider improving test coverageStatic analysis indicates that lines 129 and 160 are not covered by tests. These lines are part of error handling paths:
- Line 129: Error logging in
DefaultErrorHandler- Line 160: Panic in
configDefaultfor invalidAbsoluteTimeoutWhile these scenarios might be challenging to test, consider adding unit tests to cover these edge cases. This would improve the overall robustness of the package.
Also applies to: 160-160
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-129: middleware/session/config.go#L129
Added line #L129 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- middleware/session/config.go (4 hunks)
🧰 Additional context used
📓 Learnings (1)
middleware/session/config.go (1)
Learnt from: sixcolors PR: gofiber/fiber#3016 File: middleware/session/config.go:16-26 Timestamp: 2024-09-25T16:17:00.969Z Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
🪛 GitHub Check: codecov/patch
middleware/session/config.go
[warning] 129-129: middleware/session/config.go#L129
Added line #L129 was not covered by tests
[warning] 160-160: middleware/session/config.go#L160
Added line #L160 was not covered by tests
🔇 Additional comments (8)
middleware/session/config.go (8)
8-8: LGTM: Import statement additionThe addition of the
logpackage fromgitpro.ttaallkk.top/gofiber/fiber/v3/logis appropriate for the new error handling functionality introduced in this file.
19-21: LGTM: Addition of Next functionThe
Nextfunction allows for conditional skipping of the middleware, providing more flexibility in request handling.
28-31: LGTM: Addition of ErrorHandlerThe
ErrorHandlerfunction allows for custom error handling, improving the middleware's flexibility and error management capabilities.
66-71: LGTM: Replacement of Expiration with IdleTimeoutThe
IdleTimeoutfield replaces the previousExpirationfield, providing a clearer semantic meaning for session inactivity expiration. The default value and documentation are appropriate.
73-78: LGTM: Addition of AbsoluteTimeoutThe
AbsoluteTimeoutfield adds the ability to set a maximum duration for a session, independent of activity. This enhances session management capabilities.
155-157: LGTM: IdleTimeout handlingThe check for
IdleTimeoutensures that a valid duration is always set, defaulting to theConfigDefaultvalue if necessary.
158-161: LGTM: AbsoluteTimeout validationThe check ensuring that
AbsoluteTimeoutis greater than or equal toIdleTimeoutis crucial for maintaining logical session timeout behavior.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 160-160: middleware/session/config.go#L160
Added line #L160 was not covered by tests
Line range hint
169-184: LGTM: Improved KeyLookup parsingThe parsing of
KeyLookuphas been enhanced with better error messages and handling of different sources. This improves the robustness of the configuration process.
Feature: Re-write Session Middleware with Handler
Summary
This pull request introduces significant changes to the session middleware, enhancing its functionality and improving its robustness. The primary focus is on the re-write of the session middleware with a new handler.
Status
session.New()returns middleware handler)Changelog
Middleware Changes
middleware/session/middleware.goto define the session middleware and its configuration.CSRF Session Manager Changes
getRaw,setRaw, anddelRawmethods to use the session from the context first before falling back to the store.log) from the session manager.Configuration Changes
ExpirationtoIdleTimeoutto better reflect the session idle duration.Session Struct Changes
expis nowidleTimeoutto indicate the idle timeout duration for sessions.idleTimeoutinstead ofexp.Savemethod to handle idle timeout correctly.Changes From This PR
This is an new middleware approach. Changes have been made to:
session.Newto return theMiddlewarehandler instead of the oldsession.Storestruct.A suggested standard use for the session might look something like this after these modifications:
Testing
Notes
Please review the changes and provide feedback or approval for merging.
Fixes #2741