From 03e589ef0d6906177578c05f8489d25679869b3d Mon Sep 17 00:00:00 2001 From: Jan Schatteman Date: Fri, 19 Apr 2024 18:52:05 +0200 Subject: [PATCH] HHH-17967 - Add test for issue (already fixed on main) - Backport the minimal necessary bits of HHH-16931 and https://github.com/hibernate/hibernate-orm/pull/7883 to fix the NPE Signed-off-by: Jan Schatteman --- .../internal/ConcreteSqmSelectQueryPlan.java | 2 +- .../tree/domain/AbstractSqmAttributeJoin.java | 6 +- .../sqm/tree/domain/AbstractSqmFrom.java | 24 +++++ .../tree/select/AbstractSqmSelectQuery.java | 8 +- .../sqm/tree/select/SqmSelectStatement.java | 88 +++++++++---------- .../query/sqm/tree/select/SqmSubQuery.java | 4 - .../test/query/criteria/CountQueryTests.java | 75 ++++++++++++---- 7 files changed, 136 insertions(+), 71 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/ConcreteSqmSelectQueryPlan.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/ConcreteSqmSelectQueryPlan.java index fc252ccb5e..56a63c166d 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/ConcreteSqmSelectQueryPlan.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/ConcreteSqmSelectQueryPlan.java @@ -218,7 +218,7 @@ public class ConcreteSqmSelectQueryPlan implements SelectQueryPlan { if ( queryOptions.getTupleTransformer() != null ) { return makeRowTransformerTupleTransformerAdapter( sqm, queryOptions ); } - else if ( resultType == null ) { + else if ( resultType == null || resultType == Object.class ) { return RowTransformerStandardImpl.instance(); } else { diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/domain/AbstractSqmAttributeJoin.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/domain/AbstractSqmAttributeJoin.java index 51125281c7..260ec3b0d8 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/domain/AbstractSqmAttributeJoin.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/domain/AbstractSqmAttributeJoin.java @@ -33,7 +33,7 @@ public abstract class AbstractSqmAttributeJoin extends AbstractSqmQualifiedJoin implements SqmAttributeJoin { - private final boolean fetched; + private boolean fetched; public AbstractSqmAttributeJoin( SqmFrom lhs, @@ -88,6 +88,10 @@ public abstract class AbstractSqmAttributeJoin return fetched; } + public void clearFetched() { + fetched = false; + } + @Override public X accept(SemanticQueryWalker walker) { return walker.visitQualifiedAttributeJoin( this ); diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/domain/AbstractSqmFrom.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/domain/AbstractSqmFrom.java index f1bd1cae1f..7e0a936d8d 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/domain/AbstractSqmFrom.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/domain/AbstractSqmFrom.java @@ -15,6 +15,7 @@ import java.util.Set; import java.util.function.Consumer; import java.util.stream.Collectors; +import org.hibernate.Internal; import org.hibernate.metamodel.mapping.ModelPartContainer; import org.hibernate.metamodel.model.domain.BagPersistentAttribute; import org.hibernate.metamodel.model.domain.EntityDomainType; @@ -254,6 +255,29 @@ public abstract class AbstractSqmFrom extends AbstractSqmPath implements findRoot().addOrderedJoin( join ); } + @Internal + public void removeLeftFetchJoins() { + if ( joins != null ) { + for ( SqmJoin join : new ArrayList<>(joins) ) { + if ( join instanceof AbstractSqmAttributeJoin ) { + final AbstractSqmAttributeJoin attributeJoin = (AbstractSqmAttributeJoin) join; + if ( attributeJoin.isFetched() ) { + if ( join.getSqmJoinType() == SqmJoinType.LEFT ) { + joins.remove( join ); + final List> orderedJoins = findRoot().getOrderedJoins(); + if (orderedJoins != null) { + orderedJoins.remove( join ); + } + } + else { + attributeJoin.clearFetched(); + } + } + } + } + } + } + @Override public void visitSqmJoins(Consumer> consumer) { if ( joins != null ) { diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/AbstractSqmSelectQuery.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/AbstractSqmSelectQuery.java index 9001f11b02..d17299ec57 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/AbstractSqmSelectQuery.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/AbstractSqmSelectQuery.java @@ -45,7 +45,7 @@ public abstract class AbstractSqmSelectQuery implements SqmSelectQuery { private final Map> cteStatements; private SqmQueryPart sqmQueryPart; - private Class resultType; + private final Class resultType; public AbstractSqmSelectQuery(Class resultType, NodeBuilder builder) { this( new SqmQuerySpec<>( builder ), resultType, builder ); @@ -202,8 +202,11 @@ public abstract class AbstractSqmSelectQuery return resultType; } + /** + * Don't use this method. It has no effect. + */ protected void setResultType(Class resultType) { - this.resultType = resultType; + // No-op } @Override @@ -410,7 +413,6 @@ public abstract class AbstractSqmSelectQuery break; } default: { - setResultType( (Class) Object[].class ); resultSelection = ( Selection ) nodeBuilder().array( selections ); } } diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmSelectStatement.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmSelectStatement.java index d6cd358792..17d03ca0f3 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmSelectStatement.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmSelectStatement.java @@ -37,7 +37,9 @@ import org.hibernate.query.sqm.tree.expression.SqmStar; import org.hibernate.query.sqm.tree.expression.ValueBindJpaCriteriaParameter; import org.hibernate.query.sqm.tree.expression.SqmParameter; import org.hibernate.query.sqm.tree.from.SqmFromClause; +import org.hibernate.query.sqm.tree.from.SqmRoot; +import static org.hibernate.query.sqm.tree.SqmCopyContext.noParamCopyContext; import static org.hibernate.query.sqm.tree.jpa.ParameterCollector.collectParameters; /** @@ -119,6 +121,10 @@ public class SqmSelectStatement extends AbstractSqmSelectQuery implements if ( existing != null ) { return existing; } + return createCopy( context, getResultType() ); + } + + private SqmSelectStatement createCopy(SqmCopyContext context, Class resultType) { final Set> parameters; if ( this.parameters == null ) { parameters = null; @@ -129,17 +135,19 @@ public class SqmSelectStatement extends AbstractSqmSelectQuery implements parameters.add( parameter.copy( context ) ); } } - final SqmSelectStatement statement = context.registerCopy( + //noinspection unchecked + final SqmSelectStatement statement = (SqmSelectStatement) context.registerCopy( this, new SqmSelectStatement<>( nodeBuilder(), copyCteStatements( context ), - getResultType(), + resultType, getQuerySource(), parameters ) ); - statement.setQueryPart( getQueryPart().copy( context ) ); + //noinspection unchecked + statement.setQueryPart( (SqmQueryPart) getQueryPart().copy( context ) ); return statement; } @@ -266,9 +274,6 @@ public class SqmSelectStatement extends AbstractSqmSelectQuery implements checkSelectionIsJpaCompliant( selection ); } getQuerySpec().setSelection( (JpaSelection) selection ); - if ( getResultType() == Object.class ) { - setResultType( (Class) selection.getJavaType() ); - } return this; } @@ -309,7 +314,6 @@ public class SqmSelectStatement extends AbstractSqmSelectQuery implements break; } default: { - setResultType( (Class) Object[].class ); resultSelection = ( Selection ) nodeBuilder().array( selections ); } } @@ -460,49 +464,43 @@ public class SqmSelectStatement extends AbstractSqmSelectQuery implements } @Override - public JpaCriteriaQuery createCountQuery() { - final SqmCopyContext context = new NoParamSqmCopyContext() { - @Override - public boolean copyFetchedFlag() { - return false; + public SqmSelectStatement createCountQuery() { + final SqmSelectStatement copy = createCopy( noParamCopyContext(), Object.class ); + final SqmQueryPart queryPart = copy.getQueryPart(); + final SqmQuerySpec querySpec; + //TODO: detect queries with no 'group by', but aggregate functions + // in 'select' list (we don't even need to hit the database to + // know they return exactly one row) + if ( queryPart.isSimpleQueryPart() + && !( querySpec = (SqmQuerySpec) queryPart ).isDistinct() + && querySpec.getGroupingExpressions().isEmpty() ) { + for ( SqmRoot root : querySpec.getRootList() ) { + root.removeLeftFetchJoins(); } - }; - final NodeBuilder nodeBuilder = nodeBuilder(); - final Set> parameters; - if ( this.parameters == null ) { - parameters = null; + querySpec.getSelectClause().setSelection( nodeBuilder().count( new SqmStar( nodeBuilder() )) ); + if ( querySpec.getFetch() == null && querySpec.getOffset() == null ) { + querySpec.setOrderByClause( null ); + } + + return (SqmSelectStatement) copy; } else { - parameters = new LinkedHashSet<>( this.parameters.size() ); - for ( SqmParameter parameter : this.parameters ) { - parameters.add( parameter.copy( context ) ); + final JpaSelection selection = queryPart.getFirstQuerySpec().getSelection(); + if ( selection.isCompoundSelection() ) { + char c = 'a'; + for ( JpaSelection item : selection.getSelectionItems() ) { + item.alias( Character.toString( ++c ) + '_' ); + } } + else { + selection.alias( "a_" ); + } + final SqmSubQuery subquery = new SqmSubQuery<>( copy, queryPart, null, nodeBuilder() ); + final SqmSelectStatement query = nodeBuilder().createQuery( Long.class ); + query.from( subquery ); + query.select( nodeBuilder().count( new SqmStar(nodeBuilder())) ); + return query; } - final SqmSelectStatement selectStatement = new SqmSelectStatement<>( - nodeBuilder, - copyCteStatements( context ), - Long.class, - SqmQuerySource.CRITERIA, - parameters - ); - final SqmQuerySpec querySpec = new SqmQuerySpec<>( nodeBuilder ); - - final SqmSubQuery subquery = new SqmSubQuery<>( selectStatement, Tuple.class, nodeBuilder ); - final SqmQueryPart queryPart = getQueryPart().copy( context ); - resetSelections( queryPart ); - // Reset the - if ( queryPart.getFetch() == null && queryPart.getOffset() == null ) { - queryPart.setOrderByClause( null ); - } - //noinspection unchecked - subquery.setQueryPart( (SqmQueryPart) queryPart ); - - querySpec.setFromClause( new SqmFromClause( 1 ) ); - querySpec.setSelectClause( new SqmSelectClause( false, 1, nodeBuilder ) ); - selectStatement.setQueryPart( querySpec ); - selectStatement.select( nodeBuilder.count( new SqmStar( nodeBuilder ) ) ); - selectStatement.from( subquery ); - return selectStatement; } private void resetSelections(SqmQueryPart queryPart) { diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmSubQuery.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmSubQuery.java index 2163994409..357d1cb439 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmSubQuery.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmSubQuery.java @@ -258,7 +258,6 @@ public class SqmSubQuery extends AbstractSqmSelectQuery implements SqmSele break; } default: { - setResultType( (Class) Object[].class ); resultSelection = ( Selection ) nodeBuilder().array( selections ); } } @@ -609,9 +608,6 @@ public class SqmSubQuery extends AbstractSqmSelectQuery implements SqmSele public void applyInferableType(SqmExpressible type) { //noinspection unchecked expressibleType = (SqmExpressible) type; - if ( expressibleType != null && expressibleType.getExpressibleJavaType() != null ) { - setResultType( expressibleType.getExpressibleJavaType().getJavaTypeClass() ); - } } private void applyInferableType(Class type) { diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/query/criteria/CountQueryTests.java b/hibernate-core/src/test/java/org/hibernate/orm/test/query/criteria/CountQueryTests.java index fe79235212..b5b7cba92c 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/query/criteria/CountQueryTests.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/query/criteria/CountQueryTests.java @@ -28,19 +28,49 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import jakarta.persistence.Column; +import jakarta.persistence.Entity; +import jakarta.persistence.GeneratedValue; +import jakarta.persistence.GenerationType; +import jakarta.persistence.Id; +import jakarta.persistence.Inheritance; +import jakarta.persistence.InheritanceType; +import jakarta.persistence.MappedSuperclass; +import jakarta.persistence.SequenceGenerator; import jakarta.persistence.Tuple; +import jakarta.persistence.TypedQuery; +import jakarta.persistence.criteria.Root; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.fail; /** * @author Christian Beikov */ -@DomainModel(standardModels = StandardDomainModel.CONTACTS) +@DomainModel( + standardModels = StandardDomainModel.CONTACTS, + annotatedClasses = {CountQueryTests.LogSupport.class, CountQueryTests.Contract.class} +) @SessionFactory -@JiraKey("HHH-17410") public class CountQueryTests { @Test + @JiraKey( "HHH-17967" ) + public void testForHHH17967(SessionFactoryScope scope) { + scope.inTransaction( + session -> { + HibernateCriteriaBuilder cb = session.getCriteriaBuilder(); + JpaCriteriaQuery cq = cb.createQuery( Contract.class ); + Root root = cq.from( Contract.class ); + cq.select( root ); + TypedQuery query = session.createQuery( cq.createCountQuery() ); + query.getSingleResult(); + } + ); + } + + @Test + @JiraKey("HHH-17410") public void testBasic(SessionFactoryScope scope) { scope.inTransaction( session -> { @@ -58,6 +88,7 @@ public class CountQueryTests { } @Test + @JiraKey("HHH-17410") public void testFetches(SessionFactoryScope scope) { scope.inTransaction( session -> { @@ -66,7 +97,7 @@ public class CountQueryTests { "select e from Contact e join fetch e.alternativeContact", Contact.class ) ); - verifyCollectionCount( session, cb.createQuery( + verifyCount( session, cb.createQuery( "select e from Contact e left join fetch e.addresses", Contact.class ) ); @@ -75,6 +106,7 @@ public class CountQueryTests { } @Test + @JiraKey("HHH-17410") public void testConstructor(SessionFactoryScope scope) { scope.inTransaction( session -> { @@ -89,6 +121,7 @@ public class CountQueryTests { @Test @RequiresDialectFeature(feature = DialectFeatureChecks.SupportsRecursiveCtes.class) + @JiraKey("HHH-17410") public void testCte(SessionFactoryScope scope) { scope.inTransaction( session -> { @@ -107,6 +140,7 @@ public class CountQueryTests { } @Test + @JiraKey("HHH-17410") public void testValues(SessionFactoryScope scope) { scope.inTransaction( session -> { @@ -126,6 +160,7 @@ public class CountQueryTests { } @Test + @JiraKey("HHH-17410") public void testParameters(SessionFactoryScope scope) { scope.inTransaction( session -> { @@ -134,7 +169,7 @@ public class CountQueryTests { final JpaCriteriaQuery cq = cb.createTupleQuery(); final JpaRoot root = cq.from( Contact.class ); final JpaParameterExpression parameter = cb.parameter( Contact.Gender.class ); - + cq.multiselect( root.get( "id" ), root.get( "name" ) ); cq.where( root.get( "gender" ).equalTo( parameter ) ); final Long count = session.createQuery( cq.createCountQuery() ) @@ -232,19 +267,6 @@ public class CountQueryTests { assertEquals( resultList.size(), count.intValue() ); } - private void verifyCollectionCount(SessionImplementor session, JpaCriteriaQuery query) { - final List resultList = session.createQuery( query ).getResultList(); - final Long count = session.createQuery( query.createCountQuery() ).getSingleResult(); - int ormSize = 0; - for ( Contact contact : resultList ) { - ormSize++; - ormSize += Math.max( contact.getAddresses().size() - 1, 0 ); - ormSize += Math.max( contact.getPhoneNumbers().size() - 1, 0 ); - } - - assertEquals( ormSize, count.intValue() ); - } - @AfterEach public void dropTestData(SessionFactoryScope scope) { scope.inTransaction( (session) -> { @@ -252,4 +274,23 @@ public class CountQueryTests { session.createMutationQuery( "delete Contact" ).executeUpdate(); } ); } + + @MappedSuperclass + public static abstract class LogSupport { + @Column(name = "SOMESTRING") + private String s; + } + + @Entity + @Inheritance(strategy = InheritanceType.SINGLE_TABLE) + public static class Contract extends LogSupport { + @Id + @Column(name = "PK") + @SequenceGenerator(name = "CONTRACT_GENERATOR", sequenceName = "CONTRACT_SEQ", allocationSize = 1) + @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "CONTRACT_GENERATOR") + private Long syntheticId; + @Column(name = "CUSTOMER_NAME") + private String customerName; + } + }