ensure explicit left side considered for ON clause in join_from

Fixed an issue in :meth:`_sql.Select.join_from` where the join condition
between the left and right tables specified in the method call could be
incorrectly determined based on an intermediate table already present in
the FROM clause, rather than matching the foreign keys between the
immediate left and right arguments. The join condition is now determined by
matching primary keys between the two tables explicitly passed to
:meth:`_sql.Select.join_from`, ensuring consistent and predictable join
behavior regardless of the order of join operations or other tables present
in the query.  The fix is applied to both the Core and ORM implementations
of :meth:`_sql.Select.join_from`.

Fixes: #12931
Change-Id: Id457d441ee8a1bd64a002e57a9dcfd6fc56ff15e

diff --git a/doc/build/changelog/unreleased_21/12931.rst b/doc/build/changelog/unreleased_21/12931.rst
new file mode 100644 (file)
index 0000000..fcecdff
--- /dev/null
+++ b/doc/build/changelog/unreleased_21/12931.rst
@@ -0,0 +1,14 @@
+.. change::
+    :tags: bug, sql
+    :tickets: 12931
+
+    Fixed an issue in :meth:`_sql.Select.join_from` where the join condition
+    between the left and right tables specified in the method call could be
+    incorrectly determined based on an intermediate table already present in
+    the FROM clause, rather than matching the foreign keys between the
+    immediate left and right arguments. The join condition is now determined by
+    matching primary keys between the two tables explicitly passed to
+    :meth:`_sql.Select.join_from`, ensuring consistent and predictable join
+    behavior regardless of the order of join operations or other tables present
+    in the query.  The fix is applied to both the Core and ORM implementations
+    of :meth:`_sql.Select.join_from`.

diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py
index e906fcbc0f0d63052fdd379d22ab382908ab3f89..2dd6e950a7100192d49859e73601044f600cd39f 100644 (file)
--- a/lib/sqlalchemy/orm/context.py
+++ b/lib/sqlalchemy/orm/context.py
@@ -1959,6 +1959,7 @@ class _ORMSelectCompileState(_ORMCompileState, SelectState):
 
         """
 
+        explicit_left = left
         if left is None:
             # left not given (e.g. no relationship object/name specified)
             # figure out the best "left" side based on our existing froms /
@@ -2004,6 +2005,9 @@ class _ORMSelectCompileState(_ORMCompileState, SelectState):
             # self._from_obj list
             left_clause = self.from_clauses[replace_from_obj_index]
 
+            if explicit_left is not None and onclause is None:
+                onclause = _ORMJoin._join_condition(explicit_left, right)
+
             self.from_clauses = (
                 self.from_clauses[:replace_from_obj_index]
                 + [

diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py
index fa2079500b0022cea2cb1f539adba43a5de04162..b9b0104615b67ad2afc24130c6dddb21512c52c9 100644 (file)
--- a/lib/sqlalchemy/sql/selectable.py
+++ b/lib/sqlalchemy/sql/selectable.py
@@ -5024,6 +5024,7 @@ class SelectState(util.MemoizedSlots, CompileState):
                 if onclause is not None:
                     assert isinstance(onclause, ColumnElement)
 
+            explicit_left = left
             isouter = flags["isouter"]
             full = flags["full"]
 
@@ -5054,6 +5055,9 @@ class SelectState(util.MemoizedSlots, CompileState):
                 # self._from_obj list
                 left_clause = self.from_clauses[replace_from_obj_index]
 
+                if explicit_left is not None and onclause is None:
+                    onclause = Join._join_condition(explicit_left, right)
+
                 self.from_clauses = (
                     self.from_clauses[:replace_from_obj_index]
                     + (

diff --git a/test/orm/test_core_compilation.py b/test/orm/test_core_compilation.py
index b49d4286bf22e5a089411f11499778f58804d720..3b98d60be780c5e969026940b7e912aebd511cf6 100644 (file)
--- a/test/orm/test_core_compilation.py
+++ b/test/orm/test_core_compilation.py
@@ -27,6 +27,8 @@ from sqlalchemy.orm import contains_eager
 from sqlalchemy.orm import deferred
 from sqlalchemy.orm import join as orm_join
 from sqlalchemy.orm import joinedload
+from sqlalchemy.orm import Mapped
+from sqlalchemy.orm import mapped_column
 from sqlalchemy.orm import query_expression
 from sqlalchemy.orm import relationship
 from sqlalchemy.orm import Session
@@ -893,6 +895,81 @@ class JoinTest(QueryTest, AssertsCompiledSQL):
 
         self.assert_compile(stmt, expected, checkparams=expected_params)
 
+    @testing.fixture
+    def grandchild_fixture(self, decl_base):
+        class Parent(decl_base):
+            __tablename__ = "parent"
+            id: Mapped[int] = mapped_column(primary_key=True)
+            data: Mapped[str]
+
+        class Child(decl_base):
+            __tablename__ = "child"
+            id: Mapped[int] = mapped_column(primary_key=True)
+            parent_id: Mapped[int] = mapped_column(ForeignKey("parent.id"))
+
+        class GrandchildWParent(decl_base):
+            __tablename__ = "grandchildwparent"
+            id: Mapped[int] = mapped_column(primary_key=True)
+            parent_id: Mapped[int] = mapped_column(ForeignKey("parent.id"))
+            child_id: Mapped[int] = mapped_column(ForeignKey("child.id"))
+
+        return Parent, Child, GrandchildWParent
+
+    @testing.variation(
+        "jointype",
+        ["child_grandchild", "parent_grandchild", "grandchild_alone"],
+    )
+    def test_join_from_favors_explicit_left(
+        self, grandchild_fixture, jointype
+    ):
+        """test #12931 in terms of ORM joins"""
+
+        Parent, Child, GrandchildWParent = grandchild_fixture
+
+        if jointype.child_grandchild:
+            stmt = (
+                select(Parent)
+                .join_from(Parent, Child)
+                .join_from(Child, GrandchildWParent)
+            )
+
+            self.assert_compile(
+                stmt,
+                "SELECT parent.id, parent.data FROM parent JOIN "
+                "child ON parent.id = child.parent_id "
+                "JOIN grandchildwparent "
+                "ON child.id = grandchildwparent.child_id",
+            )
+
+        elif jointype.parent_grandchild:
+            stmt = (
+                select(Parent)
+                .join_from(Parent, Child)
+                .join_from(Parent, GrandchildWParent)
+            )
+
+            self.assert_compile(
+                stmt,
+                "SELECT parent.id, parent.data FROM parent "
+                "JOIN child ON parent.id = child.parent_id "
+                "JOIN grandchildwparent "
+                "ON parent.id = grandchildwparent.parent_id",
+            )
+        elif jointype.grandchild_alone:
+            stmt = (
+                select(Parent).join_from(Parent, Child).join(GrandchildWParent)
+            )
+
+            self.assert_compile(
+                stmt,
+                "SELECT parent.id, parent.data FROM parent "
+                "JOIN child ON parent.id = child.parent_id "
+                "JOIN grandchildwparent "
+                "ON child.id = grandchildwparent.child_id",
+            )
+        else:
+            jointype.fail()
+
 
 class LoadersInSubqueriesTest(QueryTest, AssertsCompiledSQL):
     """The Query object calls eanble_eagerloads(False) when you call

diff --git a/test/sql/test_select.py b/test/sql/test_select.py
index 2bef71dd1e5f5dabaa7c132f4443c1737063a529..e65500378191ee6237f36ec2d2706f96fd8633aa 100644 (file)
--- a/test/sql/test_select.py
+++ b/test/sql/test_select.py
@@ -57,6 +57,14 @@ grandchild = Table(
     Column("child_id", ForeignKey("child.id")),
 )
 
+grandchild_w_parent = Table(
+    "grandchildwparent",
+    metadata,
+    Column("id", Integer, primary_key=True),
+    Column("parent_id", ForeignKey("parent.id")),
+    Column("child_id", ForeignKey("child.id")),
+)
+
 
 class SelectTest(fixtures.TestBase, AssertsCompiledSQL):
     __dialect__ = "default"
@@ -174,6 +182,68 @@ class SelectTest(fixtures.TestBase, AssertsCompiledSQL):
             "ON mytable.myid = myothertable.otherid",
         )
 
+    @testing.variation(
+        "jointype",
+        ["child_grandchild", "parent_grandchild", "grandchild_alone"],
+    )
+    def test_join_from_multiple_explicit_left_side_implicit_onclause(
+        self, jointype
+    ):
+        """test #12931
+
+        when join_from() is indicated, favor the explicit "left" side given
+        over the "left side of hte join" for creating onclause.
+
+        when join() is indicated, use the normal behavior of assuming
+        right side of the previous join is the new left side.
+
+        """
+
+        if jointype.child_grandchild:
+            stmt = (
+                select(parent)
+                .join_from(parent, child)
+                .join_from(child, grandchild_w_parent)
+            )
+
+            self.assert_compile(
+                stmt,
+                "SELECT parent.id, parent.data FROM parent JOIN "
+                "child ON parent.id = child.parent_id "
+                "JOIN grandchildwparent "
+                "ON child.id = grandchildwparent.child_id",
+            )
+
+        elif jointype.parent_grandchild:
+            stmt = (
+                select(parent)
+                .join_from(parent, child)
+                .join_from(parent, grandchild_w_parent)
+            )
+
+            self.assert_compile(
+                stmt,
+                "SELECT parent.id, parent.data FROM parent "
+                "JOIN child ON parent.id = child.parent_id "
+                "JOIN grandchildwparent "
+                "ON parent.id = grandchildwparent.parent_id",
+            )
+        elif jointype.grandchild_alone:
+            stmt = (
+                select(parent)
+                .join_from(parent, child)
+                .join(grandchild_w_parent)
+            )
+            self.assert_compile(
+                stmt,
+                "SELECT parent.id, parent.data FROM parent "
+                "JOIN child ON parent.id = child.parent_id "
+                "JOIN grandchildwparent "
+                "ON child.id = grandchildwparent.child_id",
+            )
+        else:
+            jointype.fail()
+
     def test_outerjoin_nofrom_explicit_left_side_explicit_onclause(self):
         stmt = select(table1).outerjoin_from(
             table1, table2, table1.c.myid == table2.c.otherid