fix(mysql): strip multi trailing semicolons before LIMIT/EXPLAIN - #2475
fix(mysql): strip multi trailing semicolons before LIMIT/EXPLAIN#2475Bartok9 wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe MySQL connector adds regex-based trailing-semicolon stripping for ChangesMySQL semicolon handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/wren/tests/unit/test_mysql_semicolon.py (1)
15-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for trailing whitespace without semicolons.
The regex
[;\s]+\Zstrips trailing whitespace even when no semicolons are present (e.g.,"SELECT 1 \n"→"SELECT 1"). This behavior is a side effect of the character class and isn't explicitly covered by the current tests. A one-line test would document this contract.🧪 Suggested additional test
def test_helper_double_semicolon(): assert _strip_trailing_semicolon("SELECT 1;;") == "SELECT 1" + + +def test_helper_strips_trailing_whitespace_without_semicolons(): + assert _strip_trailing_semicolon("SELECT 1 \n") == "SELECT 1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/wren/tests/unit/test_mysql_semicolon.py` around lines 15 - 21, Add a unit test alongside test_helper_preserves_semicolon_in_string_literal and test_helper_double_semicolon that verifies _strip_trailing_semicolon("SELECT 1 \n") returns "SELECT 1", documenting that trailing whitespace is removed even without a semicolon.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/wren/tests/unit/test_mysql_semicolon.py`:
- Around line 15-21: Add a unit test alongside
test_helper_preserves_semicolon_in_string_literal and
test_helper_double_semicolon that verifies _strip_trailing_semicolon("SELECT 1
\n") returns "SELECT 1", documenting that trailing whitespace is removed even
without a semicolon.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aa9e9353-657f-4257-ac90-8941b6a8d3d3
📒 Files selected for processing (2)
core/wren/src/wren/connector/mysql.pycore/wren/tests/unit/test_mysql_semicolon.py
|
The shared CI failure here ( |
8511b08 to
ca87818
Compare
|
Done — rebased onto latest |
str.rstrip(";").rstrip() leaves a terminator when whitespace sits
between trailing semicolons (SELECT 1; ;). Use the terminating-run
regex shared with other connectors.
ca87818 to
a0045db
Compare
|
This overlaps with #2480 — I think we can consolidate on that one. Both make the same functional change at the same call sites:
So the outcome is identical, but #2480 uses the shared helper instead of adding another per-connector copy — which is the point of the consolidation (#2441). The multi-semicolon fix from this PR is fully preserved by #2480, so nothing is lost. Plan is to keep #2480 and close this one once it lands. Thanks for the fix — it's carried over intact. |
|
Sounds good — makes total sense to consolidate on #2480 via the shared |
|
@Bartok9, there are some conflicts. |
|
Closing in favor of #2480, which landed the shared |
2 participants
Summary
sql.rstrip().rstrip(";").rstrip()with a terminating-run regex strip beforeLIMITinjection andEXPLAIN.SELECT 1; ;style inputs that still ended with;and broke EXPLAIN/LIMIT rewrite.Motivation
Chained
rstriponly removes a contiguous run of;after whitespace strip. When whitespace appears between trailing semicolons, a;remains. Apache-2.0 path:core/wren/**.Verification
Proof without the fix:
"SELECT 1; ;".rstrip().rstrip(";").rstrip()→'SELECT 1;'.Duplicates
No open PR on MySQL multi-semicolon strip.
Summary by CodeRabbit
Bug Fixes
LIMITqueries andEXPLAINdry-runs now construct SQL consistently after semicolon trimming.Tests