feat: add storage version and session release migrations for v3 #248
Conversation
This commit adds two critical migrations to fix session-related issues in PR #3841: 1. Storage Version Migration (storage_versions.go): - Migrates storage package imports to their latest versions - Handles v2 → v3 upgrades for redis and postgres - Handles unversioned → v2 upgrades for 17 adapters - Leaves v1/unversioned packages unchanged - Comprehensive version mapping based on actual package versions 2. Session Release Migration (session_release.go): - Adds defer sess.Release() for legacy Store Pattern in Fiber v3 - Detects sess, err := store.Get(c) patterns - Intelligently inserts defer after error check blocks - Idempotent - won't add duplicate defer statements Both migrations include comprehensive test coverage: - 5 test scenarios for storage versions - 4 test scenarios for session release - All 270 tests passing - 0 linting issues Fixes: gofiber/recipes#3841
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
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 introduces two crucial automated migrations designed to streamline the upgrade process to Fiber v3 and resolve identified session-related issues. The first migration ensures that 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 pull request introduces two crucial migrations for Fiber v3: updating storage package imports to their correct major versions and ensuring proper session cleanup by adding defer sess.Release() calls. Both migrations are thoughtfully designed, leveraging regular expressions to accurately identify and modify code patterns. The session_release.go migration demonstrates intelligence in handling diverse error checking styles and ensures idempotency, preventing redundant defer statements. Similarly, the storage_versions.go migration utilizes a clear mapping to update import paths, also maintaining idempotency. The comprehensive test suites accompanying both migrations cover various scenarios and edge cases, affirming the robustness and reliability of these changes for a smooth transition to Fiber v3.
WalkthroughThis PR adds two new migration functions to the v3 migration framework. MigrateStorageVersions rewrites storage package imports to include version suffixes (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 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: 2
🧹 Nitpick comments (5)
cmd/internal/migrations/v3/session_release.go (3)
23-23: Regex may produce false positives on non-session store.Get calls.The regex
(\w+)\.(Get(?:ByID)?)\(matches anyx.Get(orx.GetByID(pattern, not specifically session store calls. This could incorrectly insertdefer .Release()for unrelated.Get()methods (e.g., map access helpers, HTTP client calls).Consider adding context validation—such as checking that the file imports
middleware/sessionor that the store variable is assigned fromsession.NewStore()—before inserting the defer.
63-65: ConditionblockEnd >= len(lines)is overly strict.When
blockEnd == len(lines) - 1(i.e., the closing brace is on the last line), the conditionblockEnd >= len(lines)is false, so processing continues. However, at line 69-72, ifblockEndis the last valid index,searchEndmay still be valid but there are no lines afterblockEndto search. This is handled, but the comment at line 125 says "Returns the line index after the closing brace" which is inconsistent with returning the actual brace line index.The logic works but the comment is misleading—
findErrorBlockEndreturns the index of the closing brace, not the index after it.-// findErrorBlockEnd finds the end of an error handling block -// Returns the line index after the closing brace, or -1 if not found +// findErrorBlockEnd finds the end of an error handling block +// Returns the line index of the closing brace, or -1 if not found
126-126: Unused parameter infindErrorBlockEnd.The third parameter (originally
indent) is named_indicating it's unused. Consider removing it from the function signature if not needed.-func findErrorBlockEnd(lines []string, startIdx int, _ string) int { +func findErrorBlockEnd(lines []string, startIdx int) int {And update the call site at line 60:
- blockEnd := findErrorBlockEnd(lines, nextLineIdx, indent) + blockEnd := findErrorBlockEnd(lines, nextLineIdx)cmd/internal/migrations/v3/session_release_test.go (1)
178-181: Assertion may be fragile due to multiple}characters in content.
strings.Index(result, "}")finds the first}in the file, which could be in the import block or elsewhere, not necessarily the error block's closing brace. This makes the assertion potentially unreliable.Consider a more precise assertion:
- // 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 by checking it appears after "return err" + returnIdx := strings.Index(result, "return err") + deferIdx := strings.Index(result, "defer sess.Release()") + assert.Greater(t, deferIdx, returnIdx, "defer should come after error handling block")cmd/internal/migrations/v3/storage_versions.go (1)
15-42: LGTM!Well-documented version mapping with clear comments indicating actual versions that are currently up-to-date (postgres v3.3.1 tagged Nov 28, 2025). The explicit exclusion of v1/unversioned adapters in comments helps maintainability.
Consider adding a comment describing how to update this map when new adapter versions are released—this will help future maintainers keep versions synchronized with latest gofiber/storage releases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/internal/migrations/lists.go(1 hunks)cmd/internal/migrations/v3/session_release.go(1 hunks)cmd/internal/migrations/v3/session_release_test.go(1 hunks)cmd/internal/migrations/v3/storage_versions.go(1 hunks)cmd/internal/migrations/v3/storage_versions_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
cmd/internal/migrations/v3/storage_versions.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
cmd/internal/migrations/v3/storage_versions_test.go (1)
cmd/internal/migrations/v3/storage_versions.go (1)
MigrateStorageVersions(46-93)
cmd/internal/migrations/lists.go (2)
cmd/internal/migrations/v3/storage_versions.go (1)
MigrateStorageVersions(46-93)cmd/internal/migrations/v3/session_release.go (1)
MigrateSessionRelease(17-122)
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(17-122)
🔇 Additional comments (12)
cmd/internal/migrations/v3/session_release.go (1)
1-12: LGTM!Package declaration and imports are appropriate for the functionality.
cmd/internal/migrations/v3/session_release_test.go (3)
14-52: LGTM!Good basic test case verifying the defer insertion for the standard
store.Get(c)pattern.
54-96: LGTM!Excellent idempotency test ensuring the migration doesn't add duplicate defer statements.
98-136: LGTM!Good coverage for the
GetByIDvariant.cmd/internal/migrations/lists.go (1)
73-74: LGTM!The new migrations are correctly registered in the v3 migration list. The ordering (storage versions before session release) is logical since import updates should happen before code transformations.
cmd/internal/migrations/v3/storage_versions_test.go (5)
13-53: LGTM!Comprehensive test for basic storage version migration covering sqlite3→v2, redis→v3, postgres→v3 upgrades.
55-93: LGTM!Good idempotency test ensuring already-versioned imports remain unchanged without duplicate suffixes.
95-136: LGTM!Good test for mixed version scenarios.
138-179: LGTM!Excellent test case for v2→v3 upgrades (redis, postgres) while sqlite3 correctly stays at v2.
181-219: LGTM!Good test verifying unknown packages (aerospike, nats) remain unchanged while known packages are upgraded.
cmd/internal/migrations/v3/storage_versions.go (2)
54-83: LGTM!The implementation correctly:
- Matches both versioned and unversioned imports
- Preserves unknown packages unchanged
- Skips already-correct versions
- Uses
internal.ChangeFileContentwhich handlesgoimportsprocessing
52-52: Regex pattern could theoretically match nested paths, but this is safely handled by the package name lookup.The regex
([a-zA-Z0-9_-]+)(?:/v(\d+))?does allow matching nested paths (e.g., capturinginternalfromredis/internal). However, the code is safe because line 69 performs a map lookup againststorageVersionMap, which only contains known storage adapter names (postgres,redis,mongodb, etc.). If the regex matched a nested subpath, the lookup would fail and the original import would be returned unchanged. Additionally, the gofiber/storage ecosystem uses only single-level package names without exposed internal subpaths.
- Improve brace detection in session release migration to handle '} else {' patterns
- Add explicit check for 'else' keyword to prevent premature termination
- Update comment to accurately describe findErrorBlockEnd return value
- Add note about brace counting limitation in strings/comments
These changes address the code review feedback from CodeRabbit while
maintaining compatibility with typical Go error handling patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/internal/migrations/v3/session_release.go (1)
125-156: Brace counting limitation is properly documented.The comment at lines 127-128 correctly acknowledges that brace counting may not handle braces inside strings or comments. This is a known limitation that's acceptable for migration tooling targeting typical Go error handling patterns.
🧹 Nitpick comments (2)
cmd/internal/migrations/v3/session_release.go (2)
98-98: Consider extracting the comment text as a constant.The inline comment
"// Important: Manual cleanup required"could be extracted for easier maintenance if the message needs to change.+const releaseComment = "// Important: Manual cleanup required" + // MigrateSessionRelease adds defer sess.Release() after store.Get() calls ... - deferLine := indent + "defer " + sessVar + ".Release() // Important: Manual cleanup required" + deferLine := indent + "defer " + sessVar + ".Release() " + releaseComment
129-129: Remove unused parameter.The
indentparameter (now_ string) is unused. Consider removing it from the function signature to improve clarity.-func findErrorBlockEnd(lines []string, startIdx int, _ string) int { +func findErrorBlockEnd(lines []string, startIdx int) int {And update the call site at line 60:
- blockEnd := findErrorBlockEnd(lines, nextLineIdx, indent) + blockEnd := findErrorBlockEnd(lines, nextLineIdx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/internal/migrations/v3/session_release.go(1 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: lint
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-latest)
🔇 Additional comments (4)
cmd/internal/migrations/v3/session_release.go (4)
1-12: LGTM!Package declaration and imports are appropriate for the migration functionality.
17-57: Consider handling the case where error is discarded.The migration only triggers when there's an error check pattern (
if errVar != nil). If users wrotesess, _ := store.Get(c)(discarding the error), the migration won't insert thedefer sess.Release()call.This may be intentional since discarding the error is already a code smell, but it's worth noting that such patterns would still need manual migration.
67-84: LGTM - Idempotency check is well-implemented.The search for existing
Release()calls with a 20-line window and the improved brace detection logic (addressing the} else {case) provides reasonable idempotency guarantees. The code won't create duplicatedeferstatements on re-runs.
137-139: Single-line detection could produce false positives.The check
strings.Contains(line, "{") && strings.Contains(line, "}")detects single-lineifstatements, but may incorrectly match lines where braces appear in unequal counts or within string literals:// Would be detected as single-line but isn't complete: if err != nil { doSomething(For a migration tool with typical Go idioms, this edge case is rare. The limitation is documented.
Per René's suggestion, renamed storageVersionMap to storageMinimumVersions to better communicate that these are minimum required major versions for Fiber v3 compatibility rather than just 'latest' versions. - Renamed variable: storageVersionMap -> storageMinimumVersions - Updated documentation to emphasize these are minimum required versions - Updated all references throughout the migration logic - All tests passing, no functional changes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/internal/migrations/v3/storage_versions.go (2)
53-53: Consider compiling regex at package level.Compiling the regex inside the function works correctly, but Go best practices recommend compiling regex patterns at package level using
regexp.MustCompileto avoid repeated compilation and to fail fast on invalid patterns during initialization.+// reStorageImport matches storage imports with or without version suffix +var reStorageImport = regexp.MustCompile(`"github\.com/gofiber/storage/([a-zA-Z0-9_-]+)(?:/v(\d+))?"`) + // MigrateStorageVersions updates storage package imports to use the correct latest version. // This migration handles storage packages from github.com/gofiber/storage/*. func MigrateStorageVersions(cmd *cobra.Command, cwd string, _, _ *semver.Version) error { - // Regex to match storage imports with or without version suffix - // Examples: - // "github.com/gofiber/storage/sqlite3" - // "github.com/gofiber/storage/redis/v2" - // "github.com/gofiber/storage/postgres/v3" - reStorageImport := regexp.MustCompile(`"github\.com/gofiber/storage/([a-zA-Z0-9_-]+)(?:/v(\d+))?"`)
92-93: Consider adjusting message tense.The message "Migrating storage package versions" is printed after the migration is already complete. Consider using past tense ("Migrated") for clarity, or moving the print before the operation begins.
if !changed { return nil } - cmd.Println("Migrating storage package versions") + cmd.Println("Migrated storage package versions") return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/internal/migrations/v3/storage_versions.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/storage_versions.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). (5)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Analyse
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, macos-13)
🔇 Additional comments (3)
cmd/internal/migrations/v3/storage_versions.go (3)
1-11: LGTM!Package declaration and imports are correctly structured. All imports are used appropriately.
13-43: Well-documented version mapping.The categorization is clear and the exclusion of v1/unversioned adapters is appropriately documented. The inline version comments (e.g.,
// Latest: v3.3.1) are helpful for maintainability.
76-82: Verify intended behavior for version comparison.The current logic sets any non-matching version to the target version without semantic comparison. This means if a package were already at a higher version (e.g., hypothetical
redis/v4), it would be "downgraded" to v3.While this scenario is unlikely since the migration targets v2→v3 upgrades and higher versions don't currently exist, consider whether adding a version comparison (only upgrade if current < target) would be more robust for future-proofing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/session_release.go (1)
19-25: Pattern breadth may cause false positives on non-sessionGetcalls.The regex matches any
x, err := y.Get(orGetByID(pattern, which could inadvertently match non-session types like caches, maps, or database accessors that have aGetmethod. While thereleaseCommentsignals manual review is needed andgoimportswould catch invalid.Release()calls at write time, consider whether a more targeted pattern (e.g., matching only identifiers containing "store", "session", or "sess") would reduce noise.If you want to reduce false positives, you could restrict the store variable name:
- reStoreGet := regexp.MustCompile(`(?m)^(\s*)(\w+),\s*(\w+)\s*:=\s*(\w+)\.(Get(?:ByID)?)\(`) + // Match store-like variable names to reduce false positives + reStoreGet := regexp.MustCompile(`(?m)^(\s*)(\w+),\s*(\w+)\s*:=\s*(\w*[Ss]tore\w*)\.(Get(?:ByID)?)\(`)Alternatively, keep the broad pattern if the intent is to catch all possible session store naming conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (3)
Makefile(1 hunks)cmd/internal/migrations/v3/session_release.go(1 hunks)cmd/internal/migrations/v3/storage_versions.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/internal/migrations/v3/storage_versions.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T12:18:29.841Z
Learnt from: CR
Repo: gofiber/cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-03T12:18:29.841Z
Learning: Use the Makefile commands for common tasks (e.g., `make lint`, `make test`)
Applied to files:
Makefile
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/session_release.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
🔇 Additional comments (5)
Makefile (1)
1-1: Routine patch-release toolchain update.Bumping from go1.25.0 to go1.25.5 is a standard maintenance update. Patch releases are backward-compatible and contain bug fixes.
Please confirm this toolchain update aligns with your CI/CD pipeline and that the project remains building and linting cleanly with go1.25.5.
cmd/internal/migrations/v3/session_release.go (4)
1-14: LGTM!Imports are appropriate and the
releaseCommentconstant clearly documents the purpose of the inserted code for manual review.
54-59: Error check detection may miss compound conditions.The prefix check
if errVar+" != nil"won't match compound error conditions likeif err != nil && otherCondition {orif err == nil {(for early-exit patterns). For a migration tool this is acceptable since the most common pattern is handled, but worth noting that some valid patterns may be skipped.
69-97: Good idempotency check for existing Release calls.The search window and early termination logic correctly prevent duplicate insertions. The fix from the previous review addressing
} else {patterns is properly implemented.
127-158: Brace counting limitation is properly documented.The comment at lines 129-130 clearly acknowledges that braces within strings and comments may cause incorrect detection, which appropriately addresses the concern from the previous review. For typical Go error handling patterns, this implementation is sufficient.
sixcolors
force-pushed
the
fix/session-migration-pr-3841
branch
from
96e63b1 to
296a96b
Compare
- Extract release comment as constant for easier maintenance - Remove unused indent parameter from findErrorBlockEnd - Move regex compilation to package level (Go best practice) - Fix message tense: 'Migrating' → 'Migrated' These refactors improve code maintainability and follow Go conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/internal/migrations/v3/session_release.go (1)
69-86: Consider edge case with closing braces in nested structures.The search for existing
Release()calls stops when encountering lines starting with}. While the added!strings.Contains(trimmed, "else")check addresses the} else {concern from previous reviews, there's a subtle edge case: lines like})or},from nested function calls or composite literals could still terminate the search prematurely.For example:
sess, err := store.Get(c) if err != nil { return err } result := ProcessData(Data{ Field: "value", }) // <- `})` would stop the search here defer sess.Release() // <- existing Release() not detected, duplicate insertedThis is likely a rare scenario in practice, and acceptable given the tool's migration-focused scope. However, if you want to be more precise, consider checking indent levels rather than just brace presence:
// Track the indent level of the error block for more accurate scope detection blockIndent := len(indent) for j := blockEnd + 1; j < searchEnd; j++ { if strings.Contains(lines[j], sessVar+".Release()") { hasRelease = true break } trimmed := strings.TrimSpace(lines[j]) currentIndent := len(lines[j]) - len(strings.TrimLeft(lines[j], " \t")) // Stop only at closing braces at the same or lower indent than the original block if strings.HasPrefix(trimmed, "}") && !strings.Contains(trimmed, "{") && !strings.Contains(trimmed, "else") && currentIndent <= blockIndent { break } }cmd/internal/migrations/v3/storage_versions.go (1)
21-51: Comprehensive and well-documented version mapping.The
storageMinimumVersionsmap covers 17 packages comprehensively with clear inline documentation of target versions. The distinction between v3, v2, and v1/unversioned adapters is well-explained in the comments, making the migration strategy transparent.Optional: Consider renaming
storageMinimumVersionstostorageFiberV3VersionsorstorageTargetVersionsto clarify that these represent the specific versions required for Fiber v3 compatibility, rather than minimum version constraints. The current name could be interpreted as "at least v2" when it actually means "exactly v2 for v3 compatibility." However, the existing comments do explain this clearly, so it's a minor naming consideration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/session_release.go(1 hunks)cmd/internal/migrations/v3/storage_versions.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/storage_versions.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). (5)
- GitHub Check: Analyse
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, ubuntu-latest)
🔇 Additional comments (6)
cmd/internal/migrations/v3/session_release.go (3)
19-26: Pattern matching logic looks solid.The regex correctly captures session acquisition patterns including
store.Get()andstore.GetByID(), and properly extracts the session variable, error variable, and indentation. This will handle the common Store Pattern usage effectively.
127-158: Helper function is well-documented with known limitations.The
findErrorBlockEndfunction correctly handles both single-line and multi-line error blocks using brace counting. The comment at lines 129-130 appropriately documents the limitation regarding braces in strings/comments, which is an acceptable trade-off for a migration tool targeting typical Go error handling patterns.
19-125: Overall migration logic is sound and well-structured.The implementation correctly:
- Identifies Store Pattern usage via regex
- Locates the corresponding error handling block
- Inserts
defer sess.Release()at the appropriate position- Maintains idempotence by checking for existing Release() calls
- Preserves indentation and formatting
The acknowledged limitations (brace counting in strings/comments) are reasonable for a migration tool and well-documented.
cmd/internal/migrations/v3/storage_versions.go (3)
13-19: Regex pattern correctly handles versioned and unversioned imports.The pattern
"github\.com/gofiber/storage/([a-zA-Z0-9_-]+)(?:/v(\d+))?"properly captures both package name and optional version suffix, supporting all import formats including aliased imports.
53-95: Migration logic is clean and correct.The implementation properly:
- Extracts package name and current version from imports
- Looks up target version from the mapping
- Skips unknown packages (leaves unchanged)
- Skips already-correct versions (idempotent)
- Constructs properly versioned import paths
The integration with
internal.ChangeFileContentensures automatic import cleanup viagoimports, and error handling with context wrapping is appropriate.
77-80: Version comparison assumes upgrade-only scenario.The equality check
currentVersion == minVersionskips replacement only when versions match exactly. This works correctly for the v2→v3 migration scenario and handles unversioned→v2 upgrades as intended.Note: A future edge case could arise if a package has a version higher than the mapped target (e.g., user on v4, map specifies v3), which would incorrectly downgrade. This isn't relevant for the current migration but could be addressed in future versions with semantic version comparison if storage packages evolve beyond v3.
This is working as designed for the current migration. If you anticipate storage packages releasing v4+ in the near future, consider adding version comparison logic:
// Parse and compare semantic versions to prevent downgrades currentSemver, err := semver.NewVersion(currentVersion) minSemver, err := semver.NewVersion(minVersion) if err == nil && currentSemver.GreaterThan(minSemver) { return match // Don't downgrade }However, this adds complexity that may not be needed for the current v2→v3 migration scope.
|
govulncheck "Your code is affected by 1 vulnerability from the Go standard library. Right, safe to ignore. |
Summary
This PR adds two critical migrations to fix session-related issues identified in gofiber/recipes#3841:
1. Storage Version Migration
2. Session Release Migration
defer sess.Release()for legacy Store Pattern in Fiber v3sess, err := store.Get(c)patternsTest Coverage
Files Changed
cmd/internal/migrations/v3/storage_versions.go- Storage version migration logiccmd/internal/migrations/v3/storage_versions_test.go- 5 test casescmd/internal/migrations/v3/session_release.go- Session release migration logiccmd/internal/migrations/v3/session_release_test.go- 4 test casescmd/internal/migrations/lists.go- Integration into migration sequenceStorage Adapters Covered
v3 adapters (2): postgres, redis
v2 adapters (17): arangodb, azureblob, badger, bbolt, couchbase, dynamodb, etcd, memcache, memory, mongodb, mssql, mysql, pebble, ristretto, s3, sqlite3
v1/unversioned (13+): aerospike, cassandra, clickhouse, cloudflarekv, coherence, leveldb, minio, mockstorage, nats, neo4j, rueidis, scylladb, surrealdb, valkey
Related Issues
Fixes gofiber/recipes#3841
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.