fix(trino): coerce LIMIT before SQL interpolation - #2626
Conversation
Reject negative and non-numeric limits before building the Trino LIMIT wrapper.
WalkthroughTrino query limits are now coerced to non-negative integers before SQL interpolation. Unit tests cover negative, injection-like, and numeric-string limit inputs. ChangesTrino LIMIT validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 (1)
core/wren/src/wren/connector/trino.py (1)
499-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the type hints with the accepted limit inputs.
The tests intentionally pass
"4", but both_coerce_limitandTrinoConnector.querydeclareint | None. If numeric strings are supported, update both annotations to includestr(or use a shared alias) so static callers are not told this valid input is invalid.🤖 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/trino.py` at line 499, Update the type annotations for both _coerce_limit and TrinoConnector.query to accept str alongside int and None, preferably through a shared limit type alias if one exists or is appropriate. Keep the existing coercion behavior unchanged so numeric string inputs such as "4" remain supported.
🤖 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/trino.py`:
- Around line 459-470: Update _coerce_limit to reject negative fractional
numeric inputs before int coercion, while preserving acceptance of valid
non-negative integral limits and existing rejection of negative integers. Add a
regression test confirming limit=-0.5 raises ValueError rather than becoming
LIMIT 0.
---
Nitpick comments:
In `@core/wren/src/wren/connector/trino.py`:
- Line 499: Update the type annotations for both _coerce_limit and
TrinoConnector.query to accept str alongside int and None, preferably through a
shared limit type alias if one exists or is appropriate. Keep the existing
coercion behavior unchanged so numeric string inputs such as "4" remain
supported.
🪄 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: 3118ae98-ffaf-450d-9a5d-fdd79d8b7db5
📒 Files selected for processing (2)
core/wren/src/wren/connector/trino.pycore/wren/tests/unit/test_trino_coerce_limit.py
| def _coerce_limit(limit: int | None) -> int | None: | ||
| """Validate and coerce a user-supplied ``limit`` to a non-negative ``int``. | ||
|
|
||
| ``int(limit)`` rejects strings like ``"5 OR 1=1"`` so the value can be | ||
| safely interpolated into SQL. Negative limits are also rejected. | ||
| """ | ||
| if limit is None: | ||
| return None | ||
| coerced = int(limit) | ||
| if coerced < 0: | ||
| raise ValueError(f"limit must be non-negative, got {coerced}") | ||
| return coerced |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject fractional negative limits before coercion.
int(-0.5) becomes 0, so the check at Line 468 accepts an originally negative limit and executes LIMIT 0. Validate the original numeric value before truncation, or reject non-integral numeric inputs; add a regression test for limit=-0.5.
🤖 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/trino.py` around lines 459 - 470, Update
_coerce_limit to reject negative fractional numeric inputs before int coercion,
while preserving acceptance of valid non-negative integral limits and existing
rejection of negative integers. Add a regression test confirming limit=-0.5
raises ValueError rather than becoming LIMIT 0.
|
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 — |
2 participants
Summary
Coerce
limitto a non-negative int before Trino LIMIT subquery wrapping.Motivation
Same injection/negative-limit hole as other connectors that interpolate
{limit}.Verification
Summary by CodeRabbit