Fix JWT extractor migration loop#243
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 significantly refactors the underlying field replacement mechanism within the migration tool. The primary goal is to make the migration process for JWT extractor configurations more reliable and resilient by addressing potential issues like infinite loops and incorrect parsing of complex or inline values. The updated logic ensures that diverse configuration patterns are handled gracefully, leading to a more stable and accurate migration experience. 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 refactors replaceFieldImpl to be more robust and avoid infinite loops during migrations, which is a great improvement. It also adds valuable test coverage for the JWT extractor migration. I've identified a few areas for improvement: there are some redundant error checks in common.go, and one of the new tests in jwt_extractor_test.go appears to be for an invalid code pattern, which also seems unsupported by the current migration logic.
|
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. WalkthroughRewrites field-replacement from regex callbacks to a streaming, position-based parser preserving prefixes/indentation/comments; adds comprehensive JWT extractor migration tests covering inline, pointer, Fiber v2, and legacy import path scenarios. 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, 1 inconclusive)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/common.go (1)
207-218: Raw string literals (backticks) are not handled in string parsing.The string detection only handles double-quoted strings. If a field value contains a raw string literal using backticks (e.g.,
`cookie:jwt`), the parser could incorrectly interpret commas or newlines within it as value terminators.This may be acceptable if raw strings aren't expected in the target fields, but worth noting for robustness.
To handle all Go string types, consider extending the string detection:
switch ch { case '"': inString = true + case '`': + // Skip raw string literal + i++ + for i < len(src) && src[i] != '`' { + i++ + } case '(', '{', '[': depth++
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/common.go(1 hunks)cmd/internal/migrations/v3/jwt_extractor_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/jwt_extractor_test.go (1)
cmd/internal/migrations/v3/jwt_extractor.go (1)
MigrateJWTExtractor(15-95)
⏰ 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: Analyse
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, ubuntu-latest)
🔇 Additional comments (4)
cmd/internal/migrations/v3/common.go (1)
168-306: LGTM! The streaming parser correctly prevents infinite loops.The rewritten
replaceFieldImplproperly advances theposcursor after each replacement (pos = endat line 299), and the regex search operates onsrc[pos:]with index adjustment. This ensures forward progress and eliminates the infinite loop issue mentioned in the PR objectives.cmd/internal/migrations/v3/jwt_extractor_test.go (3)
97-118: LGTM!This test covers the inline config format which is an important edge case for the field replacement logic. The assertions properly verify that
TokenLookupis removed and replaced with the correctExtractorcall.
150-218: LGTM!Excellent realistic test case that validates the migration works correctly in the context of a complete Fiber v2 middleware implementation with error handling and JWT claims parsing. This provides good confidence the migration won't break real-world code.
253-281: LGTM!Good test coverage for pointer-style configuration (
&jwtware.Config{...}). The regex pattern inMigrateJWTExtractormatchesjwtware.Config{...}regardless of the preceding&, so this case is correctly handled.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/common.go (1)
199-251:replaceFieldImplonly tracks"strings; braces/commas inside'or`literals may confuse nestingIn the main scan loop,
inStringis toggled only whench == '"'. Any braces or commas that appear inside single‑quoted or backtick‑quoted literals will be treated as structural characters, affectingdepthand possibly causing the value to extend past its true end or stop too early. Other helpers in this file (splitArgs,extractCall,extractBlock) already handle",', and`uniformly, so it would be more robust to mirror that here.Consider extending the string handling to all three quote types, e.g. by tracking the current
quotebyte as insplitArgs/extractCall, so delimiters inside any Go string/rune literal are ignored while scanning for the end of the field value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/internal/migrations/v3/common.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). (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 (1)
cmd/internal/migrations/v3/common.go (1)
168-297: Streaming parser inreplaceFieldImpllooks sound and fixes the prior re‑matching / infinite‑loop riskThe new implementation walks
srconce with a monotonicpos, never mutatessrcin place, and correctly usesend/posso eachfield:occurrence is processed at most once. Delimiter handling viadepth,skipCommaSuffix, andnewline/commatracking covers nested literals and inline comments without obvious holes for the JWT/TokenLookup use cases, and the TODO path on unquote failure preserves comments and layout. Overall this is a solid upgrade over the previous regex callback approach.
1 participant
Summary
Testing
Codex Task
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.