HHH-4959 - Concurrent HQL parsing blocks on ReflectHelper.classForName()

Exclude JPQL and Criteria API aliases when searching for a Java constant value

(cherry picked from commit 0bd7b8eac1)

documentation/src/main/asciidoc/userguide/appendices/Configurations.adoc

@@ -290,6 +290,16 @@ Should we strictly adhere to JPA Query Language (JPQL) syntax, or more broadly s
 Setting this to `true` may cause valid HQL to throw an exception because it violates the JPQL subset.
 
 |`hibernate.query.startup_check` | `true` (default value) or `false` |Should named queries be checked during startup?
+|`hibernate.query.conventional_java_constants` | `true` (default value) or `false` |
+Setting which indicates whether or not Java constant follow the https://docs.oracle.com/javase/tutorial/java/nutsandbolts/variables.html[Java Naming conventions].
+
+Default is `true`.
+Existing applications may want to disable this (set it `false`) if non-conventional Java constants are used.
+However, there is a significant performance overhead for using non-conventional Java constants
+since Hibernate cannot determine if aliases should be treated as Java constants or not.
+
+Check out https://hibernate.atlassian.net/browse/HHH-4959[HHH-4959] for more details.
+
 |`hibernate.hql.bulk_id_strategy` | A fully-qualified class name, an instance, or a `Class` object reference |Provide a custom `org.hibernate.hql.spi.id.MultiTableBulkIdStrategy` implementation for handling multi-table bulk HQL operations.
 |`hibernate.proc.param_null_passing` | `true` or `false` (default value) |
 
@@ -310,7 +320,6 @@ Can reference
 * `StatementInspector` instance
 * `StatementInspector` implementation {@link Class} reference
 * `StatementInspector` implementation class name (fully-qualified class name)
-
 |===================================================================================================================================================================================================================================
 
 [[configurations-batch]]

hibernate-core/src/main/java/org/hibernate/boot/internal/SessionFactoryBuilderImpl.java

@@ -483,6 +483,7 @@ public class SessionFactoryBuilderImpl implements SessionFactoryBuilderImplement
 		private Map querySubstitutions;
 		private boolean strictJpaQueryLanguageCompliance;
 		private boolean namedQueryStartupCheckingEnabled;
+		private boolean conventionalJavaConstants;
 		private final boolean procedureParameterNullPassingEnabled;
 		private final boolean collectionJoinSubqueryRewriteEnabled;
 
@@ -600,6 +601,8 @@ public class SessionFactoryBuilderImpl implements SessionFactoryBuilderImplement
 			this.querySubstitutions = ConfigurationHelper.toMap( QUERY_SUBSTITUTIONS, " ,=;:\n\t\r\f", configurationSettings );
 			this.strictJpaQueryLanguageCompliance = cfgService.getSetting( JPAQL_STRICT_COMPLIANCE, BOOLEAN, false );
 			this.namedQueryStartupCheckingEnabled = cfgService.getSetting( QUERY_STARTUP_CHECKING, BOOLEAN, true );
+			this.conventionalJavaConstants = cfgService.getSetting(
+					CONVENTIONAL_JAVA_CONSTANTS, BOOLEAN, true );
 			this.procedureParameterNullPassingEnabled = cfgService.getSetting( PROCEDURE_NULL_PARAM_PASSING, BOOLEAN, false );
 			this.collectionJoinSubqueryRewriteEnabled = cfgService.getSetting( COLLECTION_JOIN_SUBQUERY, BOOLEAN, true );
 
@@ -851,6 +854,10 @@ public class SessionFactoryBuilderImpl implements SessionFactoryBuilderImplement
 			return namedQueryStartupCheckingEnabled;
 		}
 
+		public boolean isConventionalJavaConstants() {
+			return conventionalJavaConstants;
+		}
+
 		@Override
 		public boolean isProcedureParameterNullPassingEnabled() {
 			return procedureParameterNullPassingEnabled;
@@ -1132,6 +1139,10 @@ public class SessionFactoryBuilderImpl implements SessionFactoryBuilderImplement
 		return options.isNamedQueryStartupCheckingEnabled();
 	}
 
+	public boolean isConventionalJavaConstants() {
+		return options.isConventionalJavaConstants();
+	}
+
 	@Override
 	public boolean isProcedureParameterNullPassingEnabled() {
 		return options.isProcedureParameterNullPassingEnabled();

hibernate-core/src/main/java/org/hibernate/boot/internal/SessionFactoryOptionsImpl.java

@@ -88,6 +88,7 @@ public class SessionFactoryOptionsImpl implements SessionFactoryOptions {
 	private final Map querySubstitutions;
 	private final boolean strictJpaQueryLanguageCompliance;
 	private final boolean namedQueryStartupCheckingEnabled;
+	private final boolean conventionalJavaConstants;
 	private final boolean procedureParameterNullPassingEnabled;
 	private final boolean collectionJoinSubqueryRewriteEnabled;
 
@@ -160,6 +161,7 @@ public class SessionFactoryOptionsImpl implements SessionFactoryOptions {
 		this.querySubstitutions = state.getQuerySubstitutions();
 		this.strictJpaQueryLanguageCompliance = state.isStrictJpaQueryLanguageCompliance();
 		this.namedQueryStartupCheckingEnabled = state.isNamedQueryStartupCheckingEnabled();
+		this.conventionalJavaConstants = state.isConventionalJavaConstants();
 		this.procedureParameterNullPassingEnabled = state.isProcedureParameterNullPassingEnabled();
 		this.collectionJoinSubqueryRewriteEnabled = state.isCollectionJoinSubqueryRewriteEnabled();
 
@@ -339,6 +341,11 @@ public class SessionFactoryOptionsImpl implements SessionFactoryOptions {
 		return namedQueryStartupCheckingEnabled;
 	}
 
+	@Override
+	public boolean isConventionalJavaConstants() {
+		return conventionalJavaConstants;
+	}
+
 	@Override
 	public boolean isProcedureParameterNullPassingEnabled() {
 		return procedureParameterNullPassingEnabled;

hibernate-core/src/main/java/org/hibernate/boot/internal/SessionFactoryOptionsState.java

@@ -99,6 +99,8 @@ public interface SessionFactoryOptionsState {
 
 	boolean isNamedQueryStartupCheckingEnabled();
 
+	boolean isConventionalJavaConstants();
+
 	boolean isProcedureParameterNullPassingEnabled();
 
 	boolean isCollectionJoinSubqueryRewriteEnabled();

hibernate-core/src/main/java/org/hibernate/boot/spi/AbstractDelegatingSessionFactoryOptions.java

@@ -198,6 +198,11 @@ public abstract class AbstractDelegatingSessionFactoryOptions implements Session
 		return delegate.isNamedQueryStartupCheckingEnabled();
 	}
 
+	@Override
+	public boolean isConventionalJavaConstants() {
+		return delegate.isConventionalJavaConstants();
+	}
+
 	@Override
 	public boolean isProcedureParameterNullPassingEnabled() {
 		return delegate.isProcedureParameterNullPassingEnabled();

hibernate-core/src/main/java/org/hibernate/boot/spi/SessionFactoryOptions.java

@@ -122,6 +122,8 @@ public interface SessionFactoryOptions {
 
 	boolean isNamedQueryStartupCheckingEnabled();
 
+	boolean isConventionalJavaConstants();
+
 	boolean isSecondLevelCacheEnabled();
 
 	boolean isQueryCacheEnabled();

hibernate-core/src/main/java/org/hibernate/cfg/AvailableSettings.java

@@ -562,6 +562,17 @@ public interface AvailableSettings {
 	 */
 	String QUERY_STARTUP_CHECKING = "hibernate.query.startup_check";
 
+	/**
+	 * Setting which indicates whether or not Java constant follow the Java Naming conventions.
+	 * <p/>
+	 * Default is {@code true}. Existing applications may want to disable this (set it {@code false}) if non-conventional Java constants are used.
+	 * However, there is a significant performance overhead for using non-conventional Java constants since Hibernate cannot determine if aliases
+	 * should be treated as Java constants or not.
+	 *
+	 * @since 5.2
+	 */
+	String CONVENTIONAL_JAVA_CONSTANTS = "hibernate.query.conventional_java_constants";
+
 	/**
 	 * The {@link org.hibernate.exception.spi.SQLExceptionConverter} to use for converting SQLExceptions
 	 * to Hibernate's JDBCException hierarchy.  The default is to use the configured

hibernate-core/src/main/java/org/hibernate/hql/internal/ast/QueryTranslatorImpl.java

@@ -19,7 +19,6 @@ import org.hibernate.HibernateException;
 import org.hibernate.MappingException;
 import org.hibernate.QueryException;
 import org.hibernate.ScrollableResults;
-import org.hibernate.boot.registry.classloading.spi.ClassLoaderService;
 import org.hibernate.engine.query.spi.EntityGraphQueryHint;
 import org.hibernate.engine.spi.QueryParameters;
 import org.hibernate.engine.spi.RowSelection;
@@ -590,7 +589,6 @@ public class QueryTranslatorImpl implements FilterTranslator {
 		private AST dotRoot;
 
 		public JavaConstantConverter(SessionFactoryImplementor factory) {
-
 			this.factory = factory;
 		}
 
@@ -612,7 +610,7 @@ public class QueryTranslatorImpl implements FilterTranslator {
 		}
 		private void handleDotStructure(AST dotStructureRoot) {
 			final String expression = ASTUtil.getPathText( dotStructureRoot );
-			final Object constant = ReflectHelper.getConstantValue( expression, factory.getServiceRegistry().getService( ClassLoaderService.class ) );
+			final Object constant = ReflectHelper.getConstantValue( expression, factory );
 			if ( constant != null ) {
 				dotStructureRoot.setFirstChild( null );
 				dotStructureRoot.setType( HqlTokenTypes.JAVA_CONSTANT );

hibernate-core/src/main/java/org/hibernate/hql/internal/ast/tree/JavaConstantNode.java

@@ -9,7 +9,6 @@ package org.hibernate.hql.internal.ast.tree;
 import java.util.Locale;
 
 import org.hibernate.QueryException;
-import org.hibernate.boot.registry.classloading.spi.ClassLoaderService;
 import org.hibernate.dialect.Dialect;
 import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.hql.spi.QueryTranslator;
@@ -39,7 +38,7 @@ public class JavaConstantNode extends Node implements ExpectedTypeAwareNode, Ses
 		// this method to get called twice.  The first time with an empty string
 		if ( StringHelper.isNotEmpty( s ) ) {
 			constantExpression = s;
-			constantValue = ReflectHelper.getConstantValue( s, factory.getServiceRegistry().getService( ClassLoaderService.class ) );
+			constantValue = ReflectHelper.getConstantValue( s, factory );
 			heuristicType = factory.getTypeResolver().heuristicType( constantValue.getClass().getName() );
 			super.setText( s );
 		}

hibernate-core/src/main/java/org/hibernate/hql/internal/ast/util/LiteralProcessor.java

@@ -13,13 +13,11 @@ import java.text.DecimalFormat;
 import org.hibernate.HibernateException;
 import org.hibernate.MappingException;
 import org.hibernate.QueryException;
-import org.hibernate.boot.registry.classloading.spi.ClassLoaderService;
 import org.hibernate.dialect.Dialect;
 import org.hibernate.hql.internal.antlr.HqlSqlTokenTypes;
 import org.hibernate.hql.internal.antlr.SqlTokenTypes;
 import org.hibernate.hql.internal.ast.HqlSqlWalker;
 import org.hibernate.hql.internal.ast.InvalidPathException;
-import org.hibernate.hql.internal.ast.tree.BooleanLiteralNode;
 import org.hibernate.hql.internal.ast.tree.DotNode;
 import org.hibernate.hql.internal.ast.tree.FromClause;
 import org.hibernate.hql.internal.ast.tree.IdentNode;
@@ -35,7 +33,6 @@ import org.jboss.logging.Logger;
 
 import antlr.SemanticException;
 import antlr.collections.AST;
-import java.util.Locale;
 
 /**
  * A delegate that handles literals and constants for HqlSqlWalker, performing the token replacement functions and
@@ -109,7 +106,7 @@ public class LiteralProcessor implements HqlSqlTokenTypes {
 			setSQLValue( node, text, discrim );
 		}
 		else {
-			Object value = ReflectHelper.getConstantValue( text, walker.getSessionFactoryHelper().getFactory().getServiceRegistry().getService( ClassLoaderService.class ) );
+			Object value = ReflectHelper.getConstantValue( text, walker.getSessionFactoryHelper().getFactory() );
 			if ( value == null ) {
 				throw new InvalidPathException( "Invalid path: '" + text + "'" );
 			}

hibernate-core/src/main/java/org/hibernate/hql/internal/classic/WhereParser.java

@@ -16,7 +16,6 @@ import java.util.StringTokenizer;
 
 import org.hibernate.MappingException;
 import org.hibernate.QueryException;
-import org.hibernate.boot.registry.classloading.spi.ClassLoaderService;
 import org.hibernate.engine.internal.JoinSequence;
 import org.hibernate.hql.spi.QueryTranslator;
 import org.hibernate.internal.util.ReflectHelper;
@@ -419,7 +418,7 @@ public class WhereParser implements Parser {
 				Object constant;
 				if (
 						token.indexOf( '.' ) > -1 &&
-						( constant = ReflectHelper.getConstantValue( token, q.getFactory().getServiceRegistry().getService( ClassLoaderService.class ) ) ) != null
+						( constant = ReflectHelper.getConstantValue( token, q.getFactory() ) ) != null
 				) {
 					Type type;
 					try {

hibernate-core/src/main/java/org/hibernate/internal/util/ReflectHelper.java

@@ -13,12 +13,14 @@ import java.lang.reflect.Member;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
 import java.util.Locale;
+import java.util.regex.Pattern;
 
 import org.hibernate.AssertionFailure;
 import org.hibernate.MappingException;
 import org.hibernate.PropertyNotFoundException;
 import org.hibernate.boot.registry.classloading.spi.ClassLoaderService;
 import org.hibernate.boot.registry.classloading.spi.ClassLoadingException;
+import org.hibernate.engine.spi.SessionFactoryImplementor;
 import org.hibernate.property.access.internal.PropertyAccessStrategyMixedImpl;
 import org.hibernate.property.access.spi.Getter;
 import org.hibernate.type.PrimitiveType;
@@ -32,6 +34,10 @@ import org.hibernate.type.Type;
  */
 @SuppressWarnings("unchecked")
 public final class ReflectHelper {
+
+	private static final Pattern JAVA_CONSTANT_PATTERN = Pattern.compile(
+			"[a-z]+\\.([A-Z]{1}[a-z]+)+\\$?([A-Z]{1}[a-z]+)*\\.[A-Z_\\$]+" );
+
 	public static final Class[] NO_PARAM_SIGNATURE = new Class[0];
 	public static final Object[] NO_PARAMS = new Object[0];
 
@@ -229,9 +235,15 @@ public final class ReflectHelper {
 		return PropertyAccessStrategyMixedImpl.INSTANCE.buildPropertyAccess( clazz, name ).getGetter();
 	}
 
-	public static Object getConstantValue(String name, ClassLoaderService classLoaderService) {
+	public static Object getConstantValue(String name, SessionFactoryImplementor factory) {
+		boolean conventionalJavaConstants = factory.getSessionFactoryOptions().isConventionalJavaConstants();
 		Class clazz;
 		try {
+			if ( conventionalJavaConstants &&
+				!JAVA_CONSTANT_PATTERN.matcher( name ).find() ) {
+				return null;
+			}
+			ClassLoaderService classLoaderService = factory.getServiceRegistry().getService( ClassLoaderService.class );
 			clazz = classLoaderService.classForName( StringHelper.qualifier( name ) );
 		}
 		catch ( Throwable t ) {

hibernate-core/src/test/java/org/hibernate/internal/util/ReflectHelperTest.java

@@ -0,0 +1,126 @@
+/*
+ * 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.internal.util;
+
+import javax.persistence.FetchType;
+
+import org.hibernate.boot.registry.classloading.spi.ClassLoaderService;
+import org.hibernate.boot.spi.SessionFactoryOptions;
+import org.hibernate.engine.spi.SessionFactoryImplementor;
+import org.hibernate.service.spi.ServiceRegistryImplementor;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import org.mockito.Mockito;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * @author Vlad Mihalcea
+ */
+public class ReflectHelperTest {
+
+	public enum Status {
+		ON,
+		OFF
+	}
+
+	private SessionFactoryImplementor sessionFactoryImplementorMock;
+
+	private SessionFactoryOptions sessionFactoryOptionsMock;
+
+	private ServiceRegistryImplementor serviceRegistryMock;
+
+	private ClassLoaderService classLoaderServiceMock;
+
+	@Before
+	public void init() {
+		sessionFactoryImplementorMock = Mockito.mock(SessionFactoryImplementor.class);
+		sessionFactoryOptionsMock = Mockito.mock(SessionFactoryOptions.class);
+		when(sessionFactoryImplementorMock.getSessionFactoryOptions()).thenReturn( sessionFactoryOptionsMock );
+
+		serviceRegistryMock = Mockito.mock(ServiceRegistryImplementor.class);
+		when( sessionFactoryImplementorMock.getServiceRegistry() ).thenReturn( serviceRegistryMock );
+
+		classLoaderServiceMock = Mockito.mock(ClassLoaderService.class);
+		when( serviceRegistryMock.getService( eq( ClassLoaderService.class ) ) ).thenReturn( classLoaderServiceMock );
+	}
+
+	@Test
+	public void test_getConstantValue_simpleAlias() {
+		when( sessionFactoryOptionsMock.isConventionalJavaConstants() ).thenReturn( true );
+
+		Object value = ReflectHelper.getConstantValue( "alias.b", sessionFactoryImplementorMock);
+		assertNull(value);
+		verify(classLoaderServiceMock, never()).classForName( anyString() );
+	}
+
+	@Test
+	public void test_getConstantValue_simpleAlias_non_conventional() {
+		when( sessionFactoryOptionsMock.isConventionalJavaConstants() ).thenReturn( false );
+
+		Object value = ReflectHelper.getConstantValue( "alias.b", sessionFactoryImplementorMock);
+		assertNull(value);
+		verify(classLoaderServiceMock, times(1)).classForName( eq( "alias" ) );
+	}
+
+	@Test
+	public void test_getConstantValue_nestedAlias() {
+		when( sessionFactoryOptionsMock.isConventionalJavaConstants() ).thenReturn( true );
+
+		Object value = ReflectHelper.getConstantValue( "alias.b.c", sessionFactoryImplementorMock);
+		assertNull(value);
+		verify(classLoaderServiceMock, never()).classForName( anyString() );
+	}
+
+	@Test
+	public void test_getConstantValue_nestedAlias_non_conventional() {
+		when( sessionFactoryOptionsMock.isConventionalJavaConstants() ).thenReturn( false );
+
+		Object value = ReflectHelper.getConstantValue( "alias.b.c", sessionFactoryImplementorMock);
+		assertNull(value);
+		verify(classLoaderServiceMock, times(1)).classForName( eq( "alias.b" ) );
+	}
+
+	@Test
+	public void test_getConstantValue_outerEnum() {
+		when( sessionFactoryOptionsMock.isConventionalJavaConstants() ).thenReturn( true );
+
+		when( classLoaderServiceMock.classForName( "javax.persistence.FetchType" ) ).thenReturn( (Class) FetchType.class );
+		Object value = ReflectHelper.getConstantValue( "javax.persistence.FetchType.LAZY", sessionFactoryImplementorMock);
+		assertEquals( FetchType.LAZY, value );
+		verify(classLoaderServiceMock, times(1)).classForName( eq("javax.persistence.FetchType") );
+	}
+
+	@Test
+	public void test_getConstantValue_enumClass() {
+		when( sessionFactoryOptionsMock.isConventionalJavaConstants() ).thenReturn( true );
+
+		when( classLoaderServiceMock.classForName( "org.hibernate.internal.util.ReflectHelperTest$Status" ) ).thenReturn( (Class) Status.class );
+		Object value = ReflectHelper.getConstantValue( "org.hibernate.internal.util.ReflectHelperTest$Status", sessionFactoryImplementorMock);
+		assertNull(value);
+		verify(classLoaderServiceMock, never()).classForName( eq("org.hibernate.internal.util") );
+	}
+
+	@Test
+	public void test_getConstantValue_nestedEnum() {
+
+		when( sessionFactoryOptionsMock.isConventionalJavaConstants() ).thenReturn( true );
+		when( classLoaderServiceMock.classForName( "org.hibernate.internal.util.ReflectHelperTest$Status" ) ).thenReturn( (Class) Status.class );
+		Object value = ReflectHelper.getConstantValue( "org.hibernate.internal.util.ReflectHelperTest$Status.ON", sessionFactoryImplementorMock);
+		assertEquals( Status.ON, value );
+		verify(classLoaderServiceMock, times(1)).classForName( eq("org.hibernate.internal.util.ReflectHelperTest$Status") );
+	}
+}