From e4b4847edef0f3f95658839de2a24027ec3b7e41 Mon Sep 17 00:00:00 2001 From: Christian Beikov Date: Mon, 7 Aug 2023 18:48:40 +0200 Subject: [PATCH] HHH-17033 Fix invalid SQL being generated for implicit join in entity join on clause --- .../sqm/sql/BaseSqmToSqlAstConverter.java | 20 +--- .../sql/ast/tree/from/TableGroup.java | 5 + .../orm/test/query/hql/EntityJoinTest.java | 13 +-- .../query/hql/ImplicitJoinInOnClauseTest.java | 102 ++++++++++++++++++ 4 files changed, 119 insertions(+), 21 deletions(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/ImplicitJoinInOnClauseTest.java diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java index 5c3509165f..1dccce7310 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/BaseSqmToSqlAstConverter.java @@ -3409,6 +3409,7 @@ public abstract class BaseSqmToSqlAstConverter extends Base tableGroup, predicate.get() ); + lhsTableGroup.addTableGroupJoin( tableGroupJoin ); if ( sqmJoin.getJoinPredicate() != null ) { final SqmJoin oldJoin = currentlyProcessingJoin; @@ -3423,9 +3424,6 @@ public abstract class BaseSqmToSqlAstConverter extends Base ); } - // Note that we add the entity join after processing the predicate because implicit joins needed in there - // can be just ordered right before the entity join without changing the semantics - lhsTableGroup.addTableGroupJoin( tableGroupJoin ); if ( transitive ) { consumeExplicitJoins( sqmJoin, tableGroupJoin.getJoinedGroup() ); } @@ -3465,6 +3463,7 @@ public abstract class BaseSqmToSqlAstConverter extends Base queryPartTableGroup, null ); + parentTableGroup.addTableGroupJoin( tableGroupJoin ); // add any additional join restrictions if ( sqmJoin.getJoinPredicate() != null ) { @@ -3474,9 +3473,6 @@ public abstract class BaseSqmToSqlAstConverter extends Base currentlyProcessingJoin = oldJoin; } - // Note that we add the entity join after processing the predicate because implicit joins needed in there - // can be just ordered right before the entity join without changing the semantics - parentTableGroup.addTableGroupJoin( tableGroupJoin ); if ( transitive ) { consumeExplicitJoins( sqmJoin, queryPartTableGroup ); } @@ -3499,6 +3495,7 @@ public abstract class BaseSqmToSqlAstConverter extends Base tableGroup, null ); + parentTableGroup.addTableGroupJoin( tableGroupJoin ); // add any additional join restrictions if ( sqmJoin.getJoinPredicate() != null ) { @@ -3508,9 +3505,6 @@ public abstract class BaseSqmToSqlAstConverter extends Base currentlyProcessingJoin = oldJoin; } - // Note that we add the entity join after processing the predicate because implicit joins needed in there - // can be just ordered right before the entity join without changing the semantics - parentTableGroup.addTableGroupJoin( tableGroupJoin ); if ( transitive ) { consumeExplicitJoins( sqmJoin, tableGroup ); } @@ -3760,12 +3754,8 @@ public abstract class BaseSqmToSqlAstConverter extends Base false, this ); - // Implicit joins in the ON clause of attribute joins need to be added as nested table group joins - // We don't have to do that for entity joins etc. as these do not have an inherent dependency on the lhs. - // We can just add the implicit join before the currently processing join - // See consumeEntityJoin for details - final boolean nested = currentClauseStack.getCurrent() == Clause.FROM - && currentlyProcessingJoin instanceof SqmAttributeJoin; + // Implicit joins in the ON clause need to be added as nested table group joins + final boolean nested = currentClauseStack.getCurrent() == Clause.FROM; if ( nested ) { parentTableGroup.addNestedTableGroupJoin( tableGroupJoin ); } diff --git a/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/from/TableGroup.java b/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/from/TableGroup.java index 3a68a05dde..911fdfd462 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/from/TableGroup.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/ast/tree/from/TableGroup.java @@ -191,6 +191,11 @@ public interface TableGroup extends SqlAstNode, ColumnReferenceQualifier, SqmPat return join; } } + for ( TableGroupJoin join : getNestedTableGroupJoins() ) { + if ( join.getJoinedGroup() == tableGroup ) { + return join; + } + } return null; } diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/EntityJoinTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/EntityJoinTest.java index f7508cbb77..e48a5239c0 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/EntityJoinTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/EntityJoinTest.java @@ -208,14 +208,15 @@ public class EntityJoinTest { assertThat( roots.size(), is( 1 ) ); final TableGroup rootTableGroup = roots.get( 0 ); - assertThat( rootTableGroup.getTableGroupJoins().size(), is( 2 ) ); + assertThat( rootTableGroup.getTableGroupJoins().size(), is( 1 ) ); + assertThat( rootTableGroup.getNestedTableGroupJoins().size(), is( 1 ) ); - // The first table group is an uninitialized lazy table group for the path in the on clause - final TableGroupJoin firstTableGroupJoin = rootTableGroup.getTableGroupJoins().get( 0 ); - assertThat( firstTableGroupJoin.getJoinedGroup(), instanceOf( LazyTableGroup.class ) ); - assertThat( ((LazyTableGroup) firstTableGroupJoin.getJoinedGroup()).getUnderlyingTableGroup(), is( CoreMatchers.nullValue() ) ); + // An uninitialized lazy table group for the path in the on clause + final TableGroupJoin nestedTableGroupJoin = rootTableGroup.getNestedTableGroupJoins().get( 0 ); + assertThat( nestedTableGroupJoin.getJoinedGroup(), instanceOf( LazyTableGroup.class ) ); + assertThat( ((LazyTableGroup) nestedTableGroupJoin.getJoinedGroup()).getUnderlyingTableGroup(), is( CoreMatchers.nullValue() ) ); - final TableGroupJoin tableGroupJoin = rootTableGroup.getTableGroupJoins().get( 1 ); + final TableGroupJoin tableGroupJoin = rootTableGroup.getTableGroupJoins().get( 0 ); assertThat( tableGroupJoin.getJoinedGroup().getModelPart(), is( customerEntityDescriptor ) ); } ); diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/ImplicitJoinInOnClauseTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/ImplicitJoinInOnClauseTest.java new file mode 100644 index 0000000000..058b79f32c --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/query/hql/ImplicitJoinInOnClauseTest.java @@ -0,0 +1,102 @@ +/* + * Hibernate, Relational Persistence for Idiomatic Java + * + * License: GNU Lesser General Public License (LGPL), version 2.1 or later + * See the lgpl.txt file in the root directory or http://www.gnu.org/licenses/lgpl-2.1.html + */ +package org.hibernate.orm.test.query.hql; + +import java.util.List; + +import org.hibernate.testing.orm.junit.DomainModel; +import org.hibernate.testing.orm.junit.JiraKey; +import org.hibernate.testing.orm.junit.SessionFactory; +import org.hibernate.testing.orm.junit.SessionFactoryScope; +import org.junit.jupiter.api.Test; + +import jakarta.persistence.Entity; +import jakarta.persistence.Id; +import jakarta.persistence.ManyToOne; + +/** + * @author Christian Beikov + */ +@DomainModel( annotatedClasses = { + ImplicitJoinInOnClauseTest.RootEntity.class, + ImplicitJoinInOnClauseTest.FirstLevelReferencedEntity.class, + ImplicitJoinInOnClauseTest.SecondLevelReferencedEntityA.class, + ImplicitJoinInOnClauseTest.SecondLevelReferencedEntityB.class, + ImplicitJoinInOnClauseTest.ThirdLevelReferencedEntity.class, + ImplicitJoinInOnClauseTest.UnrelatedEntity.class +}) +@SessionFactory +@JiraKey( "HHH-17033" ) +public class ImplicitJoinInOnClauseTest { + + @Test + public void testImplicitJoinInEntityJoinPredicate(SessionFactoryScope scope) { + scope.inTransaction( + (session) -> { + // this should get financial records which have a lastUpdateBy user set + List result = session.createQuery( + "select r.id, flr.id, ur1.id, ur2.id, ur3.id from RootEntity r " + + "inner join r.firstLevelReference as flr " + + "left join UnrelatedEntity ur1 on ur1.id = flr.secondLevelReferenceA.id " + + "left join UnrelatedEntity ur2 on ur2.id = flr.secondLevelReferenceB.id " + + "left join UnrelatedEntity ur3 on ur3.id = flr.secondLevelReferenceB.thirdLevelReference.id", + Object[].class + ).list(); + } + ); + } + + + @Entity(name = "RootEntity") + public static class RootEntity { + @Id + private Long id; + @ManyToOne + private FirstLevelReferencedEntity firstLevelReference; + + } + + @Entity(name = "FirstLevelReferencedEntity") + public static class FirstLevelReferencedEntity { + @Id + private Long id; + @ManyToOne + private SecondLevelReferencedEntityA secondLevelReferenceA; + @ManyToOne + private SecondLevelReferencedEntityB secondLevelReferenceB; + + } + @Entity(name = "SecondLevelReferencedEntityA") + public static class SecondLevelReferencedEntityA { + @Id + private Long id; + private String name; + } + + @Entity(name = "SecondLevelReferencedEntityB") + public static class SecondLevelReferencedEntityB { + @Id + private Long id; + @ManyToOne + private ThirdLevelReferencedEntity thirdLevelReference; + } + + @Entity(name = "ThirdLevelReferencedEntity") + public static class ThirdLevelReferencedEntity { + @Id + private Long id; + private String name; + + } + @Entity(name = "UnrelatedEntity") + public static class UnrelatedEntity { + @Id + private Long id; + private String name; + } + +}