From: Mike Bayer Date: Fri, 18 Jan 2019 04:38:40 +0000 (-0500) Subject: Cleanup with query aliasing X-Git-Tag: rel_1_3_0b2~17 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1c56b6049a3fdd1122a4c82ae5757332d3753146;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 2bd79a2cdd..387a72d0b9 100644 --- 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 6c565c2e60..7762784696 100644 --- 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 3ebb91e966..9780caa1c6 100644 --- 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 9b68136bdd..fc2fb670cc 100644 --- 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 5aefe61ccc..046d5c3e22 100644 --- 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