- 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

4 files changed, 109 insertions, 12 deletions

diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst
index 17d8942d6..6f9384ccb 100644
--- a/doc/build/changelog/changelog_10.rst
+++ b/doc/build/changelog/changelog_10.rst
@@ -20,6 +20,19 @@
 
     .. 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
 
         Made a small improvement to the heuristics of relationship when
diff --git a/doc/build/changelog/migration_10.rst b/doc/build/changelog/migration_10.rst
index f4ead01aa..a6f73709a 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 d02c071db..2bd68899b 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 23d220dcc..540056dae 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,