Handle AcquireAgent without response usage #250
Conversation
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 enhances the client usage migrator to handle a specific pattern of 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 correctly extends the client usage migrator to handle AcquireAgent flows where the response is not consumed. The changes are well-tested with both unit and integration tests, which is great to see. I've identified a minor opportunity to improve maintainability by refactoring some duplicated code. Overall, this is a solid contribution to the migration tool.
| if structStart >= len(lines) { | ||
| out = append(out, line) | ||
| respLine := fmt.Sprintf("%s_, 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+"}") | ||
|
|
||
| i = parseEnd | ||
| changed = true | ||
| continue | ||
| } | ||
|
|
||
| structMatch := structAssignPattern.FindStringSubmatch(lines[structStart]) | ||
| bytesMatch := bytesAssignPattern.FindStringSubmatch(lines[structStart]) | ||
| stringMatch := stringAssignPattern.FindStringSubmatch(lines[structStart]) | ||
| methodName := methodFromExpr(methodExpr) | ||
| configLine := buildConfig(headers) | ||
| if len(structMatch) == 0 && len(bytesMatch) == 0 && len(stringMatch) == 0 { | ||
| respLine := fmt.Sprintf("%s_, 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+"}") | ||
|
|
||
| i = structStart - 1 | ||
| changed = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
The logic for generating the replacement code when no response is used is duplicated in two places (lines 177-181 and 192-196). To improve maintainability and reduce redundancy, this common logic should be extracted into a shared helper function or a local closure.
This will make the code cleaner and reduce the risk of introducing bugs if one of the blocks is updated in the future and the other is not.
Here's an example of how you could refactor it:
handleNoResponse := func() {
respLine := fmt.Sprintf("%s_, 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+"}")
changed = true
}
if structStart >= len(lines) {
handleNoResponse()
i = parseEnd
continue
}
// ... (structMatch, bytesMatch, stringMatch assignments)
if len(structMatch) == 0 && len(bytesMatch) == 0 && len(stringMatch) == 0 {
handleNoResponse()
i = structStart - 1
continue
}There was a problem hiding this comment.
Pull request overview
This PR enhances the client usage migrator to handle AcquireAgent flows that only parse requests without consuming the response. Previously, the migrator only handled cases where the response was used (via Struct, Bytes, or String methods). Now it also handles the case where only Parse() is called without any subsequent response handling.
- Adds logic to detect when no response usage follows a Parse() call
- Generates appropriate client method calls that discard the response with
_ - Includes comprehensive unit and integration tests
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/internal/migrations/v3/client_usage.go | Implements detection and rewriting logic for AcquireAgent patterns without response consumption, handling both end-of-file and mid-block scenarios |
| cmd/internal/migrations/v3/client_usage_test.go | Adds integration test covering the new migration scenario with a realistic example |
| cmd/internal/migrations/v3/client_usage_internal_test.go | Adds unit test directly testing the rewriteAcquireAgentBlocks function for the no-response-usage case |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| respLine := fmt.Sprintf("%s_, 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+"}") | ||
|
|
||
| i = parseEnd | ||
| changed = true | ||
| continue | ||
| } | ||
|
|
||
| structMatch := structAssignPattern.FindStringSubmatch(lines[structStart]) | ||
| bytesMatch := bytesAssignPattern.FindStringSubmatch(lines[structStart]) | ||
| stringMatch := stringAssignPattern.FindStringSubmatch(lines[structStart]) | ||
| methodName := methodFromExpr(methodExpr) | ||
| configLine := buildConfig(headers) | ||
| if len(structMatch) == 0 && len(bytesMatch) == 0 && len(stringMatch) == 0 { | ||
| respLine := fmt.Sprintf("%s_, 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+"}") |
There was a problem hiding this comment.
The code blocks at lines 177-185 and lines 192-196 are nearly identical, differing only in the final value of i. Consider extracting this duplicated logic into a helper function to improve maintainability and reduce the risk of inconsistencies if changes are needed in the future.
Example refactoring:
func writeNoResponseUsageCode(indent, methodName, uriExpr, configLine, parseIndent string, parseBody []string, out []string, endIndex int) ([]string, int) {
respLine := fmt.Sprintf("%s_, 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+"}")
return out, endIndex
}
WalkthroughThis pull request modifies the AcquireAgent code transformation logic to rewrite usage patterns that don't consume responses by replacing them with explicit client.Post method calls and error handling blocks. Two new tests are added to verify the transformation behavior for this scenario. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/internal/migrations/v3/client_usage.go(1 hunks)cmd/internal/migrations/v3/client_usage_internal_test.go(1 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(58-79)
⏰ 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, macos-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, ubuntu-latest)
🔇 Additional comments (2)
cmd/internal/migrations/v3/client_usage_test.go (1)
344-395: Nice end‑to‑end coverage for the no‑response AcquireAgent caseThis test captures the intended behavior well: an AcquireAgent flow that only parses/logs gets rewritten to a bare
client.Postcall with anif err != nilblock, and you validate the full migration (imports, config headers, and control flow) via gofmt-normalized comparison. This should guard the new rewrite path effectively.cmd/internal/migrations/v3/client_usage_internal_test.go (1)
91-135: Good focused unit test for the new rewrite pathThis internal test cleanly isolates the AcquireAgent “no response usage” scenario and verifies that rewriteAcquireAgentBlocks alone produces the expected
client.Post+if err != nilstructure and imports. It nicely complements the higher‑level MigrateClientUsage test.
| methodName := methodFromExpr(methodExpr) | ||
| configLine := buildConfig(headers) | ||
| if structStart >= len(lines) { | ||
| out = append(out, line) | ||
| respLine := fmt.Sprintf("%s_, 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+"}") | ||
|
|
||
| i = parseEnd | ||
| changed = true | ||
| continue | ||
| } | ||
|
|
||
| structMatch := structAssignPattern.FindStringSubmatch(lines[structStart]) | ||
| bytesMatch := bytesAssignPattern.FindStringSubmatch(lines[structStart]) | ||
| stringMatch := stringAssignPattern.FindStringSubmatch(lines[structStart]) | ||
| methodName := methodFromExpr(methodExpr) | ||
| configLine := buildConfig(headers) | ||
| if len(structMatch) == 0 && len(bytesMatch) == 0 && len(stringMatch) == 0 { | ||
| respLine := fmt.Sprintf("%s_, 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+"}") | ||
|
|
||
| i = structStart - 1 | ||
| changed = true | ||
| continue | ||
| } |
There was a problem hiding this comment.
Be more conservative when rewriting AcquireAgent blocks without response usage
The new branches correctly handle the “no response consumed” happy path (as in your tests), but they now rewrite some patterns that previously fell back to the original code. In particular, for any block that matches the AcquireAgent + Request + headers + URI + Parse shape but then does other work with the agent/request (e.g. a.Bytes(), doSomethingWith(a) or req.*) after the parse, and not necessarily on the very first non-empty line, this code will:
- Drop the
agentVar := fiber.AcquireAgent()declaration. - Leave later uses of
agentVar/reqVarintact. - Produce invalid migrated code (compile error due to undefined
agentVar/reqVar), whereas before such blocks were left untouched.
To avoid that regression while still covering the new scenario, you could add a conservative guard before the len(structMatch) == 0 && ... rewrite to ensure there are no further usages of the agent/request variables after the parse block. For example, either scan from structStart forward for agentVar / reqVar or, at minimum, bail out when the first non-empty line still references them:
- if len(structMatch) == 0 && len(bytesMatch) == 0 && len(stringMatch) == 0 {
- respLine := fmt.Sprintf("%s_, 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+"}")
-
- i = structStart - 1
- changed = true
- continue
- }
+ if len(structMatch) == 0 && len(bytesMatch) == 0 && len(stringMatch) == 0 {
+ // If the first non-empty line after the parse block still touches the
+ // agent or request variables, keep the original AcquireAgent flow to
+ // avoid dropping their declaration while leaving usages behind.
+ nextLine := lines[structStart]
+ if strings.Contains(nextLine, agentVar) || strings.Contains(nextLine, reqVar) {
+ out = append(out, line)
+ continue
+ }
+
+ respLine := fmt.Sprintf("%s_, 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+"}")
+
+ i = structStart - 1
+ changed = true
+ continue
+ }You might want to extend this to scan more than one line for agentVar / reqVar, but even this minimal guard would prevent the worst false-positives.
🤖 Prompt for AI Agents
In cmd/internal/migrations/v3/client_usage.go around lines 174 to 201, the new
branch that rewrites AcquireAgent+Request+headers+URI+Parse blocks is too
aggressive and can drop the agent/request declarations while later lines still
reference them; add a conservative guard before the len(structMatch)==0 && ...
rewrite: scan from structStart forward (at least the first non-empty line,
ideally a small window of subsequent lines) for any occurrences of the agentVar
or reqVar identifiers and if found, do not apply the rewrite (leave original
lines untouched). Ensure the guard updates the control flow so when a usage is
detected you set i to the appropriate index and continue without modifying that
block.
|
replaced by #252 |
Summary
Testing
Codex Task
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.