From f49efc7864ad2809e2cc42ec0185c2b07b8c4b83 Mon Sep 17 00:00:00 2001 From: Steve Ebersole Date: Fri, 24 Nov 2017 11:12:22 -0600 Subject: [PATCH] HHH-12122 - Checking @OrderBy for special cases should perform case-insensitive checking --- .../cfg/annotations/CollectionBinder.java | 95 +++++++------- .../AbstractCollectionPersister.java | 2 + .../antlr/OrderByFragmentTranslator.java | 2 + .../compliance/OrderByAnnotationTests.java | 119 ++++++++++++++++++ 4 files changed, 171 insertions(+), 47 deletions(-) create mode 100644 hibernate-core/src/test/java/org/hibernate/test/jpa/compliance/OrderByAnnotationTests.java diff --git a/hibernate-core/src/main/java/org/hibernate/cfg/annotations/CollectionBinder.java b/hibernate-core/src/main/java/org/hibernate/cfg/annotations/CollectionBinder.java index fd8c19c55e..4f7f20a5d2 100644 --- a/hibernate-core/src/main/java/org/hibernate/cfg/annotations/CollectionBinder.java +++ b/hibernate-core/src/main/java/org/hibernate/cfg/annotations/CollectionBinder.java @@ -112,7 +112,7 @@ import static org.hibernate.cfg.BinderHelper.toAliasTableMap; * @author inger * @author Emmanuel Bernard */ -@SuppressWarnings({"unchecked", "serial"}) +@SuppressWarnings({"unchecked", "serial", "WeakerAccess", "deprecation"}) public abstract class CollectionBinder { private static final CoreMessageLogger LOG = Logger.getMessageLogger(CoreMessageLogger.class, CollectionBinder.class.getName()); @@ -121,14 +121,14 @@ public abstract class CollectionBinder { protected Collection collection; protected String propertyName; PropertyHolder propertyHolder; - int batchSize; + private int batchSize; private String mappedBy; private XClass collectionType; private XClass targetEntity; private Ejb3JoinColumn[] inverseJoinColumns; private String cascadeStrategy; - String cacheConcurrencyStrategy; - String cacheRegionName; + private String cacheConcurrencyStrategy; + private String cacheRegionName; private boolean oneToMany; protected IndexColumn indexColumn; protected boolean cascadeDeleteEnabled; @@ -161,6 +161,10 @@ public abstract class CollectionBinder { private String explicitType; private final Properties explicitTypeParameters = new Properties(); + protected CollectionBinder(boolean isSortedCollection) { + this.isSortedCollection = isSortedCollection; + } + protected MetadataBuildingContext getBuildingContext() { return buildingContext; } @@ -173,7 +177,7 @@ public abstract class CollectionBinder { return false; } - public void setIsHibernateExtensionMapping(boolean hibernateExtensionMapping) { + protected void setIsHibernateExtensionMapping(boolean hibernateExtensionMapping) { this.hibernateExtensionMapping = hibernateExtensionMapping; } @@ -347,10 +351,6 @@ public abstract class CollectionBinder { return result; } - protected CollectionBinder(boolean isSortedCollection) { - this.isSortedCollection = isSortedCollection; - } - public void setMappedBy(String mappedBy) { this.mappedBy = mappedBy; } @@ -533,7 +533,7 @@ public abstract class CollectionBinder { binder.setName( propertyName ); binder.setValue( collection ); binder.setCascade( cascadeStrategy ); - if ( cascadeStrategy != null && cascadeStrategy.indexOf( "delete-orphan" ) >= 0 ) { + if ( cascadeStrategy != null && cascadeStrategy.contains( "delete-orphan" ) ) { collection.setOrphanDelete( true ); } binder.setLazy( collection.isLazy() ); @@ -552,8 +552,6 @@ public abstract class CollectionBinder { } private void applySortingAndOrdering(Collection collection) { - boolean isSorted = isSortedCollection; - boolean hadOrderBy = false; boolean hadExplicitSort = false; @@ -567,10 +565,10 @@ public abstract class CollectionBinder { } hadExplicitSort = deprecatedSort.type() != SortType.UNSORTED; if ( deprecatedSort.type() == SortType.NATURAL ) { - isSorted = true; + isSortedCollection = true; } else if ( deprecatedSort.type() == SortType.COMPARATOR ) { - isSorted = true; + isSortedCollection = true; comparatorClass = deprecatedSort.comparator(); } } @@ -649,7 +647,7 @@ public abstract class CollectionBinder { Fetch fetch = property.getAnnotation( Fetch.class ); OneToMany oneToMany = property.getAnnotation( OneToMany.class ); ManyToMany manyToMany = property.getAnnotation( ManyToMany.class ); - ElementCollection elementCollection = property.getAnnotation( ElementCollection.class ); //jpa 2 + ElementCollection elementCollection = property.getAnnotation( ElementCollection.class ); ManyToAny manyToAny = property.getAnnotation( ManyToAny.class ); FetchType fetchType; if ( oneToMany != null ) { @@ -775,14 +773,14 @@ public abstract class CollectionBinder { ); } catch (MappingException e) { - StringBuilder error = new StringBuilder( 80 ); - error.append( "mappedBy reference an unknown target entity property: " ) - .append( collType ).append( "." ).append( this.mappedBy ) - .append( " in " ) - .append( collection.getOwnerEntityName() ) - .append( "." ) - .append( property.getName() ); - throw new AnnotationException( error.toString() ); + throw new AnnotationException( + "mappedBy reference an unknown target entity property: " + + collType + "." + this.mappedBy + + " in " + + collection.getOwnerEntityName() + + "." + + property.getName() + ); } } if ( persistentClass != null @@ -1011,13 +1009,6 @@ public abstract class CollectionBinder { // } } - private String getCondition(FilterJoinTable filter) { - //set filtering - String name = filter.name(); - String cond = filter.condition(); - return getCondition( cond, name ); - } - private String getCondition(Filter filter) { //set filtering String name = filter.name(); @@ -1076,17 +1067,30 @@ public abstract class CollectionBinder { return orderByFragment; } - private static String adjustUserSuppliedValueCollectionOrderingFragment(String orderByFragment) { + public static String adjustUserSuppliedValueCollectionOrderingFragment(String orderByFragment) { + LOG.debugf( "#adjustUserSuppliedValueCollectionOrderingFragment(%)", orderByFragment ); + if ( orderByFragment != null ) { - // NOTE: "$element$" is a specially recognized collection property recognized by the collection persister - if ( orderByFragment.length() == 0 ) { - //order by element + orderByFragment = orderByFragment.trim(); + if ( orderByFragment.length() == 0 || orderByFragment.equalsIgnoreCase( "asc" ) ) { + // This indicates something like either: + // `@OrderBy()` + // `@OrderBy("asc") + // + // JPA says this should indicate an ascending natural ordering of the elements - id for + // entity associations or the value(s) for "element collections" return "$element$ asc"; } - else if ( "desc".equals( orderByFragment ) ) { + else if ( orderByFragment.equalsIgnoreCase( "desc" ) ) { + // This indicates: + // `@OrderBy("desc")` + // + // JPA says this should indicate a descending natural ordering of the elements - id for + // entity associations or the value(s) for "element collections" return "$element$ desc"; } } + return orderByFragment; } @@ -1208,7 +1212,7 @@ public abstract class CollectionBinder { return key; } - protected void bindManyToManySecondPass( + private void bindManyToManySecondPass( Collection collValue, Map persistentClasses, Ejb3JoinColumn[] joinColumns, @@ -1276,14 +1280,12 @@ public abstract class CollectionBinder { boolean mappedBy = !BinderHelper.isEmptyAnnotationValue( joinColumns[0].getMappedBy() ); if ( mappedBy ) { if ( !isCollectionOfEntities ) { - StringBuilder error = new StringBuilder( 80 ) - .append( - "Collection of elements must not have mappedBy or association reference an unmapped entity: " - ) - .append( collValue.getOwnerEntityName() ) - .append( "." ) - .append( joinColumns[0].getPropertyName() ); - throw new AnnotationException( error.toString() ); + throw new AnnotationException( + "Collection of elements must not have mappedBy or association reference an unmapped entity: " + + collValue.getOwnerEntityName() + + "." + + joinColumns[0].getPropertyName() + ); } Property otherSideProperty; try { @@ -1425,7 +1427,7 @@ public abstract class CollectionBinder { XClass elementClass; AnnotatedClassType classType; - CollectionPropertyHolder holder = null; + CollectionPropertyHolder holder; if ( BinderHelper.PRIMITIVE_NAMES.contains( collType.getName() ) ) { classType = AnnotatedClassType.NONE; elementClass = null; @@ -1522,7 +1524,6 @@ public abstract class CollectionBinder { collValue.setElement( component ); if ( StringHelper.isNotEmpty( hqlOrderBy ) ) { - String path = collValue.getOwnerEntityName() + "." + joinColumns[0].getPropertyName(); String orderBy = adjustUserSuppliedValueCollectionOrderingFragment( hqlOrderBy ); if ( orderBy != null ) { collValue.setOrderBy( orderBy ); @@ -1544,7 +1545,7 @@ public abstract class CollectionBinder { column.setLength( Ejb3Column.DEFAULT_COLUMN_LENGTH ); column.setLogicalColumnName( Collection.DEFAULT_ELEMENT_COLUMN_NAME ); //TODO create an EMPTY_JOINS collection - column.setJoins( new HashMap() ); + column.setJoins( new HashMap<>() ); column.setBuildingContext( buildingContext ); column.bind(); elementColumns[0] = column; diff --git a/hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java b/hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java index aaab76fadb..ed0e77ebbd 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/collection/AbstractCollectionPersister.java @@ -551,6 +551,7 @@ public abstract class AbstractCollectionPersister hasOrder = collectionBinding.getOrderBy() != null; if ( hasOrder ) { + LOG.debugf( "Translating order-by fragment [%s] for collection role : %s", collectionBinding.getOrderBy(), getRole() ); orderByTranslation = Template.translateOrderBy( collectionBinding.getOrderBy(), new ColumnMapperImpl(), @@ -577,6 +578,7 @@ public abstract class AbstractCollectionPersister hasManyToManyOrder = collectionBinding.getManyToManyOrdering() != null; if ( hasManyToManyOrder ) { + LOG.debugf( "Translating many-to-many order-by fragment [%s] for collection role : %s", collectionBinding.getOrderBy(), getRole() ); manyToManyOrderByTranslation = Template.translateOrderBy( collectionBinding.getManyToManyOrdering(), new ColumnMapperImpl(), diff --git a/hibernate-core/src/main/java/org/hibernate/sql/ordering/antlr/OrderByFragmentTranslator.java b/hibernate-core/src/main/java/org/hibernate/sql/ordering/antlr/OrderByFragmentTranslator.java index 21c3305e77..cb6c7c54ca 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/ordering/antlr/OrderByFragmentTranslator.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/ordering/antlr/OrderByFragmentTranslator.java @@ -36,6 +36,8 @@ public class OrderByFragmentTranslator { * @return The translation. */ public static OrderByTranslation translate(TranslationContext context, String fragment) { + LOG.tracef( "Beginning parsing of order-by fragment : " + fragment ); + GeneratedOrderByLexer lexer = new GeneratedOrderByLexer( new StringReader( fragment ) ); // Perform the parsing (and some analysis/resolution). Another important aspect is the collection diff --git a/hibernate-core/src/test/java/org/hibernate/test/jpa/compliance/OrderByAnnotationTests.java b/hibernate-core/src/test/java/org/hibernate/test/jpa/compliance/OrderByAnnotationTests.java new file mode 100644 index 0000000000..90acc9383d --- /dev/null +++ b/hibernate-core/src/test/java/org/hibernate/test/jpa/compliance/OrderByAnnotationTests.java @@ -0,0 +1,119 @@ +/* + * 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.test.jpa.compliance; + +import java.util.List; +import javax.persistence.Column; +import javax.persistence.ElementCollection; +import javax.persistence.Entity; +import javax.persistence.Id; +import javax.persistence.OrderBy; +import javax.persistence.Table; + +import org.hibernate.boot.MetadataSources; +import org.hibernate.cfg.annotations.CollectionBinder; +import org.hibernate.dialect.Dialect; +import org.hibernate.dialect.function.SQLFunctionRegistry; +import org.hibernate.engine.spi.SessionFactoryImplementor; +import org.hibernate.sql.ordering.antlr.ColumnMapper; +import org.hibernate.sql.ordering.antlr.ColumnReference; +import org.hibernate.sql.ordering.antlr.OrderByFragmentTranslator; +import org.hibernate.sql.ordering.antlr.OrderByTranslation; +import org.hibernate.sql.ordering.antlr.SqlValueReference; +import org.hibernate.sql.ordering.antlr.TranslationContext; + +import org.hibernate.testing.junit4.BaseNonConfigCoreFunctionalTestCase; +import org.junit.Test; + +import org.hamcrest.CoreMatchers; + +import static org.hamcrest.MatcherAssert.assertThat; + +/** + * @author Steve Ebersole + */ +public class OrderByAnnotationTests extends BaseNonConfigCoreFunctionalTestCase { + private static final String ELEMENT_TOKEN = "$element$"; + private static final String TABLE_ALIAS = "a"; + private static final String COLUMN_NAME = "name"; + + @Test + public void testOrderByEmpty() { + assertThat( translate( "" ), CoreMatchers.is( TABLE_ALIAS + '.' + COLUMN_NAME + " asc" ) ); + } + + @Test + public void testOrderByJustDesc() { + assertThat( translate( "desc" ), CoreMatchers.is( TABLE_ALIAS + '.' + COLUMN_NAME + " desc" ) ); + + assertThat( translate( "DESC"), CoreMatchers.is( TABLE_ALIAS + '.' + COLUMN_NAME + " desc" ) ); + } + + @Test + public void testOrderByJustAsc() { + assertThat( translate( "asc"), CoreMatchers.is( TABLE_ALIAS + '.' + COLUMN_NAME + " asc" ) ); + + assertThat( translate( "ASC"), CoreMatchers.is( TABLE_ALIAS + '.' + COLUMN_NAME + " asc" ) ); + } + + private String translate(String fragment) { + fragment = CollectionBinder.adjustUserSuppliedValueCollectionOrderingFragment( fragment ); + + final TranslationContext translationContext = translationContext(); + final OrderByTranslation translation = OrderByFragmentTranslator.translate( translationContext, fragment ); + + return translation.injectAliases( columnReference -> TABLE_ALIAS ); + } + + private TranslationContext translationContext() { + final ColumnMapper columnMapper = reference -> { + assert ELEMENT_TOKEN.equals( reference ); + return new SqlValueReference[] { + (ColumnReference) () -> COLUMN_NAME + }; + }; + + return new TranslationContext() { + @Override + public SessionFactoryImplementor getSessionFactory() { + return sessionFactory(); + } + + @Override + public Dialect getDialect() { + return getSessionFactory().getJdbcServices().getJdbcEnvironment().getDialect(); + } + + @Override + public SQLFunctionRegistry getSqlFunctionRegistry() { + return getSessionFactory().getSqlFunctionRegistry(); + } + + @Override + public ColumnMapper getColumnMapper() { + return columnMapper; + } + }; + } + +// @Override +// protected void applyMetadataSources(MetadataSources metadataSources) { +// super.applyMetadataSources( metadataSources ); +// metadataSources.addAnnotatedClass( A.class ); +// } +// +// @Entity( name = "A" ) +// @Table( name = "T_A" ) +// public static class A { +// @Id +// public Integer id; +// @ElementCollection +// @Column( name = "name" ) +// @OrderBy +// public List names; +// } +}