Add Fiber v3 migrations for AllParams and SendFile changes#221
Conversation
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 enhances the Fiber v3 migration utility by introducing new rules to automatically update deprecated API usages. It specifically addresses the transition from 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
|
|
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 AllParams handling to the parser-method migration and introduces MigrateSendFileConfig, a new migration that rewrites Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant MigrateSendFileConfig
participant ChangeFileContent
participant FileParser
participant Logger
CLI->>MigrateSendFileConfig: invoke migration (cwd)
MigrateSendFileConfig->>ChangeFileContent: scan & edit files
ChangeFileContent->>FileParser: detect `.SendFile` calls
alt SendFile patterns found
FileParser->>FileParser: validate args & compress value
FileParser->>ChangeFileContent: rewrite to fiber.SendFile{Compress: ...}
ChangeFileContent-->>MigrateSendFileConfig: report changes
MigrateSendFileConfig->>Logger: print "Migrating SendFile compress flag"
else No matches or errors
ChangeFileContent-->>MigrateSendFileConfig: nil or error
end
MigrateSendFileConfig-->>CLI: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![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 updates the Fiber v3 migrations to rewrite Ctx.AllParams calls to the new Bind().URI helper and adds a SendFile migration that maps the old boolean compress argument onto the Fiber v3 fiber.SendFile config struct. The changes look good and the tests cover the new functionality.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
![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
🧹 Nitpick comments (2)
cmd/internal/migrations/v3/sendfile_config.go (1)
13-43: Consider validating the boolean argument before transformation.The migration transforms the second argument directly into
Compress: %swithout validating that it's a boolean-like value. While this is likely acceptable for a migration tool (the Go compiler will catch type errors), consider whether additional validation or a warning would help catch malformed calls during migration.cmd/internal/migrations/v3/sendfile_config_test.go (1)
14-40: LGTM! Consider adding edge case tests.The test effectively validates the core migration functionality with both a boolean literal and a variable reference. The assertions verify both code transformation and log output.
Consider adding test cases for edge scenarios:
- SendFile calls that should not be migrated (already using fiber.SendFile{})
- SendFile with wrong argument count
- SendFile with empty second argument
These would strengthen confidence that the guards on lines 24-29 of sendfile_config.go work correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/internal/migrations/v3/parser_methods.go(2 hunks)cmd/internal/migrations/v3/parser_methods_test.go(1 hunks)cmd/internal/migrations/v3/sendfile_config.go(1 hunks)cmd/internal/migrations/v3/sendfile_config_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/internal/migrations/v3/sendfile_config_test.go (1)
cmd/internal/migrations/v3/sendfile_config.go (1)
MigrateSendFileConfig(13-43)
cmd/internal/migrations/v3/sendfile_config.go (1)
cmd/internal/helpers.go (1)
ChangeFileContent(54-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, macos-13)
🔇 Additional comments (3)
cmd/internal/migrations/v3/parser_methods_test.go (1)
29-29: LGTM! AllParams test coverage added correctly.The test now validates that
c.AllParams(&p)is properly migrated to.Bind().URI(&p), which is consistent with the ParamsParser migration pattern.Also applies to: 43-43
cmd/internal/migrations/v3/parser_methods.go (1)
17-17: LGTM! AllParams migration implemented correctly.The extension properly adds AllParams to the detection regex and maps it to
.Bind().URI(), which is consistent with the ParamsParser migration and semantically correct for Fiber v3.Also applies to: 52-53
cmd/internal/migrations/v3/sendfile_config.go (1)
15-32: ThereplaceCallhelper function is properly defined and accessible.The
replaceCallfunction is defined atcmd/internal/migrations/v3/common.go:330with the correct signature to support the usage pattern in the provided code snippet. No issues identified.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/internal/migrations/v3/sendfile_config_test.go (3)
21-30: Also assert the old calls are removed (stronger proof of rewrite)Add negative assertions to ensure boolean/variable-arg forms no longer exist after migration.
@@ content := readFile(t, file) assert.Contains(t, content, `c.SendFile("./public/index.html", fiber.SendFile{Compress: true})`) assert.Contains(t, content, `c.SendFile("./public/file.txt", fiber.SendFile{Compress: compressEnabled})`) + assert.NotContains(t, content, `c.SendFile("./public/index.html", true)`) + assert.NotContains(t, content, `c.SendFile("./public/file.txt", compressEnabled)`) assert.Contains(t, buf.String(), "Migrating SendFile compress flag")Also applies to: 36-39
71-71: Add skip-case coverage (already-configured, address-of, unqualified, alias, single-arg)These cases should remain unchanged and produce no log output. The alias-qualified case (e.g., fb.SendFile{...}) currently isn’t excluded by the migration and would surface a correctness gap; add tests and adjust the migration accordingly (see next comment).
@@ func Test_MigrateSendFileConfig_Idempotent(t *testing.T) { ... } + +func Test_MigrateSendFileConfig_SkipCases(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + src string + }{ + { + name: "already_configured_fiber_qualified", + src: `package main +import "github.com/gofiber/fiber/v2" +func handler(c fiber.Ctx) error { + return c.SendFile("./public/index.html", fiber.SendFile{Compress: true}) +} +`, + }, + { + name: "already_configured_address_of", + src: `package main +import "github.com/gofiber/fiber/v2" +func handler(c fiber.Ctx) error { + return c.SendFile("./public/index.html", &fiber.SendFile{Compress: true}) +} +`, + }, + { + name: "already_configured_unqualified", + src: `package main +import "github.com/gofiber/fiber/v2" +func handler(c fiber.Ctx) error { + return c.SendFile("./public/index.html", SendFile{Compress: true}) +} +`, + }, + { + name: "already_configured_alias_qualified", + src: `package main +import fb "github.com/gofiber/fiber/v3" +func handler(c fb.Ctx) error { + return c.SendFile("./public/index.html", fb.SendFile{Compress: true}) +} +`, + }, + { + name: "single_arg_call_no_change", + src: `package main +import "github.com/gofiber/fiber/v2" +func handler(c fiber.Ctx) error { + return c.SendFile("./public/index.html") +} +`, + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + dir, err := os.MkdirTemp("", "msendfile-skip") + require.NoError(t, err) + defer func() { require.NoError(t, os.RemoveAll(dir)) }() + + file := writeTempFile(t, dir, tc.src) + var buf bytes.Buffer + cmd := newCmd(&buf) + require.NoError(t, v3.MigrateSendFileConfig(cmd, dir, nil, nil)) + + migrated := readFile(t, file) + assert.Equal(t, tc.src, migrated) + assert.Empty(t, buf.String()) + }) + } +}
3-12: Minor: clarify v2 import in fixture is intentionalThe fixture imports fiber/v2 while asserting v3-style config in the rewrite. If intentional (string-level migration), add a short comment to avoid future confusion. Alternatively, add an integration test that chains the v3 import migration to ensure the final code compiles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/internal/migrations/v3/sendfile_config_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/sendfile_config_test.go (1)
cmd/internal/migrations/v3/sendfile_config.go (1)
MigrateSendFileConfig(13-43)
⏰ 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-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-13)
🔇 Additional comments (2)
cmd/internal/migrations/v3/sendfile_config_test.go (2)
14-20: Solid test skeleton and isolationGood use of t.Parallel(), temp dirs, and require/assert separation. Setup/teardown is clean.
42-71: Idempotency coverage looks goodVerifies unchanged content and silent second run. Nicely done.
| content := readFile(t, file) | ||
| assert.Contains(t, content, `c.SendFile("./public/index.html", fiber.SendFile{Compress: true})`) | ||
| assert.Contains(t, content, `c.SendFile("./public/file.txt", fiber.SendFile{Compress: compressEnabled})`) | ||
| assert.Contains(t, buf.String(), "Migrating SendFile compress flag") |
There was a problem hiding this comment.
Alias-qualified SendFile literals are not excluded; migration may double-wrap (bug)
The migration skips only when arg starts with “fiber.SendFile”/“SendFile{” (and address-of). It misses alias-qualified forms like “fb.SendFile{...}”, causing an incorrect rewrite. Suggest a regex guard that accepts optional package alias.
Proposed fix in cmd/internal/migrations/v3/sendfile_config.go:
@@
-import (
- "fmt"
- "strings"
-)
+import (
+ "fmt"
+ "regexp"
+ "strings"
+)
@@
- arg := strings.TrimSpace(args[1])
- if arg == "" {
- return call
- }
- if strings.HasPrefix(arg, "fiber.SendFile") || strings.HasPrefix(arg, "&fiber.SendFile") {
- return call
- }
- if strings.HasPrefix(arg, "SendFile{") || strings.HasPrefix(arg, "&SendFile{") {
- return call
- }
+ arg := strings.TrimSpace(args[1])
+ if arg == "" {
+ return call
+ }
+ // Matches: SendFile{...}, &SendFile{...}, fiber.SendFile{...}, &fiber.SendFile{...}, fb.SendFile{...}, &fb.SendFile{...}
+ var sendFileCfgRe = regexp.MustCompile(`^&?\s*(?:[A-Za-z_]\w*\.)?SendFile\s*\{`)
+ if sendFileCfgRe.MatchString(arg) {
+ return call
+ }Add/extend tests with the alias-qualified case (see previous comment). Based on learnings.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cmd/internal/migrations/v3/sendfile_config_test.go around lines 36 to 39, the
test and migration logic miss alias-qualified SendFile literals (e.g.,
fb.SendFile{...}) causing double-wrapping; update the migration guard in
cmd/internal/migrations/v3/sendfile_config.go to accept an optional package
alias prefix (allowing patterns like `(\w+\.)?SendFile` or optional `\w+\.`
before `SendFile`), and add a unit test case in sendfile_config_test.go that
verifies alias-qualified literals (e.g., `fb.SendFile{Compress: true}`) are
recognized/skipped so the migration does not rewrite them incorrectly.
1 participant
Summary
Ctx.AllParamscalls to the newBind().URIhelperfiber.SendFileconfig structTesting
https://chatgpt.com/codex/tasks/task_e_68f21d2d362c8326999b9df6034d13cd
Summary by CodeRabbit
New Features
Tests