From: Mike Bayer Date: Tue, 21 Oct 2025 12:48:12 +0000 (-0400) Subject: ensure explicit left side considered for ON clause in join_from X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=602355753964179a0af8e6e2879491ff2edbad1e;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 index 0000000000..fcecdff4d3 --- /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 e906fcbc0f..2dd6e950a7 100644 --- 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 fa2079500b..b9b0104615 100644 --- 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 b49d4286bf..3b98d60be7 100644 --- 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 2bef71dd1e..e655003781 100644 --- 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