diff --git a/doc/build/changelog/unreleased_21/13070.rst b/doc/build/changelog/unreleased_21/13070.rst new file mode 100644 index 0000000000..4444261630 --- /dev/null +++ b/doc/build/changelog/unreleased_21/13070.rst @@ -0,0 +1,13 @@ +.. change:: + :tags: bug, orm + :tickets: 13070 + + A significant change to the ORM mechanics involved with both + :func:`.orm.with_loader_criteria` as well as single table inheritance, to + more aggressively locate WHERE criteria which should be augmented by either + the custom criteria or single-table inheritance criteria; SELECT statements + that do not include the entity within the columns clause or as an explicit + FROM, but still reference the entity within the WHERE clause, are now + covered, in particular this will allow subqueries using ``EXISTS (SELECT + 1)`` such as those rendered by :meth:`.RelationshipProperty.Comparator.any` + and :meth:`.RelationshipProperty.Comparator.has`. diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py index 03366bc9eb..a25bcb1f76 100644 --- a/lib/sqlalchemy/orm/context.py +++ b/lib/sqlalchemy/orm/context.py @@ -2528,9 +2528,16 @@ class _ORMSelectCompileState(_ORMCompileState, SelectState): associated with the global context. """ + ext_infos = [ + fromclause._annotations.get("parententity", None) + for fromclause in self.from_clauses + ] + [ + elem._annotations.get("parententity", None) + for where_crit in self.select_statement._where_criteria + for elem in sql_util.surface_expressions(where_crit) + ] - for fromclause in self.from_clauses: - ext_info = fromclause._annotations.get("parententity", None) + for ext_info in ext_infos: if ( ext_info diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py index e385d08ea0..c4a9256dd8 100644 --- a/lib/sqlalchemy/orm/relationships.py +++ b/lib/sqlalchemy/orm/relationships.py @@ -846,9 +846,11 @@ class RelationshipProperty( where_criteria = single_crit & where_criteria else: where_criteria = single_crit + dest_entity = info else: is_aliased_class = False to_selectable = None + dest_entity = self.mapper if self.adapter: source_selectable = self._source_selectable() @@ -903,6 +905,18 @@ class RelationshipProperty( crit = j & sql.True_._ifnone(where_criteria) + # ensure the exists query gets picked up by the ORM + # compiler and that it has what we expect as parententity so that + # _adjust_for_extra_criteria() gets set up + dest = dest._annotate( + { + "parentmapper": dest_entity.mapper, + "entity_namespace": dest_entity, + "parententity": dest_entity, + } + )._set_propagate_attrs( + {"compile_state_plugin": "orm", "plugin_subject": dest_entity} + ) if secondary is not None: ex = ( sql.exists(1) diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py index 9736b1ce70..59de4a9176 100644 --- a/lib/sqlalchemy/sql/util.py +++ b/lib/sqlalchemy/sql/util.py @@ -468,6 +468,15 @@ def tables_from_leftmost(clause: FromClause) -> Iterator[FromClause]: yield clause +def surface_expressions(clause): + stack = [clause] + while stack: + elem = stack.pop() + yield elem + if isinstance(elem, ColumnElement): + stack.extend(elem.get_children()) + + def surface_selectables(clause): stack = [clause] while stack: diff --git a/test/orm/inheritance/test_relationship.py b/test/orm/inheritance/test_relationship.py index f97e9248d4..83f74bd60d 100644 --- a/test/orm/inheritance/test_relationship.py +++ b/test/orm/inheritance/test_relationship.py @@ -22,6 +22,7 @@ from sqlalchemy.orm import relationship from sqlalchemy.orm import Session from sqlalchemy.orm import sessionmaker from sqlalchemy.orm import subqueryload +from sqlalchemy.orm import with_loader_criteria from sqlalchemy.orm import with_polymorphic from sqlalchemy.sql.selectable import LABEL_STYLE_TABLENAME_PLUS_COL from sqlalchemy.testing import AssertsCompiledSQL @@ -476,8 +477,10 @@ class SelfReferentialJ2JSelfTest(fixtures.MappedTest): def _two_obj_fixture(self): e1 = Engineer(name="wally") e2 = Engineer(name="dilbert", reports_to=e1) + e3 = Engineer(name="not wally") + e4 = Engineer(name="not dilbert", reports_to=e3) sess = fixture_session() - sess.add_all([e1, e2]) + sess.add_all([e1, e2, e3, e4]) sess.commit() return sess @@ -492,10 +495,45 @@ class SelfReferentialJ2JSelfTest(fixtures.MappedTest): def test_has(self): sess = self._two_obj_fixture() + eq_( sess.query(Engineer) .filter(Engineer.reports_to.has(Engineer.name == "wally")) - .first(), + .one(), + Engineer(name="dilbert"), + ) + + def test_has_w_aliased(self): + sess = self._two_obj_fixture() + + managing_engineer = aliased(Engineer) + + eq_( + sess.query(Engineer) + .filter( + Engineer.reports_to.of_type(managing_engineer).has( + managing_engineer.name == "wally" + ) + ) + .one(), + Engineer(name="dilbert"), + ) + + def test_has_w_aliased_w_loader_criteria(self): + """test for #13070""" + sess = self._two_obj_fixture() + + managing_engineer = aliased(Engineer) + + eq_( + sess.query(Engineer) + .filter(Engineer.reports_to.of_type(managing_engineer).has()) + .options( + with_loader_criteria( + managing_engineer, managing_engineer.name == "wally" + ) + ) + .one(), Engineer(name="dilbert"), ) diff --git a/test/orm/inheritance/test_single.py b/test/orm/inheritance/test_single.py index a26aed1cf1..dc35db7987 100644 --- a/test/orm/inheritance/test_single.py +++ b/test/orm/inheritance/test_single.py @@ -26,6 +26,7 @@ from sqlalchemy.orm import relationship from sqlalchemy.orm import selectinload from sqlalchemy.orm import Session from sqlalchemy.orm import subqueryload +from sqlalchemy.orm import with_loader_criteria from sqlalchemy.orm import with_polymorphic from sqlalchemy.sql.selectable import LABEL_STYLE_TABLENAME_PLUS_COL from sqlalchemy.testing import AssertsCompiledSQL @@ -1734,7 +1735,8 @@ class RelationshipToSingleTest( "WHERE employees.type IN (__[POSTCOMPILE_type_2])", ) - def test_relationship_to_subclass(self): + @testing.fixture + def rel_to_subclass_fixture(self): ( JuniorEngineer, Company, @@ -1791,10 +1793,14 @@ class RelationshipToSingleTest( sess.add_all([c1, c2, m1, m2, e1, e2]) sess.commit() - eq_(c1.engineers, [e2]) - eq_(c2.engineers, [e1]) + def test_relationship_to_subclass_one(self, rel_to_subclass_fixture): + Company = self.classes.Company + JuniorEngineer = self.classes.JuniorEngineer + Engineer = self.classes.Engineer + Employee = self.classes.Employee + + sess = fixture_session() - sess.expunge_all() eq_( sess.query(Company).order_by(Company.name).all(), [ @@ -1854,26 +1860,70 @@ class RelationshipToSingleTest( [Company(name="c2")], ) - # this however fails as it does not limit the subtypes to just - # "Engineer". with joins constructed by filter(), we seem to be - # following a policy where we don't try to make decisions on how to - # join to the target class, whereas when using join() we seem to have - # a lot more capabilities. we might want to document - # "advantages of join() vs. straight filtering", or add a large - # section to "inheritance" laying out all the various behaviors Query - # has. - @testing.fails_on_everything_except() - def go(): - sess.expunge_all() - eq_( - sess.query(Company) - .filter(Company.company_id == Engineer.company_id) - .filter(Engineer.name.in_(["Tom", "Kurt"])) - .all(), - [Company(name="c2")], - ) + def test_relationship_to_subclass_implicit_from( + self, rel_to_subclass_fixture + ): + """additional tests related to #13070""" - go() + Company = self.classes.Company + Engineer = self.classes.Engineer + + sess = fixture_session() + eq_( + sess.query(Company) + .filter(Company.company_id == Engineer.company_id) + .filter(Engineer.name.in_(["Tom", "Kurt"])) + .all(), + [Company(name="c2")], + ) + + def test_relationship_to_subclass_any(self, rel_to_subclass_fixture): + """additional tests related to #13070. + + this test is using any() with single-inh criteria, however this + doesnt seem to actually need anything from the #13070 change + as the `Engineer.name` criteria itself already includes the + single-inh criteria with no need for _adjust_for_extra_criteria. + apparently. + + """ + + Company = self.classes.Company + Engineer = self.classes.Engineer + + sess = fixture_session() + eq_( + sess.query(Company) + .filter(Company.engineers.any(Engineer.name.in_(["Tom", "Kurt"]))) + .all(), + [Company(name="c2")], + ) + + def test_relationship_to_subclass_any_loader_criteria( + self, rel_to_subclass_fixture + ): + """additional tests related to #13070. + + in this version, we put the criteria in with_loader_criteria, now + we need the #13070 fix for this to work. + + """ + + Company = self.classes.Company + Engineer = self.classes.Engineer + + sess = fixture_session() + eq_( + sess.query(Company) + .filter(Company.engineers.any()) + .options( + with_loader_criteria( + Engineer, Engineer.name.in_(["Tom", "Kurt"]) + ) + ) + .all(), + [Company(name="c2")], + ) class ManyToManyToSingleTest(fixtures.MappedTest, AssertsCompiledSQL): diff --git a/test/orm/test_relationship_criteria.py b/test/orm/test_relationship_criteria.py index e22a4c8ca2..cd499a37a3 100644 --- a/test/orm/test_relationship_criteria.py +++ b/test/orm/test_relationship_criteria.py @@ -10,6 +10,7 @@ from sqlalchemy import DateTime from sqlalchemy import delete from sqlalchemy import event from sqlalchemy import exc as sa_exc +from sqlalchemy import exists from sqlalchemy import ForeignKey from sqlalchemy import func from sqlalchemy import insert @@ -263,6 +264,158 @@ class LoaderCriteriaTest(_Fixtures, testing.AssertsCompiledSQL): __dialect__ = "default" + @testing.combinations( + ( + "one", + lambda Address: select(Address.user_id) + .where(Address.user_id == 5) + .options( + with_loader_criteria(Address, Address.email_address != "foo") + ), + ( + "SELECT addresses.user_id FROM addresses WHERE" + " addresses.user_id = :user_id_1 AND addresses.email_address" + " != :email_address_1" + ), + ), + ( + "two", + lambda aliased_address: select(aliased_address.user_id) + .where(aliased_address.user_id == 5) + .options( + with_loader_criteria( + aliased_address, aliased_address.email_address != "foo" + ) + ), + ( + "SELECT addresses_1.user_id FROM addresses AS addresses_1" + " WHERE addresses_1.user_id = :user_id_1 AND" + " addresses_1.email_address != :email_address_1" + ), + ), + ( + "three", + lambda Address: select(1) + .where(Address.user_id == 5) + .options( + with_loader_criteria(Address, Address.email_address != "foo") + ), + ( + "SELECT 1 FROM addresses WHERE addresses.user_id = :user_id_1" + " AND addresses.email_address != :email_address_1" + ), + ), + ( + "four", + lambda aliased_address: select(1) + .where(aliased_address.user_id == 5) + .options( + with_loader_criteria( + aliased_address, aliased_address.email_address != "foo" + ) + ), + ( + "SELECT 1 FROM addresses AS addresses_1 WHERE" + " addresses_1.user_id = :user_id_1 AND" + " addresses_1.email_address != :email_address_1" + ), + ), + ( + "five", + lambda User, Address: select(User) + .where(User.addresses.any()) + .options( + with_loader_criteria(Address, Address.email_address != "foo") + ), + ( + "SELECT users.id, users.name FROM users WHERE EXISTS (SELECT 1" + " FROM addresses WHERE users.id = addresses.user_id AND" + " addresses.email_address != :email_address_1)" + ), + ), + ( + "six", + lambda User, aliased_address: select(User) + .where(User.addresses.of_type(aliased_address).any()) + .options( + with_loader_criteria( + aliased_address, aliased_address.email_address != "foo" + ) + ), + ( + "SELECT users.id, users.name FROM users WHERE EXISTS (SELECT 1" + " FROM addresses AS addresses_1 WHERE users.id =" + " addresses_1.user_id AND addresses_1.email_address !=" + " :email_address_1)" + ), + ), + ( + "seven", + # note plugin_subject on this one is User, not Address. + lambda User, Address: select(User) + .where(exists(1).where(User.id == Address.user_id)) + .options( + with_loader_criteria(Address, Address.email_address != "foo") + ), + ( + "SELECT users.id, users.name FROM users WHERE EXISTS (SELECT 1" + " FROM addresses WHERE users.id = addresses.user_id AND" + " addresses.email_address != :email_address_1)" + ), + ), + ( + "eight", + lambda User, aliased_address: select(User) + .where(exists(1).where(User.id == aliased_address.user_id)) + .options( + with_loader_criteria( + aliased_address, aliased_address.email_address != "foo" + ) + ), + ( + "SELECT users.id, users.name FROM users WHERE EXISTS (SELECT 1" + " FROM addresses AS addresses_1 WHERE users.id =" + " addresses_1.user_id AND addresses_1.email_address !=" + " :email_address_1)" + ), + ), + ( + "nine", + lambda User, Address: select(User) + .where(exists(Address.user_id).where(User.id == Address.user_id)) + .options( + with_loader_criteria(Address, Address.email_address != "foo") + ), + ( + "SELECT users.id, users.name FROM users WHERE EXISTS (SELECT" + " addresses.user_id FROM addresses WHERE users.id =" + " addresses.user_id AND addresses.email_address !=" + " :email_address_1)" + ), + ), + argnames="statement, expected", + id_="iaa", + ) + def test_search_harder_for_criteria( + self, user_address_fixture, statement, expected + ): + """test #13070 + + loader_criteria taking effect for SELECT(1) statements with only + WHERE criteria, has()/any() calls, exists(1) calls + + """ + User, Address = user_address_fixture + + stmt = testing.resolve_lambda( + statement, + User=User, + Address=Address, + aliased_address=aliased(Address), + ) + + self.assert_compile(stmt, expected) + def test_select_mapper_mapper_criteria(self, user_address_fixture): User, Address = user_address_fixture diff --git a/test/orm/test_relationships.py b/test/orm/test_relationships.py index f610860d5d..9bd97dd16f 100644 --- a/test/orm/test_relationships.py +++ b/test/orm/test_relationships.py @@ -6756,9 +6756,12 @@ class AnnotationsMaintainedTest(AssertsCompiledSQL, fixtures.TestBase): ) else: # will not render "type IN " + # note due to #13070 we had to also change the reference + # to Engineer.company_id which also would pull in ORM + # handling for the entity subq = ( select(Engineer.__table__) - .where(foreign(Engineer.company_id) == Company.id) + .where(foreign(Engineer.__table__.c.company_id) == Company.id) .correlate(Company) )