From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Tue, 19 Sep 2023 12:58:52 +0000 (-0400)
Subject: Ensure loader criteria used for ORM join with expression condition
X-Git-Tag: rel_2_0_22~33
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f8086a809bba358790cff032d745814b186ab8cb;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Ensure loader criteria used for ORM join with expression condition

Fixed bug where ORM :func:`_orm.with_loader_criteria` would not apply
itself to a :meth:`_sql.Select.join` where the ON clause were given as a
plain SQL comparison, rather than as a relationship target or similar.

Fixes: #10365
Change-Id: Ie6d08fb01a3079b7c3ccd3a8241031d46a56e19d
---

diff --git a/doc/build/changelog/unreleased_20/10365.rst b/doc/build/changelog/unreleased_20/10365.rst
new file mode 100644
index 0000000000..87791d72f7
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/10365.rst
@@ -0,0 +1,7 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 10365
+
+    Fixed bug where ORM :func:`_orm.with_loader_criteria` would not apply
+    itself to a :meth:`_sql.Select.join` where the ON clause were given as a
+    plain SQL comparison, rather than as a relationship target or similar.
diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py
index f22a798803..a838d8d08e 100644
--- a/lib/sqlalchemy/orm/util.py
+++ b/lib/sqlalchemy/orm/util.py
@@ -1828,8 +1828,8 @@ class _ORMJoin(expression.Join):
             prop = None
             on_selectable = None
 
+        left_selectable = left_info.selectable
         if prop:
-            left_selectable = left_info.selectable
             adapt_from: Optional[FromClause]
             if sql_util.clause_is_present(on_selectable, left_selectable):
                 adapt_from = on_selectable
@@ -1866,25 +1866,25 @@ class _ORMJoin(expression.Join):
 
             self._target_adapter = target_adapter
 
-            # we don't use the normal coercions logic for _ORMJoin
-            # (probably should), so do some gymnastics to get the entity.
-            # logic here is for #8721, which was a major bug in 1.4
-            # for almost two years, not reported/fixed until 1.4.43 (!)
-            if is_selectable(left_info):
-                parententity = left_selectable._annotations.get(
-                    "parententity", None
-                )
-            elif insp_is_mapper(left_info) or insp_is_aliased_class(left_info):
-                parententity = left_info
-            else:
-                parententity = None
+        # we don't use the normal coercions logic for _ORMJoin
+        # (probably should), so do some gymnastics to get the entity.
+        # logic here is for #8721, which was a major bug in 1.4
+        # for almost two years, not reported/fixed until 1.4.43 (!)
+        if is_selectable(left_info):
+            parententity = left_selectable._annotations.get(
+                "parententity", None
+            )
+        elif insp_is_mapper(left_info) or insp_is_aliased_class(left_info):
+            parententity = left_info
+        else:
+            parententity = None
 
-            if parententity is not None:
-                self._annotations = self._annotations.union(
-                    {"parententity": parententity}
-                )
+        if parententity is not None:
+            self._annotations = self._annotations.union(
+                {"parententity": parententity}
+            )
 
-        augment_onclause = onclause is None and _extra_criteria
+        augment_onclause = bool(_extra_criteria) and not prop
         expression.Join.__init__(self, left, right, onclause, isouter, full)
 
         assert self.onclause is not None
diff --git a/test/orm/test_relationship_criteria.py b/test/orm/test_relationship_criteria.py
index e67f9a01d5..aebdf6922a 100644
--- a/test/orm/test_relationship_criteria.py
+++ b/test/orm/test_relationship_criteria.py
@@ -314,6 +314,13 @@ class LoaderCriteriaTest(_Fixtures, testing.AssertsCompiledSQL):
             .join(User.addresses)
             .options(with_loader_criteria(User, User.name != "name")),
         ),
+        (
+            # issue #10365
+            lambda User, Address: select(Address)
+            .select_from(User)
+            .join(Address, User.id == Address.user_id)
+            .options(with_loader_criteria(User, User.name != "name")),
+        ),
         (
             lambda User, Address: select(Address)
             .select_from(orm_join(User, Address, User.addresses))
@@ -362,6 +369,13 @@ class LoaderCriteriaTest(_Fixtures, testing.AssertsCompiledSQL):
             .join(User.addresses)
             .options(with_loader_criteria(User, User.name != "name")),
         ),
+        (
+            # issue #10365 - this seems to have already worked
+            lambda User, Address: select(Address.id, User.id)
+            .select_from(User)
+            .join(Address, User.id == Address.user_id)
+            .options(with_loader_criteria(User, User.name != "name")),
+        ),
         (
             lambda User, Address: select(Address.id, User.id)
             .select_from(orm_join(User, Address, User.addresses))
@@ -411,6 +425,15 @@ class LoaderCriteriaTest(_Fixtures, testing.AssertsCompiledSQL):
                 with_loader_criteria(Address, Address.email_address != "email")
             ),
         ),
+        (
+            # issue #10365
+            lambda User, Address: select(Address)
+            .select_from(User)
+            .join(Address, User.id == Address.user_id)
+            .options(
+                with_loader_criteria(Address, Address.email_address != "email")
+            ),
+        ),
         (
             # for orm_join(), this is set up before we have the context
             # available that allows with_loader_criteria to be set up