From: Mike Bayer Date: Wed, 15 Apr 2015 21:30:23 +0000 (-0400) Subject: - Identified an inconsistency when handling :meth:`.Query.join` to the X-Git-Tag: rel_1_0_0~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=2bc8c92279a165563b7ebcf82c96cd3cd7ede2a2;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - Identified an inconsistency when handling :meth:`.Query.join` to the same target more than once; it implicitly dedupes only in the case of a relationship join, and due to :ticket:`3233`, in 1.0 a join to the same table twice behaves differently than 0.9 in that it no longer erroneously aliases. To help document this change, the verbiage regarding :ticket:`3233` in the migration notes has been generalized, and a warning has been added when :meth:`.Query.join` is called against the same target relationship more than once. fixes #3367 --- diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst index 17d8942d69..6f9384ccb7 100644 --- a/doc/build/changelog/changelog_10.rst +++ b/doc/build/changelog/changelog_10.rst @@ -18,6 +18,19 @@ .. changelog:: :version: 1.0.0 + .. change:: + :tags: bug, orm + :tickets: 3367 + + Identified an inconsistency when handling :meth:`.Query.join` to the + same target more than once; it implicitly dedupes only in the case of + a relationship join, and due to :ticket:`3233`, in 1.0 a join + to the same table twice behaves differently than 0.9 in that it no + longer erroneously aliases. To help document this change, + the verbiage regarding :ticket:`3233` in the migration notes has + been generalized, and a warning has been added when :meth:`.Query.join` + is called against the same target relationship more than once. + .. change:: :tags: bug, orm :tickets: 3364 diff --git a/doc/build/changelog/migration_10.rst b/doc/build/changelog/migration_10.rst index f4ead01aac..a6f73709a6 100644 --- a/doc/build/changelog/migration_10.rst +++ b/doc/build/changelog/migration_10.rst @@ -1092,18 +1092,83 @@ joined loader options can still be used:: .. _bug_3233: -Single inheritance join targets will no longer sometimes implicitly alias themselves ------------------------------------------------------------------------------------- +Changes and fixes in handling of duplicate join targets +-------------------------------------------------------- + +Changes here encompass bugs where an unexpected and inconsistent +behavior would occur in some scenarios when joining to an entity +twice, or to multple single-table entities against the same table, +without using a relationship-based ON clause, as well as when joining +multiple times to the same target relationship. + +Starting with a mapping as:: + + from sqlalchemy import Integer, Column, String, ForeignKey + from sqlalchemy.orm import Session, relationship + from sqlalchemy.ext.declarative import declarative_base + + Base = declarative_base() + + class A(Base): + __tablename__ = 'a' + id = Column(Integer, primary_key=True) + bs = relationship("B") + + class B(Base): + __tablename__ = 'b' + id = Column(Integer, primary_key=True) + a_id = Column(ForeignKey('a.id')) + +A query that joins to ``A.bs`` twice:: + + print s.query(A).join(A.bs).join(A.bs) + +Will render:: + + SELECT a.id AS a_id + FROM a JOIN b ON a.id = b.a_id + +The query deduplicates the redundant ``A.bs`` because it is attempting +to support a case like the following:: + + s.query(A).join(A.bs).\ + filter(B.foo == 'bar').\ + reset_joinpoint().join(A.bs, B.cs).filter(C.bar == 'bat') + +That is, the ``A.bs`` is part of a "path". As part of :ticket:`3367`, +arriving at the same endpoint twice without it being part of a +larger path will now emit a warning:: + + SAWarning: Pathed join target A.bs has already been joined to; skipping + +The bigger change involves when joining to an entity without using a +relationship-bound path. If we join to ``B`` twice:: + + print s.query(A).join(B, B.a_id == A.id).join(B, B.a_id == A.id) + +In 0.9, this would render as follows:: + + SELECT a.id AS a_id + FROM a JOIN b ON b.a_id = a.id JOIN b AS b_1 ON b_1.a_id = a.id + +This is problematic since the aliasing is implicit and in the case of different +ON clauses can lead to unpredictable results. + +In 1.0, no automatic aliasing is applied and we get:: + + SELECT a.id AS a_id + FROM a JOIN b ON b.a_id = a.id JOIN b ON b.a_id = a.id -This is a bug where an unexpected and inconsistent behavior would occur -in some scenarios when joining to a single-table-inheritance entity. The -difficulty this might cause is that the query is supposed to raise an error, -as it is invalid SQL, however the bug would cause an alias to be added which -makes the query "work". The issue is confusing because this aliasing -is not applied consistently and could change based on the nature of the query -preceding the join. +This will raise an error from the database. While it might be nice if +the "duplicate join target" acted identically if we joined both from +redundant relationships vs. redundant non-relationship based targets, +for now we are only changing the behavior in the more serious case where +implicit aliasing would have occurred previously, and only emitting a warning +in the relationship case. Ultimately, joining to the same thing twice without +any aliasing to disambiguate should raise an error in all cases. -A simple example is:: +The change also has an impact on single-table inheritance targets. Using +a mapping as follows:: from sqlalchemy import Integer, Column, String, ForeignKey from sqlalchemy.orm import Session, relationship @@ -1151,7 +1216,8 @@ the identical SQL:: WHERE a.type IN (:type_2) The above SQL is invalid, as it renders "a" within the FROM list twice. -The bug however would occur with the second query only and render this instead:: +However, the implicit aliasing bug would occur with the second query only +and render this instead:: SELECT a.id AS a_id, a.type AS a_type FROM a JOIN b ON b.a_id = a.id JOIN a AS a_1 @@ -1173,6 +1239,7 @@ as all the subclasses normally refer to the same table:: print s.query(ASub1).join(B, ASub1.b).join(asub2_alias, B.a.of_type(asub2_alias)) :ticket:`3233` +:ticket:`3367` Deferred Columns No Longer Implicitly Undefer diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py index d02c071dbd..2bd68899b9 100644 --- a/lib/sqlalchemy/orm/query.py +++ b/lib/sqlalchemy/orm/query.py @@ -1815,7 +1815,8 @@ class Query(object): # convert to a tuple. keys = (keys,) - for arg1 in util.to_list(keys): + 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 @@ -1894,6 +1895,11 @@ class Query(object): jp = self._joinpoint[edge].copy() jp['prev'] = (edge, self._joinpoint) self._update_joinpoint(jp) + + if idx == len(keylist) - 1: + util.warn( + "Pathed join target %s has already " + "been joined to; skipping" % prop) continue elif onclause is not None and right_entity is None: diff --git a/test/orm/test_joins.py b/test/orm/test_joins.py index 23d220dccb..540056dae9 100644 --- a/test/orm/test_joins.py +++ b/test/orm/test_joins.py @@ -750,6 +750,17 @@ class JoinTest(QueryTest, AssertsCompiledSQL): filter_by(id=3).outerjoin('orders','address').filter_by(id=1).all() assert [User(id=7, name='jack')] == result + def test_raises_on_dupe_target_rel(self): + User = self.classes.User + + assert_raises_message( + sa.exc.SAWarning, + "Pathed join target Order.items has already been joined to; " + "skipping", + lambda: create_session().query(User).outerjoin('orders', 'items').\ + outerjoin('orders', 'items') + ) + def test_from_joinpoint(self): Item, User, Order = (self.classes.Item, self.classes.User,