fix(filters): warn when an EXISTS subquery is not correlated - #782
Open
ruicleite96 wants to merge 2 commits into
Open
fix(filters): warn when an EXISTS subquery is not correlated#782ruicleite96 wants to merge 2 commits into
ruicleite96 wants to merge 2 commits into
Conversation
`ExistsFilter` documents itself as applying "a correlated subquery that returns
only the rows from the main query that match the specified conditions", but
`Select.correlate()` only permits correlation — it cannot infer a join. When
none of the supplied `values` reference the outer table, the subquery stands
alone: it is true whenever any row matches it, so the filter matches *every*
row of the outer query instead of raising.
The class's own documented example has this shape:
ExistsFilter(values=[User.email.like("%@example.com%")])
.append_to_statement(select(Organization), Organization)
-- EXISTS (SELECT 1 FROM user_t WHERE user_t.email LIKE '%@example.com%')
which returns every organization as soon as one user matches, rather than "only
organizations where the user's email contains @example.com" as documented.
Emits `UncorrelatedSubqueryWarning` when the conditions never mention the outer
table, and corrects the docstring and example to include the correlation.
Conditions on the outer table itself — the common single-table usage the
existing tests cover — are already correlated and do not warn.
`iterate()` cannot see the tables behind a `text()` or `literal_column()` construct, so a correlation spelled out as raw SQL was indistinguishable from no correlation at all and drew a warning on a query that was in fact correct. Skip the check when the conditions contain raw SQL rather than accuse it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #782 +/- ##
==========================================
+ Coverage 82.43% 82.48% +0.04%
==========================================
Files 105 105
Lines 9039 9053 +14
Branches 1219 1222 +3
==========================================
+ Hits 7451 7467 +16
+ Misses 1262 1260 -2
Partials 326 326 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ExistsFilterdescribes itself as applyingSelect.correlate()only permits correlation — it cannot infer a join. If none of the suppliedvaluesreference the outer table, the subquery stands alone: it is true whenever any row matches it, so the filter matches every row of the outer query rather than raising.The docstring's own example has exactly that shape:
compiles to
There is no join back to
organization, so this returns every organization as soon as a single user matches — not "only organizations where the user's email contains @example.com" as the docs state. With the correlation included it behaves as documented:This is a quiet failure: no exception, no empty result — just a filter that silently does nothing.
Change
UncorrelatedSubqueryWarning(aUserWarningsubclass, exported) when the combined conditions never reference the outer table, naming the table and showing the missing join.ExistsFilterdocstring and example to include the correlation, and note that the OR example is not correlated on its own.Applies to
NotExistsFiltertoo, which shares_get_combined_conditions.The check walks the condition tree with
sqlalchemy.sql.visitors.iteratelooking for a column owned bymodel.__table__. Raw SQL is exempt:iteratecannot see the tables behind atext()orliteral_column()construct, so a correlation spelled out as raw SQL is indistinguishable from no correlation at all. Rather than accuse a query that may well be correct, the check stays silent when it finds either.No false positives on existing usage
The existing tests use single-table conditions —
ExistsFilter(values=[Movie.genre == "Action"])againstselect(Movie). Those do reference the outer table, so they are genuinely correlated and stay silent. I confirmed this by running the exists selection with the new warning promoted to an error:20 passed, 0 failed (remaining engines error only because Spanner/CockroachDB containers will not start on this machine).
I also checked the shapes where the traversal could plausibly miss a correlation. These stay silent, as they should: an explicit join condition, a single-table self condition,
relationship.has(), a nestedand_(), and the raw-SQL constructs above.One shape does warn, and should: joined-table inheritance where the condition names a parent-table column, e.g.
ExistsFilter(values=[Person.name == "x"])againstselect(Employee).Employee.__table__is only the child table, socorrelate()does not coverpersonand the subquery really is standalone.Tests
tests/unit/test_filters_correlation.py— no database needed, parametrised over both filters:text()orliteral_column()do not warn.10 passing.
ruff check,ruff formatandmypyare clean.Alternative, if you would rather not add a warning
A docs-only version — the corrected docstring and example without the runtime warning — is a reasonable smaller change, and I am happy to trim it to that. My reason for preferring the warning is that the failure mode returns plausible-looking data, so it can survive a long time in a codebase before anyone notices.
📚 Documentation preview: https://litestar-org.github.io/advanced-alchemy-docs-preview/782