Repository navigation
fix: translate exists/1 subqueries - #135
l-vincent-l wants to merge 1 commit into
Conversation
exists(subquery) had no expr/3 clause and fell through to the catch-all, which rendered it as a bare `?`. The next bound parameter then stood in for the whole condition, so the filter matched every row or none depending on its value: Ash policies written with exists(...) let every actor through. It is now translated to exists(SELECT ...), as in ecto_sqlite3.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe LibSQL adapter now renders correlated ChangesSQL expression translation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: translating exists/1 subqueries into SQL exists(SELECT ...) expressions. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the SQL at night,
And sees exists rendered right.
Parent aliases find their place,
Unsupported forms now state their case.
Two tests watch the rows appear,
Then the rabbit hops away, clear.
Comment @coderabbitai help to get the list of available commands.
b38f93f to
ed619ff
Compare
Problem
exists(subquery)has noexpr/3clause and falls through to the catch-all, which renders it as a bare?:The next bound parameter then stands in for the whole condition, and SQLite evaluates it as a boolean: depending on its value, the filter matches every row or none, without any error.
I hit this with Ash Framework, whose policies such as exists(memberships, user_id == ^actor(:id)) compile to an exists subquery: they let every actor read every record (or none, depending on the first character of the actor's id). That case also needs correlated parent_as/1 references, fixed separately in #136.
Fix
Translate exists(subquery) to exists(SELECT ...), as ecto_sqlite3 does.
Tests
Added to test/in_expression_comprehensive_test.exs: exists and not exists, each checked both when the subquery matches and when it does not. Both fail on main. mix test --exclude turso_remote: all passing. No Rust changes.
Summary by CodeRabbit
parent_as/1references andexists/1subqueries now generate SQL correctly, avoiding accidental parameter binding and incorrect query results.exists/1conditions now correctly include or exclude outer query results based on whether the subquery finds a match.Ecto.QueryErrorinstead of being rendered as placeholders.