Add session helper functions for value manipulation - #45
Conversation
Add convenient helper functions to simplify session value operations: - Get(r, key) - retrieve a value from the session - Set(r, w, key, value) - store a value in the session - Delete(r, w, key) - remove a value from the session - Clear(r, w) - remove all values from the session - Save(r, w) - explicitly persist the session All functions return ErrSessionNotFound if the session is not in context, providing safer error handling compared to FromCtx which panics.
There was a problem hiding this comment.
Pull request overview
This PR adds convenient helper functions to the server/session package to simplify session value manipulation. The new functions provide a cleaner API with explicit error handling instead of requiring users to call session.FromCtx and directly manipulate the Values map.
Changes:
- Added five new helper functions:
Get,Set,Delete,Clear, andSavefor session value operations - Introduced
ErrSessionNotFounderror for safer error handling when session is not in context - Comprehensive test suite with 100% coverage for the new functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| server/session/values.go | Implements the new session helper functions with error handling and context validation |
| server/session/values_test.go | Comprehensive test suite covering all new functions including error cases and persistence scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) | ||
|
|
||
| s.HandleFunc("GET /set/{$}", func(w http.ResponseWriter, r *http.Request) { | ||
| session.Set(r, w, "to_delete", "value") |
There was a problem hiding this comment.
The return value from session.Set is not checked for errors. While this call is unlikely to fail in this test context, it's a best practice to check all error returns in tests for consistency and to catch unexpected issues.
| }) | ||
|
|
||
| s.HandleFunc("GET /delete/{$}", func(w http.ResponseWriter, r *http.Request) { | ||
| session.Delete(r, w, "to_delete") |
There was a problem hiding this comment.
The return value from session.Delete is not checked for errors. While this call is unlikely to fail in this test context, it's a best practice to check all error returns in tests for consistency and to catch unexpected issues.
| session.Delete(r, w, "to_delete") | |
| if err := session.Delete(r, w, "to_delete"); err != nil { | |
| t.Errorf("Delete failed: %v", err) | |
| } |
| _, exists1, _ := session.Get(r, "key1") | ||
| _, exists2, _ := session.Get(r, "key2") |
There was a problem hiding this comment.
The error returns from session.Get calls are ignored using the blank identifier. While this may be acceptable if the test is only checking value existence, it's better practice to check the errors to catch unexpected issues. Consider adding error checking for consistency with other tests.
| _, exists1, _ := session.Get(r, "key1") | |
| _, exists2, _ := session.Get(r, "key2") | |
| _, exists1, err := session.Get(r, "key1") | |
| if err != nil { | |
| t.Errorf("Get failed for key1: %v", err) | |
| } | |
| _, exists2, err := session.Get(r, "key2") | |
| if err != nil { | |
| t.Errorf("Get failed for key2: %v", err) | |
| } |
| // Delete removes a value from the session by key. | ||
| // Returns an error if the session is not in the request context. |
There was a problem hiding this comment.
The documentation should clarify that sessions are automatically saved when the response is written via the saver middleware, and that this function only modifies the session in memory. Add a note similar to the one in the Save function documentation to explain when/how the session is persisted.
| session.Set(r, w, "key1", "value1") | ||
| session.Set(r, w, "key2", "value2") | ||
| session.Set(r, w, "key3", "value3") |
There was a problem hiding this comment.
The return values from session.Set calls are not checked for errors. While these calls are unlikely to fail in this test context, it's a best practice to check all error returns in tests for consistency and to catch unexpected issues. Consider adding error checks similar to those used elsewhere in the test file.
| session.Set(r, w, "key1", "value1") | |
| session.Set(r, w, "key2", "value2") | |
| session.Set(r, w, "key3", "value3") | |
| if err := session.Set(r, w, "key1", "value1"); err != nil { | |
| t.Fatalf("Set failed: %v", err) | |
| } | |
| if err := session.Set(r, w, "key2", "value2"); err != nil { | |
| t.Fatalf("Set failed: %v", err) | |
| } | |
| if err := session.Set(r, w, "key3", "value3"); err != nil { | |
| t.Fatalf("Set failed: %v", err) | |
| } |
|
|
||
| // Delete removes a value from the session by key. | ||
| // Returns an error if the session is not in the request context. | ||
| func Delete(r *http.Request, w http.ResponseWriter, key string) error { |
There was a problem hiding this comment.
The w http.ResponseWriter parameter is accepted but never used in this function. While it may be included for API consistency, this could confuse users. Consider either removing this parameter or documenting clearly in the function comment that the session will be automatically saved when the response is written, and this parameter is reserved for future use or API consistency.
| // Set stores a value in the session. | ||
| // Returns an error if the session is not in the request context. |
There was a problem hiding this comment.
The documentation should clarify that sessions are automatically saved when the response is written via the saver middleware, and that this function only modifies the session in memory. Users might expect that calling Set immediately persists the session, but it doesn't - it only modifies the session object which is later saved automatically.
| // Clear removes all values from the session. | ||
| // Returns an error if the session is not in the request context. |
There was a problem hiding this comment.
The documentation should clarify that sessions are automatically saved when the response is written via the saver middleware, and that this function only modifies the session in memory. Add a note similar to the one in the Save function documentation to explain when/how the session is persisted.
| session.Set(r, w, "key", "value") | ||
|
|
||
| err := session.Save(r, w) | ||
| if err != nil { |
There was a problem hiding this comment.
The return value from session.Set is not checked for errors. While this call is unlikely to fail in this test context, it's a best practice to check all error returns in tests for consistency and to catch unexpected issues.
| session.Set(r, w, "key", "value") | |
| err := session.Save(r, w) | |
| if err != nil { | |
| if err := session.Set(r, w, "key", "value"); err != nil { | |
| t.Errorf("Set failed: %v", err) | |
| } | |
| if err := session.Save(r, w); err != nil { |
| }) | ||
|
|
||
| s.HandleFunc("GET /set/{$}", func(w http.ResponseWriter, r *http.Request) { | ||
| session.Set(r, w, "persistent", "data") |
There was a problem hiding this comment.
The return value from session.Set is not checked for errors. While this call is unlikely to fail in this test context, it's a best practice to check all error returns in tests for consistency and to catch unexpected issues.
| session.Set(r, w, "persistent", "data") | |
| if err := session.Set(r, w, "persistent", "data"); err != nil { | |
| t.Fatalf("Set failed: %v", err) | |
| } |
Summary
This PR adds convenient helper functions to simplify session value operations in the
server/sessionpackage:session.Get(r, key)- retrieve a value from the sessionsession.Set(r, w, key, value)- store a value in the sessionsession.Delete(r, w, key)- remove a value from the sessionsession.Clear(r, w)- remove all values from the sessionsession.Save(r, w)- explicitly persist the sessionMotivation
Previously, users had to:
session.FromCtx(r.Context())to get the sessionValuesmapThe new helpers provide a cleaner API and safer error handling - they return
ErrSessionNotFoundinstead of panicking when the session is not in context.Usage
Testing
values_test.gogo vetandgofmt