feat(migrator): enhance client usage migration #252
Conversation
…er imports and handling deprecated methods
ReneWerner87
requested review from
efectn,
gaby and
sixcolors
and removed request for
a team
Summary of ChangesHello @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 significantly enhances the 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 enhances the client usage migration script by adding support for more V2 patterns, such as agent.Debug(), agent.Reuse(), and removing fiber.ReleaseAgent() calls. It also introduces logic to remove the fiber package import when it's no longer used after migration. The changes are well-tested with several new test cases covering various scenarios.
My main feedback is regarding the implementation of the unused import removal, which is not robust enough to handle all cases. I've left a specific comment with a suggestion to improve it. Otherwise, the changes look good and improve the utility of the migration tool.
There was a problem hiding this comment.
Pull request overview
This PR enhances the client usage migration functionality by adding support for migrating Debug() and Reuse() method calls, removing defer fiber.ReleaseAgent() statements, and automatically removing unused fiber imports after migration. The changes ensure a smoother migration path from fiber v2/v3 client APIs to the new v3 client package.
Key Changes:
- Added patterns to detect and handle
Debug()andReuse()method calls (removed during migration as they don't exist in v3) - Added logic to remove
defer fiber.ReleaseAgent()statements during AcquireAgent migration - Implemented automatic removal of unused fiber imports when all client usage has been migrated to the client package
- Added comprehensive test coverage for the new functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/internal/migrations/v3/client_usage.go | Added regex patterns for Debug/Reuse/ReleaseAgent detection, implemented skipLines logic to remove ReleaseAgent defers, added debug field to simpleAgentConfig struct, and implemented removeUnusedFiberImport function to clean up imports |
| cmd/internal/migrations/v3/client_usage_test.go | Added four new test cases covering Debug/Reuse handling, fiber import removal when unused, fiber import preservation when still used, and AcquireAgent migration without Parse block |
💡 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. WalkthroughAdds alias-aware Fiber v3 client-usage migrations: per-alias pattern builders and rewrite entrypoints, import-alias detection/removal, per-alias agent→client rewrites, and updated error/variable initialization; expands tests for alias/import/migration scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40–75 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 0
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/client_usage.go (1)
259-296: Potential nil pointer dereference in bytesMatch and stringMatch cases.Unlike
rewriteSimpleAgentBlocks(which wraps status/body access inif err == nil {}), these cases unconditionally callresp.StatusCode()andresp.Body()after the error block. If the original code's error handler doesn't return/panic (e.g., just logs), the migrated code could panic when accessing a nilresp.Consider wrapping status/body access for consistency with
buildSimpleAgentReplacement:respLine := fmt.Sprintf("%sresp, err := client.%s(%s%s)", indent, methodName, uriExpr, configLine) out = append(out, respLine) out = append(out, parseIndent+"if err != nil {") out = append(out, parseBody[:len(parseBody)-1]...) out = append(out, parseIndent+"}") - out = append(out, fmt.Sprintf("%s%s %s resp.StatusCode()", indent, statusVar, assignOp)) - out = append(out, fmt.Sprintf("%s%s %s resp.Body()", indent, bodyVar, assignOp)) + out = append(out, fmt.Sprintf("%sif err == nil {", indent)) + out = append(out, fmt.Sprintf("%s\t%s %s resp.StatusCode()", indent, statusVar, assignOp)) + out = append(out, fmt.Sprintf("%s\t%s %s resp.Body()", indent, bodyVar, assignOp)) + out = append(out, fmt.Sprintf("%s}", indent))
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/client_usage.go (1)
383-386: Thedebugfield is set but never used.The
debugfield is added to trackDebug()calls but is never read fromcfg.debuginbuildSimpleConfigor elsewhere. While intentional per the comment (v3 uses hooks), consider either:
- Removing the field entirely if it's not needed
- Adding a TODO comment to output migration guidance when debug usage is detected
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/client_usage.go(10 hunks)cmd/internal/migrations/v3/client_usage_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(63-85)
⏰ 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-13)
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, windows-latest)
🔇 Additional comments (8)
cmd/internal/migrations/v3/client_usage.go (4)
91-104: First-pass ReleaseAgent removal looks clean.The two-pass approach using a
skipLinesmap to mark defer ReleaseAgent lines for removal before the main traversal is a good pattern. This avoids complex lookahead during the main rewrite loop.
483-491: Debug/Reuse pattern handling is correctly implemented.The patterns are matched and silently skipped without breaking the migration flow. The inline comments explaining the v3 behavior (hooks for debug, not needed for reuse) provide helpful context.
810-822: The extractImportSection helper is straightforward.Handles both block and single import formats. Only returns the first import section if multiple exist, but this is an edge case unlikely to cause issues in practice.
787-808: Aliased import concern is theoretical—no aliased fiber imports exist in the codebase.The function correctly handles fiber import removal for the actual patterns used in this codebase. A search across all files found only standard unaliased imports (
import "github.com/gofiber/fiber/v2"andimport "github.com/gofiber/fiber/v3"), with no instances of aliased fiber imports likeimport f "github.com/gofiber/fiber/v3"that would trigger the described limitation. The current implementation is appropriate for the migration tool's needs.cmd/internal/migrations/v3/client_usage_test.go (4)
664-723: Comprehensive test for Debug/Reuse removal.Good coverage of the Debug() and Reuse() method removal. The test correctly verifies that both methods are silently stripped from the migrated code and the fiber import is replaced with the client import.
725-782: Good coverage for unused fiber import removal.Properly tests the scenario where fiber is only used for client methods that get migrated away, confirming the import is removed.
784-844: Important edge case coverage for fiber import retention.This test correctly validates that fiber imports are preserved when other fiber identifiers (like
fiber.StatusOK) remain in use after client migration. Good complementary test to the removal case.
905-913: Test documents potential nil pointer risk after non-fatal error handling.The expected output shows
resp.StatusCode()andresp.Body()being called after a non-fatal error handler (just logs, doesn't return). Iferr != nil,respcould be nil, causing a panic. This test documents the current behavior but the migration code should ideally wrap these inif err == nil {}as noted in the implementation review.
…liases and removing unused fiber imports
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/internal/migrations/v3/client_usage_internal_test.go (1)
72-77: Variable shadowing in expected output may cause confusion.The expected output re-declares
retCodeandretBody(lines 72-73), which shadows the outer variables declared in thevarblock (lines 62-63). While this is intentional to avoid nil-pointer dereference, it means the outerretCodeandretBodyremain zero-values. The assignments at lines 85-86 (_ = retCode,_ = retBody) will use the outer (unassigned) variables, not the inner ones.If this is the intended behavior for migrated code, consider adding a brief comment in the test explaining this trade-off. Otherwise, the migration logic may need adjustment to avoid shadowing.
cmd/internal/migrations/v3/client_usage.go (2)
859-891: Consider deduplicating aliases.If a file somehow has both non-aliased import in a block and as a single-line import (unlikely but possible), "fiber" would be added twice to the slice. This could cause redundant processing. Consider using a map or set to deduplicate.
func findFiberImportAliases(content string) []string { - var aliases []string + seen := make(map[string]struct{}) + var aliases []string // Pattern for aliased import in block... if matches := aliasedBlockPattern.FindAllStringSubmatch(content, -1); matches != nil { for _, m := range matches { - aliases = append(aliases, m[1]) + if _, ok := seen[m[1]]; !ok { + seen[m[1]] = struct{}{} + aliases = append(aliases, m[1]) + } } } // Pattern for non-aliased import in block... if nonAliasedBlockPattern.MatchString(content) { - aliases = append(aliases, "fiber") + if _, ok := seen["fiber"]; !ok { + seen["fiber"] = struct{}{} + aliases = append(aliases, "fiber") + } } // ... similar for other patterns
907-918: Redundant regex patterns.The two regex patterns on lines 910 and 914 are effectively identical—the
s(dotall) flag in(?ms)has no effect since there's no.in the pattern. The second replacement is unnecessary.func cleanupEmptyImportBlock(content string) string { // Remove import blocks that only contain whitespace/newlines emptyBlockPattern := regexp.MustCompile(`(?m)^import\s*\(\s*\)\s*\n?`) content = emptyBlockPattern.ReplaceAllString(content, "") - - // Remove import blocks with only whitespace between parentheses - emptyBlockWithWhitespace := regexp.MustCompile(`(?ms)^import\s*\(\s*\)\s*\n?`) - content = emptyBlockWithWhitespace.ReplaceAllString(content, "") - return content }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/internal/migrations/v3/client_usage.go(11 hunks)cmd/internal/migrations/v3/client_usage_internal_test.go(2 hunks)cmd/internal/migrations/v3/client_usage_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/internal/migrations/v3/client_usage.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(71-109)
🪛 GitHub Actions: golangci-lint
cmd/internal/migrations/v3/client_usage.go
[error] 111-111: golangci-lint: File is not properly formatted (gofumpt).
🪛 GitHub Check: lint
cmd/internal/migrations/v3/client_usage.go
[failure] 111-111:
File is not properly formatted (gofumpt)
⏰ 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). (3)
- 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 (7)
cmd/internal/migrations/v3/client_usage.go (3)
843-848: Import removal is conservative but safe.The logic returns early if any fiber alias is still used, which means files with multiple aliases (e.g., both
fiberandf) won't have unused aliases removed here. This is acceptable sinceimports.Processis called inChangeFileContent(seecmd/internal/helpers.go:89-92) which will clean up genuinely unused imports. No change needed.
53-67: LGTM!The
buildAliasPatternsfunction correctly escapes the alias usingregexp.QuoteMetaand builds comprehensive patterns for various Fiber client usage patterns.
270-288: Defensive variable initialization prevents nil dereference.The pattern of declaring variables and assigning them only within
if err == nilblock correctly prevents nil-pointer dereference onresp. This addresses the past review concern about potential NPE.Note: This introduces variable shadowing when original code had outer declarations (like
var retCode intin avarblock). The inner declarations will shadow the outer ones. This is a reasonable trade-off for safety, and users can manually consolidate declarations post-migration if needed.cmd/internal/migrations/v3/client_usage_test.go (4)
325-330: Expected output reflects variable shadowing pattern.The expected output correctly shows the new variable declarations (
var retCode int,var retBody []byte) that shadow the outer declarations in thevarblock. This is consistent with the implementation's nil-safety approach. The test validates that the migration produces this pattern.
670-729: Good test coverage for Debug() and Reuse() handling.This test verifies that the migration correctly removes
Debug()andReuse()calls that don't exist in v3, while still properly migrating the client usage. The expected output correctly shows these method calls are stripped.
984-1038: Excellent coverage for aliased import migration.This test validates the alias-aware migration with a custom alias (
f). It correctly expects the aliased fiber import to be removed when no longer used, while the client import is added. Good edge case coverage.
852-930: Real-world test case validates the migration correctly.This test mirrors the exact user scenario from the PR and validates that the AcquireAgent/Parse/Bytes flow is correctly migrated to the client-based approach with proper error handling and nil-safety.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/internal/migrations/v3/client_usage.go (1)
111-111: Fix gofumpt formatting issue.The function parameters should be on the same line per gofumpt conventions.
Apply this diff:
-func rewriteAcquireAgentBlocksWithAlias(content, alias string) (string, bool) { +func rewriteAcquireAgentBlocksWithAlias(content, alias string) (string, bool) {
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/client_usage.go (1)
899-910: Remove redundant regex pattern.The pattern on line 906 is redundant—it adds the
sflag (making.match newlines) but the pattern doesn't use., so it matches the same strings as line 902. The first pattern alone is sufficient.Apply this diff:
func cleanupEmptyImportBlock(content string) string { // Remove import blocks that only contain whitespace/newlines emptyBlockPattern := regexp.MustCompile(`(?m)^import\s*\(\s*\)\s*\n?`) content = emptyBlockPattern.ReplaceAllString(content, "") - - // Remove import blocks with only whitespace between parentheses - emptyBlockWithWhitespace := regexp.MustCompile(`(?ms)^import\s*\(\s*\)\s*\n?`) - content = emptyBlockWithWhitespace.ReplaceAllString(content, "") return content }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/internal/migrations/v3/client_usage.go(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/client_usage.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, macos-13)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, macos-latest)
🔇 Additional comments (11)
cmd/internal/migrations/v3/client_usage.go (11)
18-24: LGTM! Error pattern definitions are well-structured.The new error handling patterns correctly target the migration from slice-based error handling (
errs []error) to single error handling (err error), which aligns with the v3 client API changes.
53-67: LGTM! Alias-aware pattern builder is well-designed.The function correctly uses
regexp.QuoteMetato escape the alias and creates comprehensive patterns for all client usage scenarios. This enables proper migration regardless of the import alias used.
71-109: LGTM! Migration flow properly handles multiple aliases.The updated logic correctly discovers import aliases, applies per-alias migrations, and cleans up unused imports. The default to
["fiber"]is a reasonable fallback that won't cause issues since rewrites only match when patterns match.
119-132: LGTM! Two-pass removal strategy is correct.The logic properly marks
ReleaseAgentdefer lines for removal in the first pass and skips them in the second pass, ensuring clean removal of obsolete defer statements.
266-280: Excellent nil-safety handling!The pattern of declaring variables before the
if err == nilblock and assigning values only whenerr == nilcorrectly prevents nil pointer dereferences whenrespis nil. This is a critical improvement over immediate assignment patterns.
298-304: Consistent nil-safety pattern across all cases.The nil-safety handling is consistently applied to bytes (lines 299-304) and string (lines 319-324) cases, matching the struct case pattern. This consistency improves code maintainability.
Also applies to: 318-324
519-526: LGTM! Correctly skips obsolete method calls.The code properly skips
Debug()andReuse()calls since they're not supported/needed in v3. This addresses the previous feedback about setting unused debug fields.
820-847: Robust import removal logic!This implementation properly addresses the previous concern about import removal robustness. It correctly:
- Discovers all import aliases
- Checks for actual usage outside imports
- Removes imports only when truly unused
This is a significant improvement over simple pattern matching.
849-883: LGTM! Comprehensive alias discovery.The function handles all import variations:
- Aliased/non-aliased imports in blocks
- Single-line aliased/non-aliased imports
- Correct default of "fiber" for non-aliased imports
885-897: LGTM! Import removal handles all formats.The function correctly removes fiber imports in both single-line and block formats, with or without aliases, using optional matching groups.
912-925: LGTM! Import section extraction is correct.The function properly extracts both import blocks and single-line imports (with optional aliases), providing good support for the usage checking logic.
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.