HHH-4065 - Incorrect SQL is used for HQL if the number of values for a filter collection parameter is changed

git-svn-id: https://svn.jboss.org/repos/hibernate/core/trunk@17959 1b8cb986-b30d-0410-93ca-fae66ebed9b2

core/src/main/java/org/hibernate/engine/query/QueryPlanCache.java

@@ -26,10 +26,13 @@ package org.hibernate.engine.query;
 
 import org.hibernate.util.SimpleMRUCache;
 import org.hibernate.util.SoftLimitMRUCache;
+import org.hibernate.util.CollectionHelper;
 import org.hibernate.engine.SessionFactoryImplementor;
 import org.hibernate.engine.query.sql.NativeSQLQuerySpecification;
 import org.hibernate.QueryException;
 import org.hibernate.MappingException;
+import org.hibernate.impl.FilterImpl;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -40,6 +43,7 @@ import java.util.Iterator;
 import java.util.Set;
 import java.util.HashSet;
 import java.util.Collections;
+import java.util.Collection;
 
 /**
  * Acts as a cache for compiled query plans, as well as query-parameter metadata.
@@ -174,7 +178,7 @@ public class QueryPlanCache implements Serializable {
 	private static class HQLQueryPlanKey implements Serializable {
 		private final String query;
 		private final boolean shallow;
-		private final Set filterNames;
+		private final Set filterKeys;
 		private final int hashCode;
 
 		public HQLQueryPlanKey(String query, boolean shallow, Map enabledFilters) {
@@ -182,17 +186,23 @@ public class QueryPlanCache implements Serializable {
 			this.shallow = shallow;
 
 			if ( enabledFilters == null || enabledFilters.isEmpty() ) {
-				filterNames = Collections.EMPTY_SET;
+				filterKeys = Collections.EMPTY_SET;
 			}
 			else {
-				Set tmp = new HashSet();
+				Set tmp = new HashSet(
-				tmp.addAll( enabledFilters.keySet() );
+						CollectionHelper.determineProperSizing( enabledFilters ),
-				this.filterNames = Collections.unmodifiableSet( tmp );
+						CollectionHelper.LOAD_FACTOR
+				);
+				Iterator itr = enabledFilters.values().iterator();
+				while ( itr.hasNext() ) {
+					tmp.add( new DynamicFilterKey( ( FilterImpl ) itr.next() ) );
+				}
+				this.filterKeys = Collections.unmodifiableSet( tmp );
 			}
 
 			int hash = query.hashCode();
 			hash = 29 * hash + ( shallow ? 1 : 0 );
-			hash = 29 * hash + filterNames.hashCode();
+			hash = 29 * hash + filterKeys.hashCode();
 			this.hashCode = hash;
 		}
 
@@ -206,17 +216,64 @@ public class QueryPlanCache implements Serializable {
 
 			final HQLQueryPlanKey that = ( HQLQueryPlanKey ) o;
 
-			if ( shallow != that.shallow ) {
+			return shallow == that.shallow
-				return false;
+					&& filterKeys.equals( that.filterKeys )
+					&& query.equals( that.query );
+
 		}
-			if ( !filterNames.equals( that.filterNames ) ) {
+
-				return false;
+		public int hashCode() {
+			return hashCode;
 		}
-			if ( !query.equals( that.query ) ) {
+	}
+
+	private static class DynamicFilterKey implements Serializable {
+		private final String filterName;
+		private final Map parameterMetadata;
+		private final int hashCode;
+
+		private DynamicFilterKey(FilterImpl filter) {
+			this.filterName = filter.getName();
+			if ( filter.getParameters().isEmpty() ) {
+				parameterMetadata = Collections.EMPTY_MAP;
+			}
+			else {
+				parameterMetadata = new HashMap(
+						CollectionHelper.determineProperSizing( filter.getParameters() ),
+						CollectionHelper.LOAD_FACTOR
+				);
+				Iterator itr = filter.getParameters().entrySet().iterator();
+				while ( itr.hasNext() ) {
+					final Integer valueCount;
+					final Map.Entry entry = ( Map.Entry ) itr.next();
+					if ( Collection.class.isInstance( entry.getValue() ) ) {
+						valueCount = new Integer( ( (Collection) entry.getValue() ).size() );
+					}
+					else {
+						valueCount = new Integer(1);
+					}
+					parameterMetadata.put( entry.getKey(), valueCount );
+				}
+			}
+
+			int hash = filterName.hashCode();
+			hash = 31 * hash + parameterMetadata.hashCode();
+			this.hashCode = hash;
+		}
+
+		public boolean equals(Object o) {
+			if ( this == o ) {
+				return true;
+			}
+			if ( o == null || getClass() != o.getClass() ) {
 				return false;
 			}
 
-			return true;
+			DynamicFilterKey that = ( DynamicFilterKey ) o;
+
+			return filterName.equals( that.filterName )
+					&& parameterMetadata.equals( that.parameterMetadata );
+
 		}
 
 		public int hashCode() {
@@ -262,20 +319,11 @@ public class QueryPlanCache implements Serializable {
 
 			final FilterQueryPlanKey that = ( FilterQueryPlanKey ) o;
 
-			if ( shallow != that.shallow ) {
+			return shallow == that.shallow
-				return false;
+					&& filterNames.equals( that.filterNames )
-			}
+					&& query.equals( that.query )
-			if ( !filterNames.equals( that.filterNames ) ) {
+					&& collectionRole.equals( that.collectionRole );
-				return false;
-			}
-			if ( !query.equals( that.query ) ) {
-				return false;
-			}
-			if ( !collectionRole.equals( that.collectionRole ) ) {
-				return false;
-			}
 
-			return true;
 		}
 
 		public int hashCode() {

core/src/main/java/org/hibernate/util/CollectionHelper.java

@@ -29,6 +29,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 /**
  * Various help for handling collections.
@@ -37,6 +38,8 @@ import java.util.Map;
  * @author Steve Ebersole
  */
 public final class CollectionHelper {
+    public static final int MINIMUM_INITIAL_CAPACITY = 16;
+	public static final float LOAD_FACTOR = 0.75f;
 
 	public static final List EMPTY_LIST = Collections.unmodifiableList( new ArrayList(0) );
 	public static final Collection EMPTY_COLLECTION = Collections.unmodifiableCollection( new ArrayList(0) );
@@ -54,7 +57,40 @@ public final class CollectionHelper {
 	 * @return The sized map.
 	 */
 	public static Map mapOfSize(int size) {
-		final int currentSize = (int) (size / 0.75f);
+		return new HashMap( determineProperSizing( size ), LOAD_FACTOR );
-		return new HashMap( Math.max( currentSize+ 1, 16), 0.75f );
+	}
+
+	/**
+	 * Given a map, determine the proper initial size for a new Map to hold the same number of values.
+	 * Specifically we want to account for load size and load factor to prevent immediate resizing.
+	 *
+	 * @param original The original map
+	 * @return The proper size.
+	 */
+	public static int determineProperSizing(Map original) {
+		return determineProperSizing( original.size() );
+	}
+
+	/**
+	 * Given a set, determine the proper initial size for a new set to hold the same number of values.
+	 * Specifically we want to account for load size and load factor to prevent immediate resizing.
+	 *
+	 * @param original The original set
+	 * @return The proper size.
+	 */
+	public static int determineProperSizing(Set original) {
+		return determineProperSizing( original.size() );
+	}
+
+	/**
+	 * Determine the proper initial size for a new collection in order for it to hold the given a number of elements.
+	 * Specifically we want to account for load size and load factor to prevent immediate resizing.
+	 *
+	 * @param numberOfElements The number of elements to be stored.
+	 * @return The proper size.
+	 */
+	public static int determineProperSizing(int numberOfElements) {
+		int actual = ( (int) (numberOfElements / LOAD_FACTOR) ) + 1;
+		return Math.max( actual, MINIMUM_INITIAL_CAPACITY );
 	}
 }

testsuite/src/test/java/org/hibernate/test/filter/DynamicFilterTest.java

@@ -39,6 +39,7 @@ import org.slf4j.LoggerFactory;
  *
  * @author Steve
  */
+@SuppressWarnings({ "WhileLoopReplaceableByForEach", "unchecked" })
 public class DynamicFilterTest extends FunctionalTestCase {
 
 	private Logger log = LoggerFactory.getLogger( DynamicFilterTest.class );
@@ -99,7 +100,7 @@ public class DynamicFilterTest extends FunctionalTestCase {
 		ts = ( ( SessionImplementor ) session ).getTimestamp();
 		session.enableFilter( "fulfilledOrders" ).setParameter( "asOfDate", testData.lastMonth.getTime() );
 		sp = ( Salesperson ) session.createQuery( "from Salesperson as s where s.id = :id" )
-		        .setLong( "id", testData.steveId.longValue() )
+		        .setLong( "id", testData.steveId )
 		        .uniqueResult();
 		assertEquals( "Filtered-collection not bypassing 2L-cache", 1, sp.getOrders().size() );
 
@@ -142,6 +143,17 @@ public class DynamicFilterTest extends FunctionalTestCase {
 
 		session.clear();
 
+		session.disableFilter("regionlist");
+		session.enableFilter( "regionlist" ).setParameterList( "regions", new String[]{"LA", "APAC", "APAC"} );
+		// Second test retreival through hql with the collection as non-eager with different region list
+		salespersons = session.createQuery( "select s from Salesperson as s" ).list();
+		assertEquals( "Incorrect salesperson count", 1, salespersons.size() );
+		sp = ( Salesperson ) salespersons.get( 0 );
+		assertEquals( "Incorrect order count", 1, sp.getOrders().size() );
+
+		session.clear();
+
+
 		// test retreival through hql with the collection join fetched
 		salespersons = session.createQuery( "select s from Salesperson as s left join fetch s.orders" ).list();
 		assertEquals( "Incorrect salesperson count", 1, salespersons.size() );
@@ -224,7 +236,7 @@ public class DynamicFilterTest extends FunctionalTestCase {
 
 		log.info( "Criteria query against Product..." );
 		List products = session.createCriteria( Product.class )
-		        .add( Restrictions.eq( "stockNumber", new Integer( 124 ) ) )
+		        .add( Restrictions.eq( "stockNumber", 124 ) )
 		        .list();
 		assertEquals( "Incorrect product count", 1, products.size() );
 
@@ -288,10 +300,10 @@ public class DynamicFilterTest extends FunctionalTestCase {
 		session.enableFilter("region").setParameter("region", "APAC");
 
 		DetachedCriteria lineItemSubquery = DetachedCriteria.forClass(LineItem.class)
-				.add(Restrictions.ge( "quantity", new Long(1L) ))
+				.add( Restrictions.ge( "quantity", 1L ) )
-				.createCriteria("product")
+				.createCriteria( "product" )
-				.add(Restrictions.eq("name", "Acme Hair Gel"))
+				.add( Restrictions.eq( "name", "Acme Hair Gel" ) )
-				.setProjection(Property.forName("id"));
+				.setProjection( Property.forName( "id" ) );
 
 		List orders = session.createCriteria(Order.class)
 				.add(Subqueries.exists(lineItemSubquery))
@@ -309,7 +321,7 @@ public class DynamicFilterTest extends FunctionalTestCase {
 				.setProjection(Property.forName("id"));
 
 		lineItemSubquery = DetachedCriteria.forClass(LineItem.class)
-				.add(Restrictions.ge("quantity", new Long(1L) ))
+				.add(Restrictions.ge("quantity", 1L ))
 				.createCriteria("product")
 				.add(Subqueries.propertyIn("id", productSubquery))
 				.setProjection(Property.forName("id"));
@@ -662,7 +674,7 @@ public class DynamicFilterTest extends FunctionalTestCase {
 
 		// Force the categories to not get initialized here
 		List result = session.createQuery( "from Product as p where p.id = :id" )
-		        .setLong( "id", testData.prod1Id.longValue() )
+		        .setLong( "id", testData.prod1Id )
 		        .list();
 		assertTrue( "No products returned from HQL", !result.isEmpty() );