HHH-19678 fix error reporting for JPA @orderby annotation

gavinking · gavinking · commit e9455d377afe · 2025-08-03T16:23:07.000+10:00
diff --git a/hibernate-core/src/main/antlr/org/hibernate/grammars/ordering/OrderingLexer.g4 b/hibernate-core/src/main/antlr/org/hibernate/grammars/ordering/OrderingLexer.g4
@@ -137,8 +137,31 @@ COMMA		: ',';
 DOT			: '.';
 
 PLUS 		: '+';
-MINUS 		:	'-';
+MINUS 		: '-';
 MULTIPLY 	: '*';
 DIVIDE 		: '/';
 MODULO		: '%';
 
+// Not used, but necessary for error reporting
+
+EQUAL : '=';
+NOT_EQUAL : '!=' | '^=' | '<>';
+GREATER : '>';
+GREATER_EQUAL : '>=';
+LESS : '<';
+LESS_EQUAL : '<=';
+
+LEFT_BRACKET : '[';
+RIGHT_BRACKET : ']';
+LEFT_BRACE : '{';
+RIGHT_BRACE : '}';
+AMPERSAND : '&';
+SEMICOLON :	';';
+COLON : ':';
+PIPE : '|';
+DOUBLE_PIPE : '||';
+QUESTION_MARK :	'?';
+ARROW :	'->';
+BANG: '!';
+AT: '@';
+HASH: '#';
diff --git a/hibernate-core/src/main/antlr/org/hibernate/grammars/ordering/OrderingParser.g4 b/hibernate-core/src/main/antlr/org/hibernate/grammars/ordering/OrderingParser.g4
@@ -23,7 +23,7 @@ package org.hibernate.grammars.ordering;
 // todo (6.0) : add hooks for keyword-as-identifier logging like we do for HQL?
 
 orderByFragment
-	: sortSpecification (COMMA sortSpecification)*
+	: sortSpecification (COMMA sortSpecification)* EOF
 	;
 
 sortSpecification
diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ManyToManyCollectionPart.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/ManyToManyCollectionPart.java
@@ -123,16 +123,16 @@ public ModelPart findSubPart(String name, EntityMappingType targetType) {
 		// to allow deferring the initialization of the target table group, omitting it if possible.
 		// This is not possible for one-to-many associations because we need to create the target table group eagerly,
 		// to preserve the cardinality. Also, the OneToManyTableGroup has no reference to the parent table group
-		if ( getTargetKeyPropertyNames().contains( name ) ) {
+		if ( foreignKey != null && getTargetKeyPropertyNames().contains( name ) ) {
 			final ModelPart keyPart = foreignKey.getKeyPart();
-			if ( keyPart instanceof EmbeddableValuedModelPart embeddableValuedModelPart
-					&& keyPart instanceof VirtualModelPart ) {
-				return embeddableValuedModelPart.findSubPart( name, targetType );
-			}
-			return keyPart;
+			return keyPart instanceof EmbeddableValuedModelPart embeddableValuedModelPart
+				&& keyPart instanceof VirtualModelPart
+					? embeddableValuedModelPart.findSubPart( name, targetType )
+					: keyPart;
+		}
+		else {
+			return super.findSubPart( name, targetType );
 		}
-
-		return super.findSubPart( name, targetType );
 	}
 
 	@Override
diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/OrderByFragmentImpl.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/OrderByFragmentImpl.java
@@ -28,8 +28,7 @@ public List<OrderingSpecification> getFragmentSpecs() {
 	@Override
 	public void apply(QuerySpec ast, TableGroup tableGroup, SqlAstCreationState creationState) {
 		for ( int i = 0; i < fragmentSpecs.size(); i++ ) {
-			final OrderingSpecification orderingSpec = fragmentSpecs.get( i );
-
+			final var orderingSpec = fragmentSpecs.get( i );
 			orderingSpec.getExpression().apply(
 					ast,
 					tableGroup,
diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/OrderByFragmentTranslator.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/OrderByFragmentTranslator.java
@@ -4,19 +4,27 @@
  */
 package org.hibernate.metamodel.mapping.ordering;
 
+import org.antlr.v4.runtime.ANTLRErrorListener;
+import org.antlr.v4.runtime.BaseErrorListener;
+import org.antlr.v4.runtime.CommonTokenStream;
+import org.antlr.v4.runtime.RecognitionException;
+import org.antlr.v4.runtime.Recognizer;
+import org.hibernate.QueryException;
 import org.hibernate.grammars.ordering.OrderingLexer;
 import org.hibernate.grammars.ordering.OrderingParser;
 import org.hibernate.metamodel.mapping.PluralAttributeMapping;
 import org.hibernate.metamodel.mapping.ordering.ast.ParseTreeVisitor;
 
 
 import org.antlr.v4.runtime.BailErrorStrategy;
-import org.antlr.v4.runtime.BufferedTokenStream;
 import org.antlr.v4.runtime.CharStreams;
-import org.antlr.v4.runtime.ConsoleErrorListener;
 import org.antlr.v4.runtime.DefaultErrorStrategy;
 import org.antlr.v4.runtime.atn.PredictionMode;
 import org.antlr.v4.runtime.misc.ParseCancellationException;
+import org.hibernate.query.SyntaxException;
+import org.hibernate.query.sqm.ParsingException;
+
+import static org.hibernate.query.hql.internal.StandardHqlTranslator.prettifyAntlrError;
 
 /**
  * Responsible for performing the translation of the order-by fragment associated
@@ -46,11 +54,15 @@ public static OrderByFragment translate(
 		return new OrderByFragmentImpl( visitor.visitOrderByFragment( parseTree ) );
 	}
 
+	public static void check(String fragment) {
+		final var parseTree = buildParseTree( fragment );
+		// TODO: check against the model
+	}
 
 	private static OrderingParser.OrderByFragmentContext buildParseTree(String fragment) {
 		final var lexer = new OrderingLexer( CharStreams.fromString( fragment ) );
 
-		final var parser = new OrderingParser( new BufferedTokenStream( lexer ) );
+		final var parser = new OrderingParser( new CommonTokenStream( lexer ) );
 
 		// try to use SLL(k)-based parsing first - it's faster
 		parser.getInterpreter().setPredictionMode( PredictionMode.SLL );
@@ -67,11 +79,23 @@ private static OrderingParser.OrderByFragmentContext buildParseTree(String fragm
 
 			// fall back to LL(k)-based parsing
 			parser.getInterpreter().setPredictionMode( PredictionMode.LL );
-			parser.addErrorListener( ConsoleErrorListener.INSTANCE );
+//			parser.addErrorListener( ConsoleErrorListener.INSTANCE );
 			parser.setErrorHandler( new DefaultErrorStrategy() );
 
+			final ANTLRErrorListener errorListener = new BaseErrorListener() {
+				@Override
+				public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol, int line, int charPositionInLine, String msg, RecognitionException e) {
+					throw new SyntaxException( prettifyAntlrError( offendingSymbol, line, charPositionInLine, msg, e, fragment, true ), fragment );
+				}
+			};
+			parser.addErrorListener( errorListener );
+
 			return parser.orderByFragment();
 		}
+		catch ( ParsingException ex ) {
+			// Note that this is supposed to represent a bug in the parser
+			throw new QueryException( "Failed to interpret syntax [" + ex.getMessage() + "]", fragment, ex );
+		}
 	}
 
 }
diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/CollectionPartPath.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/CollectionPartPath.java
@@ -57,14 +57,12 @@ public SequencePart resolvePathPart(
 			String identifier,
 			boolean isTerminal,
 			TranslationContext translationContext) {
-		if ( referencedPart instanceof ModelPartContainer ) {
-			final ModelPart subPart = ( (ModelPartContainer) referencedPart ).findSubPart( name, null );
-
+		if ( referencedPart instanceof ModelPartContainer modelPartContainer ) {
+			final ModelPart subPart = modelPartContainer.findSubPart( name, null );
 			return new DomainPathContinuation( navigablePath.append( name ), this, subPart );
 		}
-
-		throw new PathResolutionException(
-				"Could not resolve order-by path : " + navigablePath + " -> " + name
-		);
+		else {
+			throw new PathResolutionException( name );
+		}
 	}
 }
diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/DomainPathContinuation.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/DomainPathContinuation.java
@@ -5,7 +5,6 @@
 package org.hibernate.metamodel.mapping.ordering.ast;
 
 import org.hibernate.metamodel.mapping.EmbeddableValuedModelPart;
-import org.hibernate.metamodel.mapping.EmbeddableMappingType;
 import org.hibernate.metamodel.mapping.ModelPart;
 import org.hibernate.metamodel.mapping.internal.AbstractDomainPath;
 import org.hibernate.metamodel.mapping.ordering.TranslationContext;
@@ -49,13 +48,11 @@ public SequencePart resolvePathPart(
 			String identifier,
 			boolean isTerminal,
 			TranslationContext translationContext) {
-		if ( referencedModelPart instanceof EmbeddableValuedModelPart ) {
-			final EmbeddableMappingType embeddableMappingType = (EmbeddableMappingType) referencedModelPart.getPartMappingType();
+		if ( referencedModelPart instanceof EmbeddableValuedModelPart embeddableValuedModelPart ) {
+			final var embeddableMappingType = embeddableValuedModelPart.getEmbeddableTypeDescriptor();
 			final ModelPart subPart = embeddableMappingType.findSubPart( name, null );
 			if ( subPart == null ) {
-				throw new PathResolutionException(
-						"Could not resolve path token : " + referencedModelPart + " -> " + name
-				);
+				throw new PathResolutionException( name );
 			}
 
 			return new DomainPathContinuation(
@@ -65,9 +62,6 @@ public SequencePart resolvePathPart(
 			);
 		}
 
-		throw new PathResolutionException(
-				"Domain path of type `" + referencedModelPart.getPartMappingType() +
-						"` -> `" + name + "`"
-		);
+		throw new PathResolutionException( name );
 	}
 }
diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/FkDomainPathContinuation.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/FkDomainPathContinuation.java
@@ -41,7 +41,7 @@ public SequencePart resolvePathPart(
 			boolean isTerminal,
 			TranslationContext translationContext) {
 		if ( !possiblePaths.contains( name ) ) {
-			throw new PathResolutionException( "Domain path of type `" + referencedModelPart.getPartMappingType() + "` -> `" + name + "`" );
+			throw new PathResolutionException( name );
 		}
 
 		final HashSet<String> furtherPaths = new HashSet<>();
diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/PathConsumer.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/PathConsumer.java
@@ -6,9 +6,7 @@
 
 import org.hibernate.metamodel.mapping.PluralAttributeMapping;
 import org.hibernate.metamodel.mapping.ordering.TranslationContext;
-import org.hibernate.query.hql.internal.BasicDotIdentifierConsumer;
 
-import org.jboss.logging.Logger;
 
 /**
  * Represents the translation of an individual part of a path in `@OrderBy` translation
@@ -52,7 +50,12 @@ public void consumeIdentifier(
 		}
 		pathSoFar.append( unquotedIdentifier );
 
-		currentPart = currentPart.resolvePathPart( unquotedIdentifier, identifier, isTerminal, translationContext );
+		try {
+			currentPart = currentPart.resolvePathPart( unquotedIdentifier, identifier, isTerminal, translationContext );
+		}
+		catch (PathResolutionException pre) {
+			throw new PathResolutionException( "Unable to resolve path '" + pathSoFar + "'" );
+		}
 	}
 
 	private void reset() {
diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/PathResolutionException.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/PathResolutionException.java
@@ -5,13 +5,14 @@
 package org.hibernate.metamodel.mapping.ordering.ast;
 
 import org.hibernate.HibernateException;
+import org.hibernate.metamodel.mapping.NonTransientException;
 
 /**
  * Indicates a problem resolving a domain-path occurring in an order-by fragment
  *
  * @author Steve Ebersole
  */
-public class PathResolutionException extends HibernateException {
+public class PathResolutionException extends HibernateException implements NonTransientException {
 	public PathResolutionException(String message) {
 		super( message );
 	}
diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/PluralAttributePath.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/PluralAttributePath.java
@@ -44,35 +44,27 @@ public PluralAttributeMapping getReferenceModelPart() {
 	}
 
 	@Override
-	public DomainPath resolvePathPart(
+	public SequencePart resolvePathPart(
 			String name,
 			String identifier,
 			boolean isTerminal,
 			TranslationContext translationContext) {
 		final ModelPart subPart = pluralAttributeMapping.findSubPart( name, null );
-
 		if ( subPart != null ) {
-			if ( subPart instanceof CollectionPart ) {
-				return new CollectionPartPath( this, (CollectionPart) subPart );
+			if ( subPart instanceof CollectionPart collectionPart ) {
+				return new CollectionPartPath( this, collectionPart );
 			}
-			if ( subPart instanceof EmbeddableValuedModelPart ) {
+			else if ( subPart instanceof EmbeddableValuedModelPart ) {
 				return new DomainPathContinuation( navigablePath.append( name ), this, subPart );
 			}
-			if ( subPart instanceof ToOneAttributeMapping ) {
-				return new FkDomainPathContinuation(
-						navigablePath.append( name ),
-						this,
-						(ToOneAttributeMapping) subPart
-				);
+			else if ( subPart instanceof ToOneAttributeMapping toOneMapping ) {
+				return new FkDomainPathContinuation( navigablePath.append( name ), this, toOneMapping );
+			}
+			else {
+				// leaf case:
+				return new CollectionPartPath( this, pluralAttributeMapping.getElementDescriptor() )
+						.resolvePathPart( name, identifier, isTerminal, translationContext );
 			}
-
-			// leaf case:
-			final CollectionPartPath elementPath = new CollectionPartPath(
-					this,
-					pluralAttributeMapping.getElementDescriptor()
-			);
-
-			return (DomainPath) elementPath.resolvePathPart( name, identifier, isTerminal, translationContext);
 		}
 
 		// the above checks for explicit element or index descriptor references
@@ -81,14 +73,10 @@ public DomainPath resolvePathPart(
 		if ( pluralAttributeMapping.getElementDescriptor() instanceof EmbeddableValuedModelPart elementDescriptor ) {
 			final ModelPart elementSubPart = elementDescriptor.findSubPart( name, null );
 			if ( elementSubPart != null ) {
-				// create the CollectionSubPath to use as the `lhs` for the element sub-path
-				final CollectionPartPath elementPath = new CollectionPartPath(
-						this,
-						(CollectionPart) elementDescriptor
-				);
-
 				return new DomainPathContinuation(
-						elementPath.getNavigablePath().append( name ),
+						// create the CollectionSubPath to use as the `lhs` for the element sub-path
+						new CollectionPartPath( this, (CollectionPart) elementDescriptor )
+								.getNavigablePath().append( name ),
 						this,
 						elementSubPart
 				);
@@ -98,13 +86,10 @@ public DomainPath resolvePathPart(
 		if ( pluralAttributeMapping.getIndexDescriptor() instanceof EmbeddableValuedModelPart indexDescriptor ) {
 			final ModelPart indexSubPart = indexDescriptor.findSubPart( name, null );
 			if ( indexSubPart != null ) {
-				// create the CollectionSubPath to use as the `lhs` for the element sub-path
-				final CollectionPartPath indexPath = new CollectionPartPath(
-						this,
-						(CollectionPart) indexDescriptor
-				);
 				return new DomainPathContinuation(
-						indexPath.getNavigablePath().append( name ),
+						// create the CollectionSubPath to use as the `lhs` for the element sub-path
+						new CollectionPartPath( this, (CollectionPart) indexDescriptor )
+								.getNavigablePath().append( name ),
 						this,
 						indexSubPart
 				);
diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/RootSequencePart.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/ast/RootSequencePart.java
@@ -29,7 +29,7 @@ public SequencePart resolvePathPart(
 			TranslationContext translationContext) {
 		// could be a column-reference (isTerminal would have to be true) or a domain-path
 
-		final DomainPath subDomainPath = pluralAttributePath.resolvePathPart(
+		final SequencePart subDomainPath = pluralAttributePath.resolvePathPart(
 				name,
 				identifier,
 				isTerminal,
@@ -50,9 +50,6 @@ public SequencePart resolvePathPart(
 			);
 		}
 
-		throw new PathResolutionException(
-				"Could not resolve order-by path : " +
-						pluralAttributePath.getReferenceModelPart().getCollectionDescriptor().getRole() + " -> " + name
-		);
+		throw new PathResolutionException( name );
 	}
 }
diff --git a/hibernate-core/src/main/java/org/hibernate/query/hql/internal/StandardHqlTranslator.java b/hibernate-core/src/main/java/org/hibernate/query/hql/internal/StandardHqlTranslator.java
@@ -96,7 +96,7 @@ private HqlParser.StatementContext parseHql(String hql) {
 		// Build the parse tree
 		final HqlParser hqlParser = HqlParseTreeBuilder.INSTANCE.buildHqlParser( hql, hqlLexer );
 
-		// try to use SLL(k)-based parsing first - its faster
+		// try to use SLL(k)-based parsing first - it's faster
 		hqlParser.getInterpreter().setPredictionMode( PredictionMode.SLL );
 		hqlParser.removeErrorListeners();
 		hqlParser.setErrorHandler( new BailErrorStrategy() );
@@ -125,7 +125,7 @@ public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol, int
 		}
 		catch ( ParsingException ex ) {
 			// Note that this is supposed to represent a bug in the parser
-			// Ee wrap and rethrow in order to attach the HQL query to the error
+			// We wrap and rethrow in order to attach the HQL query to the error
 			throw new QueryException( "Failed to interpret HQL syntax [" + ex.getMessage() + "]", hql, ex );
 		}
 	}

Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,7 @@ package org.hibernate.grammars.ordering;`
`23`	`23`	`// todo (6.0) : add hooks for keyword-as-identifier logging like we do for HQL?`
`24`	`24`
`25`	`25`	`orderByFragment`
`26`		`- : sortSpecification (COMMA sortSpecification)*`
	`26`	`+ : sortSpecification (COMMA sortSpecification)* EOF`
`27`	`27`	`;`
`28`	`28`
`29`	`29`	`sortSpecification`
Original file line number	Diff line number	Diff line change
`@@ -41,7 +41,7 @@ public SequencePart resolvePathPart(`
`41`	`41`	`boolean isTerminal,`
`42`	`42`	`TranslationContext translationContext) {`
`43`	`43`	`if ( !possiblePaths.contains( name ) ) {`
`44`		- throw new PathResolutionException( "Domain path of type `" + referencedModelPart.getPartMappingType() + "` -> `" + name + "`" );
	`44`	`+ throw new PathResolutionException( name );`
`45`	`45`	`}`
`46`	`46`
`47`	`47`	`final HashSet<String> furtherPaths = new HashSet<>();`
Original file line number	Diff line number	Diff line change
`@@ -5,13 +5,14 @@`
`5`	`5`	`package org.hibernate.metamodel.mapping.ordering.ast;`
`6`	`6`
`7`	`7`	`import org.hibernate.HibernateException;`
	`8`	`+import org.hibernate.metamodel.mapping.NonTransientException;`
`8`	`9`
`9`	`10`	`/**`
`10`	`11`	`* Indicates a problem resolving a domain-path occurring in an order-by fragment`
`11`	`12`	`*`
`12`	`13`	`* @author Steve Ebersole`
`13`	`14`	`*/`
`14`		`-public class PathResolutionException extends HibernateException {`
	`15`	`+public class PathResolutionException extends HibernateException implements NonTransientException {`
`15`	`16`	`public PathResolutionException(String message) {`
`16`	`17`	`super( message );`
`17`	`18`	`}`