Fix Add migration and import handling#165
Conversation
github-actions
Bot
added
☢️ Bug
|
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. WalkthroughUse filepath for cross-platform go.mod resolution; simplify Fiber import replacement to a literal Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant CLI as CLI Migrator
participant FS as Filesystem
Dev->>CLI: Run migrate (target v3)
CLI->>FS: Read project files (go.mod, *.go)
rect rgba(200,230,255,0.25)
note over CLI: Import & go.mod migration
CLI->>CLI: Find Fiber imports via fiberImportRegex
CLI->>FS: Rewrite imports -> github.com/gofiber/fiber/v<major>
CLI->>FS: Update go.mod require -> v<major>
end
CLI-->>Dev: Report "Migrating Go packages"
sequenceDiagram
autonumber
participant CLI as CLI Migrator (v3)
participant Src as Source File
participant SB as String Builder
CLI->>Src: Load file content
rect rgba(220,255,220,0.25)
note over CLI,SB: Add-method scanning flow
CLI->>CLI: Locate ".Add(" matches
loop For each match
CLI->>CLI: Inspect preceding identifier
alt Identifier is Header or httpServerActiveRequests
CLI->>SB: Preserve original call
else
CLI->>CLI: Parse arguments (handle nesting)
alt ≥2 args
CLI->>SB: Wrap first arg -> []string{arg1} and reconstruct call
else
CLI->>SB: Preserve call
end
end
end
end
CLI->>Src: Write transformed content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ReneWerner87, 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 refines and fixes existing migration tools, primarily focusing on Go package import handling and specific method/config migrations. The changes consolidate Fiber core import updates into a single, more robust MigrateGoPkgs function, eliminating a redundant migrator. Additionally, it improves the precision of the MigrateAddMethod and MigrateCSRFConfig functions to prevent unintended modifications to unrelated code, ensuring that migrations are applied only where necessary. A new regression test has also been added to validate the Go package migration process.
Highlights
- Go Package Migration Refinement: The
MigrateGoPkgsfunction has been enhanced to consolidate Fiber core import updates, integrating logic that was previously handled separately. This streamlines the migration process for Go packages. - Redundant Migrator Removal: The redundant
MigrateFiberCoreImportsmigrator has been removed, as its functionality is now fully integrated into the improvedMigrateGoPkgsfunction, reducing code duplication. - Improved
AddMethod Migration Precision: TheMigrateAddMethodinv3/common.gohas been made more robust. It now selectively applies changes, preventing unintended modifications to unrelatedAddmethod calls (e.g.,Header.Add,httpServerActiveRequests.Add) by analyzing the context of the call. - Targeted CSRF Config Migration: The
MigrateCSRFConfighas been enhanced to ensure that the replacement of 'Expiration:' with 'IdleTimeout:' only occurs withincsrf.Configstructs, avoiding incorrect changes in other parts of the codebase, such aspaseto.JSONToken. - New Regression Test for Go Package Migration: A new regression test (
Test_MigrateGoPkgs) has been added to verify the correct functionality of the Go package migration, specifically ensuring that Fiber import paths andgo.modentries are updated as expected.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the Go package migration logic by consolidating Fiber core import updates into a single migrator and adds a regression test for it. It also improves the robustness of the MigrateAddMethod and MigrateCSRFConfig migrators by making them more specific to avoid incorrect replacements in unrelated code. The changes are generally positive and improve the migration tool. However, I've identified one potential issue in MigrateCSRFConfig where a regular expression is not robust enough to handle nested structures, which could lead to code corruption in some edge cases. I've left a detailed comment with a suggestion for a more robust implementation.
| // MigrateCSRFConfig updates csrf middleware configuration fields | ||
| func MigrateCSRFConfig(cmd *cobra.Command, cwd string, _, _ *semver.Version) error { | ||
| replacer := strings.NewReplacer("Expiration:", "IdleTimeout:") | ||
| reConfig := regexp.MustCompile(`csrf\.Config{[^}]*}`) |
There was a problem hiding this comment.
The regular expression csrf\.Config{[^}]*} is not robust enough to handle all cases, specifically when the csrf.Config struct literal contains nested blocks with curly braces, such as an inline function literal. The [^}]* pattern will stop at the first closing brace it finds, which can lead to incorrect partial replacements and corrupt the code.
For example, consider this code:
var _ = csrf.New(csrf.Config{
Expiration: 10 * time.Minute,
Next: func(c *fiber.Ctx) bool {
if c.Path() == "/login" {
return true
}
return false
},
})The regex will only match up to the } of the Next function, and the replacement will be applied incorrectly.
A more robust approach would be to parse the struct literal by matching braces, similar to how extractCall handles parentheses for function calls. You could create a helper function extractBraceBlock to find the matching closing brace for a struct literal and then perform the replacement on the correctly identified block.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (4)
cmd/internal/migrations/v3/common.go (2)
362-367: Consider caching the regex compilation.The regex is compiled on every function call. Since this is a migration tool that processes multiple files, consider moving the regex compilation outside the function or using
sync.Oncefor better performance.Move the regex compilation to package level:
+var addMethodRegex = regexp.MustCompile(`\.Add\(`) + func MigrateAddMethod(cmd *cobra.Command, cwd string, _, _ *semver.Version) error { err := internal.ChangeFileContent(cwd, func(content string) string { - re := regexp.MustCompile(`\.Add\(`) - matches := re.FindAllStringIndex(content, -1) + matches := addMethodRegex.FindAllStringIndex(content, -1)
464-464: Consider caching the regex compilation for performance.Similar to the Add method, this regex is compiled on every function call.
Move the regex compilation to package level:
+var csrfConfigRegex = regexp.MustCompile(`csrf\.Config{[^}]*}`) + func MigrateCSRFConfig(cmd *cobra.Command, cwd string, _, _ *semver.Version) error { - reConfig := regexp.MustCompile(`csrf\.Config{[^}]*}`) reSession := regexp.MustCompile(`\s*SessionKey:\s*[^,]+,?\n`) reKeyLookup := regexp.MustCompile(`(\s*)KeyLookup:\s*([^,\n]+)(,?)(\n?)`) err := internal.ChangeFileContent(cwd, func(content string) string { - content = reConfig.ReplaceAllStringFunc(content, func(s string) string { + content = csrfConfigRegex.ReplaceAllStringFunc(content, func(s string) string {cmd/internal/migrations/common_test.go (1)
20-27: Consider adding edge cases to the test.The test covers the happy path well. Consider adding test cases for:
- Multiple import styles (aliased imports, dot imports)
- Error scenarios (missing go.mod, read-only files)
Consider adding additional test cases:
func Test_MigrateGoPkgs_ErrorCases(t *testing.T) { // Test with missing go.mod dir, err := os.MkdirTemp("", "mgpkgs_err") require.NoError(t, err) defer func() { require.NoError(t, os.RemoveAll(dir)) }() // Don't create go.mod file mainContent := `package main import "github.com/gofiber/fiber/v2" func main() {}` require.NoError(t, os.WriteFile(filepath.Join(dir, "main.go"), []byte(mainContent), 0o600)) var buf bytes.Buffer cmd := newCmd(&buf) target := semver.MustParse("3.0.0") err = migrations.MigrateGoPkgs(cmd, dir, nil, target) require.Error(t, err) assert.Contains(t, err.Error(), "go.mod") }cmd/internal/migrations/v3/common_test.go (1)
515-523: Consider adding assertion for migration log.The test verifies that Expiration is preserved but doesn't check if the migration was logged. Based on the pattern in other tests, consider adding this assertion.
Add the assertion for consistent test behavior:
content := readFile(t, file) assert.Contains(t, content, "Expiration:") assert.NotContains(t, content, "IdleTimeout:") + assert.Contains(t, buf.String(), "Migrating CSRF middleware configs")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
cmd/internal/migrations/common.go(2 hunks)cmd/internal/migrations/common_test.go(1 hunks)cmd/internal/migrations/v3/common.go(2 hunks)cmd/internal/migrations/v3/common_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
cmd/internal/migrations/common_test.gocmd/internal/migrations/v3/common_test.go
🧬 Code graph analysis (4)
cmd/internal/migrations/v3/common.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(36-68)
cmd/internal/migrations/common_test.go (2)
cmd/internal/migrations/common.go (1)
MigrateGoPkgs(21-51)cmd/migrate_test.go (1)
Test_Migrate_V2_to_V3(20-99)
cmd/internal/migrations/common.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(36-68)
cmd/internal/migrations/v3/common_test.go (1)
cmd/internal/migrations/v3/common.go (2)
MigrateAddMethod(360-423)MigrateCSRFConfig(463-508)
🪛 GitHub Check: CodeQL
cmd/internal/migrations/common.go
[failure] 18-18: Missing regular expression anchor
When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it.
🪛 GitHub Actions: golangci-lint
cmd/internal/migrations/common.go
[error] 32-32: golangci-lint: G304: Potential file inclusion via variable (gosec) in cmd/internal/migrations/common.go:32 (during golangci-lint run).
🔇 Additional comments (11)
cmd/internal/migrations/common.go (4)
6-6: LGTM! Cross-platform path handling improvement.Good addition of
path/filepathpackage to support cross-platform go.mod path construction.
17-19: Naming improvement for regex variables.The rename from
pkgImportRegextofiberImportRegexis more descriptive and clearly indicates its purpose for Fiber-specific imports.
21-29: Clean simplification of import replacement logic.The refactored approach using
fiberImportRegexwith a simple literal string replacement is cleaner than the previous captured groups pattern. This makes the code more readable and maintainable.
31-32: False positive G304 warning – intended CLI behaviorThe
cwdparameter here is explicitly provided by the end-user invoking the CLI (or defaults to the current working directory), so this code is operating on a path the user has knowingly granted access to. As a result, the gosec G304 warning can be safely suppressed rather than introducing unnecessary validation logic.Suggested change:
– fileContent, err := os.ReadFile(modFile) + // #nosec G304: path is user-controlled via CLI flags/intended cwd + fileContent, err := os.ReadFile(modFile)Likely an incorrect or invalid review comment.
cmd/internal/migrations/v3/common.go (3)
359-423: Well-designed Add method migration with proper exclusion logic.The refactored
MigrateAddMethodfunction is a significant improvement. It properly identifies and preserves standard HTTP header operations and metric counters while transforming Fiber-specific Add calls. The use of a custom scanner with robust parsing logic for handling nested structures is excellent.
383-391: Robust identifier extraction logic.The backward search for identifier boundaries is well-implemented, correctly handling alphanumeric characters and underscores.
463-470: Targeted CSRF config migration approach.Good improvement to narrow the scope of the Expiration to IdleTimeout replacement using a regex that specifically targets
csrf.Config{...}blocks. This prevents unintended replacements in other contexts.cmd/internal/migrations/common_test.go (1)
15-50: Comprehensive test for Go package migration.Excellent test coverage for the
MigrateGoPkgsfunction. The test properly validates:
- Import statement migration from v2 to v3
- go.mod file updates with correct version
- Command output verification
- Proper cleanup with temp directory
The test structure follows good practices and aligns well with the implementation changes.
cmd/internal/migrations/v3/common_test.go (3)
301-325: Excellent test for Add method exclusion logic.This test properly validates that the migration correctly identifies and preserves standard HTTP header operations and metric counters, which is critical for preventing false positive transformations.
327-347: Good coverage for MIME constant migration.The test properly validates the replacement of deprecated JavaScript MIME type constants.
506-534: Important test for Paseto compatibility.Excellent test to ensure that the CSRF migration doesn't incorrectly transform Paseto token fields. This prevents breaking third-party library integrations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/internal/migrations/common.go (1)
18-18: Anchor the Fiber import regex to avoid substring matches and satisfy CodeQL.Unanchored
github\.com/gofiber/fiber/v\d+can match inside longer tokens (e.g.,somethinggithub.com/...). Use word boundaries to constrain matches without breaking subpackage paths. This also addresses the existing CodeQL “Missing regular expression anchor” finding.Apply this diff:
- fiberImportRegex = regexp.MustCompile(`github\.com/gofiber/fiber/v\d+`) + fiberImportRegex = regexp.MustCompile(`\bgithub\.com/gofiber/fiber/v\d+\b`)
🧹 Nitpick comments (1)
cmd/internal/migrations/common.go (1)
23-24: Migration regex replacement is safe—consider optional AST-based import rewritingThe boundary-anchored regex in
cmd/internal/migrations/common.go(lines 23–24) correctly converts allv2imports tov3, including subpackages, without touching production code. We ran ripgrep across the repository (excludingvendor/) and found no residualgithub.com/gofiber/fiber/v2in non-test files—only in migration definitions and test fixtures, where they’re expected.Optional refactor (suggested):
- For future precision, parse each
.gofile withgo/parserand rewrite only the AST’sImportSpecnodes, avoiding any modifications in comments or string literals.Existing tests already cover:
- Aliased imports (
import fiber "github.com/gofiber/fiber/v2")- Grouped import blocks with subpackages
- String literals or comments containing the old path
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cmd/internal/migrations/common.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
🧬 Code graph analysis (1)
cmd/internal/migrations/common.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(36-68)
🪛 GitHub Check: CodeQL
cmd/internal/migrations/common.go
[failure] 18-18: Missing regular expression anchor
When this is used as a regular expression on a URL, it may match anywhere, and arbitrary hosts may come before or after it.
🔇 Additional comments (2)
cmd/internal/migrations/common.go (2)
6-6: Good: cross‑platform go.mod resolution via filepath.Join.This fixes Windows path issues and keeps the logic localized to this migrator.
31-33: LGTM: explicit go.mod path + #nosec rationale.Joining with cwd fixes relative-path issues and the G304 suppression is appropriate given a fixed filename under controlled cwd.
| var ( | ||
| pkgRegex = regexp.MustCompile(`(github\.com/gofiber/fiber/)(v\d+)( *?)(v[\w.-]+)`) | ||
| pkgImportRegex = regexp.MustCompile(`(?m)^(\s*(?:[\w.]+\s+)?")github\.com/gofiber/fiber/v\d+("$)`) | ||
| pkgRegex = regexp.MustCompile(`(github\.com/gofiber/fiber/)(v\d+)( *?)(v[\w.-]+)`) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden go.mod regex: allow tabs/any whitespace and +incompatible/build metadata.
( *?) won’t catch tabs, and v[\w.-]+ misses + (e.g., v3.0.0+incompatible). Widen the character class and whitespace to ensure robust replacement across real-world go.mod lines.
Apply this diff:
- pkgRegex = regexp.MustCompile(`(github\.com/gofiber/fiber/)(v\d+)( *?)(v[\w.-]+)`)
+ pkgRegex = regexp.MustCompile(`(github\.com/gofiber/fiber/)(v\d+)(\s*)(v[\w.+-]+)`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pkgRegex = regexp.MustCompile(`(github\.com/gofiber/fiber/)(v\d+)( *?)(v[\w.-]+)`) | |
| pkgRegex = regexp.MustCompile(`(github\.com/gofiber/fiber/)(v\d+)(\s*)(v[\w.+-]+)`) |
🤖 Prompt for AI Agents
In cmd/internal/migrations/common.go around line 17, the current pkgRegex uses
`( *?)` which won't match tabs/other whitespace and `v[\w.-]+` which omits `+`
(needed for versions like v3.0.0+incompatible); update the regex to accept any
whitespace (e.g., `\s*?` or `\s+?`) and include `+` in the version character
class (e.g., `v[\w.+-]+`) so the pattern becomes robust for real-world go.mod
lines.
2 participants
Summary
MigrateGoPkgsMigrateFiberCoreImportsmigratorTesting
go test ./...https://chatgpt.com/codex/tasks/task_e_68a8d64d2edc83268d262916712862b3
Summary by CodeRabbit
New Features
Bug Fixes
Tests