diff options
| -rw-r--r-- | doc/build/changelog/migration_13.rst | 67 | ||||
| -rw-r--r-- | doc/build/changelog/unreleased_13/4365.rst | 18 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/query.py | 426 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/strategies.py | 16 | ||||
| -rw-r--r-- | lib/sqlalchemy/sql/selectable.py | 14 | ||||
| -rw-r--r-- | lib/sqlalchemy/sql/util.py | 89 | ||||
| -rw-r--r-- | test/orm/test_eager_relations.py | 5 | ||||
| -rw-r--r-- | test/orm/test_joins.py | 297 |
8 files changed, 772 insertions, 160 deletions
diff --git a/doc/build/changelog/migration_13.rst b/doc/build/changelog/migration_13.rst index cec3d37ff..a3a1ab2cd 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 000000000..da34bbc94 --- /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 cace2e54a..93b9a85be 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 9d83952d3..47791f9b9 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 64886b326..f64f152c4 100644 --- a/lib/sqlalchemy/sql/selectable.py +++ b/lib/sqlalchemy/sql/selectable.py @@ -987,6 +987,19 @@ class Join(FromClause): 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): constraints = collections.defaultdict(list) @@ -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 f15cc95ed..12cfe09d1 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 3c669d90d..a2a110b8c 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 54be45233..f74851bd0 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), |