fix(mssql): strip trailing semicolons before sqlglot LIMIT rewrite - #2476
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:
WalkthroughThe MSSQL connector strips trailing semicolons before sqlglot-based pagination processing. Unit tests cover multi-semicolon removal, limited-query rewriting, and preservation of a single semicolon when no limit is applied. ChangesMSSQL semicolon handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (2)
core/wren/tests/unit/test_mssql_semicolon.py (2)
6-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for
_flatten_pagination_limitwith trailing semicolons.
_flatten_pagination_limitwas also modified to strip trailing semicolons (line 139 inmssql.py), but no test exercises that path with multi-semicolon input. A test likeMSSqlConnector()._flatten_pagination_limit("SELECT * FROM (SELECT 1) t LIMIT 5;;")would verify the strip + flatten interaction.🤖 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_mssql_semicolon.py` around lines 6 - 20, Add a unit test covering MSSqlConnector._flatten_pagination_limit with SQL ending in multiple semicolons, such as “SELECT * FROM (SELECT 1) t LIMIT 5;;”. Assert the result strips the trailing semicolons and correctly flattens the pagination limit.
11-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the limit-injection assertion.
The
"5" in outcondition is extremely weak — any output containing the digit5would satisfy theorchain, potentially masking a failure where LIMIT injection didn't work. Since"FETCH NEXT"is the real success signal for tsql dialect, consider asserting it directly and removing the catch-all"5" in out.♻️ Proposed fix
def test_raw_cursor_sql_injects_limit_after_multi_semicolon(): out = MSSqlConnector._raw_cursor_sql("SELECT 1;;", 5) - assert "FETCH NEXT" in out.upper() or "TOP" in out.upper() or "LIMIT" in out.upper() or "5" in out + assert "FETCH NEXT" in out.upper() # Must not leave the double terminator which becomes a Block parse assert ";;" not in out🤖 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_mssql_semicolon.py` around lines 11 - 15, Strengthen test_raw_cursor_sql_injects_limit_after_multi_semicolon by removing the weak catch-all `"5" in out` condition and asserting the expected tsql limit-injection marker directly, such as `"FETCH NEXT" in out.upper()`. Keep the separate assertion that the output contains no double 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_mssql_semicolon.py`:
- Around line 6-20: Add a unit test covering
MSSqlConnector._flatten_pagination_limit with SQL ending in multiple semicolons,
such as “SELECT * FROM (SELECT 1) t LIMIT 5;;”. Assert the result strips the
trailing semicolons and correctly flattens the pagination limit.
- Around line 11-15: Strengthen
test_raw_cursor_sql_injects_limit_after_multi_semicolon by removing the weak
catch-all `"5" in out` condition and asserting the expected tsql limit-injection
marker directly, such as `"FETCH NEXT" in out.upper()`. Keep the separate
assertion that the output contains no double semicolon.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cd1e510c-8a25-44dc-8edc-cf63d70417f0
📒 Files selected for processing (2)
core/wren/src/wren/connector/mssql.pycore/wren/tests/unit/test_mssql_semicolon.py
|
The shared CI failure here ( |
a8e23da to
bf141c6
Compare
|
Done — rebased onto latest |
bf141c6 to
3c0bc30
Compare
|
@Bartok9, we should use the shared trailing function in this pr. |
sqlglot parses SELECT 1;; as a Block, so LIMIT injection silently no-oped. Strip the terminating ;/whitespace run first.
sqlglot tsql emits SELECT TOP n for simple selects; the unit test required FETCH NEXT only. Accept TOP or FETCH NEXT after strip.
0875477 to
b1f7e8f
Compare
|
@goldmedal Done — refactored to use the shared |
F821/NameError: `_strip_trailing_semicolon` is not defined after the shared helper rename; call the public `strip_trailing_semicolon` import.
2 participants
Summary
;/whitespace run before MSSQL sqlglot parse paths (_raw_cursor_sqlwith limit,_flatten_pagination_limit).SELECT 1;;previously parsed as aBlock, so LIMIT injection silently no-oped and the driver still saw multi-statement noise.Motivation
Apache-2.0
core/wren/**. Proof on sqlglot tsql:SELECT 1;→ SelectSELECT 1;;/SELECT 1; ;→ Block (LIMIT rewrite skipped)Verification
Duplicates
No open PR for this sqlglot/Block interaction.
Summary by CodeRabbit