From: Mike Bayer Date: Mon, 12 Nov 2018 23:27:34 +0000 (-0500) Subject: Allow join() to pick the best candidate from multiple froms/entities X-Git-Tag: rel_1_3_0b1~9^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7dcfd1e019e1c0ebceba06d6684f5bf64a2efb71;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Allow join() to pick the best candidate from multiple froms/entities Refactored :meth:`.Query.join` to further clarify the individual components of structuring the join. This refactor adds the ability for :meth:`.Query.join` to determine the most appropriate "left" side of the join when there is more than one element in the FROM list or the query is against multiple entities. In particular this targets the regression we saw in :ticket:`4363` but is also of general use. The codepaths within :meth:`.Query.join` are now easier to follow and the error cases are decided more specifically at an earlier point in the operation. Fixes: #4365 Change-Id: I403f451243904a020ceab4c3f94bead550c7b2d5 --- diff --git a/doc/build/changelog/migration_13.rst b/doc/build/changelog/migration_13.rst index cec3d37ffa..a3a1ab2cde 100644 --- a/doc/build/changelog/migration_13.rst +++ b/doc/build/changelog/migration_13.rst @@ -190,6 +190,73 @@ to ``None``:: :ticket:`4308` +.. _change_4365: + +Query.join() handles ambiguity in deciding the "left" side more explicitly +--------------------------------------------------------------------------- + +Historically, given a query like the following:: + + u_alias = aliased(User) + session.query(User, u_alias).join(Address) + +given the standard tutorial mappings, the query would produce a FROM clause +as: + +.. sourcecode:: sql + + SELECT ... + FROM users AS users_1, users JOIN addresses ON users.id = addresses.user_id + +That is, the JOIN would implcitly be against the first entity that matches. +The new behavior is that an exception requests that this ambiguity be +resolved:: + + sqlalchemy.exc.InvalidRequestError: Can't determine which FROM clause to + join from, there are multiple FROMS which can join to this entity. + Try adding an explicit ON clause to help resolve the ambiguity. + +The solution is to provide an ON clause, either as an expression:: + + # join to User + session.query(User, u_alias).join(Address, Address.user_id == User.id) + + # join to u_alias + session.query(User, u_alias).join(Address, Address.user_id == u_alias.id) + +Or to use the relationship attribute, if available:: + + # join to User + session.query(User, u_alias).join(Address, User.addresses) + + # join to u_alias + session.query(User, u_alias).join(Address, u_alias.addresses) + +The change includes that a join can now correctly link to a FROM clause that +is not the first element in the list if the join is otherwise non-ambiguous:: + + session.query(func.current_timestamp(), User).join(Address) + +Prior to this enhancement, the above query would raise:: + + sqlalchemy.exc.InvalidRequestError: Don't know how to join from + CURRENT_TIMESTAMP; please use select_from() to establish the + left entity/selectable of this join + +Now the query works fine: + +.. sourcecode:: sql + + SELECT CURRENT_TIMESTAMP AS current_timestamp_1, users.id AS users_id, + users.name AS users_name, users.fullname AS users_fullname, + users.password AS users_password + FROM users JOIN addresses ON users.id = addresses.user_id + +Overall the change is directly towards Python's "explicit is better than +implicit" philosophy. + +:ticket:`4365` + .. _change_4353: Many-to-one replacement won't raise for "raiseload" or detached for "old" object diff --git a/doc/build/changelog/unreleased_13/4365.rst b/doc/build/changelog/unreleased_13/4365.rst new file mode 100644 index 0000000000..da34bbc94b --- /dev/null +++ b/doc/build/changelog/unreleased_13/4365.rst @@ -0,0 +1,18 @@ +.. change:: + :tags: bug, orm + :tickets: 4365 + + Refactored :meth:`.Query.join` to further clarify the individual components + of structuring the join. This refactor adds the ability for + :meth:`.Query.join` to determine the most appropriate "left" side of the + join when there is more than one element in the FROM list or the query is + against multiple entities. If more than one FROM/entity matches, an error + is raised that asks for an ON clause to be specified to resolve the + ambiguity. In particular this targets the regression we saw in + :ticket:`4363` but is also of general use. The codepaths within + :meth:`.Query.join` are now easier to follow and the error cases are + decided more specifically at an earlier point in the operation. + + .. seealso:: + + :ref:`change_4365` diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py index cace2e54af..93b9a85be8 100644 --- a/lib/sqlalchemy/orm/query.py +++ b/lib/sqlalchemy/orm/query.py @@ -2182,28 +2182,37 @@ class Query(object): # convert to a tuple. keys = (keys,) + # Query.join() accepts a list of join paths all at once. + # step one is to iterate through these paths and determine the + # intent of each path individually. as we encounter a path token, + # we add a new ORMJoin construct to the self._from_obj tuple, + # either by adding a new element to it, or by replacing an existing + # element with a new ORMJoin. keylist = util.to_list(keys) for idx, arg1 in enumerate(keylist): if isinstance(arg1, tuple): # "tuple" form of join, multiple # tuples are accepted as well. The simpler - # "2-arg" form is preferred. May deprecate - # the "tuple" usage. + # "2-arg" form is preferred. arg1, arg2 = arg1 else: arg2 = None # determine onclause/right_entity. there # is a little bit of legacy behavior still at work here - # which means they might be in either order. may possibly - # lock this down to (right_entity, onclause) in 0.6. + # which means they might be in either order. if isinstance( arg1, (interfaces.PropComparator, util.string_types)): - right_entity, onclause = arg2, arg1 + right, onclause = arg2, arg1 else: - right_entity, onclause = arg1, arg2 + right, onclause = arg1, arg2 - left_entity = prop = None + if onclause is None: + r_info = inspect(right) + if not r_info.is_selectable and not hasattr(r_info, 'mapper'): + raise sa_exc.ArgumentError( + "Expected mapped entity or " + "selectable/table as join target") if isinstance(onclause, interfaces.PropComparator): of_type = getattr(onclause, '_of_type', None) @@ -2211,44 +2220,46 @@ class Query(object): of_type = None if isinstance(onclause, util.string_types): - left_entity = self._joinpoint_zero() - - descriptor = _entity_descriptor(left_entity, onclause) - onclause = descriptor + # string given, e.g. query(Foo).join("bar"). + # we look to the left entity or what we last joined + # towards + onclause = _entity_descriptor(self._joinpoint_zero(), onclause) # check for q.join(Class.propname, from_joinpoint=True) - # and Class is that of the current joinpoint + # and Class corresponds at the mapper level to the current + # joinpoint. this match intentionally looks for a non-aliased + # class-bound descriptor as the onclause and if it matches the + # current joinpoint at the mapper level, it's used. This + # is a very old use case that is intended to make it easier + # to work with the aliased=True flag, which is also something + # that probably shouldn't exist on join() due to its high + # complexity/usefulness ratio elif from_joinpoint and \ isinstance(onclause, interfaces.PropComparator): - left_entity = onclause._parententity + jp0 = self._joinpoint_zero() + info = inspect(jp0) - info = inspect(self._joinpoint_zero()) - left_mapper, left_selectable, left_is_aliased = \ - getattr(info, 'mapper', None), \ - info.selectable, \ - getattr(info, 'is_aliased_class', None) - - if left_mapper is left_entity: - left_entity = self._joinpoint_zero() - descriptor = _entity_descriptor(left_entity, - onclause.key) - onclause = descriptor + if getattr(info, 'mapper', None) is onclause._parententity: + onclause = _entity_descriptor(jp0, onclause.key) if isinstance(onclause, interfaces.PropComparator): - if right_entity is None: + # descriptor/property given (or determined); this tells + # us explicitly what the expected "left" side of the join is. + if right is None: if of_type: - right_entity = of_type + right = of_type else: - right_entity = onclause.property.mapper + right = onclause.property.mapper + + left = onclause._parententity - left_entity = onclause._parententity + alias = self._polymorphic_adapters.get(left, None) - alias = self._polymorphic_adapters.get(left_entity, None) # could be None or could be ColumnAdapter also if isinstance(alias, ORMAdapter) and \ - alias.mapper.isa(left_entity): - left_entity = alias.aliased_class - onclause = getattr(left_entity, onclause.key) + alias.mapper.isa(left): + left = alias.aliased_class + onclause = getattr(left, onclause.key) prop = onclause.property if not isinstance(onclause, attributes.QueryableAttribute): @@ -2257,7 +2268,7 @@ class Query(object): if not create_aliases: # check for this path already present. # don't render in that case. - edge = (left_entity, right_entity, prop.key) + edge = (left, right, prop.key) if edge in self._joinpoint: # The child's prev reference might be stale -- # it could point to a parent older than the @@ -2270,50 +2281,248 @@ class Query(object): jp['prev'] = (edge, self._joinpoint) self._update_joinpoint(jp) + # warn only on the last element of the list if idx == len(keylist) - 1: util.warn( "Pathed join target %s has already " "been joined to; skipping" % prop) continue + else: + # no descriptor/property given; we will need to figure out + # what the effective "left" side is + prop = left = None - elif onclause is not None and right_entity is None: - # TODO: no coverage here - raise NotImplementedError("query.join(a==b) not supported.") - + # figure out the final "left" and "right" sides and create an + # ORMJoin to add to our _from_obj tuple self._join_left_to_right( - left_entity, - right_entity, onclause, - outerjoin, full, create_aliases, prop) + left, right, onclause, prop, create_aliases, + outerjoin, full + ) - def _join_left_to_right(self, left, right, - onclause, outerjoin, full, create_aliases, prop): - """append a JOIN to the query's from clause.""" + def _join_left_to_right( + self, left, right, onclause, prop, + create_aliases, outerjoin, full): + """given raw "left", "right", "onclause" parameters consumed from + a particular key within _join(), add a real ORMJoin object to + our _from_obj list (or augment an existing one) + + """ self._polymorphic_adapters = self._polymorphic_adapters.copy() if left is None: - if self._from_obj: - left = self._from_obj[0] - elif self._entities: - left = self._entities[0].entity_zero_or_selectable + # left not given (e.g. no relationship object/name specified) + # figure out the best "left" side based on our existing froms / + # entities + assert prop is None + left, replace_from_obj_index, use_entity_index = \ + self._join_determine_implicit_left_side(left, right, onclause) + else: + # left is given via a relationship/name. Determine where in our + # "froms" list it should be spliced/appended as well as what + # existing entity it corresponds to. + assert prop is not None + replace_from_obj_index, use_entity_index = \ + self._join_place_explicit_left_side(left) + + # this should never happen because we would not have found a place + # to join on + assert left is not right or create_aliases + + # the right side as given often needs to be adapted. additionally + # a lot of things can be wrong with it. handle all that and + # get back the the new effective "right" side + r_info, right, onclause = self._join_check_and_adapt_right_side( + left, right, onclause, prop, create_aliases, + ) - if left is None: - if self._entities: - problem = "Don't know how to join from %s" % self._entities[0] + if replace_from_obj_index is not None: + # splice into an existing element in the + # self._from_obj list + left_clause = self._from_obj[replace_from_obj_index] + + self._from_obj = ( + self._from_obj[:replace_from_obj_index] + + (orm_join( + left_clause, right, + onclause, isouter=outerjoin, full=full), ) + + self._from_obj[replace_from_obj_index + 1:]) + else: + # add a new element to the self._from_obj list + + if use_entity_index is not None: + # why doesn't this work as .entity_zero_or_selectable? + left_clause = self._entities[use_entity_index].selectable + else: + left_clause = left + + self._from_obj = self._from_obj + ( + orm_join( + left_clause, right, onclause, + isouter=outerjoin, full=full), + ) + + def _join_determine_implicit_left_side(self, left, right, onclause): + """When join conditions don't express the left side explicitly, + determine if an existing FROM or entity in this query + can serve as the left hand side. + + """ + + # when we are here, it means join() was called without an ORM- + # specific way of telling us what the "left" side is, e.g.: + # + # join(RightEntity) + # + # or + # + # join(RightEntity, RightEntity.foo == LeftEntity.bar) + # + + r_info = inspect(right) + + replace_from_obj_index = use_entity_index = None + + if self._from_obj: + # we have a list of FROMs already. So by definition this + # join has to connect to one of those FROMs. + + indexes = sql_util.find_left_clause_to_join_from( + self._from_obj, + r_info.selectable, onclause) + + if len(indexes) == 1: + replace_from_obj_index = indexes[0] + left = self._from_obj[replace_from_obj_index] + elif len(indexes) > 1: + raise sa_exc.InvalidRequestError( + "Can't determine which FROM clause to join " + "from, there are multiple FROMS which can " + "join to this entity. Try adding an explicit ON clause " + "to help resolve the ambiguity.") else: - problem = "No entities to join from" + raise sa_exc.InvalidRequestError( + "Don't know how to join to %s; please use " + "an ON clause to more clearly establish the left " + "side of this join" % (right, ) + ) + + elif self._entities: + # we have no explicit FROMs, so the implicit left has to + # come from our list of entities. + + potential = {} + for entity_index, ent in enumerate(self._entities): + entity = ent.entity_zero_or_selectable + if entity is None: + continue + ent_info = inspect(entity) + if ent_info is r_info: # left and right are the same, skip + continue + + # by using a dictionary with the selectables as keys this + # de-duplicates those selectables as occurs when the query is + # against a series of columns from the same selectable + if isinstance(ent, _MapperEntity): + potential[ent.selectable] = (entity_index, entity) + else: + potential[ent_info.selectable] = (None, entity) + all_clauses = list(potential.keys()) + indexes = sql_util.find_left_clause_to_join_from( + all_clauses, r_info.selectable, onclause) + + if len(indexes) == 1: + use_entity_index, left = potential[all_clauses[indexes[0]]] + elif len(indexes) > 1: + raise sa_exc.InvalidRequestError( + "Can't determine which FROM clause to join " + "from, there are multiple FROMS which can " + "join to this entity. Try adding an explicit ON clause " + "to help resolve the ambiguity.") + else: + raise sa_exc.InvalidRequestError( + "Don't know how to join to %s; please use " + "an ON clause to more clearly establish the left " + "side of this join" % (right, ) + ) + else: raise sa_exc.InvalidRequestError( - "%s; please use " + "No entities to join from; please use " "select_from() to establish the left " - "entity/selectable of this join" % problem) + "entity/selectable of this join") - if left is right and \ - not create_aliases: - raise sa_exc.InvalidRequestError( - "Can't construct a join from %s to %s, they " - "are the same entity" % - (left, right)) + return left, replace_from_obj_index, use_entity_index + + def _join_place_explicit_left_side(self, left): + """When join conditions express a left side explicitly, determine + where in our existing list of FROM clauses we should join towards, + or if we need to make a new join, and if so is it from one of our + existing entities. + + """ + + # when we are here, it means join() was called with an indicator + # as to an exact left side, which means a path to a + # RelationshipProperty was given, e.g.: + # + # join(RightEntity, LeftEntity.right) + # + # or + # + # join(LeftEntity.right) + # + # as well as string forms: + # + # join(RightEntity, "right") + # + # etc. + # + + replace_from_obj_index = use_entity_index = None + + l_info = inspect(left) + if self._from_obj: + indexes = sql_util.find_left_clause_that_matches_given( + self._from_obj, l_info.selectable) + + if len(indexes) > 1: + raise sa_exc.InvalidRequestError( + "Can't identify which entity in which to assign the " + "left side of this join. Please use a more specific " + "ON clause.") + + # have an index, means the left side is already present in + # an existing FROM in the self._from_obj tuple + if indexes: + replace_from_obj_index = indexes[0] + + # no index, means we need to add a new element to the + # self._from_obj tuple + + # no from element present, so we will have to add to the + # self._from_obj tuple. Determine if this left side matches up + # with existing mapper entities, in which case we want to apply the + # aliasing / adaptation rules present on that entity if any + if replace_from_obj_index is None and \ + self._entities and hasattr(l_info, 'mapper'): + for idx, ent in enumerate(self._entities): + # TODO: should we be checking for multiple mapper entities + # matching? + if isinstance(ent, _MapperEntity) and ent.corresponds_to(left): + use_entity_index = idx + break + + return replace_from_obj_index, use_entity_index + + def _join_check_and_adapt_right_side( + self, left, right, onclause, prop, create_aliases): + """transform the "right" side of the join as well as the onclause + according to polymorphic mapping translations, aliasing on the query + or on the join, special cases where the right and left side have + overlapping tables. + + """ l_info = inspect(left) r_info = inspect(right) @@ -2341,34 +2550,10 @@ class Query(object): "Can't join table/selectable '%s' to itself" % l_info.selectable) - right, onclause = self._prepare_right_side( - r_info, right, onclause, - create_aliases, - prop, overlap) - - # if joining on a MapperProperty path, - # track the path to prevent redundant joins - if not create_aliases and prop: - self._update_joinpoint({ - '_joinpoint_entity': right, - 'prev': ((left, right, prop.key), self._joinpoint) - }) - else: - self._joinpoint = {'_joinpoint_entity': right} - - self._join_to_left(l_info, left, right, onclause, outerjoin, full) - - def _prepare_right_side(self, r_info, right, onclause, create_aliases, - prop, overlap): - info = r_info - right_mapper, right_selectable, right_is_aliased = \ - getattr(info, 'mapper', None), \ - info.selectable, \ - getattr(info, 'is_aliased_class', False) - - if right_mapper: - self._join_entities += (info, ) + getattr(r_info, 'mapper', None), \ + r_info.selectable, \ + getattr(r_info, 'is_aliased_class', False) if right_mapper and prop and \ not right_mapper.common_parent(prop.mapper): @@ -2377,6 +2562,11 @@ class Query(object): "the right side of join condition %s" % (right, onclause) ) + # _join_entities is used as a hint for single-table inheritance + # purposes at the moment + if hasattr(r_info, 'mapper'): + self._join_entities += (r_info, ) + if not right_mapper and prop: right_mapper = prop.mapper @@ -2408,9 +2598,8 @@ class Query(object): ( right_mapper.with_polymorphic and isinstance( right_mapper._with_polymorphic_selectable, - expression.Alias) - or - overlap # test for overlap: + expression.Alias) or overlap + # test for overlap: # orm/inheritance/relationships.py # SelfReferentialM2MTest ) @@ -2447,52 +2636,17 @@ class Query(object): ) ) - return right, onclause - - def _join_to_left(self, l_info, left, right, onclause, outerjoin, full): - info = l_info - left_mapper = getattr(info, 'mapper', None) - left_selectable = info.selectable - - if self._from_obj: - replace_clause_index, clause = sql_util.find_join_source( - self._from_obj, - left_selectable) - if clause is not None: - try: - clause = orm_join(clause, - right, - onclause, isouter=outerjoin, full=full) - except sa_exc.ArgumentError as ae: - raise sa_exc.InvalidRequestError( - "Could not find a FROM clause to join from. " - "Tried joining to %s, but got: %s" % (right, ae)) - - self._from_obj = \ - self._from_obj[:replace_clause_index] + \ - (clause, ) + \ - self._from_obj[replace_clause_index + 1:] - return - - if left_mapper: - for ent in self._entities: - if ent.corresponds_to(left): - clause = ent.selectable - break - else: - clause = left + # if joining on a MapperProperty path, + # track the path to prevent redundant joins + if not create_aliases and prop: + self._update_joinpoint({ + '_joinpoint_entity': right, + 'prev': ((left, right, prop.key), self._joinpoint) + }) else: - clause = left_selectable + self._joinpoint = {'_joinpoint_entity': right} - assert clause is not None - try: - clause = orm_join( - clause, right, onclause, isouter=outerjoin, full=full) - except sa_exc.ArgumentError as ae: - raise sa_exc.InvalidRequestError( - "Could not find a FROM clause to join from. " - "Tried joining to %s, but got: %s" % (right, ae)) - self._from_obj = self._from_obj + (clause,) + return right, inspect(right), onclause def _reset_joinpoint(self): self._joinpoint = self._joinpath @@ -4049,8 +4203,8 @@ class _BundleEntity(_QueryEntity): return None def corresponds_to(self, entity): - # TODO: this seems to have no effect for - # _ColumnEntity either + # TODO: we might be able to implement this but for now + # we are working around it return False @property @@ -4226,8 +4380,6 @@ class _ColumnEntity(_QueryEntity): self.froms.add(ext_info.selectable) def corresponds_to(self, entity): - # TODO: just returning False here, - # no tests fail if self.entity_zero is None: return False elif _is_aliased_class(entity): diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 9d83952d33..47791f9b96 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -1544,14 +1544,24 @@ class JoinedLoader(AbstractRelationshipLoader): if entity not in context.eager_joins and \ not should_nest_selectable and \ context.from_clause: - index, clause = sql_util.find_join_source( + indexes = sql_util.find_left_clause_that_matches_given( context.from_clause, entity.selectable) - if clause is not None: + + if len(indexes) > 1: + # for the eager load case, I can't reproduce this right + # now. For query.join() I can. + raise sa_exc.InvalidRequestError( + "Can't identify which entity in which to joined eager " + "load from. Please use an exact match when specifying " + "the join path.") + + if indexes: + clause = context.from_clause[indexes[0]] # join to an existing FROM clause on the query. # key it to its list index in the eager_joins dict. # Query._compile_context will adapt as needed and # append to the FROM clause of the select(). - entity_key, default_towrap = index, clause + entity_key, default_towrap = indexes[0], clause if entity_key is None: entity_key, default_towrap = entity, entity.selectable diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py index 64886b3269..f64f152c48 100644 --- a/lib/sqlalchemy/sql/selectable.py +++ b/lib/sqlalchemy/sql/selectable.py @@ -986,6 +986,19 @@ class Join(FromClause): else: return and_(*crit) + @classmethod + def _can_join(cls, left, right, consider_as_foreign_keys=None): + if isinstance(left, Join): + left_right = left.right + else: + left_right = None + + constraints = cls._joincond_scan_left_right( + a=left, b=right, a_subset=left_right, + consider_as_foreign_keys=consider_as_foreign_keys) + + return bool(constraints) + @classmethod def _joincond_scan_left_right( cls, a, a_subset, b, consider_as_foreign_keys): @@ -1059,6 +1072,7 @@ class Join(FromClause): "Please specify the 'onclause' of this " "join explicitly." % (a.description, b.description)) + def select(self, whereclause=None, **kwargs): r"""Create a :class:`.Select` from this :class:`.Join`. diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py index f15cc95ed6..12cfe09d15 100644 --- a/lib/sqlalchemy/sql/util.py +++ b/lib/sqlalchemy/sql/util.py @@ -49,12 +49,97 @@ def find_join_source(clauses, join_to): """ selectables = list(_from_objects(join_to)) + idx = [] for i, f in enumerate(clauses): for s in selectables: if f.is_derived_from(s): - return i, f + idx.append(i) + return idx + + +def find_left_clause_that_matches_given(clauses, join_from): + """Given a list of FROM clauses and a selectable, + return the indexes from the list of + clauses which is derived from the selectable. + + """ + + selectables = list(_from_objects(join_from)) + liberal_idx = [] + for i, f in enumerate(clauses): + for s in selectables: + # basic check, if f is derived from s. + # this can be joins containing a table, or an aliased table + # or select statement matching to a table. This check + # will match a table to a selectable that is adapted from + # that table. With Query, this suits the case where a join + # is being made to an adapted entity + if f.is_derived_from(s): + liberal_idx.append(i) + break + + # in an extremely small set of use cases, a join is being made where + # there are multiple FROM clauses where our target table is represented + # in more than one, such as embedded or similar. in this case, do + # another pass where we try to get a more exact match where we aren't + # looking at adaption relationships. + if len(liberal_idx) > 1: + conservative_idx = [] + for idx in liberal_idx: + f = clauses[idx] + for s in selectables: + if set(surface_selectables(f)).\ + intersection(surface_selectables(s)): + conservative_idx.append(idx) + break + if conservative_idx: + return conservative_idx + + return liberal_idx + + +def find_left_clause_to_join_from(clauses, join_to, onclause): + """Given a list of FROM clauses, a selectable, + and optional ON clause, return a list of integer indexes from the + clauses list indicating the clauses that can be joined from. + + The presense of an "onclause" indicates that at least one clause can + definitely be joined from; if the list of clauses is of length one + and the onclause is given, returns that index. If the list of clauses + is more than length one, and the onclause is given, attempts to locate + which clauses contain the same columns. + + """ + idx = [] + selectables = set(_from_objects(join_to)) + + # if we are given more than one target clause to join + # from, use the onclause to provide a more specific answer. + # otherwise, don't try to limit, after all, "ON TRUE" is a valid + # on clause + if len(clauses) > 1 and onclause is not None: + resolve_ambiguity = True + cols_in_onclause = _find_columns(onclause) + else: + resolve_ambiguity = False + cols_in_onclause = None + + for i, f in enumerate(clauses): + for s in selectables.difference([f]): + if resolve_ambiguity: + if set(f.c).union(s.c).issuperset(cols_in_onclause): + idx.append(i) + break + elif Join._can_join(f, s) or onclause is not None: + idx.append(i) + break + + # onclause was given and none of them resolved, so assume + # all indexes can match + if not idx and onclause is not None: + return range(len(clauses)) else: - return None, None + return idx def visit_binary_product(fn, expr): diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py index 3c669d90d6..a2a110b8c8 100644 --- a/test/orm/test_eager_relations.py +++ b/test/orm/test_eager_relations.py @@ -3445,9 +3445,10 @@ class MixedEntitiesTest(_fixtures.FixtureTest, testing.AssertsCompiledSQL): ) self.assert_sql_count(testing.db, go, 1) - @testing.exclude( - 'sqlite', '>', (0, ), "sqlite flat out blows it on the multiple JOINs") def test_two_entities_with_joins(self): + # early versions of SQLite could not handle this test + # however as of 2018 and probably for some years before that + # it has no issue with this. Item, Order, User, Address = (self.classes.Item, self.classes.Order, self.classes.User, diff --git a/test/orm/test_joins.py b/test/orm/test_joins.py index 54be452334..f74851bd0e 100644 --- a/test/orm/test_joins.py +++ b/test/orm/test_joins.py @@ -472,17 +472,22 @@ class JoinTest(QueryTest, AssertsCompiledSQL): sess.query(User).outerjoin, "address", foob="bar", bar="bat" ) - def test_left_is_none(self): + def test_left_w_no_entity(self): User = self.classes.User Address = self.classes.Address sess = create_session() - assert_raises_message( - sa_exc.InvalidRequestError, - r"Don't know how to join from x; please use select_from\(\) to " - r"establish the left entity/selectable of this join", - sess.query(literal_column('x'), User).join, Address + self.assert_compile( + sess.query(User, literal_column('x'), ).join(Address), + "SELECT users.id AS users_id, users.name AS users_name, x " + "FROM users JOIN addresses ON users.id = addresses.user_id" + ) + + self.assert_compile( + sess.query(literal_column('x'), User).join(Address), + "SELECT x, users.id AS users_id, users.name AS users_name " + "FROM users JOIN addresses ON users.id = addresses.user_id" ) def test_left_is_none_and_query_has_no_entities(self): @@ -1064,6 +1069,235 @@ class JoinTest(QueryTest, AssertsCompiledSQL): use_default_dialect=True ) + def test_invalid_join_entity_from_single_from_clause(self): + Address, Item = ( + self.classes.Address, self.classes.Item) + sess = create_session() + + q = sess.query(Address).select_from(Address) + + assert_raises_message( + sa.exc.InvalidRequestError, + "Don't know how to join to .*Item.*; " + "please use an ON clause to more clearly establish the " + "left side of this join", + q.join, Item + ) + + def test_invalid_join_entity_from_no_from_clause(self): + Address, Item = ( + self.classes.Address, self.classes.Item) + sess = create_session() + + q = sess.query(Address) + + assert_raises_message( + sa.exc.InvalidRequestError, + "Don't know how to join to .*Item.*; " + "please use an ON clause to more clearly establish the " + "left side of this join", + q.join, Item + ) + + def test_invalid_join_entity_from_multiple_from_clause(self): + """test adding joins onto multiple FROM clauses where + we still need to say there's nothing to JOIN from""" + + User, Address, Item = ( + self.classes.User, self.classes.Address, self.classes.Item) + sess = create_session() + + q = sess.query(Address, User).join(Address.dingaling).\ + join(User.orders) + + assert_raises_message( + sa.exc.InvalidRequestError, + "Don't know how to join to .*Item.*; " + "please use an ON clause to more clearly establish the " + "left side of this join", + q.join, Item + ) + + def test_join_explicit_left_multiple_from_clause(self): + """test adding joins onto multiple FROM clauses where + it is ambiguous which FROM should be used when an + ON clause is given""" + + User = self.classes.User + + sess = create_session() + + u1 = aliased(User) + + # in this case, two FROM objects, one + # is users, the other is u1_alias. + # User.addresses looks for the "users" table and can match + # to both u1_alias and users if the match is not specific enough + q = sess.query(User, u1).\ + select_from(User, u1).\ + join(User.addresses) + + self.assert_compile( + q, + "SELECT users.id AS users_id, users.name AS users_name, " + "users_1.id AS users_1_id, users_1.name AS users_1_name " + "FROM users AS users_1, " + "users JOIN addresses ON users.id = addresses.user_id" + ) + + q = sess.query(User, u1).\ + select_from(User, u1).\ + join(u1.addresses) + + self.assert_compile( + q, + "SELECT users.id AS users_id, users.name AS users_name, " + "users_1.id AS users_1_id, users_1.name AS users_1_name " + "FROM users, " + "users AS users_1 JOIN addresses ON users_1.id = addresses.user_id" + ) + + def test_join_explicit_left_multiple_adapted(self): + """test adding joins onto multiple FROM clauses where + it is ambiguous which FROM should be used when an + ON clause is given""" + + User = self.classes.User + + sess = create_session() + + u1 = aliased(User) + u2 = aliased(User) + + # in this case, two FROM objects, one + # is users, the other is u1_alias. + # User.addresses looks for the "users" table and can match + # to both u1_alias and users if the match is not specific enough + assert_raises_message( + sa_exc.InvalidRequestError, + "Can't identify which entity in which to assign the " + "left side of this join.", + sess.query(u1, u2).select_from(u1, u2).join, + User.addresses + ) + + # more specific ON clause + self.assert_compile( + sess.query(u1, u2).select_from(u1, u2).join(u2.addresses), + "SELECT users_1.id AS users_1_id, users_1.name AS users_1_name, " + "users_2.id AS users_2_id, users_2.name AS users_2_name " + "FROM users AS users_1, " + "users AS users_2 JOIN addresses ON users_2.id = addresses.user_id" + ) + + def test_join_entity_from_multiple_from_clause(self): + """test adding joins onto multiple FROM clauses where + it is ambiguous which FROM should be used""" + + User, Order, Address, Dingaling = ( + self.classes.User, + self.classes.Order, + self.classes.Address, + self.classes.Dingaling) + + sess = create_session() + + q = sess.query(Address, User).join(Address.dingaling).\ + join(User.orders) + + a1 = aliased(Address) + + assert_raises_message( + sa.exc.InvalidRequestError, + "Can't determine which FROM clause to join from, there are " + "multiple FROMS which can join to this entity. " + "Try adding an explicit ON clause to help resolve the ambiguity.", + q.join, a1 + ) + + # to resolve, add an ON clause + + # the user->orders join is chosen to join to a1 + self.assert_compile( + q.join(a1, Order.address_id == a1.id), + "SELECT addresses.id AS addresses_id, " + "addresses.user_id AS addresses_user_id, " + "addresses.email_address AS addresses_email_address, " + "users.id AS users_id, users.name AS users_name " + "FROM addresses JOIN dingalings " + "ON addresses.id = dingalings.address_id, " + "users JOIN orders " + "ON users.id = orders.user_id " + "JOIN addresses AS addresses_1 " + "ON orders.address_id = addresses_1.id" + ) + + # the address->dingalings join is chosen to join to a1 + self.assert_compile( + q.join(a1, Dingaling.address_id == a1.id), + "SELECT addresses.id AS addresses_id, " + "addresses.user_id AS addresses_user_id, " + "addresses.email_address AS addresses_email_address, " + "users.id AS users_id, users.name AS users_name " + "FROM addresses JOIN dingalings " + "ON addresses.id = dingalings.address_id " + "JOIN addresses AS addresses_1 " + "ON dingalings.address_id = addresses_1.id, " + "users JOIN orders ON users.id = orders.user_id" + ) + + def test_join_entity_from_multiple_entities(self): + """test adding joins onto multiple FROM clauses where + it is ambiguous which FROM should be used""" + + Order, Address, Dingaling = ( + self.classes.Order, + self.classes.Address, + self.classes.Dingaling) + + sess = create_session() + + q = sess.query(Order, Dingaling) + + a1 = aliased(Address) + + assert_raises_message( + sa.exc.InvalidRequestError, + "Can't determine which FROM clause to join from, there are " + "multiple FROMS which can join to this entity. " + "Try adding an explicit ON clause to help resolve the ambiguity.", + q.join, a1 + ) + + # to resolve, add an ON clause + + # Order is chosen to join to a1 + self.assert_compile( + q.join(a1, Order.address_id == a1.id), + "SELECT orders.id AS orders_id, orders.user_id AS orders_user_id, " + "orders.address_id AS orders_address_id, " + "orders.description AS orders_description, " + "orders.isopen AS orders_isopen, dingalings.id AS dingalings_id, " + "dingalings.address_id AS dingalings_address_id, " + "dingalings.data AS dingalings_data " + "FROM dingalings, orders " + "JOIN addresses AS addresses_1 " + "ON orders.address_id = addresses_1.id" + ) + + # Dingaling is chosen to join to a1 + self.assert_compile( + q.join(a1, Dingaling.address_id == a1.id), + "SELECT orders.id AS orders_id, orders.user_id AS orders_user_id, " + "orders.address_id AS orders_address_id, " + "orders.description AS orders_description, " + "orders.isopen AS orders_isopen, dingalings.id AS dingalings_id, " + "dingalings.address_id AS dingalings_address_id, " + "dingalings.data AS dingalings_data " + "FROM orders, dingalings JOIN addresses AS addresses_1 " + "ON dingalings.address_id = addresses_1.id" + ) + def test_multiple_adaption(self): Item, Order, User = (self.classes.Item, self.classes.Order, @@ -1614,16 +1848,27 @@ class JoinTest(QueryTest, AssertsCompiledSQL): assert_raises_message( sa_exc.InvalidRequestError, - "Can't join table/selectable 'users' to itself", + "Don't know how to join to .*User.* please use an ON clause to ", sess.query(users.c.id).join, User ) assert_raises_message( sa_exc.InvalidRequestError, - "Can't join table/selectable 'users' to itself", + "Don't know how to join to .*User.* please use an ON clause to ", sess.query(users.c.id).select_from(users).join, User ) + def test_on_clause_no_right_side(self): + User = self.classes.User + Address = self.classes.Address + sess = create_session() + + assert_raises_message( + sa_exc.ArgumentError, + "Expected mapped entity or selectable/table as join target", + sess.query(User).join, User.id == Address.user_id + ) + def test_select_from(self): """Test that the left edge of the join can be set reliably with select_from().""" @@ -1759,10 +2004,25 @@ class JoinFromSelectableTest(fixtures.MappedTest, AssertsCompiledSQL): 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"Can't construct a join from ", - sess.query(subq.c.count, T1.id).join, subq, subq.c.t1_id == T1.id, + # without select_from + self.assert_compile( + sess.query(subq.c.count, T1.id).join(subq, subq.c.t1_id == T1.id), + "SELECT anon_1.count AS anon_1_count, table1.id AS table1_id " + "FROM table1 JOIN " + "(SELECT table2.t1_id AS t1_id, count(table2.id) AS count " + "FROM table2 GROUP BY table2.t1_id) " + "AS anon_1 ON anon_1.t1_id = table1.id" + ) + + # with select_from, same query + self.assert_compile( + sess.query(subq.c.count, T1.id).select_from(T1). + join(subq, subq.c.t1_id == T1.id), + "SELECT anon_1.count AS anon_1_count, table1.id AS table1_id " + "FROM table1 JOIN " + "(SELECT table2.t1_id AS t1_id, count(table2.id) AS count " + "FROM table2 GROUP BY table2.t1_id) " + "AS anon_1 ON anon_1.t1_id = table1.id" ) def test_mapped_select_to_mapped_implicit_left(self): @@ -1772,12 +2032,17 @@ class JoinFromSelectableTest(fixtures.MappedTest, AssertsCompiledSQL): subq = sess.query(T2.t1_id, func.count(T2.id).label('count')).\ group_by(T2.t1_id).subquery() - assert_raises_message( - sa_exc.InvalidRequestError, - "Can't join table/selectable 'table1' to itself", - sess.query(T1.id, subq.c.count).join, T1, subq.c.t1_id == T1.id + # without select_from + self.assert_compile( + sess.query(T1.id, subq.c.count). + join(T1, subq.c.t1_id == T1.id), + "SELECT table1.id AS table1_id, anon_1.count AS anon_1_count " + "FROM (SELECT table2.t1_id AS t1_id, count(table2.id) AS count " + "FROM table2 GROUP BY table2.t1_id) AS anon_1 " + "JOIN table1 ON anon_1.t1_id = table1.id" ) + # with select_from, same query self.assert_compile( sess.query(T1.id, subq.c.count).select_from(subq). join(T1, subq.c.t1_id == T1.id),