HHH-19678, HHH-19676 validation of @OrderBy

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: '#';

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

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

hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/PluralAttributeMappingImpl.java

@@ -65,7 +65,6 @@
 import org.hibernate.sql.results.graph.collection.internal.DelayedCollectionFetch;
 import org.hibernate.sql.results.graph.collection.internal.EagerCollectionFetch;
 import org.hibernate.sql.results.graph.collection.internal.SelectEagerCollectionFetch;
-import org.jboss.logging.Logger;
 
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
@@ -80,7 +79,6 @@
 public class PluralAttributeMappingImpl
 		extends AbstractAttributeMapping
 		implements PluralAttributeMapping, FetchProfileAffectee, FetchOptions {
-	private static final Logger log = Logger.getLogger( PluralAttributeMappingImpl.class );
 
 	/**
 	 * Allows callback after creation of the attribute mapping.
@@ -96,8 +94,7 @@
 		void injectAttributeMapping(PluralAttributeMapping attributeMapping);
 	}
 
-	@SuppressWarnings("rawtypes")
-	private final CollectionMappingType collectionMappingType;
+	private final CollectionMappingType<?> collectionMappingType;
 	private final String referencedPropertyName;
 	private final String mapKeyPropertyName;
 
@@ -237,11 +234,12 @@
 		return fetchablePath.getLocalName().endsWith( bidirectionalAttributeName );
 	}
 
-	@SuppressWarnings("unused")
 	public void finishInitialization(
+			@SuppressWarnings("unused")
 			Property bootProperty,
 			Collection bootDescriptor,
+			@SuppressWarnings("unused")
 			MappingModelCreationProcess creationProcess) {
 		final boolean hasOrder = bootDescriptor.getOrderBy() != null;
 		final boolean hasManyToManyOrder = bootDescriptor.getManyToManyOrdering() != null;
 
@@ -249,13 +247,6 @@
 			final TranslationContext context = collectionDescriptor::getFactory;
 
 			if ( hasOrder ) {
-				if ( log.isTraceEnabled() ) {
-					log.tracef(
-							"Translating order-by fragment [%s] for collection role: %s",
-							bootDescriptor.getOrderBy(),
-							collectionDescriptor.getRole()
-					);
-				}
 				orderByFragment = OrderByFragmentTranslator.translate(
 						bootDescriptor.getOrderBy(),
 						this,
@@ -264,13 +255,6 @@
 			}
 
 			if ( hasManyToManyOrder ) {
-				if ( log.isTraceEnabled() ) {
-					log.tracef(
-							"Translating many-to-many order-by fragment [%s] for collection role: %s",
-							bootDescriptor.getOrderBy(),
-							collectionDescriptor.getRole()
-					);
-				}
 				manyToManyOrderByFragment = OrderByFragmentTranslator.translate(
 						bootDescriptor.getManyToManyOrdering(),
 						this,
@@ -286,8 +270,7 @@
 	}
 
 	@Override
-	@SuppressWarnings("rawtypes")
-	public CollectionMappingType getMappedType() {
+	public CollectionMappingType<?> getMappedType() {
 		return collectionMappingType;
 	}
 
@@ -384,32 +367,34 @@
 
 	@Override
 	public void applySoftDeleteRestrictions(TableGroup tableGroup, PredicateConsumer predicateConsumer) {
-		if ( !hasSoftDelete() ) {
-			// short-circuit
-			return;
-		}
+		if ( hasSoftDelete() ) {
+			final var descriptor = getCollectionDescriptor();
+			if ( descriptor.isOneToMany() || descriptor.isManyToMany() ) {
+				// see if the associated entity has soft-delete defined
+				final var elementDescriptor = (EntityCollectionPart) getElementDescriptor();
+				final var associatedEntityDescriptor = elementDescriptor.getAssociatedEntityMappingType();
+				final var softDeleteMapping = associatedEntityDescriptor.getSoftDeleteMapping();
+				if ( softDeleteMapping != null ) {
+					final String primaryTableName =
+							associatedEntityDescriptor.getSoftDeleteTableDetails().getTableName();
+					final TableReference primaryTableReference =
+							tableGroup.resolveTableReference( primaryTableName );
+					final Predicate softDeleteRestriction =
+							softDeleteMapping.createNonDeletedRestriction( primaryTableReference );
+					predicateConsumer.applyPredicate( softDeleteRestriction );
+				}
+			}
 
-		if ( getCollectionDescriptor().isOneToMany()
-				|| getCollectionDescriptor().isManyToMany() ) {
-			// see if the associated entity has soft-delete defined
-			final EntityCollectionPart elementDescriptor = (EntityCollectionPart) getElementDescriptor();
-			final EntityMappingType associatedEntityDescriptor = elementDescriptor.getAssociatedEntityMappingType();
-			final SoftDeleteMapping softDeleteMapping = associatedEntityDescriptor.getSoftDeleteMapping();
+			// apply the collection's soft-delete mapping, if one
+			final var softDeleteMapping = getSoftDeleteMapping();
 			if ( softDeleteMapping != null ) {
-				final String primaryTableName = associatedEntityDescriptor.getSoftDeleteTableDetails().getTableName();
-				final TableReference primaryTableReference = tableGroup.resolveTableReference( primaryTableName );
-				final Predicate softDeleteRestriction = softDeleteMapping.createNonDeletedRestriction( primaryTableReference );
+				final TableReference primaryTableReference =
+						tableGroup.resolveTableReference( getSoftDeleteTableDetails().getTableName() );
+				final Predicate softDeleteRestriction =
+						softDeleteMapping.createNonDeletedRestriction( primaryTableReference );
 				predicateConsumer.applyPredicate( softDeleteRestriction );
 			}
 		}
-
-		// apply the collection's soft-delete mapping, if one
-		final SoftDeleteMapping softDeleteMapping = getSoftDeleteMapping();
-		if ( softDeleteMapping != null ) {
-			final TableReference primaryTableReference = tableGroup.resolveTableReference( getSoftDeleteTableDetails().getTableName() );
-			final Predicate softDeleteRestriction = softDeleteMapping.createNonDeletedRestriction( primaryTableReference );
-			predicateConsumer.applyPredicate( softDeleteRestriction );
-		}
 	}
 
 	@Override
@@ -418,9 +403,9 @@
 			TableGroup tableGroup,
 			String resultVariable,
 			DomainResultCreationState creationState) {
-		final TableGroup collectionTableGroup = creationState.getSqlAstCreationState()
-				.getFromClauseAccess()
-				.getTableGroup( navigablePath );
+		final TableGroup collectionTableGroup =
+				creationState.getSqlAstCreationState().getFromClauseAccess()
+						.getTableGroup( navigablePath );
 
 		assert collectionTableGroup != null;
 
@@ -440,7 +425,7 @@
 			boolean selected,
 			String resultVariable,
 			DomainResultCreationState creationState) {
-		final SqlAstCreationState sqlAstCreationState = creationState.getSqlAstCreationState();
+		final var sqlAstCreationState = creationState.getSqlAstCreationState();
 
 		final boolean added = creationState.registerVisitedAssociationKey( fkDescriptor.getAssociationKey() );
 

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,

hibernate-core/src/main/java/org/hibernate/metamodel/mapping/ordering/OrderByFragmentTranslator.java

@@ -4,20 +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.jboss.logging.Logger;
 
 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
@@ -28,7 +35,6 @@
  * @see jakarta.persistence.OrderBy
  */
 public class OrderByFragmentTranslator {
-	private static final Logger LOG = Logger.getLogger( OrderByFragmentTranslator.class.getName() );
 
 	/**
 	 * Perform the translation of the user-supplied fragment, returning the translation.
@@ -43,28 +49,22 @@
 			String fragment,
 			PluralAttributeMapping pluralAttributeMapping,
 			TranslationContext context) {
-		if ( LOG.isTraceEnabled() ) {
-			LOG.tracef(
-					"Beginning parsing of order-by fragment [%s] : %s",
-					pluralAttributeMapping.getCollectionDescriptor().getRole(),
-					fragment
-			);
-		}
-
-		final OrderingParser.OrderByFragmentContext parseTree = buildParseTree( context, fragment );
-
-		final ParseTreeVisitor visitor = new ParseTreeVisitor( pluralAttributeMapping, context );
-
+		final var parseTree = buildParseTree( fragment );
+		final var visitor = new ParseTreeVisitor( pluralAttributeMapping, context );
 		return new OrderByFragmentImpl( visitor.visitOrderByFragment( parseTree ) );
 	}
 
+	public static void check(String fragment) {
+		final var parseTree = buildParseTree( fragment );
+		// TODO: check against the model (requires the PluralAttributeMapping)
+	}
 
-	private static OrderingParser.OrderByFragmentContext buildParseTree(TranslationContext context, String fragment) {
-		final OrderingLexer lexer = new OrderingLexer( CharStreams.fromString( fragment ) );
+	private static OrderingParser.OrderByFragmentContext buildParseTree(String fragment) {
+		final var lexer = new OrderingLexer( CharStreams.fromString( fragment ) );
 
-		final OrderingParser parser = new OrderingParser( new BufferedTokenStream( lexer ) );
+		final var parser = new OrderingParser( new CommonTokenStream( lexer ) );
 
-		// try to use SLL(k)-based parsing first - its faster
+		// try to use SLL(k)-based parsing first - it's faster
 		parser.getInterpreter().setPredictionMode( PredictionMode.SLL );
 		parser.removeErrorListeners();
 		parser.setErrorHandler( new BailErrorStrategy() );
@@ -79,11 +79,23 @@
 
 			// 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 );
+		}
 	}
 
 }

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 );
+		}
 	}
 }

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 );
 	}
 }