fix(bigquery): coerce LIMIT before SQL interpolation - #2636
Conversation
Reject negative/non-numeric limit values before the subquery LIMIT wrap.
WalkthroughThe BigQuery connector now coerces optional limits to integers, rejects negative values, and validates limits before SQL interpolation. Unit tests cover null, invalid, injection-like, and valid integer inputs. ChangesBigQuery limit validation
Estimated code review effort: 2 (Simple) | ~10 minutes 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/wren/tests/unit/test_bigquery_coerce_limit.py (1)
16-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtend tests to cover the runtime contract.
test_apply_limit_uses_intpasses an already-integer2, so it does not test coercion. It also bypassesBigQueryConnector.query, so it cannot detect a regression that executes SQL before validation. Add a numeric-string case, a-0.5regression case, and a mocked query-path test that confirms invalid input does not callconnection.query.Also applies to: 26-27
🤖 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_bigquery_coerce_limit.py` around lines 16 - 18, Extend the tests around _coerce_limit and BigQueryConnector.query to cover numeric-string coercion, rejection of -0.5, and the query execution path. Mock the connection and verify invalid limits raise before connection.query is called, while retaining coverage that valid numeric strings are coerced correctly.
🤖 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/bigquery.py`:
- Around line 14-15: Update _coerce_limit to validate the original limit for
negativity before applying int(limit), ensuring values such as -0.5 and
Decimal("-0.5") are rejected rather than converted to zero; preserve the
existing handling for valid integral limits.
---
Nitpick comments:
In `@core/wren/tests/unit/test_bigquery_coerce_limit.py`:
- Around line 16-18: Extend the tests around _coerce_limit and
BigQueryConnector.query to cover numeric-string coercion, rejection of -0.5, and
the query execution path. Mock the connection and verify invalid limits raise
before connection.query is called, while retaining coverage that valid numeric
strings are coerced correctly.
🪄 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: c511f411-1536-42bb-b3ab-e3cebf167029
📒 Files selected for processing (2)
core/wren/src/wren/connector/bigquery.pycore/wren/tests/unit/test_bigquery_coerce_limit.py
| coerced = int(limit) | ||
| if coerced < 0: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'limit|BigQueryConnector|\.query\(' core/wren/src core/wren/testsRepository: Canner/WrenAI
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant limit helper and query path.
sed -n '1,110p' core/wren/src/wren/connector/bigquery.py
printf '\n--- coere_limit usages ---\n'
rg -n -C 3 '_coerce_limit|def query|raw_cursor_sql' core/wren/src/wren/connector/bigquery.py
# Behavioral probe for Python conversion semantics across representative inputs.
python3 - <<'PY'
from decimal import Decimal
values = [-1, -0.5, -0.9; Decimal("-0.5"), Decimal("-0"), 0, 0.5, "1", None]
results = []
for limit in values:
try:
coerced = int(limit)
ok = (limit is None) or (coerced >= 0)
results.append((repr(limit), coerced, ok))
except Exception as exc:
results.append((repr(limit), "<exception>", type(exc).__name__))
for row in results:
print(row)
PYRepository: Canner/WrenAI
Length of output: 3943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral probe for Python conversion semantics across representative inputs.
python3 - <<'PY'
from decimal import Decimal
values = [-1, -0.5, -0.9, Decimal("-0.5"), Decimal("-0"), 0, 0.5, "1", None]
def current(limit):
coerced = int(limit)
return (coerced is None or coerced >= 0), coerced
for limit in values:
try:
ok, coerced = current(limit)
print(repr(limit), "=>", coerced, "accepted_by_current=", ok)
except Exception as exc:
print(repr(limit), "=> exception", type(exc).__name__)
PYRepository: Canner/WrenAI
Length of output: 482
Validate the original limit before truncation.
int(limit) converts -0.5 and Decimal("-0.5") to 0, so _coerce_limit accepts those negative limits for BigQuery and passes LIMIT 0 through. Check negativity on the original value before converting, or reject non-integral values explicitly.
🤖 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/bigquery.py` around lines 14 - 15, Update
_coerce_limit to validate the original limit for negativity before applying
int(limit), ensuring values such as -0.5 and Decimal("-0.5") are rejected rather
than converted to zero; preserve the existing handling for valid integral
limits.
|
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 — fair close. Per-connector PRs were the wrong packaging; I'll fold the negativity check and shared Noted that BigQuery already used |
2 participants
Summary
BigQuery limit wrap used
int(limit)only inside_apply_limitafter a None check; add explicit coerce (negatives/injection) onquery().Test plan
cd core/wren && .venv/bin/python -m pytest tests/unit/test_bigquery_coerce_limit.py -q(4 passed)Summary by CodeRabbit