HHH-17837 Always render explicit plural path joins

mbellade · mbellade · commit 22448328e17f · 2024-04-30T16:24:19.000+02:00
diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/SqmUtil.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/SqmUtil.java
@@ -25,13 +25,14 @@
 import org.hibernate.metamodel.MappingMetamodel;
 import org.hibernate.metamodel.mapping.BasicValuedMapping;
 import org.hibernate.metamodel.mapping.Bindable;
+import org.hibernate.metamodel.mapping.CollectionPart;
 import org.hibernate.metamodel.mapping.EntityAssociationMapping;
 import org.hibernate.metamodel.mapping.EntityIdentifierMapping;
 import org.hibernate.metamodel.mapping.EntityMappingType;
 import org.hibernate.metamodel.mapping.ForeignKeyDescriptor;
 import org.hibernate.metamodel.mapping.JdbcMapping;
-import org.hibernate.metamodel.mapping.ManagedMappingType;
 import org.hibernate.metamodel.mapping.MappingModelExpressible;
+import org.hibernate.metamodel.mapping.ModelPart;
 import org.hibernate.metamodel.mapping.ModelPartContainer;
 import org.hibernate.metamodel.mapping.PluralAttributeMapping;
 import org.hibernate.query.IllegalQueryOperationException;
@@ -46,6 +47,7 @@
 import org.hibernate.query.sqm.SqmQuerySource;
 import org.hibernate.query.sqm.spi.JdbcParameterBySqmParameterAccess;
 import org.hibernate.query.sqm.spi.SqmParameterMappingModelResolutionAccess;
+import org.hibernate.query.sqm.sql.SqmToSqlAstConverter;
 import org.hibernate.query.sqm.tree.SqmDmlStatement;
 import org.hibernate.query.sqm.tree.SqmJoinType;
 import org.hibernate.query.sqm.tree.SqmStatement;
@@ -65,6 +67,7 @@
 import org.hibernate.query.sqm.tree.select.SqmSelection;
 import org.hibernate.query.sqm.tree.select.SqmSortSpecification;
 import org.hibernate.spi.NavigablePath;
+import org.hibernate.sql.ast.Clause;
 import org.hibernate.sql.ast.SqlTreeCreationException;
 import org.hibernate.sql.ast.tree.expression.JdbcParameter;
 import org.hibernate.sql.ast.tree.from.TableGroup;
@@ -79,6 +82,7 @@
 import org.hibernate.type.internal.ConvertedBasicTypeImpl;
 import org.hibernate.type.spi.TypeConfiguration;
 
+import static org.hibernate.internal.util.NullnessUtil.castNonNull;
 import static org.hibernate.query.sqm.tree.jpa.ParameterCollector.collectParameters;
 
 /**
@@ -132,13 +136,45 @@ public static IllegalQueryOperationException expectingNonSelect(SqmStatement<?>
 	}
 
 	/**
-	 * Utility that returns {@code true} if the specified {@link SqmPath sqmPath} should be
-	 * dereferenced using the target table mapping, i.e. when the path's lhs is an explicit join.
+	 * Utility that returns the entity association target's mapping type if the specified
+	 * {@code sqmPath} should be dereferenced using the target table, i.e. when the path's
+	 * lhs is an explicit join, or defaults to the provided {@code modelPartContainer} otherwise.
 	 */
-	public static boolean needsTargetTableMapping(SqmPath<?> sqmPath, ModelPartContainer modelPartContainer) {
-		return modelPartContainer.getPartMappingType() != modelPartContainer
-				&& sqmPath.getLhs() instanceof SqmFrom<?, ?>
-				&& modelPartContainer.getPartMappingType() instanceof ManagedMappingType;
+	public static ModelPartContainer getTargetMappingIfNeeded(
+			SqmPath<?> sqmPath,
+			ModelPartContainer modelPartContainer,
+			SqmToSqlAstConverter sqlAstCreationState) {
+		final Clause clause = sqlAstCreationState.getCurrentClauseStack().getCurrent();
+		if ( clause != Clause.FROM && modelPartContainer.getPartMappingType() != modelPartContainer
+				&& sqmPath.getLhs() instanceof SqmFrom<?, ?> ) {
+			final ModelPart modelPart;
+			if ( modelPartContainer instanceof PluralAttributeMapping ) {
+				modelPart = getCollectionPart(
+						(PluralAttributeMapping) modelPartContainer,
+						castNonNull( sqmPath.getNavigablePath().getParent() )
+				);
+			}
+			else {
+				modelPart = modelPartContainer;
+			}
+			if ( modelPart instanceof EntityAssociationMapping ) {
+				return ( (EntityAssociationMapping) modelPart ).getAssociatedEntityMappingType();
+			}
+		}
+		return modelPartContainer;
+	}
+
+	private static CollectionPart getCollectionPart(PluralAttributeMapping attribute, NavigablePath path) {
+		final CollectionPart.Nature nature = CollectionPart.Nature.fromNameExact( path.getLocalName() );
+		if ( nature != null ) {
+			switch ( nature ) {
+				case ELEMENT:
+					return attribute.getElementDescriptor();
+				case INDEX:
+					return attribute.getIndexDescriptor();
+			}
+		}
+		return null;
 	}
 
 	/**
diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/BasicValuedPathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/BasicValuedPathInterpretation.java
@@ -13,7 +13,6 @@
 import org.hibernate.metamodel.MappingMetamodel;
 import org.hibernate.metamodel.mapping.BasicValuedModelPart;
 import org.hibernate.metamodel.mapping.EntityMappingType;
-import org.hibernate.metamodel.mapping.ManagedMappingType;
 import org.hibernate.metamodel.mapping.MappingType;
 import org.hibernate.metamodel.mapping.ModelPart;
 import org.hibernate.metamodel.mapping.ModelPartContainer;
@@ -35,7 +34,7 @@
 import org.hibernate.sql.ast.tree.update.Assignable;
 
 import static org.hibernate.internal.util.NullnessUtil.castNonNull;
-import static org.hibernate.query.sqm.internal.SqmUtil.needsTargetTableMapping;
+import static org.hibernate.query.sqm.internal.SqmUtil.getTargetMappingIfNeeded;
 
 /**
  * @author Steve Ebersole
@@ -82,20 +81,12 @@ public static <T> BasicValuedPathInterpretation<T> from(
 			}
 		}
 
-		final ModelPart modelPart;
-		if ( needsTargetTableMapping( sqmPath, modelPartContainer ) ) {
-			// We have to make sure we render the column of the target table
-			modelPart = ( (ManagedMappingType) modelPartContainer.getPartMappingType() ).findSubPart(
-					sqmPath.getReferencedPathSource().getPathName(),
-					treatTarget
-			);
-		}
-		else {
-			modelPart = modelPartContainer.findSubPart(
-					sqmPath.getReferencedPathSource().getPathName(),
-					treatTarget
-			);
-		}
+		// Use the target type to find the sub part if needed, otherwise just use the container
+		final ModelPart modelPart = getTargetMappingIfNeeded(
+				sqmPath,
+				modelPartContainer,
+				sqlAstCreationState
+		).findSubPart( sqmPath.getReferencedPathSource().getPathName(), treatTarget );
 
 		if ( modelPart == null ) {
 			if ( jpaQueryComplianceEnabled ) {
diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EmbeddableValuedPathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EmbeddableValuedPathInterpretation.java
@@ -13,7 +13,6 @@
 import org.hibernate.metamodel.MappingMetamodel;
 import org.hibernate.metamodel.mapping.EmbeddableValuedModelPart;
 import org.hibernate.metamodel.mapping.EntityMappingType;
-import org.hibernate.metamodel.mapping.ManagedMappingType;
 import org.hibernate.metamodel.mapping.ModelPartContainer;
 import org.hibernate.metamodel.model.domain.EntityDomainType;
 import org.hibernate.query.sqm.sql.SqmToSqlAstConverter;
@@ -29,7 +28,7 @@
 import org.hibernate.sql.ast.tree.from.TableGroup;
 import org.hibernate.sql.ast.tree.update.Assignable;
 
-import static org.hibernate.query.sqm.internal.SqmUtil.needsTargetTableMapping;
+import static org.hibernate.query.sqm.internal.SqmUtil.getTargetMappingIfNeeded;
 
 /**
  * @author Steve Ebersole
@@ -65,20 +64,12 @@ else if ( lhs.getNodeType() instanceof EntityDomainType ) {
 		}
 
 		final ModelPartContainer modelPartContainer = tableGroup.getModelPart();
-		final EmbeddableValuedModelPart mapping;
-		if ( needsTargetTableMapping( sqmPath, modelPartContainer ) ) {
-			// We have to make sure we render the column of the target table
-			mapping = (EmbeddableValuedModelPart) ( (ManagedMappingType) modelPartContainer.getPartMappingType() ).findSubPart(
-					sqmPath.getReferencedPathSource().getPathName(),
-					treatTarget
-			);
-		}
-		else {
-			mapping = (EmbeddableValuedModelPart) modelPartContainer.findSubPart(
-					sqmPath.getReferencedPathSource().getPathName(),
-					treatTarget
-			);
-		}
+		// Use the target type to find the sub part if needed, otherwise just use the container
+		final EmbeddableValuedModelPart mapping = (EmbeddableValuedModelPart) getTargetMappingIfNeeded(
+				sqmPath,
+				modelPartContainer,
+				sqlAstCreationState
+		).findSubPart( sqmPath.getReferencedPathSource().getPathName(), treatTarget );
 
 		return new EmbeddableValuedPathInterpretation<>(
 				mapping.toSqlExpression(
diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EntityValuedPathInterpretation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/sql/internal/EntityValuedPathInterpretation.java
@@ -162,7 +162,9 @@ private static <T> EntityValuedPathInterpretation<T> from(
 		// we try to make use of it and the FK model part if possible based on the inferred mapping
 		if ( mapping instanceof EntityAssociationMapping ) {
 			final EntityAssociationMapping associationMapping = (EntityAssociationMapping) mapping;
-			final ModelPart keyTargetMatchPart = associationMapping.getKeyTargetMatchPart();
+			final ModelPart keyTargetMatchPart = associationMapping.getForeignKeyDescriptor().getPart(
+					associationMapping.getSideNature()
+			);
 
 			if ( associationMapping.isFkOptimizationAllowed() ) {
 				final boolean forceUsingForeignKeyAssociationSidePart;
diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmQuerySpec.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmQuerySpec.java
@@ -629,8 +629,14 @@ public void appendHqlString(StringBuilder sb) {
 
 	public boolean groupByClauseContains(NavigablePath path) {
 		for ( SqmExpression<?> expression : groupByClauseExpressions ) {
-			if ( expression instanceof SqmPath && ( (SqmPath<?>) expression ).getNavigablePath() == path ) {
-				return true;
+			if ( expression instanceof SqmPath ) {
+				final NavigablePath expressionPath = ( (SqmPath<?>) expression ).getNavigablePath();
+				if ( expressionPath == path ) {
+					return true;
+				}
+				else if ( CollectionPart.Nature.fromNameExact( path.getLocalName() ) != null && expressionPath == path.getParent() ) {
+					return true;
+				}
 			}
 		}
 		return false;
diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/ManyToManyJoinTableAndInheritanceTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/ManyToManyJoinTableAndInheritanceTest.java
@@ -93,7 +93,7 @@ public void testLeftJoinSelectId(SessionFactoryScope scope) {
 			).getSingleResult();
 			assertThat( result ).isEqualTo( 1L );
 			inspector.assertExecutedCount( 1 );
-			inspector.assertNumberOfOccurrenceInQuery( 0, "join", 1 );
+			inspector.assertNumberOfOccurrenceInQuery( 0, "join", 2 );
 		} );
 	}
 
diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/JoinTableOptimizationTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/JoinTableOptimizationTest.java
@@ -41,8 +41,7 @@ public void testOnlyCollectionTableJoined(SessionFactoryScope scope) {
 				s -> {
 					s.createQuery( "select p.id from Document d left join d.people p where p.id is not null" ).list();
 					statementInspector.assertExecutedCount( 1 );
-					// Assert only the collection table is joined
-					statementInspector.assertNumberOfJoins( 0, 1 );
+					statementInspector.assertNumberOfJoins( 0, 2 );
 				}
 		);
 	}
diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/MapIssueTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/ql/MapIssueTest.java
@@ -45,8 +45,7 @@ public void testOnlyCollectionTableJoined(SessionFactoryScope scope) {
 				s -> {
 					s.createQuery( "select 1 from MapOwner as o left join o.contents c where c.id is not null" ).list();
 					statementInspector.assertExecutedCount( 1 );
-					// Assert only the collection table is joined
-					statementInspector.assertNumberOfJoins( 0, 1 );
+					statementInspector.assertNumberOfJoins( 0, 2 );
 				}
 		);
 	}

Original file line number	Diff line number	Diff line change
`@@ -93,7 +93,7 @@ public void testLeftJoinSelectId(SessionFactoryScope scope) {`
`93`	`93`	`).getSingleResult();`
`94`	`94`	`assertThat( result ).isEqualTo( 1L );`
`95`	`95`	`inspector.assertExecutedCount( 1 );`
`96`		`- inspector.assertNumberOfOccurrenceInQuery( 0, "join", 1 );`
	`96`	`+ inspector.assertNumberOfOccurrenceInQuery( 0, "join", 2 );`
`97`	`97`	`} );`
`98`	`98`	`}`
`99`	`99`
Original file line number	Diff line number	Diff line change
`@@ -41,8 +41,7 @@ public void testOnlyCollectionTableJoined(SessionFactoryScope scope) {`
`41`	`41`	`s -> {`
`42`	`42`	`s.createQuery( "select p.id from Document d left join d.people p where p.id is not null" ).list();`
`43`	`43`	`statementInspector.assertExecutedCount( 1 );`
`44`		`- // Assert only the collection table is joined`
`45`		`- statementInspector.assertNumberOfJoins( 0, 1 );`
	`44`	`+ statementInspector.assertNumberOfJoins( 0, 2 );`
`46`	`45`	`}`
`47`	`46`	`);`
`48`	`47`	`}`
Original file line number	Diff line number	Diff line change
`@@ -45,8 +45,7 @@ public void testOnlyCollectionTableJoined(SessionFactoryScope scope) {`
`45`	`45`	`s -> {`
`46`	`46`	`s.createQuery( "select 1 from MapOwner as o left join o.contents c where c.id is not null" ).list();`
`47`	`47`	`statementInspector.assertExecutedCount( 1 );`
`48`		`- // Assert only the collection table is joined`
`49`		`- statementInspector.assertNumberOfJoins( 0, 1 );`
	`48`	`+ statementInspector.assertNumberOfJoins( 0, 2 );`
`50`	`49`	`}`
`51`	`50`	`);`
`52`	`51`	`}`