Skip to content

fix(oracle): strip trailing semicolon before subquery-wrapping#2423

Open
Bartok9 wants to merge 1 commit into
Canner:mainfrom
Bartok9:fix/oracle-strip-semicolon
Open

fix(oracle): strip trailing semicolon before subquery-wrapping#2423
Bartok9 wants to merge 1 commit into
Canner:mainfrom
Bartok9:fix/oracle-strip-semicolon

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Both OracleConnector.query (when a limit is given) and OracleConnector.dry_run wrap user SQL as SELECT * FROM ({sql}) t WHERE ROWNUM <= N. When the user SQL ends in ;, the subquery is invalid and Oracle raises ORA-00911: invalid character.
  • User-facing impact: running (with a row limit) or validating a query that ends in a semicolon fails on Oracle even though the same query executes fine standalone.

Motivation

The sibling connectors already strip a trailing semicolon before subquery-wrapping: postgres, canner, trino, clickhouse all have a _strip_trailing_semicolon helper. Oracle wrapped raw SQL in both code paths.

The fix adds _strip_trailing_semicolon() (same [;\s]+\Z regex as the postgres connector) and applies it in both query() and dry_run(). Only the terminating run of semicolons/whitespace is stripped, so semicolons inside string literals (SELECT 'a;b') are preserved.

Verification

  • uv run --no-sync pytest tests/unit/test_oracle_semicolon.py -q — 4 passed (with the oracle extra installed).

Real behavior proof

Regression test core/wren/tests/unit/test_oracle_semicolon.py asserts the executed SQL and FAILS without the fix. It pytest.importorskip("oracledb") so it skips cleanly when the oracle extra isn't installed.

Without the fix (helper absent):

ImportError: cannot import name '_strip_trailing_semicolon' from 'wren.connector.oracle'
1 error

With the fix:

....                                                                     [100%]
4 passed in 0.21s

The query test asserts the executed SQL is exactly SELECT * FROM (SELECT 1) t WHERE ROWNUM <= 5 (no ;)); the dry_run test asserts ... WHERE ROWNUM <= 0.

Scope / risk

  • Touches only core/wren/src/wren/connector/oracle.py (Apache-2.0 path) + a new unit test.
  • Does NOT touch type parsing or connection logic.
  • The no-limit query() path (which doesn't wrap) is unchanged; string-literal semicolons preserved (covered by a helper test).

Summary by CodeRabbit

  • Bug Fixes
    • Improved Oracle query handling so statements ending with a trailing semicolon no longer fail when used in limited queries or dry runs.
    • Preserved semicolons inside string literals while only trimming unnecessary trailing semicolons and surrounding whitespace.
  • Tests
    • Added coverage for Oracle SQL wrapping behavior, including trailing-semicolon handling and unchanged behavior when no trailing semicolon is present.

Both OracleConnector.query (with a limit) and dry_run wrap user SQL as
SELECT * FROM ({sql}) t WHERE ROWNUM <= N. Oracle rejects a trailing
semicolon inside a subquery (ORA-00911: invalid character), so a query
ending in ; fails even though it runs fine standalone. Strip only the
terminating semicolon/whitespace run so semicolons inside string literals
are preserved. Mirrors the postgres/canner/trino/clickhouse connectors.
@github-actions github-actions Bot added python Pull requests that update Python code core labels Jun 30, 2026
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b2ec4c2e-3f66-4f9b-bf51-b7dce38b6318

📥 Commits

Reviewing files that changed from the base of the PR and between 3122f4f and c5d8595.

📒 Files selected for processing (2)
  • core/wren/src/wren/connector/oracle.py
  • core/wren/tests/unit/test_oracle_semicolon.py

Walkthrough

Adds a _strip_trailing_semicolon(sql) helper to the Oracle connector that uses a compiled regex to remove trailing semicolons and whitespace. The helper is applied in OracleConnector.query (ROWNUM <= N wrapping) and dry_run (ROWNUM <= 0 wrapping). A new unit test module validates all cases via mocked cursor.

Changes

Oracle Trailing Semicolon Stripping

Layer / File(s) Summary
Helper and connector usage
core/wren/src/wren/connector/oracle.py
Adds import re, defines _strip_trailing_semicolon(sql) with a compiled trailing-semicolon regex, and applies it in OracleConnector.query and dry_run when building Oracle subquery wrappers.
Unit tests
core/wren/tests/unit/test_oracle_semicolon.py
New test module that constructs a mock OracleConnector, captures SQL passed to cursor.execute, and asserts correct ROWNUM wrapping after semicolon removal for query, dry_run, and the helper function directly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Canner/WrenAI#2407: Applies the same trailing-semicolon stripping pattern to another connector's query and dry_run SQL wrapping logic.

Suggested reviewers

  • goldmedal

🐇 A semicolon lurked at the end of the line,
Confusing poor Oracle time after time.
But now a regex swoops in with flair,
Strips the stray punctuation right out of there.
No more ORA errors, the query runs fine! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main fix: stripping trailing semicolons before Oracle subquery wrapping.
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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant