-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(postgres): coerce LIMIT before SQL interpolation #2625
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
Changes from all commits
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 | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,61 @@ | ||||||||||||||||||||||||||||||||||
| """Postgres query must coerce LIMIT before SQL interpolation. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| ``postgres`` imports ``psycopg`` at module load. Unit CI does not install the | ||||||||||||||||||||||||||||||||||
| ``postgres`` extra, so stub ``psycopg`` in ``sys.modules`` before importing the | ||||||||||||||||||||||||||||||||||
| connector module (same pattern as ``test_postgres_semicolon_unlimited``). | ||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| from __future__ import annotations | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||||||
| import types | ||||||||||||||||||||||||||||||||||
| from unittest.mock import MagicMock, patch | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import pytest | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def _ensure_psycopg_stub() -> None: | ||||||||||||||||||||||||||||||||||
| if "psycopg" in sys.modules: | ||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||
| mod = types.ModuleType("psycopg") | ||||||||||||||||||||||||||||||||||
| errors = types.ModuleType("psycopg.errors") | ||||||||||||||||||||||||||||||||||
| sys.modules["psycopg"] = mod | ||||||||||||||||||||||||||||||||||
| sys.modules["psycopg.errors"] = errors | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
Comment on lines
+17
to
+24
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'test_postgres_coerce_limit|postgres\.py$|settings|pytest|requirements|pyproject' || true
echo "== target test =="
if [ -f core/wren/tests/unit/test_postgres_coerce_limit.py ]; then
wc -l core/wren/tests/unit/test_postgres_coerce_limit.py
sed -n '1,90p' core/wren/tests/unit/test_postgres_coerce_limit.py | nl -ba
fi
echo "== postgres connector references =="
if [ -f core/wren/src/wren/connector/postgres.py ]; then
wc -l core/wren/src/wren/connector/postgres.py
rg -n "psycopg|QueryCanceled|coerce|limit" core/wren/src/wren/connector/postgres.py || true
echo "--- relevant context ---"
rg -n -C 4 "psycopg|QueryCanceled|execute|limit" core/wren/src/wren/connector/postgres.py || true
fi
echo "== imports/usages of coerce_limit tests or module =="
rg -n "test_postgres_coerce_limit|psycopg\.errors|QueryCanceled|_ensure_psycopg_stub|stub_psycopg" . || true
echo "== python semantic probe =="
python3 - <<'PY'
import types, sys
mod = types.ModuleType("psycopg")
errors = types.ModuleType("psycopg.errors")
sys.modules["psycopg"] = mod
sys.modules["psycopg.errors"] = errors
try:
errors.QueryCanceled
except AttributeError as e:
print("empty stub missing errors.QueryCanceled:", e)
try:
mod.QueryCanceled
except AttributeError as e:
pass
errors.QueryCanceled = Exception
mod.errors = errors
from psycopg.errors import QueryCanceled
print("fixed stub has errors.QueryCanceled:", QueryCanceled)
PYRepository: Canner/WrenAI Length of output: 602 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== target test =="
cat -n core/wren/tests/unit/test_postgres_coerce_limit.py
echo "== postgres connector relevant symbols/context =="
wc -l core/wren/src/wren/connector/postgres.py
rg -n -C 5 "psycopg|QueryCanceled|execute|limit|get_limit|coerce" core/wren/src/wren/connector/postgres.py || true
echo "== imports/usages =="
rg -n "test_postgres_coerce_limit|psycopg\.errors|QueryCanceled|_ensure_psycopg_stub|stub_psycopg" core/wren/tests/unit/test_postgres_coerce_limit.py core/wren/tests/connectors/test_postgres.py core/wren/src/wren/connector/postgres.py || true
echo "== python semantic probe =="
python3 - <<'PY'
import types, sys
mod = types.ModuleType("psycopg")
errors = types.ModuleType("psycopg.errors")
sys.modules["psycopg"] = mod
sys.modules["psycopg.errors"] = errors
try:
errors.QueryCanceled
except AttributeError as e:
print("current_stub_missing_errors_QueryCanceled")
print(repr(e))
try:
mod.errors
except AttributeError:
print("current_stub_missing_mod_errors")
errors.QueryCanceled = Exception
mod.errors = errors
from psycopg.errors import QueryCanceled
print("fixed_import_has_errors_QueryCanceled")
print(QueryCanceled)
PYRepository: Canner/WrenAI Length of output: 9269 Complete the
Proposed fix mod = types.ModuleType("psycopg")
errors = types.ModuleType("psycopg.errors")
+ errors.QueryCanceled = type("QueryCanceled", (Exception,), {})
+ mod.errors = errors
sys.modules["psycopg"] = mod
sys.modules["psycopg.errors"] = errors📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| _ensure_psycopg_stub() | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| from wren.connector import postgres as pg_mod # noqa: E402 | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def _connector(): | ||||||||||||||||||||||||||||||||||
| c = object.__new__(pg_mod.PostgresConnector) | ||||||||||||||||||||||||||||||||||
| c.connection = MagicMock() | ||||||||||||||||||||||||||||||||||
| c._closed = False | ||||||||||||||||||||||||||||||||||
| return c | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def test_reject_negative_limit(): | ||||||||||||||||||||||||||||||||||
| c = _connector() | ||||||||||||||||||||||||||||||||||
| with pytest.raises(ValueError, match="non-negative"): | ||||||||||||||||||||||||||||||||||
| c.query("SELECT 1", limit=-1) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def test_reject_injection_string(): | ||||||||||||||||||||||||||||||||||
| c = _connector() | ||||||||||||||||||||||||||||||||||
| with pytest.raises(ValueError): | ||||||||||||||||||||||||||||||||||
| c.query("SELECT 1", limit="1; DROP TABLE t") | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def test_numeric_string_limit_interpolated(): | ||||||||||||||||||||||||||||||||||
| c = _connector() | ||||||||||||||||||||||||||||||||||
| cursor = MagicMock() | ||||||||||||||||||||||||||||||||||
| cursor.__enter__ = MagicMock(return_value=cursor) | ||||||||||||||||||||||||||||||||||
| cursor.__exit__ = MagicMock(return_value=False) | ||||||||||||||||||||||||||||||||||
| c.connection.cursor.return_value = cursor | ||||||||||||||||||||||||||||||||||
| with patch.object(pg_mod, "_build_pg_arrow_table", return_value="tbl"): | ||||||||||||||||||||||||||||||||||
| out = c.query("SELECT 1", limit="3") | ||||||||||||||||||||||||||||||||||
| assert out == "tbl" | ||||||||||||||||||||||||||||||||||
| executed = cursor.execute.call_args[0][0] | ||||||||||||||||||||||||||||||||||
| assert "LIMIT 3" in executed | ||||||||||||||||||||||||||||||||||
| assert "DROP" not in executed | ||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate negativity before integer truncation.
int(-0.5)produces0, so the current check allows a negative runtime value and emitsLIMIT 0. Check the original numeric value before coercion, or reject non-integral limits explicitly; add a regression test forlimit=-0.5.🤖 Prompt for AI Agents