From e331c2870e8caeda510c317a21893a2e7a488fdc Mon Sep 17 00:00:00 2001 From: Gavin King Date: Sat, 8 Jan 2022 10:43:03 +0100 Subject: [PATCH] more cosmetic improvements to HQL error reporting makes the messages and exception types a bit more consistent --- .../internal/QualifiedJoinPathConsumer.java | 8 +-- .../hql/internal/SemanticQueryBuilder.java | 67 ++++++++++++------- .../query/sqm/UnknownEntityException.java | 2 +- .../test/annotations/ConfigurationTest.java | 4 +- .../loader/LoaderWithInvalidQueryTest.java | 2 +- .../MappedSuperclassInheritanceTest.java | 2 +- 6 files changed, 53 insertions(+), 32 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/query/hql/internal/QualifiedJoinPathConsumer.java b/hibernate-core/src/main/java/org/hibernate/query/hql/internal/QualifiedJoinPathConsumer.java index 8b2c3efba0..f43bb2bcc4 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/hql/internal/QualifiedJoinPathConsumer.java +++ b/hibernate-core/src/main/java/org/hibernate/query/hql/internal/QualifiedJoinPathConsumer.java @@ -118,7 +118,7 @@ public class QualifiedJoinPathConsumer implements DotIdentifierConsumer { // identifier is an alias (identification variable) if ( isTerminal ) { - throw new SemanticException( "Cannot join to root : " + identifier ); + throw new SemanticException( "Cannot join to root '" + identifier + "'" ); } return new AttributeJoinDelegate( @@ -181,9 +181,9 @@ public class QualifiedJoinPathConsumer implements DotIdentifierConsumer { throw new HqlInterpretationException( String.format( Locale.ROOT, - "Could not locate specified joinable path : %s -> %s", - lhs.getNavigablePath(), - name + "Could not resolve joined attribute '%s' of '%s'", + name, + lhs.getNavigablePath() ) ); } diff --git a/hibernate-core/src/main/java/org/hibernate/query/hql/internal/SemanticQueryBuilder.java b/hibernate-core/src/main/java/org/hibernate/query/hql/internal/SemanticQueryBuilder.java index 6a362fd509..907bccef25 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/hql/internal/SemanticQueryBuilder.java +++ b/hibernate-core/src/main/java/org/hibernate/query/hql/internal/SemanticQueryBuilder.java @@ -423,7 +423,10 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem final SqmRoot root = visitTargetEntity( dmlTargetContext ); if ( root.getReferencedPathSource() instanceof SqmPolymorphicRootDescriptor ) { throw new SemanticException( - "Can't create an INSERT for a non entity name: " + root.getReferencedPathSource().getHibernateEntityName() + String.format( + "Target type '%s' in insert statement is not an entity", + root.getReferencedPathSource().getHibernateEntityName() + ) ); } @@ -511,7 +514,10 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem final SqmRoot root = visitTargetEntity( dmlTargetContext ); if ( root.getReferencedPathSource() instanceof SqmPolymorphicRootDescriptor ) { throw new SemanticException( - "Can't create an UPDATE for a non entity name: " + root.getReferencedPathSource().getHibernateEntityName() + String.format( + "Target type '%s' in update statement is not an entity", + root.getReferencedPathSource().getHibernateEntityName() + ) ); } @@ -779,7 +785,7 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem sqmQueryPart.setFetchExpression( visitLimitClause( limitClauseContext ) ); } else { - throw new SemanticException("Can't use both, limit and fetch clause!" ); + throw new SemanticException("Can't use both limit and fetch clause" ); } } } @@ -961,7 +967,7 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem ); } catch (ClassLoadingException e) { - throw new SemanticException( "Unable to resolve class named for dynamic instantiation : " + className ); + throw new SemanticException( "Could not resolve class '" + className + "' named for instantiation" ); } } else { @@ -1362,7 +1368,7 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem .getJpaMetamodel() .getHqlEntityReference( entityName ); if ( entityReference == null ) { - throw new UnknownEntityException( "Could not resolve entity name [" + entityName + "] as DML target", entityName ); + throw new UnknownEntityException( "Could not resolve target entity '" + entityName + "'", entityName ); } checkFQNEntityNameJpaComplianceViolationIfNeeded( entityName, entityReference ); if ( entityReference instanceof SqmPolymorphicRootDescriptor && getCreationOptions().useStrictJpaCompliance() ) { @@ -1476,7 +1482,7 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem } throw new SemanticException( "Could not resolve entity or correlation path '" + name + "'" ); } - throw new SemanticException( "Could not resolve entity '" + name + "'" ); + throw new UnknownEntityException( "Could not resolve root entity '" + name + "'", name ); } checkFQNEntityNameJpaComplianceViolationIfNeeded( name, entityDescriptor ); @@ -1491,7 +1497,7 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem if ( processingStateStack.depth() > 1 ) { throw new SemanticException( - "Illegal implicit-polymorphic domain path in sub-query : " + entityDescriptor.getName() + "Illegal implicit-polymorphic domain path in subquery '" + entityDescriptor.getName() +"'" ); } } @@ -2140,7 +2146,7 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem throw new NotYetImplementedFor6Exception( "Path continuation from `id()` reference not yet implemented" ); } - throw new SemanticException( "Path does not reference an identifiable-type : " + sqmPath.getNavigablePath().getFullPath() ); + throw new SemanticException( "Path does not resolve to an entity type '" + sqmPath.getNavigablePath().getFullPath() + "'" ); } @Override @@ -2159,15 +2165,17 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem final SingularPersistentAttribute versionAttribute = identifiableType.findVersionAttribute(); if ( versionAttribute == null ) { throw new SemanticException( - "`" + sqmPath.getNavigablePath().getFullPath() + "` resolved to an identifiable-type (`" + - identifiableType.getTypeName() + "`) which does not define a version" + String.format( + "Path '%s' resolved to entity type '%s' which does not define a version", + sqmPath.getNavigablePath().getFullPath(), identifiableType.getTypeName() + ) ); } return sqmPath.get( versionAttribute ); } - throw new SemanticException( "Path does not reference an identifiable-type : " + sqmPath.getNavigablePath().getFullPath() ); + throw new SemanticException( "Path does not resolve to an entity type '" + sqmPath.getNavigablePath().getFullPath() + "'" ); } @Override @@ -2186,14 +2194,18 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem final List> attributes = identifiableType.findNaturalIdAttributes(); if ( attributes == null ) { throw new SemanticException( - "`" + sqmPath.getNavigablePath().getFullPath() + "` resolved to an identifiable-type (`" + - identifiableType.getTypeName() + "`) which does not define a natural id" + String.format( + "Path '%s' resolved to entity type '%s' which does not define a natural id", + sqmPath.getNavigablePath().getFullPath(), identifiableType.getTypeName() + ) ); } else if ( attributes.size() >1 ) { throw new SemanticException( - "`" + sqmPath.getNavigablePath().getFullPath() + "` resolved to an identifiable-type (`" + - identifiableType.getTypeName() + "`) which defines multiple natural ids" + String.format( + "Path '%s' resolved to entity type '%s' which defines multiple natural ids", + sqmPath.getNavigablePath().getFullPath(), identifiableType.getTypeName() + ) ); } @@ -2203,7 +2215,7 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem return sqmPath.get( naturalIdAttribute ); } - throw new SemanticException( "Path does not reference an identifiable-type : " + sqmPath.getNavigablePath().getFullPath() ); + throw new SemanticException( "Path does not resolve to an entity type '" + sqmPath.getNavigablePath().getFullPath() + "'" ); } @Override @@ -3501,7 +3513,7 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem public Object visitFormat(HqlParser.FormatContext ctx) { String format = QuotingHelper.unquoteStringLiteral( ctx.getChild( 0 ).getText() ); if (!FORMAT.matcher(format).matches()) { - throw new SemanticException("illegal format pattern: '" + format + "'"); + throw new SemanticException("illegal format pattern '" + format + "'"); } return new SqmFormat( format, @@ -3681,7 +3693,7 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem if ( !(referencedPathSource instanceof PluralPersistentAttribute ) ) { throw new PathException( - "Illegal attempt to treat non-plural path as a plural path : " + pluralAttributePath.getNavigablePath() + "Path is not a plural path '" + pluralAttributePath.getNavigablePath() + "'" ); } final SqmSubQuery subQuery = new SqmSubQuery<>( @@ -3809,7 +3821,7 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem final String padCharText = ctx.STRING_LITERAL().getText(); if ( padCharText.length() != 3 ) { - throw new SemanticException( "Pad character for pad() function must be single character, found: " + padCharText ); + throw new SemanticException( "Pad character for pad() function must be single character, found '" + padCharText + "'" ); } return new SqmLiteral<>( @@ -3878,7 +3890,7 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem : " "; // JPA says space is the default if ( trimCharText.length() != 1 ) { - throw new SemanticException( "Trim character for trim() function must be single character, found: " + trimCharText ); + throw new SemanticException( "Trim character for trim() function must be single character, found '" + trimCharText + "'" ); } return new SqmLiteral<>( @@ -4181,10 +4193,15 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem public SqmPath visitCollectionElementNavigablePath(HqlParser.CollectionElementNavigablePathContext ctx) { final SqmPath pluralAttributePath = consumeDomainPath( (HqlParser.PathContext) ctx.getChild( 2 ) ); final SqmPathSource referencedPathSource = pluralAttributePath.getReferencedPathSource(); + final TerminalNode firstNode = (TerminalNode) ctx.getChild( 0 ); if ( !(referencedPathSource instanceof PluralPersistentAttribute ) ) { throw new PathException( - "Illegal attempt to treat non-plural path as a plural path : " + pluralAttributePath.getNavigablePath() + String.format( + "Argument of '%s' is not a plural path '%s'", + firstNode.getSymbol().getText(), + pluralAttributePath.getNavigablePath() + ) ); } @@ -4194,7 +4211,6 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem if ( attribute.getCollectionClassification() != CollectionClassification.MAP ) { throw new StrictJpaComplianceViolation( StrictJpaComplianceViolation.Type.VALUE_FUNCTION_ON_NON_MAP ); } - final TerminalNode firstNode = (TerminalNode) ctx.getChild( 0 ); if ( firstNode.getSymbol().getType() == HqlParser.ELEMENTS ) { throw new StrictJpaComplianceViolation( StrictJpaComplianceViolation.Type.HQL_COLLECTION_FUNCTION ); } @@ -4219,8 +4235,13 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem final SqmPathSource referencedPathSource = pluralAttributePath.getReferencedPathSource(); if ( !(referencedPathSource instanceof PluralPersistentAttribute ) ) { + final TerminalNode firstNode = (TerminalNode) ctx.getChild( 0 ); throw new PathException( - "Illegal attempt to treat non-plural path as a plural path : " + pluralAttributePath.getNavigablePath() + String.format( + "Argument of '%s' is not a plural path '%s'", + firstNode.getSymbol().getText(), + pluralAttributePath.getNavigablePath() + ) ); } diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/UnknownEntityException.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/UnknownEntityException.java index 574e588a56..87ef2fb0ca 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/UnknownEntityException.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/UnknownEntityException.java @@ -22,7 +22,7 @@ public class UnknownEntityException extends SemanticException { private final String entityName; public UnknownEntityException(String entityName) { - this( "Unable to resolve [" + entityName + "] as entity", entityName ); + this( "Could not resolve entity '" + entityName + "'", entityName ); } public UnknownEntityException(String message, String entityName) { diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/ConfigurationTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/ConfigurationTest.java index 5fde12928c..55d29a0d25 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/ConfigurationTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/ConfigurationTest.java @@ -15,8 +15,8 @@ import org.hibernate.Transaction; import org.hibernate.cfg.Configuration; import org.hibernate.cfg.Environment; import org.hibernate.engine.spi.SessionFactoryImplementor; -import org.hibernate.query.SemanticException; +import org.hibernate.query.sqm.UnknownEntityException; import org.hibernate.test.annotations.Boat; import org.hibernate.test.annotations.Ferry; import org.hibernate.test.annotations.Port; @@ -68,7 +68,7 @@ public class ConfigurationTest { fail( "Boat should not be mapped" ); } catch (IllegalArgumentException expected) { - assertEquals( SemanticException.class, expected.getCause().getClass() ); + assertEquals( UnknownEntityException.class, expected.getCause().getClass() ); // expected outcome // see org.hibernate.test.jpa.compliance.tck2_2.QueryApiTest#testInvalidQueryMarksTxnForRollback diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/loader/LoaderWithInvalidQueryTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/loader/LoaderWithInvalidQueryTest.java index 1207e6074a..437ef460e0 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/loader/LoaderWithInvalidQueryTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/loader/LoaderWithInvalidQueryTest.java @@ -44,7 +44,7 @@ public class LoaderWithInvalidQueryTest extends BaseEntityManagerFunctionalTestC Throwable[] suppressed = rootCause.getSuppressed(); assertEquals( 2, suppressed.length ); assertTrue( ExceptionUtil.rootCause( suppressed[0] ).getMessage().contains( "Could not resolve attribute 'valid'" ) ); - assertTrue( ExceptionUtil.rootCause( suppressed[1] ).getMessage().contains( "Could not resolve entity '_Person'" ) ); + assertTrue( ExceptionUtil.rootCause( suppressed[1] ).getMessage().contains( "Could not resolve root entity '_Person'" ) ); } } diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/MappedSuperclassInheritanceTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/MappedSuperclassInheritanceTest.java index c31644d10b..4cb1567ba6 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/MappedSuperclassInheritanceTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/inheritance/MappedSuperclassInheritanceTest.java @@ -76,7 +76,7 @@ public class MappedSuperclassInheritanceTest extends BaseEntityManagerFunctional fail(); } catch (Exception expected) { SemanticException rootException = (SemanticException) ExceptionUtil.rootCause( expected); - assertEquals("Could not resolve entity 'Employee'", rootException.getMessage()); + assertEquals("Could not resolve root entity 'Employee'", rootException.getMessage()); } } ); }