From 5881ad0b15023593d0e05875baee9bf5a2e16f2e Mon Sep 17 00:00:00 2001 From: Gavin King Date: Sat, 2 Aug 2025 21:39:27 +1000 Subject: [PATCH 1/4] remove unnecessary logging and add more 'var' usage --- .../internal/PluralAttributeMappingImpl.java | 77 ++++++++----------- .../ordering/OrderByFragmentTranslator.java | 24 ++---- .../mapping/ordering/ast/PathConsumer.java | 11 +-- 3 files changed, 38 insertions(+), 74 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/PluralAttributeMappingImpl.java b/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/PluralAttributeMappingImpl.java index 82946aa30054..dbb0c052da59 100644 --- a/hibernate-core/src/main/java/org/hibernate/metamodel/mapping/internal/PluralAttributeMappingImpl.java +++ b/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 @@ public interface Aware { void injectAttributeMapping(PluralAttributeMapping attributeMapping); } - @SuppressWarnings("rawtypes") - private final CollectionMappingType collectionMappingType; + private final CollectionMappingType collectionMappingType; private final String referencedPropertyName; private final String mapKeyPropertyName; @@ -237,10 +234,11 @@ public boolean isBidirectionalAttributeName(NavigablePath fetchablePath, ToOneAt 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 @@ public void finishInitialization( 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 @@ public void finishInitialization( } 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 @@ public NavigableRole getNavigableRole() { } @Override - @SuppressWarnings("rawtypes") - public CollectionMappingType getMappedType() { + public CollectionMappingType getMappedType() { return collectionMappingType; } @@ -384,32 +367,34 @@ public boolean hasPartitionedSelectionMapping() { @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 @@ public DomainResult createDomainResult( 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 @@ public Fetch generateFetch( boolean selected, String resultVariable, DomainResultCreationState creationState) { - final SqlAstCreationState sqlAstCreationState = creationState.getSqlAstCreationState(); + final var sqlAstCreationState = creationState.getSqlAstCreationState(); final boolean added = creationState.registerVisitedAssociationKey( fkDescriptor.getAssociationKey() ); 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 index aae2f64dc3af..50b786778b80 100644 --- 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 @@ -9,7 +9,6 @@ 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; @@ -28,7 +27,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 +41,18 @@ public static OrderByFragment translate( 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 ) ); } - 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 BufferedTokenStream( 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() ); 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 index e8dac1055155..ebb46a2f895d 100644 --- 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 @@ -18,7 +18,6 @@ * @author Steve Ebersole */ public class PathConsumer { - private static final Logger log = Logger.getLogger( BasicDotIdentifierConsumer.class ); private final TranslationContext translationContext; @@ -48,19 +47,11 @@ public void consumeIdentifier( reset(); } - if ( pathSoFar.length() != 0 ) { + if ( !pathSoFar.isEmpty() ) { pathSoFar.append( '.' ); } pathSoFar.append( unquotedIdentifier ); -// log.tracef( -// "BasicDotIdentifierHandler#consumeIdentifier( %s, %s, %s ) - %s", -// unquotedIdentifier, -// isBase, -// isTerminal, -// pathSoFar -// ); - currentPart = currentPart.resolvePathPart( unquotedIdentifier, identifier, isTerminal, translationContext ); } From 987a8c9ba579cdabc07d2b4d7e4780c45ef39fd4 Mon Sep 17 00:00:00 2001 From: Gavin King Date: Sat, 2 Aug 2025 23:43:25 +1000 Subject: [PATCH 2/4] HHH-19678 fix error reporting for JPA @OrderBy annotation --- .../grammars/ordering/OrderingLexer.g4 | 25 +++++++++- .../grammars/ordering/OrderingParser.g4 | 2 +- .../internal/ManyToManyCollectionPart.java | 16 +++--- .../mapping/ordering/OrderByFragmentImpl.java | 3 +- .../ordering/OrderByFragmentTranslator.java | 32 ++++++++++-- .../ordering/ast/CollectionPartPath.java | 12 ++--- .../ordering/ast/DomainPathContinuation.java | 14 ++---- .../ast/FkDomainPathContinuation.java | 2 +- .../mapping/ordering/ast/PathConsumer.java | 9 ++-- .../ordering/ast/PathResolutionException.java | 3 +- .../ordering/ast/PluralAttributePath.java | 49 +++++++------------ .../ordering/ast/RootSequencePart.java | 7 +-- .../hql/internal/StandardHqlTranslator.java | 4 +- 13 files changed, 101 insertions(+), 77 deletions(-) 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 index e25975f6d6de..d8e006cbf45e 100644 --- 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: '#'; \ No newline at end of file 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 index 3e57b1cf7dc9..239a1a8d5c89 100644 --- 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 index 882bf4812a69..a0403f518e67 100644 --- 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 index f5d46e584f76..cd51263a11a1 100644 --- 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 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 index 50b786778b80..dbd59a47c920 100644 --- 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,6 +4,12 @@ */ 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; @@ -11,12 +17,14 @@ 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 index 03063d25be60..c6b934f126c8 100644 --- 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 index 73a5d077dcba..1a600ad2abaa 100644 --- 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 index df4ae2efa4e0..1506ee6d34d2 100644 --- 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 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 index ebb46a2f895d..287165ecbc1c 100644 --- 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 index 34502d8bebed..af8fc5010e1e 100644 --- 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 index 365f8973ca5f..87697ed362a1 100644 --- 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 index ae82005543c1..71017b8a4f4a 100644 --- 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 index 150e4619c9dd..a40c1589fdc1 100644 --- 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 ); } } From 1d0d8dda4202c4fa096b00f3db83f6bdbbdc9cb6 Mon Sep 17 00:00:00 2001 From: Gavin King Date: Sat, 2 Aug 2025 23:46:52 +1000 Subject: [PATCH 3/4] HHH-19676 validate syntax in JPA @OrderBy in Hibernate Processor (still need to do more semantic validation) --- .../annotation/AnnotationMetaEntity.java | 26 +++++++++++++++++++ .../hibernate/processor/util/Constants.java | 1 + 2 files changed, 27 insertions(+) diff --git a/tooling/metamodel-generator/src/main/java/org/hibernate/processor/annotation/AnnotationMetaEntity.java b/tooling/metamodel-generator/src/main/java/org/hibernate/processor/annotation/AnnotationMetaEntity.java index d10b371b2a28..9bf0ce3c3b63 100644 --- a/tooling/metamodel-generator/src/main/java/org/hibernate/processor/annotation/AnnotationMetaEntity.java +++ b/tooling/metamodel-generator/src/main/java/org/hibernate/processor/annotation/AnnotationMetaEntity.java @@ -8,6 +8,7 @@ import org.checkerframework.checker.nullness.qual.Nullable; import org.hibernate.AssertionFailure; import org.hibernate.grammars.hql.HqlLexer; +import org.hibernate.metamodel.mapping.ordering.OrderByFragmentTranslator; import org.hibernate.metamodel.model.domain.EntityDomainType; import org.hibernate.processor.Context; import org.hibernate.processor.ImportContextImpl; @@ -20,6 +21,7 @@ import org.hibernate.processor.util.TypeUtils; import org.hibernate.processor.validation.ProcessorSessionFactory; import org.hibernate.processor.validation.Validation; +import org.hibernate.query.SyntaxException; import org.hibernate.query.criteria.JpaEntityJoin; import org.hibernate.query.criteria.JpaRoot; import org.hibernate.query.criteria.JpaSelection; @@ -1232,6 +1234,30 @@ private void validatePersistentMembers(List membersOfClass) { if ( hasAnnotation(memberOfClass, MANY_TO_ONE, ONE_TO_ONE, ONE_TO_MANY, MANY_TO_MANY) ) { validateAssociation(memberOfClass); } + if ( hasAnnotation(memberOfClass, ORDER_BY) ) { + validateOrderBy( memberOfClass ); + } + } + } + + private void validateOrderBy(Element memberOfClass) { + final AnnotationMirror annotation = + castNonNull(getAnnotationMirror( memberOfClass, ORDER_BY)); + final AnnotationValue annotationValue = getAnnotationValue(annotation); + if ( annotationValue != null ) { + final String fragment = annotationValue.getValue().toString(); + if ( !fragment.isBlank() ) { + try { + OrderByFragmentTranslator.check( fragment ); + } + catch (SyntaxException e) { + final String message = "Error in ordering: " + e.getMessage(); + context.message( memberOfClass, annotation, annotationValue, message, Diagnostic.Kind.ERROR ); + } + catch (Exception ignored) { + // do nothing with it + } + } } } diff --git a/tooling/metamodel-generator/src/main/java/org/hibernate/processor/util/Constants.java b/tooling/metamodel-generator/src/main/java/org/hibernate/processor/util/Constants.java index 9967ab57a225..59e77cf9aed2 100644 --- a/tooling/metamodel-generator/src/main/java/org/hibernate/processor/util/Constants.java +++ b/tooling/metamodel-generator/src/main/java/org/hibernate/processor/util/Constants.java @@ -37,6 +37,7 @@ public final class Constants { public static final String ACCESS = "jakarta.persistence.Access"; public static final String CONVERT = "jakarta.persistence.Convert"; public static final String GENERATED_VALUE = "jakarta.persistence.GeneratedValue"; + public static final String ORDER_BY = "jakarta.persistence.OrderBy"; public static final String NAMED_QUERY = "jakarta.persistence.NamedQuery"; public static final String NAMED_QUERIES = "jakarta.persistence.NamedQueries"; From 9c03be0c83aec3b0bb340d986f2d2088955455d6 Mon Sep 17 00:00:00 2001 From: Gavin King Date: Sun, 3 Aug 2025 10:24:23 +1000 Subject: [PATCH 4/4] HHH-19676 add test --- .../ordering/OrderByFragmentTranslator.java | 2 +- .../test/orderedcollection/PrintJob.java | 23 ++++++++++++++++ .../test/orderedcollection/Printer.java | 27 +++++++++++++++++++ .../SortedCollectionTest.java | 27 +++++++++++++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/PrintJob.java create mode 100644 tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/Printer.java create mode 100644 tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/SortedCollectionTest.java 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 index dbd59a47c920..b1344d38e757 100644 --- 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 @@ -56,7 +56,7 @@ public static OrderByFragment translate( public static void check(String fragment) { final var parseTree = buildParseTree( fragment ); - // TODO: check against the model + // TODO: check against the model (requires the PluralAttributeMapping) } private static OrderingParser.OrderByFragmentContext buildParseTree(String fragment) { diff --git a/tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/PrintJob.java b/tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/PrintJob.java new file mode 100644 index 000000000000..ac37dd5616d8 --- /dev/null +++ b/tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/PrintJob.java @@ -0,0 +1,23 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * Copyright Red Hat Inc. and Hibernate Authors + */ +package org.hibernate.processor.test.orderedcollection; + +import jakarta.persistence.Entity; +import jakarta.persistence.GeneratedValue; +import jakarta.persistence.Id; +import jakarta.persistence.Lob; + +/** + * @author Hardy Ferentschik + */ +@Entity +public class PrintJob { + @Id + @GeneratedValue + private long id; + + @Lob + private byte[] data; +} diff --git a/tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/Printer.java b/tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/Printer.java new file mode 100644 index 000000000000..a6ea48103f86 --- /dev/null +++ b/tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/Printer.java @@ -0,0 +1,27 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * Copyright Red Hat Inc. and Hibernate Authors + */ +package org.hibernate.processor.test.orderedcollection; + +import jakarta.persistence.Entity; +import jakarta.persistence.GeneratedValue; +import jakarta.persistence.Id; +import jakarta.persistence.OneToMany; +import jakarta.persistence.OrderBy; + +import java.util.Set; + +/** + * @author Hardy Ferentschik + */ +@Entity +public class Printer { + @Id + @GeneratedValue + private long id; + + @OneToMany + @OrderBy("id desc, data") + private Set printQueue; +} diff --git a/tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/SortedCollectionTest.java b/tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/SortedCollectionTest.java new file mode 100644 index 000000000000..523fdf900941 --- /dev/null +++ b/tooling/metamodel-generator/src/test/java/org/hibernate/processor/test/orderedcollection/SortedCollectionTest.java @@ -0,0 +1,27 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * Copyright Red Hat Inc. and Hibernate Authors + */ +package org.hibernate.processor.test.orderedcollection; + +import org.hibernate.processor.test.util.CompilationTest; +import org.hibernate.processor.test.util.WithClasses; +import org.junit.jupiter.api.Test; + +import static org.hibernate.processor.test.util.TestUtil.assertMetamodelClassGeneratedFor; +import static org.hibernate.processor.test.util.TestUtil.assertPresenceOfFieldInMetamodelFor; + +/** + * @author Hardy Ferentschik + */ +@CompilationTest +class SortedCollectionTest { + + @Test + @WithClasses({ Printer.class, PrintJob.class }) + void testGenerics() { + assertMetamodelClassGeneratedFor( Printer.class ); + assertMetamodelClassGeneratedFor( PrintJob.class ); + assertPresenceOfFieldInMetamodelFor( Printer.class, "printQueue", "There sorted set attribute is missing" ); + } +}