feat(migrate): add session Release() migration with scope-aware tracking #251
Conversation
Implements migration to add defer sess.Release() calls for v3 session Store Pattern usage. In v3, sessions obtained via store.Get() and store.GetByID() must be manually released back to the pool. Key Features: - Parses imports to find v3 session package (skips v2) - Tracks session.NewStore() variables specifically - Scope-aware: verifies store variable is from session.NewStore() in current function scope to prevent false positives - Handles closures accessing parent scope variables - Adds defer Release() after error checks or immediately if no check - Prevents duplicates by checking for existing Release() calls - Safe with nil (Release() has nil guard) Edge Cases Handled: ✅ No error checking (sess, _ := store.Get(c)) ✅ Already has defer (no duplicates) ✅ Multiline error blocks ✅ Middleware pattern (correctly excluded - middleware manages lifecycle) ✅ False positives (cache.Get, Ent ORM, CSRF - correctly excluded) ✅ Various store variable names (store, sessionStore, myStore) ✅ V2 imports (correctly skipped - migration runs after v2→v3 upgrade) ✅ Cross-function variable name collision (store in session vs cache) ✅ Closures/anonymous functions accessing parent scope ✅ Real-world examples from gofiber/recipes verified Test Coverage: - 13 comprehensive tests covering all edge cases - Includes real-world patterns from csrf-with-session and ent-mysql - 0 linting issues Fixes gofiber/recipes#3841
sixcolors
requested review from
ReneWerner87,
efectn and
gaby
and removed request for
a team
Summary of ChangesHello @sixcolors, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a critical migration tool for Fiber v3, automating the insertion of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a very well-structured pull request that significantly improves the session release migration logic. The breakdown into multiple steps (finding alias, finding store variables, and then adding release calls with scope-awareness) is excellent and much more robust than the previous implementation. The detailed PR description and the comprehensive test suite covering numerous edge cases are also commendable.
I found one critical issue in the scope verification logic that would cause the migration to fail for projects using a custom alias for the session package. I've provided a detailed comment with a suggested fix and a recommendation to add a test case for this scenario.
Once that is addressed, this will be a fantastic improvement.
There was a problem hiding this comment.
Pull request overview
This PR implements a migration to automatically add defer sess.Release() calls for Fiber v3 session Store Pattern usage, preventing memory leaks by ensuring sessions are returned to the pool. The migration uses scope-aware tracking to avoid false positives when the same variable names are reused across different functions or for different types.
Key Changes:
- Adds import detection to identify v3 session package and its alias
- Implements variable tracking to find
session.NewStore()assignments - Adds scope verification via backward search to prevent false positives across function boundaries
- Handles closures accessing parent scope variables
- Smart placement of
defer Release()after error checks or immediately if no error handling exists
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| cmd/internal/migrations/v3/session_release.go | Implements the core migration logic with import detection, store variable tracking, scope-aware verification, and Release() call insertion |
| cmd/internal/migrations/v3/session_release_test.go | Adds 9 comprehensive test cases covering middleware patterns, false positives, variable naming, v2 imports, real-world examples, cross-function collisions, and closure support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces regex/line-based injection with an AST- and type-checked pass that finds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/internal/migrations/v3/session_release_test.go (1)
178-181: Test assertion logic is incorrect.
strings.Index(result, "}")finds the first}in the file, which is the closing brace of the import block, not the error handling block. The test may pass coincidentally but doesn't verify the intended behavior.- // Verify defer comes after the error block - deferIdx := strings.Index(result, "defer sess.Release()") - errorBlockEnd := strings.Index(result, "}") - assert.Greater(t, deferIdx, errorBlockEnd, "defer should come after error block") + // Verify defer comes after the error block (find the "return err" followed by "}") + errorBlockPattern := "return err\n }\n" + errorBlockEnd := strings.Index(result, errorBlockPattern) + len(errorBlockPattern) + deferIdx := strings.Index(result, "defer sess.Release()") + assert.Greater(t, deferIdx, errorBlockEnd, "defer should come after error block")
🧹 Nitpick comments (2)
cmd/internal/migrations/v3/session_release.go (1)
191-211: Consider extracting magic number and improving scope detection.The 30-line look-ahead is reasonable but could be a named constant for clarity. The scope check at line 204 using
len(indent) == 0may not reliably detect function boundaries since function bodies typically have indentation.+const releaseSearchAhead = 30 // Lines to search ahead for existing Release() calls + // Check if Release() is already present for this session variable // Search from right after the Get() line hasRelease := false -searchEnd := i + 30 // Look ahead up to 30 lines +searchEnd := i + releaseSearchAheadcmd/internal/migrations/v3/session_release_test.go (1)
676-751: Critical edge case test - but missing aliased import coverage.This test validates the crucial scope-aware tracking to prevent false positives across functions. However, the test suite is missing coverage for aliased imports (e.g.,
sshadow "github.com/gofiber/fiber/v3/middleware/session"), which would expose the bug inisSessionStoreInScopewheresession.NewStore()is hardcoded.Consider adding a test case for aliased imports:
func Test_MigrateSessionRelease_AliasedImport(t *testing.T) { t.Parallel() dir, err := os.MkdirTemp("", "msessionrelease") require.NoError(t, err) defer func() { require.NoError(t, os.RemoveAll(dir)) }() content := `package main import ( "github.com/gofiber/fiber/v3" sess "github.com/gofiber/fiber/v3/middleware/session" ) func handler(c fiber.Ctx) error { store := sess.NewStore() session, err := store.Get(c) if err != nil { return err } session.Set("key", "value") return session.Save() } ` err = os.WriteFile(filepath.Join(dir, "main.go"), []byte(content), 0o600) require.NoError(t, err) cmd := &cobra.Command{} err = MigrateSessionRelease(cmd, dir, nil, nil) require.NoError(t, err) data, err := os.ReadFile(filepath.Join(dir, "main.go")) require.NoError(t, err) result := string(data) assert.Contains(t, result, "defer session.Release() // Important: Manual cleanup required") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/session_release.go(2 hunks)cmd/internal/migrations/v3/session_release_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/internal/migrations/v3/session_release.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
cmd/internal/migrations/v3/session_release_test.go (1)
cmd/internal/migrations/v3/session_release.go (1)
MigrateSessionRelease(25-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Agent
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, windows-latest)
🔇 Additional comments (10)
cmd/internal/migrations/v3/session_release.go (5)
16-55: LGTM! Clean three-step migration logic.The function is well-structured with clear separation of concerns: finding the session package alias, identifying store variables, and adding Release() calls. The early returns for files without session imports or stores are efficient.
57-77: LGTM! Correct import alias detection.The regex correctly handles both default imports and aliased imports for the v3 session package. Returning empty string for non-v3 imports ensures v2 code is properly skipped.
79-108: LGTM! Properly uses the session package alias.The function correctly incorporates
sessionPkgAliasinto the regex patterns, handling bothNewStore()calls and type declarations with the appropriate alias.
226-231: Silent skip when error block parsing fails may leave sessions unreleased.If
findErrorBlockEndreturns an invalid index, the code continues without addingdefer sess.Release(). This could result in memory leaks for edge cases with unusual error block formatting.Consider adding a fallback to insert the defer immediately after the Get() call when block parsing fails, or at minimum log a warning:
if blockEnd < 0 || blockEnd >= len(lines) { - continue + // Fallback: add defer immediately after Get() if we can't parse the error block + deferLine := indent + "defer " + sessVar + ".Release() " + releaseComment + result = append(result, deferLine) + continue }
256-287: LGTM! Reasonable heuristic with documented limitations.The brace-counting approach handles typical Go error patterns. The limitation regarding braces in strings/comments is documented appropriately.
cmd/internal/migrations/v3/session_release_test.go (5)
184-230: Good coverage for middleware pattern exclusion.This test correctly verifies that
session.FromContext()usage (middleware pattern) doesn't trigger Release() injection, as the middleware manages the lifecycle.
232-287: LGTM! Good false-positive prevention test.Covers multiple non-session Get patterns (CSRF locals, Ent GetX, cache.Get) to ensure they don't trigger Release() injection.
289-389: LGTM! Good coverage for variable naming patterns and v2 skip behavior.The variable name test validates that different naming conventions (store, sessionStore, myStore) all receive Release() calls. The v2 import test confirms the migration correctly skips files that haven't been upgraded to v3 imports yet.
391-555: Excellent real-world validation tests.Testing against actual patterns from gofiber/recipes (CSRF-with-session, Ent-MySQL) provides confidence that the migration handles real codebases correctly.
557-674: Good edge case coverage for error handling variations and similar APIs.The nil-safe Release() documentation in the test comments helps explain why it's safe to add Release() even without error checking.
…sed parsing Addresses feedback from PR #251 review comments (Gemini Code Assist, GitHub Copilot, CodeRabbit): 1. CRITICAL FIX: Custom import alias support - Pass sessionPkgAlias parameter through addReleaseCalls() -> isSessionStoreInScope() - Fixes hardcoded 'session' pattern that broke custom aliases like 'sess', 'ssession' - Add Test_MigrateSessionRelease_AliasedImport to verify 2. FIX: Scope traversal through closures - Remove braceDepth exit check in isSessionStoreInScope() - Allow searching backwards through closures into parent scope - Stop only at named function boundaries (not anonymous closures) 3. ENHANCEMENT: AST-based brace matching - Replace naive brace counting with go/parser AST analysis - Handles strings with braces: "{}", "return { key: value }" - Handles comments with braces - Use ast.Inspect() to find precise IfStmt.Body.End() position - Fallback to simple counting only if AST parsing fails 4. CODE QUALITY: - Use strings.Builder for efficient string concatenation - Add proper switch default case - Add nolint explanation comments - Simplify test assertions for maintainability All 289 tests passing, 0 linting issues.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/session_release.go (1)
230-261: Consider handling more error check pattern variations.Line 233's check
strings.HasPrefix(nextLine, "if "+errVar+" != nil")is rigid and won't match common variations:
if err!=nil(no spaces)if nil != err(reversed comparison)if errors.Is(err, ...)(wrapped checks)Since there's a fallback (lines 258-260) that inserts
deferimmediately, the migration will still work but may placedeferbefore the error check in these cases.Consider using a more flexible regex pattern:
- if strings.HasPrefix(nextLine, "if "+errVar+" != nil") { + errCheckPattern := regexp.MustCompile(fmt.Sprintf(`^if\s+(%s\s*!=\s*nil|nil\s*!=\s*%s|errors\.`, regexp.QuoteMeta(errVar), regexp.QuoteMeta(errVar))) + if errCheckPattern.MatchString(nextLine) {However, this adds complexity, and the current implementation is acceptable for a migration tool.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/session_release.go(4 hunks)cmd/internal/migrations/v3/session_release_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/session_release.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, macos-latest)
🔇 Additional comments (5)
cmd/internal/migrations/v3/session_release.go (2)
28-58: LGTM! Clean multi-step pipeline with early returns.The refactored implementation properly separates concerns into discovery (alias, store variables) and transformation (adding Release calls). Early returns when no session package or stores are found optimize performance.
113-160: Excellent scope verification - addresses all previous critical feedback.The implementation now:
- Accepts
sessionPkgAliasparameter and uses it for verification (lines 122-125, 137)- Pre-compiles regex patterns outside the loop for efficiency
- Captures and verifies the package alias matches the session package (lines 134-142)
- Stops at named function boundaries while allowing closures (line 153)
This properly prevents false positives when the same variable name is reused in different functions or with different packages.
cmd/internal/migrations/v3/session_release_test.go (3)
178-184: LGTM! Proper verification of defer placement after error blocks.These assertions validate that the AST-based error block detection correctly places
defer sess.Release()after the error handling block, not before or inside it.
678-753: Excellent critical test for scope verification!This test validates the most important aspect of the migration: ensuring
defer Release()is only added for actual session stores, not for other packages that happen to use the same variable name. The scope verification inisSessionStoreInScopemust correctly distinguish between:
store := session.NewStore()insessionHandler(SHOULD add Release)store := cache.NewStore()incacheHandler(should NOT add Release)The assertions properly verify that only the session store gets
defer sess.Release(), confirming the scope-aware tracking works correctly.
755-811: LGTM! Validates custom import alias support.This test confirms the migration correctly handles custom session package aliases (e.g.,
sess "github.com/gofiber/fiber/v3/middleware/session"). The assertions verify that bothGet()andGetByID()receivedefer Release()calls when the session package is imported with a custom alias.This validates the fix for past review comments about hardcoded "session" strings.
- Add MigrateSessionRelease migration for v2->v3 upgrades - Automatically adds defer sess.Release() after store.Get() calls - Uses AST-based analysis for function-scope awareness - Handles error checking patterns and proper defer placement - Includes comprehensive test coverage for real-world scenarios - Cleaned up debugging comments and redundant code
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/session_release.go (1)
350-368: Clarify the line number calculation and comment.The line number calculation at line 354 (
lineNum := pos.Line - 3) is confusing, and the inline comment doesn't explain why 3 is subtracted. Given that only 2 wrapper lines are added (lines 334:"package main\nfunc f() {\n"), the subtraction of 3 seems counterintuitive.Consider adding a clearer comment explaining the calculation:
- lineNum := pos.Line - 3 + // AST line is 1-based. Subtract 3 to account for: + // - 1 for 1-based indexing + // - 2 for the wrapper lines (package main + func f()) + lineNum := pos.Line - 3Or if the math is incorrect, adjust accordingly and verify with a test case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/session_release.go(2 hunks)cmd/internal/migrations/v3/session_release_test.go(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/internal/migrations/v3/session_release.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
cmd/internal/migrations/v3/session_release_test.go (1)
cmd/internal/migrations/v3/session_release.go (1)
MigrateSessionRelease(31-61)
🪛 GitHub Actions: golangci-lint
cmd/internal/migrations/v3/session_release.go
[error] 334-334: golangci-lint: Error return value of sb.WriteString is not checked (errcheck)
🪛 GitHub Check: lint
cmd/internal/migrations/v3/session_release.go
[failure] 377-377:
enforce-switch-style: switch must have a default case clause (revive)
[failure] 341-341:
Error return value of sb.WriteString is not checked (errcheck)
[failure] 339-339:
Error return value of sb.WriteString is not checked (errcheck)
[failure] 338-338:
Error return value of sb.WriteString is not checked (errcheck)
[failure] 334-334:
Error return value of sb.WriteString is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, ubuntu-latest)
🔇 Additional comments (11)
cmd/internal/migrations/v3/session_release.go (3)
31-61: LGTM! Good defensive error handling.The implementation correctly:
- Early-exits for files without session imports
- Skips v2 imports (this migration runs after v2→v3 import migration)
- Falls back gracefully to returning original content if type checking fails (line 47)
- Provides user feedback when changes are made (line 59)
87-150: LGTM! Well-structured AST analysis.The
findReleasePointsfunction correctly:
- Identifies assignment patterns matching
sess, err := store.Get(c)- Supports both
GetandGetByIDmethods- Validates the store variable is actually a session.Store via
isSessionStoreInFunction- Handles ignored errors with underscore (lines 130-135)
- Captures all necessary information for insertion
245-292: LGTM! Correct insertion logic.The
insertDeferStatementsfunction correctly:
- Processes points in reverse order to maintain accurate line numbers (line 256)
- Calculates proper indentation (lines 249-254)
- Skips duplicate insertions via
hasExistingReleasecheck (line 263)- Places defer after error-handling blocks when present (lines 277-283)
- Falls back to immediate placement when no error check exists (lines 284-288)
cmd/internal/migrations/v3/session_release_test.go (8)
14-28: LGTM! Good test scaffolding improvement.The
setupTestModulehelper correctly:
- Creates temp directory within the project (line 20) to inherit go.mod
- Returns absolute path for consistent handling
- Is properly cleaned up in each test with defer
This is essential for the type-checking approach used in the migration.
198-204: LGTM! Correct defer placement verification.The test correctly verifies that:
- The error return statement exists (line 201)
- The defer Release() statement exists (line 202)
- The defer is placed after the error block (line 203)
This ensures the migration respects error-handling flow.
206-253: LGTM! Critical test for middleware pattern exclusion.This test correctly verifies that sessions obtained via
session.FromContext(c)(line 231) do NOT receiveRelease()calls, as the middleware manages their lifecycle. This is a key requirement per the PR objectives.
706-782: Excellent! Critical edge case test for function-scoped type verification.This test validates the most important aspect of the migration: ensuring that the same variable name "store" in different functions is correctly distinguished by type. The test verifies:
- Session store in
sessionHandlergetsRelease()(lines 727-728)- Cache store in
cacheHandlerdoes NOT getRelease()(lines 739-740)- Only one defer is added total (line 762)
- Placement verification confirms it's in the correct function (lines 766-781)
This directly validates the type-checking approach and prevents false positives.
784-841: LGTM! Validates custom package alias handling.This test correctly verifies that custom session package aliases (line 799:
sess "github.com/gofiber/fiber/v3/middleware/session") are properly detected and handled. Bothstore.Get()andstore.GetByID()receiveRelease()calls when using the aliased import (lines 838-840).
417-489: LGTM! Validates real-world usage from gofiber/recipes.This test uses a simplified version of the CSRF-with-session pattern from gofiber/recipes, ensuring the migration works on actual production patterns. The assertion correctly verifies that both
store.Get()calls (lines 443, 458) each receive adefer Release()statement.
491-583: LGTM! Validates no false positives for ORM Get methods.This test ensures that Ent ORM's
GetandGetXmethods (lines 553, 558) do NOT triggerRelease()insertion since there's no session import. The assertion correctly verifies the file remains completely unchanged (line 582).
30-841: Excellent test coverage! Comprehensive validation of migration behavior.The test suite thoroughly covers:
- ✅ Basic scenarios (Get, GetByID, existing defer, multiline error checks)
- ✅ Critical edge cases (same variable name in different functions, function-scoped type checking)
- ✅ Negative cases (middleware pattern, non-session Get methods, v2 imports, ORM methods)
- ✅ Real-world patterns (CSRF with session, Ent MySQL)
- ✅ Import variants (aliased imports, multiple store variables)
- ✅ Error handling (ignored errors, no error checks)
The tests directly validate the type-checking approach and ensure no false positives. Well done!
- Fix linting issues: add nolint comments for strings.Builder, add switch default case - Fix test assertion logic for multiline error block placement verification - Update PR description to clarify AST+regex type checking approach
- Change Test_MigrateSessionRelease_CSRFWithSession to use 'sess' variable name instead of 'session' for consistency with other tests - Update assertions to check for 'defer sess.Release()' accordingly
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/internal/migrations/v3/session_release.go (2)
189-221: Function boundary detection may miss edge cases.The
findFunctionBoundariesfunction uses simple string prefix matching for"func ", which could incorrectly match:
- Function declarations in comments
- The word "func" in string literals
- Method receivers:
func (r *Receiver) Method()While this works for typical code, consider using AST-based function boundary detection for robustness:
func findFunctionBoundaries(src string, lineNum int) (start, end int) { fset := token.NewFileSet() file, err := parser.ParseFile(fset, "", src, 0) if err != nil { return -1, -1 } var fnStart, fnEnd int = -1, -1 ast.Inspect(file, func(n ast.Node) bool { if fn, ok := n.(*ast.FuncDecl); ok { start := fset.Position(fn.Pos()).Line end := fset.Position(fn.End()).Line if start <= lineNum && lineNum <= end { fnStart, fnEnd = start-1, end return false } } return true }) return fnStart, fnEnd }
223-243: Import alias detection could be more robust.The
findSessionPackageAliasesfunction uses simple string matching which could fail if:
- Import is within a block:
import ( ... )- Multiple spaces/tabs between alias and path
- Comments appear on the same line
Consider using AST-based import parsing:
func findSessionPackageAliases(src string) []string { fset := token.NewFileSet() file, err := parser.ParseFile(fset, "", src, parser.ImportsOnly) if err != nil { return []string{"session"} // fallback } var aliases []string for _, imp := range file.Imports { if imp.Path.Value == `"github.com/gofiber/fiber/v3/middleware/session"` { if imp.Name != nil { aliases = append(aliases, imp.Name.Name) } else { aliases = append(aliases, "session") } } } return aliases }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/session_release.go(2 hunks)cmd/internal/migrations/v3/session_release_test.go(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/internal/migrations/v3/session_release.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
cmd/internal/migrations/v3/session_release_test.go (1)
cmd/internal/migrations/v3/session_release.go (1)
MigrateSessionRelease(31-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, macos-13)
🔇 Additional comments (6)
cmd/internal/migrations/v3/session_release.go (4)
31-61: LGTM with minor observation on error handling.The migration function structure is sound. The silent fallback on parse errors (line 47) means users won't see migration failures, which could hide issues but also provides robustness. This appears intentional since the function returns the original content unchanged.
71-150: Well-structured AST traversal for finding Store.Get() patterns.The logic correctly identifies assignment statements with the pattern
sess, err := store.Get(c)orsess, err := store.GetByID(ctx, id)by checking:
- Two left-hand side variables
- One right-hand side expression (call)
- Define token (
:=)- Method name is "Get" or "GetByID"
The delegation to
isSessionStoreInFunctionfor validation is the right approach.
245-314: Defer insertion logic is well-designed.The implementation correctly:
- Iterates in reverse to avoid line number shifts during insertion
- Preserves indentation from the original Get() line
- Checks for existing Release() calls to avoid duplicates
- Places defer after error handling blocks when present
- Falls back to immediate placement when no error check exists
The deduplication logic in
hasExistingReleaseusing a small search window (±2 to +5 lines) is a pragmatic approach.
316-391: AST-based error block detection with sensible fallback.The implementation correctly:
- Wraps code snippet for parsing (adds 2 lines: package + func declaration)
- Uses AST to find if statement end position
- Adjusts line numbers accounting for wrapper lines (line 354:
-3accounts for 2 wrapper lines + 1-based numbering)- Falls back to brace counting when AST parsing fails
- Handles linter issues with nolint directives (lines 334, 338, 339, 341)
- Includes default case in switch statement (line 385) to satisfy linter
The brace-counting fallback is simple but adequate for typical error handling blocks.
cmd/internal/migrations/v3/session_release_test.go (2)
14-28: Well-designed test helper for module isolation.The
setupTestModulehelper creates temporary directories within the project (using"."as base) so tests inherit the parentgo.mod, enabling proper type resolution. The use oft.Helper()ensures accurate error reporting.
30-840: Comprehensive test coverage for migration scenarios.The test suite covers all critical scenarios:
- ✅ Basic patterns (Get, GetByID)
- ✅ Deduplication of existing Release() calls
- ✅ Multiline error handling blocks
- ✅ Middleware patterns that should NOT trigger insertion
- ✅ False positives from non-session Get methods
- ✅ Multiple store variable naming patterns
- ✅ v2 import exclusion
- ✅ Real-world code patterns (CSRF, Ent ORM)
- ✅ Edge case: same variable name across different functions
- ✅ Import aliasing
The test setup using
setupTestModulefor proper type resolution context is well-designed.Note: These tests depend on fixing the critical issue with undefined functions (
identHasTypeandidentAssignedFrom) in the main code. Once that's resolved, this test suite should provide solid validation.
- Remove calls to identHasType and identAssignedFrom (which are defined in common.go but not ideal for this use case) - Implement checkStoreAssignment function with precise regex matching for session.NewStore() assignments - Add regexp import for pattern matching - Improves accuracy and maintainability of session store detection
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/internal/migrations/v3/session_release_test.go (3)
14-28: Consider usingt.Cleanupfor more robust cleanup.The helper delegates cleanup to the caller's
defer, which works but could leave directories behind if a test panics before setting up the defer. Usingt.Cleanupdirectly in the helper would be more robust:func setupTestModule(t *testing.T) string { t.Helper() // Create temp dir inside the project so it inherits go.mod dir, err := os.MkdirTemp(".", "test_migration_") require.NoError(t, err) // Ensure it's absolute path absDir, err := filepath.Abs(dir) require.NoError(t, err) + t.Cleanup(func() { + _ = os.RemoveAll(absDir) + }) + return absDir }This would also allow removing the
deferstatements from each test.
362-368: Consider more precise assertion pattern.The count assertion uses
"defer sess"which is a partial match. While it works for this specific test content, a more precise pattern would be safer for future modifications:- assert.Equal(t, 3, strings.Count(result, "defer sess"), "Should add defer for all 3 store.Get() calls") + assert.Equal(t, 3, strings.Count(result, ".Release() // Important:"), "Should add defer for all 3 store.Get() calls")This counts the actual injected Release() calls rather than partial variable name matches.
812-821: Variable shadows import alias - edge case worth documenting.On line 814, the variable
sessshadows the import aliassess(from line 798). This is intentional edge case testing but could benefit from a comment explaining this is deliberate:func backgroundTask(sessionID string) { store := sess.NewStore() + // Note: variable 'sess' below intentionally shadows the import alias to test edge case handling sess, err := store.GetByID(context.Background(), sessionID)The test correctly expects the migration to use the local variable name for the Release() call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/internal/migrations/v3/session_release_test.go(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/session_release_test.go (1)
cmd/internal/migrations/v3/session_release.go (1)
MigrateSessionRelease(31-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, macos-latest)
🔇 Additional comments (6)
cmd/internal/migrations/v3/session_release_test.go (6)
30-69: LGTM!The basic test correctly validates that
defer sess.Release()is injected afterstore.Get(c)calls with proper error checking.
205-252: LGTM!Good coverage for the middleware pattern exclusion -
session.FromContextshould not receiveRelease()calls since the middleware manages the session lifecycle.
416-488: LGTM!This real-world test case correctly validates that both closures accessing the parent-scoped
storevariable receive properdefer sess.Release()calls. The test properly verifies count and content of injected Release() calls.
705-781: LGTM!Excellent test coverage for the critical scope isolation edge case. The verification logic correctly ensures that
defer sess.Release()is only added withinsessionHandler(wherestoreis fromsession.NewStore()) and not incacheHandler(wherestoreis fromcache.NewStore()).
254-310: LGTM!Good negative test coverage ensuring non-session
Getmethods (CSRF locals, Ent ORM, cache) don't trigger false positive Release() injections. The test content correctly represents various common patterns that should be excluded.
584-635: LGTM!This test correctly validates that Release() calls are added even when errors are ignored (blank identifier
_) or not checked. The comment at lines 585-586 helpfully explains why this is safe (nil check inRelease()).
… strings
- Add proper lexing state to handle string literals and comments
- Prevents miscounting braces inside strings like fmt.Errorf("error: {%s}", data)
- Prevents miscounting braces inside comments like // { or /* } */
- Maintains backward compatibility and robustness
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/session_release.go (1)
155-183: Scope/alias detection is correct but can be optimized and centralized.
isSessionStoreInFunction+checkStoreAssignmentdo the right thing semantically, but they recompute session aliases and compile a fresh regexp on every call, scanning potentially large substrings each time. For large files with many assignments this can add noticeable overhead during a repo-wide migration.You could make this cheaper and easier to reason about by:
- Computing
aliases := findSessionPackageAliases(src)once infindReleasePointsand passing it down.- Precompiling a small set of regex patterns per
(varName, alias)pair (or just using simplestrings.Contains+ a cheaper structural check) instead ofregexp.MustCompileinside the loop.This keeps behavior identical while reducing repeated work in the hot path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/internal/migrations/v3/session_release.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/session_release.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
The defer sess.Release() statements were being inserted inside error blocks instead of after them, making the defer unreachable. This fixes the line number mapping from AST positions back to the original lines array by removing the unnecessary -1 offset.
…ibe implementation - Remove overstated claims about using Go's type checker - Clarify that the migration uses AST parsing with source-level heuristics - Explicitly document current limitations (no tracking of stores passed via parameters, returned from functions, or stored in structs) - Maintains support for custom import aliases as implemented
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/session_release.go (1)
74-74: Remove unusedcwdparameter.The
cwdparameter is marked as unused with_but is still part of the function signature. If it's not needed, consider removing it entirely to clean up the API surface.Apply this diff if the parameter is truly unnecessary:
-func addReleaseCallsWithTypes(content, _ string) (string, error) { +func addReleaseCallsWithTypes(content string) (string, error) {Then update the call site on line 46:
- result, err := addReleaseCallsWithTypes(content, cwd) + result, err := addReleaseCallsWithTypes(content)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/internal/migrations/v3/session_release.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/session_release.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, windows-latest)
🔇 Additional comments (8)
cmd/internal/migrations/v3/session_release.go (8)
22-32: Documentation accurately reflects implementation limitations.The updated doc comment correctly describes the heuristic-based approach and explicitly calls out that stores passed via parameters, returned from functions, or stored in structs are not tracked. This addresses the previous review concern about overstating capabilities.
154-184: LGTM! Store assignment verification is correctly implemented.The refactored approach using
checkStoreAssignmentwith regex-based pattern matching properly handles both:=and=assignment operators, escapes variable and alias names correctly, and supports custom import aliases. This addresses the previous critical issue about undefined functions.
186-218: Function boundary detection is adequate for the heuristic approach.The simple prefix matching for
"func "aligns with the documented limitations of using source-level heuristics rather than full type checking. While this won't handle edge cases like functions mentioned in comments, it's appropriate for the stated design goals.
220-240: Alias detection logic is correct.The function properly handles both default (unaliased) and custom-aliased session package imports. The string-based parsing is appropriate for this use case and aligns with the heuristic approach.
242-289: Defer insertion logic is well-structured.The reverse iteration to maintain line indices, duplicate prevention via
hasExistingRelease, and conditional placement after error blocks or immediately after the Get() call all demonstrate careful implementation. The indentation preservation ensures the inserted code respects the existing formatting.
313-365: Error block detection correctly implemented with AST parsing.The line number calculation at lines 351-353 correctly maps AST positions back to the original source by accounting for the 2-line wrapper (
"package main\nfunc f() {\n") and 1-based line numbering. The use of AST parsing as the primary method with a brace-counting fallback provides good robustness. Thenolintcomments forstrings.Builder.WriteStringappropriately document that these calls never fail.
367-435: Fallback brace-counting implementation is thorough and correct.The function properly handles string literals (including escape sequences), both line and block comments, and counts braces accurately. The addition of the default case (lines 429-431) addresses the linter requirement. This provides a solid fallback when AST parsing fails.
99-101: The current implementation already handles all existing patterns in the codebase. A comprehensive search confirms that every store.Get() and store.GetByID() call uses short variable declaration (:=), not regular assignment (=). No patterns likesess, err = store.Get(c)exist in the code. The DEFINE-only check is therefore not a limitation in practice.Likely an incorrect or invalid review comment.
Summary
Implements migration to add
defer sess.Release()calls for Fiber v3 session Store Pattern usage. In v3, sessions obtained viastore.Get()andstore.GetByID()must be manually released back to the pool to prevent memory leaks. Fixes gofiber/recipes#3841Key Features
session.NewStore()assignmentssession.NewStore()in current function scope*session.Storeusage (not full go/types resolution)defer Release()after error checks or immediately if no error handlingRelease()calls before addingGet()returns nil on error (Release() has nil guard)Migration Approach
Edge Cases Handled
✅ Correctly Adds Release()
sess, err := store.Get(c)with error checksess, _ := store.Get(c)error ignoredsess, err := store.Get(c)no error check (just continues)sess, err := store.GetByID(ctx, id)background tasksstore,sessionStore,myStore)storeinmain()used inapp.Post(func...))✅ Correctly Excludes (No False Positives)
session.FromContext(c)- middleware manages lifecyclecache.Get(), Ent ORMclient.Get(), CSRF session fromc.Locals()✅ Duplicate Prevention
defer sess.Release()Real-World Testing
Verified against actual gofiber/recipes examples:
Test Coverage
13 comprehensive tests covering:
All tests passing, 0 linting issues.
Implementation Details
Type / Source Verification Algorithm
The migration uses AST inspection plus source-level regex-based checks (not Go's full type checker) to identify
*session.Store.Get()calls:session,sess, etc.)identHasType()andidentAssignedFrom()to match*session.Storeorsession.NewStore()in the analyzed source textGet()was calledThis approach is robust for most real-world code and prevents the main false positives, but it does not use go/types full type resolution and thus intentionally avoids complex cross-package type analysis.
Why Source-level Checks Matter
Without type verification we'd incorrectly add Release():
Review Notes for Maintainers
Critical areas to review:
isSessionStoreInFunction()- handles aliases and source-level type declarationsKnown limitations:
Recommendation: Run migration on gofiber/recipes repository to verify real-world usage patterns.
Migration Execution Order
This migration runs as part of the v3 migration chain:
MigrateContribPackages- Changes imports v2→v3MigrateSessionStore- Changessession.New()→session.NewStore()MigrateStorageVersions- Updates storage package versionsMigrateSessionRelease← This migration (runs after imports are v3)Checklist
<- This is an auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
<- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
Tests
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.