-
Notifications
You must be signed in to change notification settings - Fork 1.9k
refactor(connector): centralize LIMIT coercion #2624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
dd0e407
e6943ef
a6b5abb
e72f250
a216e19
90402c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,48 @@ def strip_trailing_semicolon(sql: str) -> str: | |
| return _TRAILING_SEMICOLONS_RE.sub("", sql) | ||
|
|
||
|
|
||
| def coerce_limit(limit: int | None) -> int | None: | ||
| """Validate and coerce a user-supplied ``limit`` to a non-negative ``int``. | ||
|
|
||
| ``ConnectorABC.query`` is typed as ``limit: int | None``. At runtime this | ||
| helper still defends against accidental non-ints so every connector that | ||
| interpolates LIMIT shares one contract: | ||
|
|
||
| - ``None`` stays unlimited | ||
| - ``bool`` is rejected (``bool`` is an ``int`` subclass) | ||
| - non-integral numbers (e.g. ``-0.5``, ``1.5``) are rejected — never truncated | ||
| - non-numeric / overflow values raise ``ValueError`` | ||
| - negatives raise ``ValueError`` | ||
|
|
||
| Invalid limits surface as a consistent ``ValueError`` instead of a | ||
| driver-level error after SQL interpolation. | ||
| """ | ||
| if limit is None: | ||
| return None | ||
| if isinstance(limit, bool): | ||
| raise ValueError("limit must be an integer, not bool") | ||
| if isinstance(limit, float): | ||
| if not limit.is_integer(): | ||
| raise ValueError(f"limit must be an integral value, got {limit!r}") | ||
| # Still route through int() below for consistency / overflow. | ||
| try: | ||
| coerced = int(limit) | ||
| except (TypeError, ValueError, OverflowError) as exc: | ||
| raise ValueError(f"limit must be an integer, got {limit!r}") from exc | ||
| # Reject values whose int() truncation would change the number (e.g. Decimal) | ||
| # when the original compares unequal as a number. | ||
| if isinstance(limit, (int, float)): | ||
| if float(limit) != float(coerced): | ||
| raise ValueError(f"limit must be an integral value, got {limit!r}") | ||
| else: | ||
| # Strings / other: require exact round-trip for numeric strings only. | ||
| # int("1.5") already failed; int("01") == 1 is fine. | ||
| pass | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| if coerced < 0: | ||
| raise ValueError(f"limit must be non-negative, got {coerced}") | ||
| return coerced | ||
|
Comment on lines
+23
to
+58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: rg -n -C 2 'coerce_limit|strip_trailing_semicolon|__all__' core/wren/src/wren
rg -n -C 3 '\bLIMIT\b|\blimit\b' core/wren/src/wren/connectorRepository: Canner/WrenAI Length of output: 50369 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== connector files =="
git ls-files 'core/wren/src/wren/connector/*.py' | sort
echo
echo "== limited query locations importing util/using limit =="
python3 - <<'PY'
from pathlib import Path
root = Path("core/wren/src/wren/connector")
for p in sorted(root.glob("*.py")):
text = p.read_text()
lines = text.splitlines()
has_query_sig = any("def query(self, sql: str, limit: int | None = None)" in line for line in lines)
if not has_query_sig:
continue
print(f"\n--- {p} ---")
for i,l in enumerate(lines, start=1):
if i in (1,23,24,25) or "def query" in l or "def _apply_limit" in l or "LIMIT" in l or "ROWNUM" in l:
print(f"{i}: {l}")
PY
echo
echo "== coerce_limit usage/ref =="
rg -n 'from wren\.connector\.base import|coerce_limit|strip_trailing_semicolon|_apply_limit|_coerce_limit' core/wren/src/wren/connectorRepository: Canner/WrenAI Length of output: 15604 Apply
🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| class ConnectorABC(ABC): | ||
| @abstractmethod | ||
| def query(self, sql: str, limit: int | None = None) -> pa.Table: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| """Unit tests for wren.connector.base.coerce_limit.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import pytest | ||
|
|
||
| from wren.connector.base import coerce_limit | ||
|
|
||
|
|
||
| def test_none_passthrough() -> None: | ||
| assert coerce_limit(None) is None | ||
|
|
||
|
|
||
| def test_accepts_int() -> None: | ||
| assert coerce_limit(10) == 10 | ||
| assert coerce_limit(0) == 0 | ||
|
|
||
|
|
||
| def test_rejects_bool() -> None: | ||
| with pytest.raises(ValueError, match="bool"): | ||
| coerce_limit(True) # type: ignore[arg-type] | ||
| with pytest.raises(ValueError, match="bool"): | ||
| coerce_limit(False) # type: ignore[arg-type] | ||
|
|
||
|
|
||
| def test_rejects_non_integral_float() -> None: | ||
| with pytest.raises(ValueError, match="integral"): | ||
| coerce_limit(-0.5) # type: ignore[arg-type] | ||
| with pytest.raises(ValueError, match="integral"): | ||
| coerce_limit(1.5) # type: ignore[arg-type] | ||
|
|
||
|
|
||
| def test_accepts_integral_float() -> None: | ||
| assert coerce_limit(2.0) == 2 # type: ignore[arg-type] | ||
|
|
||
|
|
||
| def test_rejects_injection_string() -> None: | ||
| with pytest.raises(ValueError): | ||
| coerce_limit("1; DROP TABLE foo") # type: ignore[arg-type] | ||
|
|
||
|
|
||
| def test_rejects_negative() -> None: | ||
| with pytest.raises(ValueError, match="non-negative"): | ||
| coerce_limit(-3) | ||
|
|
||
|
|
||
| def test_rejects_non_numeric() -> None: | ||
| with pytest.raises(ValueError): | ||
| coerce_limit(object()) # type: ignore[arg-type] |
Uh oh!
There was an error while loading. Please reload this page.