Add migrator for swagger packages#226
Conversation
ReneWerner87
requested review from
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 introduces a new migration tool designed to streamline the update process for projects using Fiber's Swagger-related packages. It automates the transition of old Swagger import paths and 'go.mod' dependencies to their new 'gofiber/contrib/v3' equivalents, ensuring consistency and proper versioning. This change simplifies maintenance and helps users keep their projects aligned with the latest Fiber ecosystem structure. 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 a migrator for Fiber swagger packages, updating import paths and go.mod entries to their new contrib/v3 equivalents. It also adds a utility for fetching module versions from the Go proxy, complete with caching and test support. The changes are well-structured and include idempotent tests. My review identified a critical issue in the import rewriting logic that could break user code by incorrectly applying an alias. I have also noted a few medium-severity issues in the new version-fetching utility concerning concurrency, error handling, and performance best practices.
|
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 a v3 migration that rewrites Swagger-related imports and updates go.mod entries, plus a cached, thread-safe Contrib v3 version lookup with a pluggable fetcher and restore function. Tests exercise migration behavior, idempotency, and import preservation. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI Migration
participant Scanner as Workspace Scanner
participant Fetcher as contribV3Version
participant Cache as Version Cache / singleflight
participant Files as File Reader/Writer
participant GoMod as go.mod Updater
User->>CLI: run v3 migration
CLI->>Scanner: scan workspace for go.mod and imports
Scanner->>Scanner: detect swagger dependencies
alt swagger dependency found
Scanner->>Fetcher: request target module version
Fetcher->>Cache: check cache / singleflight
alt cache miss
Cache->>Fetcher: perform HTTP GET to Go proxy
Fetcher-->>Cache: return version
end
Cache-->>Scanner: return resolved version
Scanner->>Files: rewrite Go imports (AST)
Files-->>Scanner: write updated sources
Scanner->>GoMod: update require/replace lines with new path/version
GoMod-->>Scanner: write updated go.mod
Scanner-->>CLI: changes made
else no swagger found
Scanner-->>CLI: no changes
end
CLI-->>User: finish
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 (4)
cmd/internal/migrations/v3/contrib_versions.go (2)
20-38: Potential race condition: double fetch on cache miss.When multiple goroutines call
contribV3Versionfor the same uncached module simultaneously, they all pass the cache check, release the lock, and callfetcher(module)concurrently. This results in redundant HTTP requests and duplicated work.Consider using
sync.MapwithLoadOrStoreor asingleflight.Groupto deduplicate in-flight requests.+import "golang.org/x/sync/singleflight" + +var contribV3VersionGroup singleflight.Group + func contribV3Version(module string) (string, error) { contribV3VersionMu.Lock() if v, ok := contribV3VersionCache[module]; ok { contribV3VersionMu.Unlock() return v, nil } - fetcher := contribV3VersionFetcher contribV3VersionMu.Unlock() - v, err := fetcher(module) - if err != nil { - return "", err + result, err, _ := contribV3VersionGroup.Do(module, func() (interface{}, error) { + contribV3VersionMu.Lock() + fetcher := contribV3VersionFetcher + contribV3VersionMu.Unlock() + return fetcher(module) + }) + if err != nil { + return "", err } + v := result.(string) contribV3VersionMu.Lock() contribV3VersionCache[module] = v contribV3VersionMu.Unlock() return v, nil }
50-50: Reuse a sharedhttp.Clientinstead of creating one per request.Creating a new
http.Client{}for each call prevents connection reuse. Consider using a package-level client orhttp.DefaultClient.- client := &http.Client{} - res, err := client.Do(req) + res, err := http.DefaultClient.Do(req)cmd/internal/migrations/v3/swagger_packages.go (2)
73-76: Consider removing unusedd.Info()call.The
infovariable is only used to retrieveinfo.Mode().Perm()at line 115. You could simplify by usingos.Stat(path)only when writing is needed, or use a fixed permission like0o644since go.mod files typically have standard permissions.- info, err := d.Info() - if err != nil { - return fmt.Errorf("stat %s: %w", path, err) - } - b, err := os.ReadFile(path) // #nosec G304 -- reading module files if err != nil { return fmt.Errorf("read %s: %w", path, err) } ... - if err := os.WriteFile(path, []byte(updated), info.Mode().Perm()); err != nil { + if err := os.WriteFile(path, []byte(updated), 0o644); err != nil { return fmt.Errorf("write %s: %w", path, err) }
179-189: Regex replacement applies twice for new paths, which is redundant but harmless.
reRequireOldreplacesoldPathwithnewPath, thenreRequireNewimmediately replacesnewPathwithnewPathagain. The second replacement is a no-op when the version already matches but incurs unnecessary regex overhead.Consider combining the patterns or conditionally skipping the second replacement if
oldPath != newPathwas already applied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/internal/migrations/lists.go(1 hunks)cmd/internal/migrations/v3/contrib_versions.go(1 hunks)cmd/internal/migrations/v3/swagger_packages.go(1 hunks)cmd/internal/migrations/v3/swagger_packages_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/internal/migrations/lists.go (1)
cmd/internal/migrations/v3/swagger_packages.go (1)
MigrateSwaggerPackages(30-53)
cmd/internal/migrations/v3/swagger_packages.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
cmd/internal/migrations/v3/swagger_packages_test.go (2)
cmd/internal/migrations/v3/contrib_versions.go (1)
SetContribV3VersionFetcher(80-92)cmd/internal/migrations/v3/swagger_packages.go (1)
MigrateSwaggerPackages(30-53)
⏰ 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/contrib_versions.go (1)
78-92: LGTM!The
SetContribV3VersionFetcherfunction correctly handles mutex locking, cache reset, and returns a proper restore function. The design supports clean test isolation viat.Cleanup(restore).cmd/internal/migrations/lists.go (1)
58-59: LGTM!The placement of
MigrateSwaggerPackagesbeforeMigrateContribPackagesis correct. This ensures swagger-specific imports and go.mod entries are migrated before the general contrib package migration runs.cmd/internal/migrations/v3/swagger_packages.go (2)
145-159: Forcingswaggeralias may overwrite intentional custom aliases.When the import already has a non-empty alias (e.g.,
swag "github.com/gofiber/contrib/v3/swaggo"), the current logic skips overwriting. However, for new paths (lines 145-146), it still adds theswaggeralias if missing, which is correct. The logic looks sound.LGTM! The alias enforcement ensures consistent usage across migrated codebases.
30-53: LGTM!The
MigrateSwaggerPackagesfunction cleanly orchestrates import rewriting and go.mod migration, with proper error propagation and conditional logging when changes occur.cmd/internal/migrations/v3/swagger_packages_test.go (2)
3-14: I'll help you verify the review comment about missing helper functions in the test file. Let me search for these functions to confirm whether they're actually referenced and where they're defined.
<function_calls>
#!/bin/bashFirst, let's see the full content of the test file to check for function references
cat cmd/internal/migrations/v3/swagger_packages_test.go
</function_calls>
<function_calls>
#!/bin/bashSearch for the three helper functions in the migrations/v3 directory
echo "=== Searching for writeTempFile ==="
rg -n "writeTempFile" cmd/internal/migrations/v3/ --type goecho -e "\n=== Searching for newCmd ==="
rg -n "newCmd" cmd/internal/migrations/v3/ --type goecho -e "\n=== Searching for readFile ==="
rg -n "readFile" cmd/internal/migrations/v3/ --type go
</function_calls>
112-159: I'll verify the idempotency test expectation by examining the detection logic and caching behavior. Let me search for the relevant code.
<function_calls>
find . -type f -name "*.go" | grep -E "(swagger|v3)" | head -20
</function_calls>Let me search for the migration logic:
<function_calls>
#!/bin/bash
rg -n "needsSwaggo|needsSwaggerUI" --type go -A 2 -B 2
</function_calls>Let me also look for the MigrateSwaggerPackages function and cache implementation:
<function_calls>
#!/bin/bash
rg -n "contribV3Version|SetContribV3VersionFetcher" --type go -A 3 -B 3
</function_calls>Let me search more broadly in the migrations directory:
<function_calls>
#!/bin/bash
fd "swagger" cmd/internal/migrations/v3 --type f
</function_calls>Let me look at the test file and surrounding context:
<function_calls>
#!/bin/bash
cat -n cmd/internal/migrations/v3/swagger_packages_test.go | head -180
</function_calls>Now let me find the implementation file:
<function_calls>
#!/bin/bash
ls -la cmd/internal/migrations/v3/ | grep -E ".go$"
</function_calls>Let me examine the swagger_packages.go file:
<function_calls>
#!/bin/bash
wc -l cmd/internal/migrations/v3/swagger_packages.go
</function_calls><function_calls>
#!/bin/bash
cat -n cmd/internal/migrations/v3/swagger_packages.go
</function_calls>Perfect! Now let me look for how the modules are detected and the cache mechanism:
<function_calls>
#!/bin/bash
rg -n "var contribV3Version" --type go -A 5 -B 5
</function_calls>Let me search more specifically in the v3 directory:
<function_calls>
#!/bin/bash
rg -n "contribV3Version|SetContribV3VersionFetcher" cmd/internal/migrations/v3/ --type go -A 5
</function_calls>Let me check what files exist in the v3 migrations directory:
<function_calls>
#!/bin/bash
ls -la cmd/internal/migrations/v3/
</function_calls>Let me examine the main.go file:
<function_calls>
#!/bin/bash
cat -n cmd/internal/migrations/v3/main.go
</function_calls>Now let me trace through the logic. Let me look for where contribV3Version is used:
<function_calls>
#!/bin/bash
rg -n "contribV3VersionFetcher|contribV3Version" cmd/internal/migrations/ --type go -B 2 -A 2
</function_calls>Let me search in the entire codebase:
<function_calls>
#!/bin/bash
rg -n "contribV3Version" --type go -B 2 -A 2
</function_calls>Let me look at the internal migrations directory structure:
<function_calls>
#!/bin/bash
find cmd/internal/migrations -type f -name "*.go" | sort
</function_calls>Let me check the v3 directory more carefully:
<function_calls>
#!/bin/bash
wc -l cmd/internal/migrations/v3/*.go
</function_calls>Let me read helper.go:
<function_calls>
#!/bin/bash
cat -n cmd/internal/migrations/v3/helper.go
</function_calls>Perfect! Now let me trace the logic. Let me look at how the modules are detected in MigrateSwaggerPackages:
<function_calls>
#!/bin/bash
sed -n '1,150p' cmd/internal/migrations/v3/swagger_packages.go | cat -n
</function_calls><function_calls>
#!/bin/bash
sed -n '151,300p' cmd/internal/migrations/v3/swagger_packages.go | cat -n
</function_calls>Now I understand the flow. Let me trace through the test logic more carefully by examining what happens:
Let me verify the exact cache logic:
<function_calls>
#!/bin/bash
sed -n '20,45p' cmd/internal/migrations/v3/helper.go | cat -n
</function_calls>Excellent! Now I have all the information I need. Let me verify my understanding with one more check:
<function_calls>
#!/bin/bashVerify that needsSwaggo and needsSwaggerUI are based on both imports AND go.mod
sed -n '40,90p' cmd/internal/migrations/v3/swagger_packages.go | cat -n
</function_calls>Perfect! Now I have all the information needed.
Now I have all the information to provide my final response. The verification shows that the review comment's concern is not valid.
The test expectation of
calls == 2is correct; the cache persists across both migration runs.The
contribV3Versionis a package-scoped map variable inhelper.go(line 3-4) that persists throughout the test. WhengetContribV3Version()is called (line 25-35 ofhelper.go), it checks the cache first; if a version is already cached, it returns without calling the fetcher function.In the test:
- First call: Both
swaggoandswaggeruiare needed (swaggo is imported in the file, swaggerui is listed in go.mod), so both versions are fetched via the fetcher →calls = 2- Second call: Both modules are still needed, but their versions are already in the cache, so the fetcher is not called →
callsremains 2The detection logic correctly identifies both modules as needed (lines 82-106 of
swagger_packages.go): modules are considered needed if they appear in imports OR in go.mod.
ReneWerner87
added
the
🤖 Dependencies
|
/gemini review |
|
@ReneWerner87: I'll review the changes in this PR right away. ✅ Actions performedReview triggered.
|
|
Hello @ReneWerner87, Thanks for the prompt! I've completed my review of your pull request. This is a well-structured and thoroughly tested migration, and I appreciate the attention to detail. Summary of ReviewThis pull request introduces a robust migration tool for updating Fiber projects that use Swagger-related packages. The core functionality involves:
Overall, the changes are well-implemented and demonstrate a good understanding of Go's tooling for code manipulation and dependency management. The tests are particularly strong, covering various scenarios and ensuring the reliability of the migration. Review CommentsI have a few minor suggestions, primarily focused on maintainability and best practices, which you can find in the inline comments. These are mostly Style GuideFor this review, I've adhered to common Go programming practices and conventions, as outlined in Effective Go and Go CodeReviewComments. Let me know if you have any questions! 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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/swagger_packages_test.go (1)
162-205: Strict no-op preservation test is useful; fetcher setup could be DRY’d upThe exact equality checks on both source and go.mod plus empty log output give a strong guarantee that already‑correct contrib/v3 usage is left untouched. That’s a good complement to the broader idempotency test.
If you ever touch this area again, you might consider a tiny helper to reduce the repeated
SetContribV3VersionFetcher/t.Cleanupboilerplate across the four tests (e.g., passing in a map of module→version). Not urgent, but could tighten things up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (3)
cmd/internal/migrations/v3/contrib_versions.go(1 hunks)cmd/internal/migrations/v3/swagger_packages.go(1 hunks)cmd/internal/migrations/v3/swagger_packages_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/internal/migrations/v3/contrib_versions.go
- cmd/internal/migrations/v3/swagger_packages.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/swagger_packages_test.go (2)
cmd/internal/migrations/v3/contrib_versions.go (1)
SetContribV3VersionFetcher(95-107)cmd/internal/migrations/v3/swagger_packages.go (1)
MigrateSwaggerPackages(30-53)
🔇 Additional comments (4)
cmd/internal/migrations/v3/swagger_packages_test.go (4)
1-19: Good external test structure and shared module constantsUsing
package v3_testwith sharedswaggoModule/swaggerUIModuleconstants gives nice black‑box coverage of the migration API across all tests. No issues here.
21-64: Contrib → v3/swaggo migration test looks solidThis test cleanly validates the contrib/swagger → contrib/v3/swaggo flow including aliasing, go.mod version/replace updates, and the migration log line. It’s a good end‑to‑end check of both import rewriting and module migration.
66-110: Fiber Swagger → contrib/v3/swaggerui coverage is comprehensiveThe test fully exercises the fiber/swagger migration path: import re‑targeting with alias preservation, go.mod update to contrib/v3/swaggerui at the fetched version, and confirmation of the user‑visible log. Nicely complements the first test.
112-160: Idempotency and contrib version cache behavior well coveredThis test nicely proves that:
- Running the migration multiple times is a no‑op when things are already on contrib/v3.
- No extra log noise is produced on no‑change runs.
- The contrib v3 version lookup is cached across runs (one real fetch per module, as asserted by
calls == 2).Good guard against regressions in both the migrator and the caching layer.
1 participant
Summary
Testing
Codex Task
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.