fix(mysql): strip trailing semicolon on unlimited query path

[Bartok9]

Summary

MySQL Connector.query always strips before LIMIT so unlimited SEL CET matches dry_run (core/wren Apache-2.0).

pytest core/wren/tests/unit/test_mysql_unlimited_semicolon.py — 1 passed

Author Bartok9

Summary by CodeRabbit

• Bug Fixes

  • Improved MySQL query execution by consistently removing trailing semicolons, including queries without a row limit.

• Tests

  • Added coverage to verify semicolon handling occurs before limit processing.

[coderabbitai]

Walkthrough

MySqlConnector.query now always strips trailing semicolons before optional LIMIT handling. A unit test verifies that normalization occurs before the limit branch.

Changes

MySQL semicolon normalization

Layer / File(s)	Summary
Query normalization and regression test core/wren/src/wren/connector/mysql.py, core/wren/tests/unit/test_mysql_unlimited_semicolon.py	MySqlConnector.query removes trailing semicolons before limit handling, and the test verifies this ordering.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related issues

• Strip trailing semicolon on the unlimited query() path across connectors #2497 — Tracks the MySQL no-limit query semicolon-stripping behavior implemented here.

Possibly related PRs

• Canner/WrenAI#2488 — Applies the same unconditional semicolon normalization to a connector query path.
• Canner/WrenAI#2489 — Adds analogous unlimited-path normalization and unit coverage.
• Canner/WrenAI#2555 — Moves semicolon stripping before the limit branch in Connector.query().

Suggested reviewers: goldmedal

Poem

A bunny found a semicolon sly,
Hiding where unlimited queries fly.
It trimmed the tail before limits came,
And tests now guard the tidy aim.
Hop, hop—clean SQL runs the game!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: stripping trailing semicolons for unlimited MySQL queries.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

core/wren/tests/unit/test_mysql_unlimited_semicolon.py (1)

8-16: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Test runtime behavior, not only source ordering.

This source-text assertion can pass even if query() still executes a semicolon-terminated SQL string. Add a behavior-level test with a fake cursor/connection that calls query(..., limit=None) and asserts the executed SQL is normalized.

🤖 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_unlimited_semicolon.py` around lines 8 - 16,
Replace or supplement test_query_always_strips_before_limit with a runtime test
using fake cursor and connection objects, invoke query with limit=None, and
assert the cursor receives SQL without the trailing semicolon. Keep the test
focused on executed SQL rather than source-text ordering, reusing the existing
query setup symbols where applicable.

🤖 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_unlimited_semicolon.py`:
- Around line 8-16: Replace or supplement test_query_always_strips_before_limit
with a runtime test using fake cursor and connection objects, invoke query with
limit=None, and assert the cursor receives SQL without the trailing semicolon.
Keep the test focused on executed SQL rather than source-text ordering, reusing
the existing query setup symbols where applicable.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ad6eef6e-92b8-4324-9f00-a61ad926e228

📥 Commits

Reviewing files that changed from the base of the PR and between 3d1be24 and 2c6ef6a.

📒 Files selected for processing (2)

• core/wren/src/wren/connector/mysql.py
• core/wren/tests/unit/test_mysql_unlimited_semicolon.py

[goldmedal]

Same note as on the sibling PRs in this series: the direction matches what already landed for Athena in #2535, but the test needs to exercise the connector rather than its source text.

test_mysql_unlimited_semicolon.py reads mysql.py as text and asserts the literal line sql = strip_trailing_semicolon(sql) appears before if limit is not None:. That passes even when the behavior is broken and fails on a harmless refactor — it pins the implementation, not the contract. Please write it as a mock-cursor test in the style of core/wren/tests/unit/test_athena_strip_unlimited_query.py (added with #2535): stub the cursor, call query("SELECT 1;"), assert on what reached cursor.execute.

Also worth clarifying in the description: _apply_limit already strips (mysql.py:41), so the limited path was never affected, and mysql-connector-python accepts a trailing semicolon for a single statement with multi-statement mode off. So what is the failure this fixes? If it's consistency with dry_run/limited composition rather than a driver error, that's fine — just state it that way.

[goldmedal]

Code review

Superseded — #2595 landed this change for MySQL in 5d2637c102b22c6a44911875638c52eb03e87f8b, using an else: branch on the unlimited path. I'd suggest closing this one.

Worth noting because it isn't obvious from the PR page: this still reports mergeable: true against the updated main, and merging it would not conflict. It would produce a double strip:

limit = _coerce_limit(limit)
# Always strip terminator so unlimited execute matches dry_run EXPLAIN ...
sql = strip_trailing_semicolon(sql)      # this PR
if limit is not None:
    sql = _apply_limit(sql, limit)
else:
    # Unlimited path: strip trailing terminators ...
    sql = strip_trailing_semicolon(sql)  # already on main via #2595

(verified with git merge-tree --write-tree origin/main <head>). Functionally harmless since the helper is idempotent, but it leaves two contradictory comments describing the same operation.

One process note that applies to the whole series: the test here pins implementation text rather than behaviour —

WrenAI/core/wren/tests/unit/test_mysql_unlimited_semicolon.py

Lines 8 to 16 in 2c6ef6a

	def test_query_always_strips_before_limit():
	text = SRC.read_text(encoding="utf-8")
	start = text.index("def query(self, sql: str, limit: int | None = None)")
	end = text.index("def dry_run(self, sql: str)", start)
	body = text[start:end]
	assert "sql = strip_trailing_semicolon(sql)" in body
	assert body.index("sql = strip_trailing_semicolon(sql)") < body.index(
	"if limit is not None:"
	)

It reads mysql.py as a string and asserts on substrings and their relative order, so the connector is never executed. It passes whether or not the code path works, and any reformatting breaks it. The mocked-execution tests in the later PRs (#2592, #2593, #2594) are the right shape.

[Bartok9]

Superseded by #2595 (already merged) — and you're right this would double-strip on top of it. Closing. Thanks.