ensure secondary cols not excluded from adaption

Fixed regression caused by :ticket:`9779` where using the "secondary" table
in a relationship ``and_()`` expression would fail to be aliased to match
how the "secondary" table normally renders within a
:meth:`_sql.Select.join` expression, leading to an invalid query.

Fixes: #11010
Change-Id: I535ce8b14f6a779c26b6b50b796ce64e57d7ee3d

diff --git a/doc/build/changelog/unreleased_20/11010.rst b/doc/build/changelog/unreleased_20/11010.rst
new file mode 100644 (file)
index 0000000..bd24772
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/11010.rst
@@ -0,0 +1,8 @@
+.. change::
+    :tags: bug, orm, regression
+    :tickets: 11010
+
+    Fixed regression caused by :ticket:`9779` where using the "secondary" table
+    in a relationship ``and_()`` expression would fail to be aliased to match
+    how the "secondary" table normally renders within a
+    :meth:`_sql.Select.join` expression, leading to an invalid query.

diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index 5de886f79bf6d29b2e31dc1b9a41151585adea4e..383bf24d450548152debe4851d125f694065554a 100644 (file)
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -19,6 +19,7 @@ import collections
 from collections import abc
 import dataclasses
 import inspect as _py_inspect
+import itertools
 import re
 import typing
 from typing import Any
@@ -26,6 +27,7 @@ from typing import Callable
 from typing import cast
 from typing import Collection
 from typing import Dict
+from typing import FrozenSet
 from typing import Generic
 from typing import Iterable
 from typing import Iterator
@@ -3288,6 +3290,15 @@ class JoinCondition:
             if annotation_set.issubset(col._annotations)
         }
 
+    @util.memoized_property
+    def _secondary_lineage_set(self) -> FrozenSet[ColumnElement[Any]]:
+        if self.secondary is not None:
+            return frozenset(
+                itertools.chain(*[c.proxy_set for c in self.secondary.c])
+            )
+        else:
+            return util.EMPTY_SET
+
     def join_targets(
         self,
         source_selectable: Optional[FromClause],
@@ -3338,23 +3349,25 @@ class JoinCondition:
 
         if extra_criteria:
 
-            def mark_unrelated_columns_as_ok_to_adapt(
+            def mark_exclude_cols(
                 elem: SupportsAnnotations, annotations: _AnnotationDict
             ) -> SupportsAnnotations:
-                """note unrelated columns in the "extra criteria" as OK
-                to adapt, even though they are not part of our "local"
-                or "remote" side.
+                """note unrelated columns in the "extra criteria" as either
+                should be adapted or not adapted, even though they are not
+                part of our "local" or "remote" side.
 
-                see #9779 for this case
+                see #9779 for this case, as well as #11010 for a follow up
 
                 """
 
                 parentmapper_for_element = elem._annotations.get(
                     "parentmapper", None
                 )
+
                 if (
                     parentmapper_for_element is not self.prop.parent
                     and parentmapper_for_element is not self.prop.mapper
+                    and elem not in self._secondary_lineage_set
                 ):
                     return _safe_annotate(elem, annotations)
                 else:
@@ -3363,8 +3376,8 @@ class JoinCondition:
             extra_criteria = tuple(
                 _deep_annotate(
                     elem,
-                    {"ok_to_adapt_in_join_condition": True},
-                    annotate_callable=mark_unrelated_columns_as_ok_to_adapt,
+                    {"should_not_adapt": True},
+                    annotate_callable=mark_exclude_cols,
                 )
                 for elem in extra_criteria
             )
@@ -3378,14 +3391,16 @@ class JoinCondition:
             if secondary is not None:
                 secondary = secondary._anonymous_fromclause(flat=True)
                 primary_aliasizer = ClauseAdapter(
-                    secondary, exclude_fn=_ColInAnnotations("local")
+                    secondary,
+                    exclude_fn=_local_col_exclude,
                 )
                 secondary_aliasizer = ClauseAdapter(
                     dest_selectable, equivalents=self.child_equivalents
                 ).chain(primary_aliasizer)
                 if source_selectable is not None:
                     primary_aliasizer = ClauseAdapter(
-                        secondary, exclude_fn=_ColInAnnotations("local")
+                        secondary,
+                        exclude_fn=_local_col_exclude,
                     ).chain(
                         ClauseAdapter(
                             source_selectable,
@@ -3397,14 +3412,14 @@ class JoinCondition:
             else:
                 primary_aliasizer = ClauseAdapter(
                     dest_selectable,
-                    exclude_fn=_ColInAnnotations("local"),
+                    exclude_fn=_local_col_exclude,
                     equivalents=self.child_equivalents,
                 )
                 if source_selectable is not None:
                     primary_aliasizer.chain(
                         ClauseAdapter(
                             source_selectable,
-                            exclude_fn=_ColInAnnotations("remote"),
+                            exclude_fn=_remote_col_exclude,
                             equivalents=self.parent_equivalents,
                         )
                     )
@@ -3483,18 +3498,24 @@ class JoinCondition:
 
 
 class _ColInAnnotations:
-    """Serializable object that tests for a name in c._annotations."""
+    """Serializable object that tests for names in c._annotations.
 
-    __slots__ = ("name",)
+    TODO: does this need to be serializable anymore?  can we find what the
+    use case was for that?
 
-    def __init__(self, name: str):
-        self.name = name
+    """
+
+    __slots__ = ("names",)
+
+    def __init__(self, *names: str):
+        self.names = frozenset(names)
 
     def __call__(self, c: ClauseElement) -> bool:
-        return (
-            self.name in c._annotations
-            or "ok_to_adapt_in_join_condition" in c._annotations
-        )
+        return bool(self.names.intersection(c._annotations))
+
+
+_local_col_exclude = _ColInAnnotations("local", "should_not_adapt")
+_remote_col_exclude = _ColInAnnotations("remote", "should_not_adapt")
 
 
 class Relationship(  # type: ignore

diff --git a/test/orm/test_relationship_criteria.py b/test/orm/test_relationship_criteria.py
index 69279f6004486c63137595bcc884dcd59744505b..4add92c1e725f5308d898128cd759b46e15355d7 100644 (file)
--- a/test/orm/test_relationship_criteria.py
+++ b/test/orm/test_relationship_criteria.py
@@ -2409,6 +2409,28 @@ class RelationshipCriteriaTest(_Fixtures, testing.AssertsCompiledSQL):
             "AND items_1.description != :description_1",
         )
 
+    def test_use_secondary_table_in_criteria(self, order_item_fixture):
+        """test #11010 , regression caused by #9779"""
+
+        Order, Item = order_item_fixture
+        order_items = self.tables.order_items
+
+        stmt = select(Order).join(
+            Order.items.and_(
+                order_items.c.item_id > 1, Item.description != "description"
+            )
+        )
+
+        self.assert_compile(
+            stmt,
+            "SELECT orders.id, orders.user_id, orders.address_id, "
+            "orders.description, orders.isopen FROM orders JOIN order_items "
+            "AS order_items_1 ON orders.id = order_items_1.order_id "
+            "JOIN items ON items.id = order_items_1.item_id "
+            "AND order_items_1.item_id > :item_id_1 "
+            "AND items.description != :description_1",
+        )
+
 
 class SubqueryCriteriaTest(fixtures.DeclarativeMappedTest):
     """test #10223"""