From 2fc51bd7b2dc0a604aabbd812a8d5369287e13d4 Mon Sep 17 00:00:00 2001 From: Gavin King Date: Sun, 15 Sep 2024 06:35:12 +0200 Subject: [PATCH] attempt to untangle some convoluted logic in Query hierarchy --- .../spi/AbstractCommonQueryContract.java | 146 ++++-------------- .../hibernate/query/spi/AbstractQuery.java | 13 +- .../query/spi/AbstractSelectionQuery.java | 10 +- .../internal/AbstractSqmSelectionQuery.java | 45 +++--- .../query/sqm/internal/QuerySqmImpl.java | 19 +-- 5 files changed, 65 insertions(+), 168 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractCommonQueryContract.java b/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractCommonQueryContract.java index 2071d2b31e..5745e87f3e 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractCommonQueryContract.java +++ b/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractCommonQueryContract.java @@ -15,7 +15,6 @@ import java.util.HashMap; import java.util.Map; import java.util.Set; -import org.hibernate.CacheMode; import org.hibernate.FlushMode; import org.hibernate.query.QueryFlushMode; import org.hibernate.HibernateException; @@ -40,8 +39,6 @@ import org.hibernate.query.BindableType; import org.hibernate.query.CommonQueryContract; import org.hibernate.query.QueryLogging; import org.hibernate.query.QueryParameter; -import org.hibernate.query.ResultListTransformer; -import org.hibernate.query.TupleTransformer; import org.hibernate.query.TypedParameterValue; import org.hibernate.query.criteria.JpaExpression; import org.hibernate.query.internal.QueryOptionsImpl; @@ -56,7 +53,6 @@ import org.hibernate.type.descriptor.java.JavaType; import jakarta.persistence.CacheRetrieveMode; import jakarta.persistence.CacheStoreMode; import jakarta.persistence.EntityGraph; -import jakarta.persistence.FlushModeType; import jakarta.persistence.LockModeType; import jakarta.persistence.Parameter; import jakarta.persistence.TemporalType; @@ -145,15 +141,11 @@ public abstract class AbstractCommonQueryContract implements CommonQueryContract throw new IllegalArgumentException( "Can't get max rows value from: " + expression ); } // Note that we can never have ties because this is only used when we de-duplicate results - switch ( selectStatement.getFetchClauseType() ) { - case ROWS_ONLY: - case ROWS_WITH_TIES: - return fetchValue.intValue(); - case PERCENT_ONLY: - case PERCENT_WITH_TIES: - return (int) Math.ceil( ( ( (double) size ) * fetchValue.doubleValue() ) / 100d ); - } - throw new UnsupportedOperationException( "Unsupported fetch clause type: " + selectStatement.getFetchClauseType() ); + return switch ( selectStatement.getFetchClauseType() ) { + case ROWS_ONLY, ROWS_WITH_TIES -> fetchValue.intValue(); + case PERCENT_ONLY, PERCENT_WITH_TIES -> + (int) Math.ceil( ( ((double) size) * fetchValue.doubleValue() ) / 100d ); + }; } @@ -244,13 +236,14 @@ public abstract class AbstractCommonQueryContract implements CommonQueryContract @Override public CommonQueryContract setHint(String hintName, Object value) { - applyHint( hintName, value ); + if ( !applyHint( hintName, value ) ) { + QueryLogging.QUERY_MESSAGE_LOGGER.ignoringUnrecognizedQueryHint( hintName ); + } return this; } public final boolean applyHint(String hintName, Object value) { getSession().checkOpen( true ); - try { switch ( hintName ) { case HINT_FLUSH_MODE: @@ -277,13 +270,7 @@ public abstract class AbstractCommonQueryContract implements CommonQueryContract applyDatabaseHint( (String) value ); return true; default: - if ( applySelectionHint( hintName, value ) || applyAdditionalPossibleHints( hintName, value ) ) { - return true; - } - else { - QueryLogging.QUERY_MESSAGE_LOGGER.ignoringUnrecognizedQueryHint( hintName ); - return false; - } + return applySelectionHint( hintName, value ); } } catch ( ClassCastException e ) { @@ -300,36 +287,41 @@ public abstract class AbstractCommonQueryContract implements CommonQueryContract return true; } else { + final MutableQueryOptions queryOptions = getQueryOptions(); switch ( hintName ) { case HINT_READONLY: - applyReadOnlyHint( getBoolean( value ) ); + queryOptions.setReadOnly( getBoolean( value ) ); return true; case HINT_FETCH_SIZE: - applyFetchSizeHint( getInteger( value ) ); + queryOptions.setFetchSize( getInteger( value ) ); return true; case HINT_QUERY_PLAN_CACHEABLE: - applyQueryPlanCacheableHint( getBoolean( value ) ); + queryOptions.setQueryPlanCachingEnabled( getBoolean( value ) ); return true; case HINT_CACHEABLE: - applyCacheableHint( getBoolean( value ) ); + queryOptions.setResultCachingEnabled( getBoolean( value ) ); return true; case HINT_CACHE_REGION: - applyCacheRegionHint( (String) value ); + queryOptions.setResultCacheRegionName( (String) value ); return true; case HINT_CACHE_MODE: - applyCacheModeHint( getCacheMode( value ) ); + queryOptions.setCacheMode( getCacheMode( value ) ); return true; case HINT_JAVAEE_CACHE_RETRIEVE_MODE: DEPRECATION_LOGGER.deprecatedSetting( HINT_JAVAEE_CACHE_RETRIEVE_MODE, HINT_SPEC_CACHE_RETRIEVE_MODE ); //fall through to: case HINT_SPEC_CACHE_RETRIEVE_MODE: - applyJpaCacheRetrieveModeHint( value != null ? CacheRetrieveMode.valueOf( value.toString() ) : null ); + final CacheRetrieveMode retrieveMode = + value == null ? null : CacheRetrieveMode.valueOf( value.toString() ); + queryOptions.setCacheRetrieveMode( retrieveMode ); return true; case HINT_JAVAEE_CACHE_STORE_MODE: DEPRECATION_LOGGER.deprecatedSetting( HINT_JAVAEE_CACHE_STORE_MODE, HINT_SPEC_CACHE_STORE_MODE ); //fall through to: case HINT_SPEC_CACHE_STORE_MODE: - applyJpaCacheStoreModeHint( value != null ? CacheStoreMode.valueOf( value.toString() ) : null ); + final CacheStoreMode storeMode = + value == null ? null : CacheStoreMode.valueOf( value.toString() ); + queryOptions.setCacheStoreMode( storeMode ); return true; case HINT_JAVAEE_FETCH_GRAPH: DEPRECATION_LOGGER.deprecatedSetting( HINT_JAVAEE_FETCH_GRAPH, HINT_SPEC_FETCH_GRAPH ); @@ -350,37 +342,13 @@ public abstract class AbstractCommonQueryContract implements CommonQueryContract } } - protected void applyFetchSizeHint(int fetchSize) { - getQueryOptions().setFetchSize( fetchSize ); - } - - protected void applyQueryPlanCacheableHint(boolean isCacheable) { - getQueryOptions().setQueryPlanCachingEnabled( isCacheable ); - } - - protected void applyCacheModeHint(CacheMode cacheMode) { - getQueryOptions().setCacheMode( cacheMode ); - } - - protected void applyCacheableHint(boolean isCacheable) { - getQueryOptions().setResultCachingEnabled( isCacheable ); - } - - protected void applyCacheRegionHint(String regionName) { - getQueryOptions().setResultCacheRegionName( regionName ); - } - - private void applyReadOnlyHint(Boolean readOnly) { - getQueryOptions().setReadOnly( readOnly ); - } - protected void applyEntityGraphHint(String hintName, Object value) { final GraphSemantic graphSemantic = GraphSemantic.fromHintName( hintName ); - if ( value instanceof RootGraphImplementor ) { - applyGraph( (RootGraphImplementor) value, graphSemantic ); + if ( value instanceof RootGraphImplementor rootGraphImplementor ) { + applyGraph( rootGraphImplementor, graphSemantic ); } - else if ( value instanceof String ) { - applyGraph( (String) value, graphSemantic ); + else if ( value instanceof String string ) { + applyGraph( string, graphSemantic ); } else { throw new IllegalArgumentException( "The value of the hint '" + hintName @@ -465,11 +433,11 @@ public abstract class AbstractCommonQueryContract implements CommonQueryContract } protected final void applyLockModeHint(Object value) { - if ( value instanceof LockMode ) { - applyHibernateLockMode( (LockMode) value ); + if ( value instanceof LockMode lockMode ) { + applyHibernateLockMode( lockMode ); } - else if ( value instanceof LockModeType ) { - applyLockModeType( (LockModeType) value ); + else if ( value instanceof LockModeType lockModeType ) { + applyLockModeType( lockModeType ); } else if ( value instanceof String ) { applyHibernateLockMode( interpretLockMode( value ) ); @@ -503,10 +471,6 @@ public abstract class AbstractCommonQueryContract implements CommonQueryContract getQueryOptions().getLockOptions().setFollowOnLocking( followOnLocking ); } - protected boolean applyAdditionalPossibleHints(String hintName, Object value) { - return false; - } - // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // Options @@ -543,16 +507,6 @@ public abstract class AbstractCommonQueryContract implements CommonQueryContract return this; } - protected boolean applyJpaCacheRetrieveModeHint(CacheRetrieveMode retrieveMode) { - getQueryOptions().setCacheRetrieveMode( retrieveMode ); - return true; - } - - protected boolean applyJpaCacheStoreModeHint(CacheStoreMode storeMode) { - getQueryOptions().setCacheStoreMode( storeMode ); - return true; - } - protected void applyTimeoutHint(int timeout) { setTimeout( timeout ); } @@ -593,51 +547,11 @@ public abstract class AbstractCommonQueryContract implements CommonQueryContract return getQueryOptions().getLimit().getMaxRowsJpa(); } - public void applyMaxResults(int maxResult) { - if ( maxResult < 0 ) { - throw new IllegalArgumentException( "max-results cannot be negative" ); - } - getSession().checkOpen(); - getQueryOptions().getLimit().setMaxRows( maxResult ); - } - public int getFirstResult() { getSession().checkOpen(); return getQueryOptions().getLimit().getFirstRowJpa(); } - public void applyFirstResult(int startPosition) { - if ( startPosition < 0 ) { - throw new IllegalArgumentException( "first-result value cannot be negative : " + startPosition ); - } - - getSession().checkOpen(); - getQueryOptions().getLimit().setFirstRow( startPosition ); - } - - protected FlushModeType getJpaFlushMode() { - getSession().checkOpen(); - final FlushMode flushMode = getQueryOptions().getFlushMode() == null - ? getSession().getHibernateFlushMode() - : getQueryOptions().getFlushMode(); - return FlushModeTypeHelper.getFlushModeType( flushMode ); - } - - protected void applyJpaFlushMode(FlushModeType flushModeType) { - getSession().checkOpen(); - setHibernateFlushMode( FlushModeTypeHelper.getFlushMode( flushModeType ) ); - } - - public boolean applyTupleTransformer(TupleTransformer transformer) { - getQueryOptions().setTupleTransformer( transformer ); - return true; - } - - public boolean applyResultListTransformer(ResultListTransformer transformer) { - getQueryOptions().setResultListTransformer( transformer ); - return true; - } - // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ // Parameter handling diff --git a/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractQuery.java b/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractQuery.java index ab954dbfa8..319e649436 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractQuery.java +++ b/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractQuery.java @@ -36,7 +36,6 @@ import org.hibernate.graph.GraphSemantic; import org.hibernate.internal.CoreLogging; import org.hibernate.internal.CoreMessageLogger; import org.hibernate.jpa.AvailableHints; -import org.hibernate.jpa.internal.util.FlushModeTypeHelper; import org.hibernate.jpa.internal.util.LockModeTypeHelper; import org.hibernate.query.BindableType; import org.hibernate.query.IllegalQueryOperationException; @@ -197,20 +196,13 @@ public abstract class AbstractQuery @Override public QueryImplementor setQueryFlushMode(QueryFlushMode queryFlushMode) { - super.setQueryFlushMode(queryFlushMode); + super.setQueryFlushMode( queryFlushMode ); return this; } - @Override - public FlushModeType getFlushMode() { -// getSession().checkOpen(); - return FlushModeTypeHelper.getFlushModeType( getHibernateFlushMode() ); - } - @Override public QueryImplementor setFlushMode(FlushModeType flushModeType) { -// getSession().checkOpen(); - setHibernateFlushMode( FlushModeTypeHelper.getFlushMode( flushModeType ) ); + super.setFlushMode( flushModeType ); return this; } @@ -400,7 +392,6 @@ public abstract class AbstractQuery } @Override - @SuppressWarnings( {"unchecked", "rawtypes"} ) public Set> getParameters() { return super.getParameters(); } diff --git a/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractSelectionQuery.java b/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractSelectionQuery.java index dddfbb81e5..e278f9a18a 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractSelectionQuery.java +++ b/hibernate-core/src/main/java/org/hibernate/query/spi/AbstractSelectionQuery.java @@ -334,18 +334,24 @@ public abstract class AbstractSelectionQuery @Override public FlushModeType getFlushMode() { + getSession().checkOpen(); return FlushModeTypeHelper.getFlushModeType( getHibernateFlushMode() ); } @Override public SelectionQuery setFlushMode(FlushModeType flushMode) { + getSession().checkOpen(); getQueryOptions().setFlushMode( FlushModeTypeHelper.getFlushMode( flushMode ) ); return this; } @Override public SelectionQuery setMaxResults(int maxResult) { - super.applyMaxResults( maxResult ); + if ( maxResult < 0 ) { + throw new IllegalArgumentException( "Max results cannot be negative" ); + } + getSession().checkOpen(); + getQueryOptions().getLimit().setMaxRows(maxResult); return this; } @@ -353,7 +359,7 @@ public abstract class AbstractSelectionQuery public SelectionQuery setFirstResult(int startPosition) { getSession().checkOpen(); if ( startPosition < 0 ) { - throw new IllegalArgumentException( "first-result value cannot be negative : " + startPosition ); + throw new IllegalArgumentException( "First result cannot be negative" ); } getQueryOptions().getLimit().setFirstRow( startPosition ); return this; diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/AbstractSqmSelectionQuery.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/AbstractSqmSelectionQuery.java index d1d5d814b4..64a4b2e4f6 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/AbstractSqmSelectionQuery.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/AbstractSqmSelectionQuery.java @@ -25,7 +25,6 @@ import org.hibernate.query.spi.AbstractSelectionQuery; import org.hibernate.query.spi.HqlInterpretation; import org.hibernate.query.spi.MutableQueryOptions; import org.hibernate.query.spi.QueryEngine; -import org.hibernate.query.spi.QueryInterpretationCache; import org.hibernate.query.spi.QueryOptions; import org.hibernate.query.spi.SelectQueryPlan; import org.hibernate.query.sqm.NodeBuilder; @@ -35,7 +34,6 @@ import org.hibernate.query.sqm.tree.from.SqmRoot; import org.hibernate.query.sqm.tree.select.SqmQueryGroup; import org.hibernate.query.sqm.tree.select.SqmQueryPart; import org.hibernate.query.sqm.tree.select.SqmQuerySpec; -import org.hibernate.query.sqm.tree.select.SqmSelectClause; import org.hibernate.query.sqm.tree.select.SqmSelectStatement; import org.hibernate.query.sqm.tree.select.SqmSelectableNode; import org.hibernate.query.sqm.tree.select.SqmSelection; @@ -87,7 +85,7 @@ abstract class AbstractSqmSelectionQuery extends AbstractSelectionQuery { protected boolean needsDistinct(boolean containsCollectionFetches, boolean hasLimit, SqmSelectStatement sqmStatement) { return containsCollectionFetches - && ( hasLimit || sqmStatement.usesDistinct() || hasAppliedGraph( getQueryOptions() ) ); + && ( hasLimit || sqmStatement.usesDistinct() || hasAppliedGraph( getQueryOptions() ) ); } protected static boolean hasAppliedGraph(MutableQueryOptions queryOptions) { @@ -186,7 +184,7 @@ abstract class AbstractSqmSelectionQuery extends AbstractSelectionQuery { final List> keyDefinition = keyedPage.getKeyDefinition(); final List> appliedKeyDefinition = keyedPage.getKeyInterpretation() == KEY_OF_FIRST_ON_NEXT_PAGE - ? Order.reverse(keyDefinition) : keyDefinition; + ? Order.reverse( keyDefinition ) : keyDefinition; setMaxResults( page.getMaxResults() + 1 ); if ( key == null ) { @@ -309,12 +307,9 @@ abstract class AbstractSqmSelectionQuery extends AbstractSelectionQuery { } protected TupleMetadata buildTupleMetadata(SqmStatement statement, Class resultType) { - if ( statement instanceof SqmSelectStatement ) { - final SqmSelectStatement select = (SqmSelectStatement) statement; - final SqmSelectClause selectClause = select.getQueryPart().getFirstQuerySpec().getSelectClause(); + if ( statement instanceof SqmSelectStatement select ) { final List> selections = - selectClause - .getSelections(); + select.getQueryPart().getFirstQuerySpec().getSelectClause().getSelections(); return isTupleMetadataRequired( resultType, selections.get(0) ) ? getTupleMetadata( selections ) : null; @@ -326,15 +321,15 @@ abstract class AbstractSqmSelectionQuery extends AbstractSelectionQuery { private static boolean isTupleMetadataRequired(Class resultType, SqmSelection selection) { return isHqlTuple( selection ) - || !isInstantiableWithoutMetadata( resultType ) + || !isInstantiableWithoutMetadata( resultType ) && !isSelectionAssignableToResultType( selection, resultType ); } private static boolean isInstantiableWithoutMetadata(Class resultType) { return resultType == null - || resultType.isArray() - || Object.class == resultType - || List.class == resultType; + || resultType.isArray() + || Object.class == resultType + || List.class == resultType; } private TupleMetadata getTupleMetadata(List> selections) { @@ -398,10 +393,8 @@ abstract class AbstractSqmSelectionQuery extends AbstractSelectionQuery { } protected static void validateCriteriaQuery(SqmQueryPart queryPart) { - if ( queryPart instanceof SqmQuerySpec ) { - final SqmQuerySpec sqmQuerySpec = (SqmQuerySpec) queryPart; - final List> selections = sqmQuerySpec.getSelectClause().getSelections(); - if ( selections.isEmpty() ) { + if ( queryPart instanceof SqmQuerySpec sqmQuerySpec ) { + if ( sqmQuerySpec.getSelectClause().getSelections().isEmpty() ) { // make sure there is at least one root final List> sqmRoots = sqmQuerySpec.getFromClause().getRoots(); if ( sqmRoots == null || sqmRoots.isEmpty() ) { @@ -416,25 +409,23 @@ abstract class AbstractSqmSelectionQuery extends AbstractSelectionQuery { } } } - else { - final SqmQueryGroup queryGroup = (SqmQueryGroup) queryPart; + else if ( queryPart instanceof SqmQueryGroup queryGroup ) { for ( SqmQueryPart part : queryGroup.getQueryParts() ) { validateCriteriaQuery( part ); } } + else { + assert false; + } } protected static HqlInterpretation interpretation( - NamedHqlQueryMementoImpl memento, + NamedHqlQueryMementoImpl memento, Class expectedResultType, SharedSessionContractImplementor session) { final QueryEngine queryEngine = session.getFactory().getQueryEngine(); - final QueryInterpretationCache interpretationCache = queryEngine.getInterpretationCache(); - final HqlInterpretation interpretation = interpretationCache.resolveHqlInterpretation( - memento.getHqlString(), - expectedResultType, - queryEngine.getHqlTranslator() - ); - return interpretation; + return queryEngine.getInterpretationCache() + .resolveHqlInterpretation( memento.getHqlString(), expectedResultType, + queryEngine.getHqlTranslator() ); } } diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/QuerySqmImpl.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/QuerySqmImpl.java index 037b89a31a..3d90b52094 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/QuerySqmImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/internal/QuerySqmImpl.java @@ -702,26 +702,26 @@ public class QuerySqmImpl @Override public SqmQueryImplementor setTupleTransformer(TupleTransformer transformer) { - applyTupleTransformer( transformer ); + getQueryOptions().setTupleTransformer( transformer ); //noinspection unchecked return (SqmQueryImplementor) this; } @Override public SqmQueryImplementor setResultListTransformer(ResultListTransformer transformer) { - applyResultListTransformer( transformer ); + getQueryOptions().setResultListTransformer( transformer ); return this; } @Override public SqmQueryImplementor setMaxResults(int maxResult) { - applyMaxResults( maxResult ); + super.setMaxResults( maxResult ); return this; } @Override public SqmQueryImplementor setFirstResult(int startPosition) { - applyFirstResult( startPosition ); + super.setFirstResult( startPosition ); return this; } @@ -733,13 +733,13 @@ public class QuerySqmImpl @Override public SqmQueryImplementor setQueryFlushMode(QueryFlushMode queryFlushMode) { - super.setQueryFlushMode(queryFlushMode); + super.setQueryFlushMode( queryFlushMode ); return this; } @Override public SqmQueryImplementor setFlushMode(FlushModeType flushMode) { - applyJpaFlushMode( flushMode ); + super.setFlushMode( flushMode ); return this; } @@ -761,11 +761,6 @@ public class QuerySqmImpl return getLockOptions().getLockMode().toJpaLockMode(); } - @Override - public FlushModeType getFlushMode() { - return getJpaFlushMode(); - } - @Override public Query setOrder(Order order) { super.setOrder(order); @@ -819,7 +814,7 @@ public class QuerySqmImpl @Override public SqmQueryImplementor setHint(String hintName, Object value) { - applyHint( hintName, value ); + super.setHint( hintName, value ); return this; }