Improve session release migration targeting #254
Conversation
ReneWerner87
requested review from
efectn,
gaby and
sixcolors
and removed request for
a team
|
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 Go AST helper utilities and tests, and integrates them into the session-release migration to ensure defer sess.Release() is only injected when the assigned variable originates from a session store created via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
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 refines the session release migration process by integrating new AST parsing utilities. These enhancements allow the migration tool to accurately identify and target session stores originating specifically from 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 introduces reusable AST helpers to improve the accuracy of the session release migration. The new logic correctly identifies session stores by parsing import aliases and call-based assignments, preventing false positives on stores from other packages. The changes also include new tests to cover aliased imports and non-session store scenarios.
My review has identified a couple of areas for improvement:
- The new AST helper
collectAssignedCallIdentshas a logic issue that makes it brittle for functions with multiple return values. - The migration logic for identifying session store creation seems to be checking for a non-existent function
NewStore, which could lead to incorrect migrations.
Details and suggestions are in the comments below.
There was a problem hiding this comment.
Pull request overview
This PR improves the precision of the session release migration by using AST analysis to identify session stores from the correct package, preventing false positives from similarly-named packages or methods.
- Introduces reusable AST helper functions for parsing Go files and extracting import aliases and call-based assignments
- Enhances the session release migration to filter store variables based on package origin
- Adds tests to verify correct handling of non-session packages and aliased imports
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/internal/migrations/v3/ast_helpers.go | New file containing reusable AST utility functions for import alias detection and call expression tracking |
| cmd/internal/migrations/v3/session_release.go | Enhanced migration logic to use AST analysis for identifying session stores, preventing false positives |
| cmd/internal/migrations/v3/session_release_test.go | Added test cases covering non-session store packages and aliased import scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
♻️ Duplicate comments (1)
cmd/internal/migrations/v3/ast_helpers.go (1)
37-44: Blank and dot imports correctly filtered.The implementation properly skips
_(blank) and.(dot) imports, which addresses the past review concern. These import types either don't provide a usable alias or would require different handling for the store variable matching logic.
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/ast_helpers.go (1)
64-76: Potential index out of bounds if Lhs is shorter than Rhs.While unlikely in well-formed Go code, if
len(assign.Lhs) < len(assign.Rhs), accessingassign.Lhs[idx]at line 70 could panic. This could occur with malformed or partial AST representations.Consider adding a bounds check:
for idx, rhs := range assign.Rhs { call, ok := rhs.(*ast.CallExpr) if !ok || !predicate(call) { continue } + if idx >= len(assign.Lhs) { + continue + } + ident, ok := assign.Lhs[idx].(*ast.Ident) if !ok { continue }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/internal/migrations/v3/ast_helpers.go(1 hunks)cmd/internal/migrations/v3/ast_helpers_test.go(1 hunks)cmd/internal/migrations/v3/session_release.go(3 hunks)cmd/internal/migrations/v3/session_release_test.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/internal/migrations/v3/session_release_test.go (1)
cmd/internal/migrations/v3/session_release.go (1)
MigrateSessionRelease(20-165)
cmd/internal/migrations/v3/session_release.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, macos-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, ubuntu-latest)
🔇 Additional comments (8)
cmd/internal/migrations/v3/session_release.go (2)
31-61: LGTM! AST-based targeting logic looks correct.The implementation properly:
- Parses the file and collects session package aliases
- Identifies store variables assigned from
session.New()calls- Guards against processing files without the session import or store variables
The fix to use
"New"instead of"NewStore"at line 56 aligns with the actual Fiber v3 session API.
76-79: Good guard against false positives.This check ensures
defer sess.Release()is only inserted when the store variable was actually created from the session package'sNew()function, preventing incorrect modifications to code using similarly-named methods from other packages.cmd/internal/migrations/v3/ast_helpers_test.go (2)
18-64: Good test coverage for import alias edge cases.The table-driven tests properly cover:
- Default alias derivation from path basename
- Explicit alias handling
- Blank import (
_) filtering- Dot import (
.) filtering- Unrelated imports returning empty set
This addresses the past review concern about handling special import cases.
66-97: Test correctly validates assignment collection behavior.The test verifies:
- First identifier from multi-return assignments is captured (
primary)- Single assignments are captured (
single)- Blank identifiers (
_) are correctly skipped- Calls to non-matching functions (
other()) are excludedOne observation:
secondaryfromprimary, secondary := target()is not captured because the implementation matchesLhs[idx]toRhs[idx], and there's only one RHS expression. This is the expected behavior for tracking store variables.cmd/internal/migrations/v3/session_release_test.go (3)
29-29: API usage updated correctly.Changed from
session.NewStore()tosession.New()to match the actual Fiber v3 session middleware API.
184-219: Excellent test for false positive prevention.This test validates that
store.Get()calls on stores from non-Fiber session packages are correctly ignored, preventing unintended code modifications.
221-258: Good coverage for aliased import handling.This test ensures the migration correctly handles cases where the session package is imported with a custom alias (
alias), and properly inserts the defer statement using the actual session variable name (session).cmd/internal/migrations/v3/ast_helpers.go (1)
13-20: Clean implementation of Go file parsing.The function properly wraps the parse error with context, making debugging easier when parsing fails.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the session release migration by using AST parsing to more accurately identify session store creation. This is a great enhancement that reduces the chance of false positives. The new AST helper functions are well-designed and reusable. I've identified a potential issue in collectAssignedCallIdents where it doesn't fully handle multiple return values from a function call, and I've provided a suggestion to make it more robust, along with a corresponding test update.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/ast_helpers_test.go (1)
53-63: Avoid range-variable capture pitfalls in parallel subtests (if supporting Go < 1.22)In older Go versions, capturing
ttdirectly in the subtest closure can cause all subtests to see the final loop value. If you still target Go < 1.22, consider rebinding inside the loop:for name, tt := range tests { name, tt := name, tt t.Run(name, func(t *testing.T) { t.Parallel() // use tt here }) }If the project requires Go 1.22+ only, this is purely stylistic and can be skipped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/ast_helpers.go(1 hunks)cmd/internal/migrations/v3/ast_helpers_test.go(1 hunks)
⏰ 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, windows-latest)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, ubuntu-latest)
🔇 Additional comments (5)
cmd/internal/migrations/v3/ast_helpers.go (3)
11-20: parseGoFile wrapper is clean and sufficientStraightforward use of
parser.ParseFilewith a freshFileSetand wrapped error context; nothing to change here.
22-50: Import alias collection correctly handles default, explicit, and special importsThe helper nicely filters by exact import path, skips blank/dot imports, and falls back to
path.Base(importPath)when no alias is provided, which is exactly what the session migration needs.
52-93: Assignment walker correctly supports multi-return and multi-RHS callsThe split between
len(assign.Rhs) == 1(multi-return from a single call) and the indexed multi-RHS path coversa, b := target(),_, x := target(), anda, b := other(), target()while ignoring_and non-ident LHS. This is a solid, generally reusable helper.cmd/internal/migrations/v3/ast_helpers_test.go (2)
11-64: Good coverage for parsing and import alias handlingTests exercise invalid content, default vs explicit aliases, and ensure blank/dot/unrelated imports are ignored, which aligns well with the helper’s responsibilities.
66-101: Assignment helper tests nicely cover multi-return and mixed-RHS casesThe test exercises
target()in multi-return, single-return, ignored-identifier, and mixedother(), target()assignments, plus a selector LHS, which gives strong confidence incollectAssignedCallIdents.
|
replace #251 |
|
replaced by #251 |
Summary
Testing
Codex Task
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.