Cleanup with query aliasing

Try to simplify some of the "adapter" stuff in query:

1. identify that join(.., aliased=True) doesn't work if the
right side has no mapper.   The adaption of the right side is
done via the mapper with aliased(), so that doesn't effect
a selectable only.  raise an error, so we can simplify
the code.

2. build fewer adapter objects.   these are confusing to follow
and we should try to figure out exactly what purpose which
one serves where and make that clear.

Change-Id: I18dfcd01e6ad533aa0b8d557fc637ee2766ed050

diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py
index 2bd79a2cdd8dc62f1adf753a7061b0bb12cef6e6..387a72d0b981b2e39ea86cccaa126c349a863065 100644 (file)
--- a/lib/sqlalchemy/orm/query.py
+++ b/lib/sqlalchemy/orm/query.py
@@ -112,7 +112,7 @@ class Query(object):
     _join_entities = ()
     _select_from_entity = None
     _mapper_adapter_map = {}
-    _filter_aliases = None
+    _filter_aliases = ()
     _from_obj_alias = None
     _joinpath = _joinpoint = util.immutabledict()
     _execution_options = util.immutabledict()
@@ -334,7 +334,7 @@ class Query(object):
             orm_only = False
 
         if as_filter and self._filter_aliases:
-            for fa in self._filter_aliases.visitor_iterator:
+            for fa in self._filter_aliases:
                 adapters.append((orm_only, fa.replace))
 
         if self._from_obj_alias:
@@ -356,14 +356,12 @@ class Query(object):
             return clause
 
         def replace(elem):
+            is_orm_adapt = (
+                "_orm_adapt" in elem._annotations
+                or "parententity" in elem._annotations
+            )
             for _orm_only, adapter in adapters:
-                # if 'orm only', look for ORM annotations
-                # in the element before adapting.
-                if (
-                    not _orm_only
-                    or "_orm_adapt" in elem._annotations
-                    or "parententity" in elem._annotations
-                ):
+                if not _orm_only or is_orm_adapt:
                     e = adapter(elem)
                     if e is not None:
                         return e
@@ -2652,33 +2650,51 @@ class Query(object):
         if hasattr(r_info, "mapper"):
             self._join_entities += (r_info,)
 
-        if not right_mapper and prop:
-            right_mapper = prop.mapper
-
         need_adapter = False
 
-        if r_info.is_clause_element and right_selectable._is_lateral:
-            # orm_only is disabled to suit the case where we have to
-            # adapt an explicit correlate(Entity) - the select() loses
-            # the ORM-ness in this case right now, ideally it would not
-            right = self._adapt_clause(right, True, False)
+        # test for joining to an unmapped selectable as the target
+        if r_info.is_clause_element:
 
-        if right_mapper and right is right_selectable:
-            if not right_selectable.is_derived_from(right_mapper.mapped_table):
-                raise sa_exc.InvalidRequestError(
-                    "Selectable '%s' is not derived from '%s'"
-                    % (
-                        right_selectable.description,
-                        right_mapper.mapped_table.description,
-                    )
-                )
+            if prop:
+                right_mapper = prop.mapper
 
-            if isinstance(right_selectable, expression.SelectBase):
-                # TODO: this isn't even covered now!
-                right_selectable = right_selectable.alias()
-                need_adapter = True
+            if right_selectable._is_lateral:
+                # orm_only is disabled to suit the case where we have to
+                # adapt an explicit correlate(Entity) - the select() loses
+                # the ORM-ness in this case right now, ideally it would not
+                right = self._adapt_clause(right, True, False)
 
-            right = aliased(right_mapper, right_selectable)
+            elif prop:
+                # joining to selectable with a mapper property given
+                # as the ON clause
+
+                if not right_selectable.is_derived_from(
+                    right_mapper.mapped_table
+                ):
+                    raise sa_exc.InvalidRequestError(
+                        "Selectable '%s' is not derived from '%s'"
+                        % (
+                            right_selectable.description,
+                            right_mapper.mapped_table.description,
+                        )
+                    )
+
+                # if the destination selectable is a plain select(),
+                # turn it into an alias().
+                if isinstance(right_selectable, expression.SelectBase):
+                    right_selectable = right_selectable.alias()
+                    need_adapter = True
+
+                # make the right hand side target into an ORM entity
+                right = aliased(right_mapper, right_selectable)
+            elif create_aliases:
+                # it *could* work, but it doesn't right now and I'd rather
+                # get rid of aliased=True completely
+                raise sa_exc.InvalidRequestError(
+                    "The aliased=True parameter on query.join() only works "
+                    "with an ORM entity, not a plain selectable, as the "
+                    "target."
+                )
 
         aliased_entity = (
             right_mapper
@@ -2699,35 +2715,29 @@ class Query(object):
             right = aliased(right, flat=True)
             need_adapter = True
 
-        # if an alias() of the right side was generated here,
-        # apply an adapter to all subsequent filter() calls
-        # until reset_joinpoint() is called.
         if need_adapter:
-            self._filter_aliases = ORMAdapter(
-                right,
-                equivalents=right_mapper
-                and right_mapper._equivalent_columns
-                or {},
-                chain_to=self._filter_aliases,
+            assert right_mapper
+
+            # if an alias() of the right side was generated,
+            # apply an adapter to all subsequent filter() calls
+            # until reset_joinpoint() is called.
+            adapter = ORMAdapter(
+                right, equivalents=right_mapper._equivalent_columns
             )
+            self._filter_aliases += (adapter,)
+
+            # if an alias() on the right side was generated,
+            # which is intended to wrap a the right side in a subquery,
+            # ensure that columns retrieved from this target in the result
+            # set are also adapted.
+            if not create_aliases:
+                self._mapper_loads_polymorphically_with(right_mapper, adapter)
 
         # if the onclause is a ClauseElement, adapt it with any
         # adapters that are in place right now
         if isinstance(onclause, expression.ClauseElement):
             onclause = self._adapt_clause(onclause, True, True)
 
-        # if an alias() on the right side was generated,
-        # which is intended to wrap a the right side in a subquery,
-        # ensure that columns retrieved from this target in the result
-        # set are also adapted.
-        if aliased_entity and not create_aliases:
-            self._mapper_loads_polymorphically_with(
-                right_mapper,
-                ORMAdapter(
-                    right, equivalents=right_mapper._equivalent_columns
-                ),
-            )
-
         # if joining on a MapperProperty path,
         # track the path to prevent redundant joins
         if not create_aliases and prop:
@@ -2744,7 +2754,7 @@ class Query(object):
 
     def _reset_joinpoint(self):
         self._joinpoint = self._joinpath
-        self._filter_aliases = None
+        self._filter_aliases = ()
 
     @_generative(_no_statement_condition)
     def reset_joinpoint(self):

diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py
index 6c565c2e6000f38f7f5fdea05b0bc4ec6c23434e..776278469603cbc73f37453a5a076c76e8cdca00 100644 (file)
--- a/lib/sqlalchemy/orm/util.py
+++ b/lib/sqlalchemy/orm/util.py
@@ -363,7 +363,6 @@ class ORMAdapter(sql_util.ColumnAdapter):
         entity,
         equivalents=None,
         adapt_required=False,
-        chain_to=None,
         allow_label_resolve=True,
         anonymize_labels=False,
     ):
@@ -381,7 +380,6 @@ class ORMAdapter(sql_util.ColumnAdapter):
             self,
             selectable,
             equivalents,
-            chain_to,
             adapt_required=adapt_required,
             allow_label_resolve=allow_label_resolve,
             anonymize_labels=anonymize_labels,

diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py
index 3ebb91e9665658cc7dc76fc75ef650b18506cd7b..9780caa1c6bc8b2e30eb75479484ef813e73f6d8 100644 (file)
--- a/lib/sqlalchemy/sql/util.py
+++ b/lib/sqlalchemy/sql/util.py
@@ -830,7 +830,6 @@ class ColumnAdapter(ClauseAdapter):
         self,
         selectable,
         equivalents=None,
-        chain_to=None,
         adapt_required=False,
         include_fn=None,
         exclude_fn=None,
@@ -848,8 +847,6 @@ class ColumnAdapter(ClauseAdapter):
             anonymize_labels=anonymize_labels,
         )
 
-        if chain_to:
-            self.chain(chain_to)
         self.columns = util.populate_column_dict(self._locate_col)
         if self.include_fn or self.exclude_fn:
             self.columns = self._IncludeExcludeMapping(self, self.columns)

diff --git a/test/orm/test_froms.py b/test/orm/test_froms.py
index 9b68136bdd5c8c4ce0cbc2fbb7b51cb02d349a7b..fc2fb670ccb7bd4cb70a977a337777613d4b9c2c 100644 (file)
--- a/test/orm/test_froms.py
+++ b/test/orm/test_froms.py
@@ -3555,7 +3555,7 @@ class TestOverlyEagerEquivalentCols(fixtures.MappedTest):
         sess.flush()
 
         q = sess.query(Base).outerjoin("sub2", aliased=True)
-        assert sub1.c.id not in q._filter_aliases.equivalents
+        assert sub1.c.id not in q._filter_aliases[0].equivalents
 
         eq_(
             sess.query(Base)

diff --git a/test/orm/test_joins.py b/test/orm/test_joins.py
index 5aefe61cccd7e9601e0d17ec8f8f732775e48554..046d5c3e22bf9f92c49d78ae106716aacfcd1b8d 100644 (file)
--- a/test/orm/test_joins.py
+++ b/test/orm/test_joins.py
@@ -2588,6 +2588,27 @@ class JoinFromSelectableTest(fixtures.MappedTest, AssertsCompiledSQL):
             "ON anon_1.t1_id = table1.id",
         )
 
+    def test_mapped_to_select_implicit_left_w_aliased(self):
+        T1, T2 = self.classes.T1, self.classes.T2
+
+        sess = Session()
+        subq = (
+            sess.query(T2.t1_id, func.count(T2.id).label("count"))
+            .group_by(T2.t1_id)
+            .subquery()
+        )
+
+        assert_raises_message(
+            sa_exc.InvalidRequestError,
+            r"The aliased=True parameter on query.join\(\) only works with "
+            "an ORM entity, not a plain selectable, as the target.",
+            # this doesn't work, so have it raise an error
+            sess.query(T1.id).join,
+            subq,
+            subq.c.t1_id == T1.id,
+            aliased=True,
+        )
+
 
 class MultiplePathTest(fixtures.MappedTest, AssertsCompiledSQL):
     @classmethod