HHH-12122 - Checking @OrderBy for special cases should perform case-insensitive checking

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;
+	private String cacheConcurrencyStrategy;
-	String cacheRegionName;
+	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 );
+				throw new AnnotationException(
-				error.append( "mappedBy reference an unknown target entity property: " )
+						"mappedBy reference an unknown target entity property: " +
-						.append( collType ).append( "." ).append( this.mappedBy )
+								collType + "." + this.mappedBy +
-						.append( " in " )
+								" in " +
-						.append( collection.getOwnerEntityName() )
+								collection.getOwnerEntityName() +
-						.append( "." )
+								"." +
-						.append( property.getName() );
+								property.getName()
-				throw new AnnotationException( error.toString() );
+				);
 			}
 		}
 		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
+			orderByFragment = orderByFragment.trim();
-			if ( orderByFragment.length() == 0 ) {
+			if ( orderByFragment.length() == 0 || orderByFragment.equalsIgnoreCase( "asc" ) ) {
-				//order by element
+				// 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 )
+				throw new AnnotationException(
-						.append(
+						"Collection of elements must not have mappedBy or association reference an unmapped entity: " +
-								"Collection of elements must not have mappedBy or association reference an unmapped entity: "
+								collValue.getOwnerEntityName() +
-						)
+								"." +
-						.append( collValue.getOwnerEntityName() )
+								joinColumns[0].getPropertyName()
-						.append( "." )
+				);
-						.append( joinColumns[0].getPropertyName() );
-				throw new AnnotationException( error.toString() );
 			}
 			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<String, Join>() );
+					column.setJoins( new HashMap<>() );
 					column.setBuildingContext( buildingContext );
 					column.bind();
 					elementColumns[0] = column;

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(),

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

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<String> names;
+//	}
+}