fix(oracle): coerce LIMIT before ROWNUM interpolation - #2634
Conversation
Reject negative / non-numeric limit values before they are embedded in
``WHERE ROWNUM <= {limit}``.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThe Oracle connector now coerces and validates query limits before SQL interpolation. Tests cover unlimited queries, invalid limits, injected values, and generated ChangesOracle limit coercion
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/wren/tests/unit/test_oracle_coerce_limit.py (1)
13-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the unlimited branch through
OracleConnector.query.This test verifies only
_coerce_limit(None). It does not prove thatOracleConnector.query(..., None)skips theROWNUMwrapper and strips a trailing semicolon. Add a query-level assertion for the unlimited path.🤖 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_oracle_coerce_limit.py` around lines 13 - 14, Add a query-level test covering OracleConnector.query with a None limit, asserting the generated SQL skips the ROWNUM wrapper and removes any trailing semicolon; keep the existing _coerce_limit(None) unit assertion unchanged.core/wren/src/wren/connector/oracle.py (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the
limittype hints with the accepted values.
_coerce_limitand theConnectorABC.querycontract are typed asint | None, but both MySQL and Oracle callers coerceint()inputs such as"5", and the MySQL tests keep that behavior. If numeric strings are supported, update the helper and shared interface contract tostr | int | None. Otherwise, reject strings before coercion.🤖 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/src/wren/connector/oracle.py` at line 21, Align the limit type annotations with the behavior supported by MySQL and Oracle: update _coerce_limit and the shared ConnectorABC.query contract to accept str | int | None, preserving existing coercion of numeric strings. Ensure the related connector implementations and tests remain consistent with this expanded contract.
🤖 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.
Inline comments:
In `@core/wren/src/wren/connector/oracle.py`:
- Line 25: Update the limit conversion around coerced in the relevant Oracle
connector logic to reject fractional values and normalize TypeError, ValueError,
and OverflowError from int(limit) into ValueError, preserving the original
exception as the cause.
---
Nitpick comments:
In `@core/wren/src/wren/connector/oracle.py`:
- Line 21: Align the limit type annotations with the behavior supported by MySQL
and Oracle: update _coerce_limit and the shared ConnectorABC.query contract to
accept str | int | None, preserving existing coercion of numeric strings. Ensure
the related connector implementations and tests remain consistent with this
expanded contract.
In `@core/wren/tests/unit/test_oracle_coerce_limit.py`:
- Around line 13-14: Add a query-level test covering OracleConnector.query with
a None limit, asserting the generated SQL skips the ROWNUM wrapper and removes
any trailing semicolon; keep the existing _coerce_limit(None) unit assertion
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 31a0ef11-ba90-42dc-88fe-7f0a3c210638
📒 Files selected for processing (2)
core/wren/src/wren/connector/oracle.pycore/wren/tests/unit/test_oracle_coerce_limit.py
| """Validate limit before interpolating into ROWNUM SQL.""" | ||
| if limit is None: | ||
| return None | ||
| coerced = int(limit) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
for value in (float("inf"), object()):
try:
int(value)
except Exception as exc:
print(type(exc).__name__)
PYRepository: Canner/WrenAI
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant file and surrounding code without executing repository code.
fd -a 'oracle.py$' . || true
if [ -f core/wren/src/wren/connector/oracle.py ]; then
wc -l core/wren/src/wren/connector/oracle.py
sed -n '1,80p' core/wren/src/wren/connector/oracle.py | cat -n
fi
# Search for related query method definition/exception handling if the file path differs.
rg -n "def query|OverflowError|oracledb\.DatabaseError|int\(limit\)|raise ValueError" core/wren/src/wren/connector/oracle.py core -g '*.py' | head -200 || true
# Deterministic Python behavior probe for int() invalid inputs and whether catch-all DatabaseError matches them.
python3 - <<'PY'
values = (float('inf'), 'abc', object(), 1.9)
for value in values:
try:
result = int(value)
print(f"{type(value).__name__}({value!r}) -> {result!r}")
except Exception as exc:
print(f"{type(value).__name__}({value!r}) -> {type(exc).__name__}: {exc!r}")
class FakeDatabaseError(Exception):
pass
print("DatabaseError catches OverflowError?", isinstance(OverflowError(), FakeDatabaseError))
print("DatabaseError catches TypeError?", isinstance(TypeError(), FakeDatabaseError))
PYRepository: Canner/WrenAI
Length of output: 11132
Normalize invalid limit conversion failures to ValueError.
int(limit) can raise OverflowError for nonfinite numbers and TypeError for unsupported objects, and it truncates fractional floats. Callers catch oracledb.DatabaseError, so these failures can escape with inconsistent exception types. Catch (TypeError, ValueError, OverflowError) and raise ValueError from the original exception.
Proposed fix
- coerced = int(limit)
+ try:
+ coerced = int(limit)
+ except (TypeError, ValueError, OverflowError) as exc:
+ raise ValueError("limit must be an integer") from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| coerced = int(limit) | |
| try: | |
| coerced = int(limit) | |
| except (TypeError, ValueError, OverflowError) as exc: | |
| raise ValueError("limit must be an integer") from exc |
🤖 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/src/wren/connector/oracle.py` at line 25, Update the limit
conversion around coerced in the relevant Oracle connector logic to reject
fractional values and normalize TypeError, ValueError, and OverflowError from
int(limit) into ValueError, preserving the original exception as the cause.
|
Closing in favour of a single consolidated change — thanks for the work, the underlying tidy-up is worth doing, just not as one PR per connector. Why this is being closed rather than reviewed:
On the
What remains is genuine but smaller: a negative limit currently surfaces as a driver-level error instead of a clear What we would take instead — a single PR,
#2624 is the natural home for that; it is being kept open with a note to that effect. On this PR specifically — |
|
Thanks @goldmedal — understood. Closing rationale makes sense; consolidating into #2624 with the shared helper and strictest semantics. |
2 participants
Summary
Oracle embeds
limitintoROWNUM <= {limit}without validation. Coerce viaint()and reject negatives (same contract as MySQL).Test plan
cd core/wren && .venv/bin/python -m pytest tests/unit/test_oracle_coerce_limit.py -q(4 passed)Summary by CodeRabbit
Bug Fixes
Tests