HHH-19646 Improve validation of configuration settings for the Query Plan Cache

hibernate-core/src/main/java/org/hibernate/query/internal/QueryEngineImpl.java

@@ -13,6 +13,7 @@
 import org.hibernate.engine.jdbc.spi.JdbcServices;
 import org.hibernate.engine.query.spi.NativeQueryInterpreter;
 import org.hibernate.internal.CoreLogging;
+import org.hibernate.internal.util.config.ConfigurationException;
 import org.hibernate.internal.util.config.ConfigurationHelper;
 import org.hibernate.metamodel.MappingMetamodel;
 import org.hibernate.metamodel.model.domain.JpaMetamodel;
@@ -171,9 +172,9 @@ private static List<FunctionContributor> sortedFunctionContributors(ServiceRegis
 		return contributors;
 	}
 
-	private static QueryInterpretationCache buildInterpretationCache(
+	public static QueryInterpretationCache buildInterpretationCache(
 			ServiceRegistry serviceRegistry, Map<String, Object> properties) {
-		final boolean explicitUseCache = ConfigurationHelper.getBoolean(
+		final boolean useCache = ConfigurationHelper.getBoolean(
 				AvailableSettings.QUERY_PLAN_CACHE_ENABLED,
 				properties,
 				// enabled by default
@@ -185,12 +186,20 @@ private static QueryInterpretationCache buildInterpretationCache(
 				properties
 		);
 
-		if ( explicitUseCache || explicitMaxPlanSize != null && explicitMaxPlanSize > 0 ) {
-			final int size = explicitMaxPlanSize != null
-					? explicitMaxPlanSize
-					: QueryEngine.DEFAULT_QUERY_PLAN_MAX_COUNT;
+		//Let's avoid some confusion and check settings consistency:
+		final int appliedMaxPlanSize = explicitMaxPlanSize == null
+				? QueryEngine.DEFAULT_QUERY_PLAN_MAX_COUNT
+				: explicitMaxPlanSize;
+		if ( !useCache && explicitMaxPlanSize != null && appliedMaxPlanSize > 0 ) {
+			throw new ConfigurationException( "Inconsistent configuration: '" + AvailableSettings.QUERY_PLAN_CACHE_MAX_SIZE + "' can only be set to a greater than zero value when '" + AvailableSettings.QUERY_PLAN_CACHE_ENABLED + "' is enabled" );
+		}
+
+		if ( appliedMaxPlanSize < 0 ) {
+			throw new ConfigurationException( "Inconsistent configuration: '" + AvailableSettings.QUERY_PLAN_CACHE_MAX_SIZE + "' can't be set to a negative value. To disable the query plan cache set '" + AvailableSettings.QUERY_PLAN_CACHE_ENABLED + "' to 'false'" );
+		}
 
-			return new QueryInterpretationCacheStandardImpl( size, serviceRegistry );
+		if ( useCache ) {
+			return new QueryInterpretationCacheStandardImpl( appliedMaxPlanSize, serviceRegistry );
 		}
 		else {
 			// disabled

hibernate-core/src/test/java/org/hibernate/orm/test/querycache/QueryEngineImplConfigValidationTest.java

@@ -0,0 +1,139 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ * Copyright Red Hat Inc. and Hibernate Authors
+ */
+package org.hibernate.orm.test.querycache;
+
+import org.hibernate.boot.registry.StandardServiceRegistry;
+import org.hibernate.cfg.AvailableSettings;
+import org.hibernate.internal.util.config.ConfigurationException;
+import org.hibernate.query.internal.QueryInterpretationCacheDisabledImpl;
+import org.hibernate.query.internal.QueryInterpretationCacheStandardImpl;
+import org.hibernate.query.spi.QueryInterpretationCache;
+import org.hibernate.service.ServiceRegistry;
+import org.hibernate.boot.registry.StandardServiceRegistryBuilder;
+import org.hibernate.query.internal.QueryEngineImpl;
+import org.hibernate.testing.orm.junit.Jira;
+import org.junit.jupiter.api.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Test for configuration validation in QueryEngineImpl.
+ * Tests the consistency checks between QUERY_PLAN_CACHE_ENABLED and QUERY_PLAN_CACHE_MAX_SIZE.
+ */
+@Jira("HHH-19646")
+public class QueryEngineImplConfigValidationTest {
+
+	@Test
+	public void testCacheEnabledWithValidMaxSize() {
+		Map<String, Object> settings = new HashMap<>();
+		settings.put( AvailableSettings.QUERY_PLAN_CACHE_ENABLED, true );
+		settings.put( AvailableSettings.QUERY_PLAN_CACHE_MAX_SIZE, 100 );
+		try (ServiceRegistry serviceRegistry = newRegistry()) {
+			QueryInterpretationCache interpretationCache = assertDoesNotThrow( () ->
+					QueryEngineImpl.buildInterpretationCache( serviceRegistry, settings )
+			);
+			testCacheEnabled( interpretationCache );
+		}
+	}
+
+	@Test
+	public void testCacheEnabledWithDefaultMaxSize() {
+		Map<String, Object> settings = new HashMap<>();
+		settings.put( AvailableSettings.QUERY_PLAN_CACHE_ENABLED, true );
+		// No explicit max size - should use default
+		try (ServiceRegistry serviceRegistry = newRegistry()) {
+			QueryInterpretationCache interpretationCache = assertDoesNotThrow( () ->
+					QueryEngineImpl.buildInterpretationCache( serviceRegistry, settings )
+			);
+			testCacheEnabled( interpretationCache );
+		}
+	}
+
+	@Test
+	public void testCacheDisabledWithNoMaxSize() {
+		Map<String, Object> settings = new HashMap<>();
+		settings.put( AvailableSettings.QUERY_PLAN_CACHE_ENABLED, false );
+		// No explicit max size - should work fine
+		try (ServiceRegistry serviceRegistry = newRegistry()) {
+			QueryInterpretationCache interpretationCache = assertDoesNotThrow( () ->
+					QueryEngineImpl.buildInterpretationCache( serviceRegistry, settings )
+			);
+			testCacheDisabled( interpretationCache );
+		}
+	}
+
+	@Test
+	public void testCacheDisabledWithPositiveMaxSize() {
+		Map<String, Object> settings = new HashMap<>();
+		settings.put( AvailableSettings.QUERY_PLAN_CACHE_ENABLED, false );
+		settings.put( AvailableSettings.QUERY_PLAN_CACHE_MAX_SIZE, 100 );
+		//Explicit max size, with cache explicitly disabled is an inconsistency we want to flag
+		try (ServiceRegistry serviceRegistry = newRegistry()) {
+			ConfigurationException exception = assertThrows( ConfigurationException.class, () ->
+					QueryEngineImpl.buildInterpretationCache( serviceRegistry, settings )
+			);
+			assertTrue( exception.getMessage().matches(
+					"Inconsistent configuration: '" + AvailableSettings.QUERY_PLAN_CACHE_MAX_SIZE + "' can only be set to a greater than zero value when '" + AvailableSettings.QUERY_PLAN_CACHE_ENABLED + "' is enabled" ) );
+		}
+	}
+
+	@Test
+	public void testCacheDisabledWithZeroMaxSize() {
+		Map<String, Object> settings = new HashMap<>();
+		settings.put( AvailableSettings.QUERY_PLAN_CACHE_ENABLED, false );
+		settings.put( AvailableSettings.QUERY_PLAN_CACHE_MAX_SIZE, 0 );
+		try (ServiceRegistry serviceRegistry = newRegistry()) {
+			QueryInterpretationCache interpretationCache = assertDoesNotThrow( () ->
+					QueryEngineImpl.buildInterpretationCache( serviceRegistry, settings )
+			);
+			testCacheDisabled( interpretationCache );
+		}
+	}
+
+	@Test
+	public void testNegativeMaxSize() {
+		Map<String, Object> settings = new HashMap<>();
+		settings.put( AvailableSettings.QUERY_PLAN_CACHE_ENABLED, true );
+		settings.put( AvailableSettings.QUERY_PLAN_CACHE_MAX_SIZE, -1 );
+		try (ServiceRegistry serviceRegistry = newRegistry()) {
+			ConfigurationException exception = assertThrows( ConfigurationException.class, () ->
+					QueryEngineImpl.buildInterpretationCache( serviceRegistry, settings )
+			);
+			assertTrue( exception.getMessage().contains( "can't be set to a negative value" ) );
+		}
+	}
+
+	@Test
+	public void testDefaultConfigurationWorks() {
+		Map<String, Object> settings = new HashMap<>();
+		// No explicit settings - should use defaults and work fine
+		try (ServiceRegistry serviceRegistry = newRegistry()) {
+			QueryInterpretationCache interpretationCache = assertDoesNotThrow( () ->
+					QueryEngineImpl.buildInterpretationCache( serviceRegistry, settings )
+			);
+			testCacheEnabled( interpretationCache );
+		}
+	}
+
+	private static StandardServiceRegistry newRegistry() {
+		return new StandardServiceRegistryBuilder().build();
+	}
+
+	private void testCacheEnabled(QueryInterpretationCache interpretationCache) {
+		assertInstanceOf( QueryInterpretationCacheStandardImpl.class, interpretationCache,
+				"Default interpretation cache should be used" );
+	}
+
+	private void testCacheDisabled(QueryInterpretationCache interpretationCache) {
+		assertInstanceOf( QueryInterpretationCacheDisabledImpl.class, interpretationCache,
+				"Cache should have been disabled" );
+	}
+}