test: cover multiple context migrations#198
Conversation
|
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. Warning Rate limit exceeded@ReneWerner87 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughExpands the v3 context migration to detect chained receiver calls before Context() and adjust replacements to preserve the chain for Fiber by converting the chain’s suffix to RequestCtx(). Adds tests covering chained and multiple Context() occurrences and verifying migration logs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer Code
participant Migr as MigrateContextMethods
participant Rx as Regex Matcher
participant Repl as Replacer
Dev->>Migr: Provide source with chained calls (e.g., foo.Bar().Context())
Migr->>Rx: Scan for chains ending in .Context()
Rx-->>Migr: Matches with root+chain (non-greedy)
Migr->>Repl: If Fiber receiver, replace suffix .Context() -> .RequestCtx()
Repl-->>Migr: Transformed source (foo.Bar().RequestCtx())
Migr-->>Dev: Output migrated code + log entry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
✨ 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 extends the context migration tool to handle more complex scenarios involving multiple and chained .Context() invocations, ensuring a more robust and complete migration from Context() to RequestCtx(). This is primarily achieved by updating the regular expression used for matching and adding comprehensive test cases to validate the new logic.
Highlights
- Enhanced Context Migration Logic: The regular expression used for migrating
Context()method calls has been updated to correctly identify and replace instances within chained method calls (e.g.,obj.Method().Context()). - Improved Migration Replacement: The replacement logic for
Context()calls now intelligently preserves any preceding method calls in a chain, ensuring only theContext()part is transformed to `RequestCtx(). - New Test Cases for Chained Calls: A new test,
Test_MigrateContextMethods_RequestCtxChained, has been added to verify the migration tool's ability to handleContext()calls that are part of a method chain. - New Test Cases for Multiple Invocations: A new test,
Test_MigrateContextMethods_MultipleContextCalls, has been introduced to ensure the migration tool correctly handles scenarios whereContext()is invoked multiple times within the same code block, including a mix of standalone and chained calls.
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 extends the context migration logic to handle chained method calls before .Context(), which is a good improvement. The changes are supported by new test cases. However, I've identified a potential issue with the regular expression used for matching, which could lead to silent failures during migration in more complex scenarios. My review includes one high-severity comment detailing this concern.
![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 (4)
cmd/internal/migrations/v3/context_methods.go (2)
50-50: Broaden the regex to tolerate whitespace, newlines, and generic type arguments in chained callsThe current pattern won’t match multi-line fluent chains or methods with type arguments (e.g.,
.Map[K,V](...)). Consider a more permissive RE2 pattern to reduce false negatives.Apply this diff:
- reReqCtx := regexp.MustCompile(`(\w+)(?:\.[\w]+\([^)]*\))*\.Context\(\)`) + // Matches: <ident>(.<Method[TypeArgs]>(...))* .Context() + // - Allows optional spaces/newlines around dots and calls + // - Allows optional generic type arguments after method names + // Note: Still heuristic; nested parentheses in args are not handled by regex. + reReqCtx := regexp.MustCompile(`\b([A-Za-z_]\w*)(?:\s*\.\s*[A-Za-z_]\w*(?:\[[^\]]*\])?\s*\([^()]*\))*\s*\.\s*Context\(\)`)
50-58: Avoid accidental rewrites inside strings/comments; consider AST-driven migration or tighter guardsRegex-based rewriting can still hit string literals or comments that contain “.Context()”. If feasible, prefer parsing with go/parser to find CallExpr whose Fun is a SelectorExpr with Sel.Name == "Context" and whose X resolves to a Fiber Ctx symbol. If AST is out of scope, minimally extend tests to assert we don’t rewrite within quotes/comments.
Would you like me to open a follow-up to prototype an AST-based rewriter, or add non-code (string/comment) fixture tests that must remain unchanged?
cmd/internal/migrations/v3/context_methods_test.go (2)
103-106: Good guard: non-Fiber types remain untouchedVerifies we don’t rewrite look-alikes on foreign types. Consider adding a chained non-Fiber call (e.g.,
x.Foo().Context()) to strengthen this guarantee.
149-173: Good multi-occurrence coverage; consider adding multi-line and generic casesCovers multiple occurrences on one file including a chain. To further harden against false negatives:
- Multi-line chain:
c.\n Status(...).\n Context().SetBody...- Generic method in chain (if present in your codebase):
c.Map[string,string](m).Context()...Optionally add tests like:
+func Test_MigrateContextMethods_RequestCtxChainedMultiline(t *testing.T) { + t.Parallel() + dir, err := os.MkdirTemp("", "mcmtestchainml") + require.NoError(t, err) + defer func() { require.NoError(t, os.RemoveAll(dir)) }() + file := writeTempFile(t, dir, `package main +import "github.com/gofiber/fiber/v2" +func handler(c fiber.Ctx) error { + c. + Status(fiber.StatusOK). + Context(). + SetBodyStreamWriter(nil) + return nil +}`) + var buf bytes.Buffer + cmd := newCmd(&buf) + require.NoError(t, v3.MigrateContextMethods(cmd, dir, nil, nil)) + content := readFile(t, file) + assert.Contains(t, content, ".RequestCtx().SetBodyStreamWriter") + assert.NotContains(t, content, ".Context().SetBodyStreamWriter") +} + +func Test_MigrateContextMethods_DoesNotRewriteInStringsOrComments(t *testing.T) { + t.Parallel() + dir, err := os.MkdirTemp("", "mcmteststrings") + require.NoError(t, err) + defer func() { require.NoError(t, os.RemoveAll(dir)) }() + file := writeTempFile(t, dir, `package main +import "github.com/gofiber/fiber/v2" +// c.Context() should not be touched in comments +func handler(c fiber.Ctx) error { + _ = "literal with c.Context() that must not change" + return nil +}`) + var buf bytes.Buffer + cmd := newCmd(&buf) + require.NoError(t, v3.MigrateContextMethods(cmd, dir, nil, nil)) + content := readFile(t, file) + assert.Contains(t, content, `literal with c.Context()`) + // If regex-based rewriting is kept, this test documents the current limitation. +}
📜 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 (2)
cmd/internal/migrations/v3/context_methods.go(1 hunks)cmd/internal/migrations/v3/context_methods_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
cmd/internal/migrations/v3/context_methods_test.gocmd/internal/migrations/v3/context_methods.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
cmd/internal/migrations/v3/context_methods.go
📚 Learning: 2024-11-29T15:24:06.083Z
Learnt from: ReneWerner87
PR: gofiber/contrib#0
File: :0-0
Timestamp: 2024-11-29T15:24:06.083Z
Learning: When customizing the Fiber context to achieve version independence, using generics in methods like `Status` and `Type` allows for fluent method chaining. Implementing a generic interface for `Ctx` on the `App` prevents class switching when registering custom contexts that use fluent methods. Providing multiple `New` methods allows users who do not wish to customize the context to continue using `fiber.New` without changes.
Applied to files:
cmd/internal/migrations/v3/context_methods.go
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/context_methods_test.go (1)
cmd/internal/migrations/v3/context_methods.go (1)
MigrateContextMethods(14-74)
⏰ 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 (2)
cmd/internal/migrations/v3/context_methods.go (1)
50-58: Chain-preserving replacement for Context() ➜ RequestCtx() is correctCapturing the root identifier and using TrimSuffix to preserve any preceding fluent chain ensures
c.Status(...).Context()migrates toc.Status(...).RequestCtx()without breaking the chain. This aligns with the Fiber v3 change where Context() was renamed to RequestCtx(). Good improvement.cmd/internal/migrations/v3/context_methods_test.go (1)
125-147: Solid coverage for chained Context() ➜ RequestCtx()This test validates the new chain-preserving behavior and confirms the migration log is emitted. Nicely scoped.
1 participant
Summary
.Context()invocations and chained call sequencesTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68ab3e316144832683ee0ff32b1e8917
Summary by CodeRabbit
Bug Fixes
Tests