Fold entities into existing joins when resolving FROM ambiguity

Fixed 1.3 regression in new "ambiguous FROMs" query logic introduced in
:ref:`change_4365` where a :class:`.Query` that explicitly places an entity
in the FROM clause with :meth:`.Query.select_from` and also joins to it
using :meth:`.Query.join` would later cause an "ambiguous FROM" error if
that entity were used in additional joins, as the entity appears twice in
the "from" list of the :class:`.Query`.  The fix resolves this ambiguity by
folding the standalone entity into the join that it's already a part of in
the same way that ultimately happens when the SELECT statement is rendered.

Fixes: #4584
Change-Id: Ic62ca09c6d329695b21ed4f1be8621edcbb18e19

diff --git a/doc/build/changelog/unreleased_13/4584.rst b/doc/build/changelog/unreleased_13/4584.rst
new file mode 100644 (file)
index 0000000..1f40a70
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/4584.rst
@@ -0,0 +1,12 @@
+.. change::
+   :tags: bug, orm
+   :tickets: 4584
+
+   Fixed 1.3 regression in new "ambiguous FROMs" query logic introduced in
+   :ref:`change_4365` where a :class:`.Query` that explicitly places an entity
+   in the FROM clause with :meth:`.Query.select_from` and also joins to it
+   using :meth:`.Query.join` would later cause an "ambiguous FROM" error if
+   that entity were used in additional joins, as the entity appears twice in
+   the "from" list of the :class:`.Query`.  The fix resolves this ambiguity by
+   folding the standalone entity into the join that it's already a part of in
+   the same way that ultimately happens when the SELECT statement is rendered.

diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py
index 3077840c65df4a1b233101c567788de872f812dc..8719b26ec2e3a287fb599ecd0acd5491690bd768 100644 (file)
--- a/lib/sqlalchemy/sql/util.py
+++ b/lib/sqlalchemy/sql/util.py
@@ -20,6 +20,7 @@ from .annotation import _shallow_annotate  # noqa
 from .base import _from_objects
 from .base import ColumnSet
 from .ddl import sort_tables  # noqa
+from .elements import _expand_cloned
 from .elements import _find_columns  # noqa
 from .elements import _label_reference
 from .elements import _textual_label_reference
@@ -149,6 +150,16 @@ def find_left_clause_to_join_from(clauses, join_to, onclause):
                 idx.append(i)
                 break
 
+    if len(idx) > 1:
+        # this is the same "hide froms" logic from
+        # Selectable._get_display_froms
+        toremove = set(
+            chain(*[_expand_cloned(f._hide_froms) for f in clauses])
+        )
+        idx = [
+            i for i in idx if clauses[i] not in toremove
+        ]
+
     # onclause was given and none of them resolved, so assume
     # all indexes can match
     if not idx and onclause is not None:

diff --git a/test/orm/test_joins.py b/test/orm/test_joins.py
index bb9747cf2fcfb5527339e5d02690ec4797699a50..4fe04f72b184f8c7eae00041ec2ccb21cf72b70f 100644 (file)
--- a/test/orm/test_joins.py
+++ b/test/orm/test_joins.py
@@ -1559,6 +1559,81 @@ class JoinTest(QueryTest, AssertsCompiledSQL):
             "ON dingalings.address_id = addresses_1.id",
         )
 
+    def test_clause_present_in_froms_twice_w_onclause(self):
+        # test [ticket:4584]
+        Order, Address, Dingaling, User = (
+            self.classes.Order,
+            self.classes.Address,
+            self.classes.Dingaling,
+            self.classes.User,
+        )
+
+        sess = create_session()
+
+        a1 = aliased(Address)
+
+        q = sess.query(Order).select_from(Order, a1, User)
+        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.outerjoin,
+            a1,
+        )
+
+        # the condition which occurs here is: Query._from_obj contains both
+        # "a1" by itself as well as a join that "a1" is part of.
+        # find_left_clause_to_join_from() needs to include removal of froms
+        # that are in the _hide_froms of joins the same way
+        # Selectable._get_display_froms does.
+        q = sess.query(Order).select_from(Order, a1, User)
+        q = q.outerjoin(a1, a1.id == Order.address_id)
+        q = q.outerjoin(User, a1.user_id == User.id)
+
+        self.assert_compile(
+            q,
+            "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 "
+            "FROM orders "
+            "LEFT OUTER JOIN addresses AS addresses_1 "
+            "ON addresses_1.id = orders.address_id "
+            "LEFT OUTER JOIN users ON addresses_1.user_id = users.id",
+        )
+
+    def test_clause_present_in_froms_twice_wo_onclause(self):
+        # test [ticket:4584]
+        Order, Address, Dingaling, User = (
+            self.classes.Order,
+            self.classes.Address,
+            self.classes.Dingaling,
+            self.classes.User,
+        )
+
+        sess = create_session()
+
+        a1 = aliased(Address)
+
+        # the condition which occurs here is: Query._from_obj contains both
+        # "a1" by itself as well as a join that "a1" is part of.
+        # find_left_clause_to_join_from() needs to include removal of froms
+        # that are in the _hide_froms of joins the same way
+        # Selectable._get_display_froms does.
+        q = sess.query(User).select_from(Dingaling, a1, User)
+        q = q.outerjoin(a1, User.id == a1.user_id)
+        q = q.outerjoin(Dingaling)
+
+        self.assert_compile(
+            q,
+            "SELECT users.id AS users_id, users.name AS users_name "
+            "FROM users LEFT OUTER JOIN addresses AS addresses_1 "
+            "ON users.id = addresses_1.user_id "
+            "LEFT OUTER JOIN dingalings "
+            "ON addresses_1.id = dingalings.address_id",
+        )
+
     def test_multiple_adaption(self):
         Item, Order, User = (
             self.classes.Item,