HHH-11901 - Fix audited collections that contain null values.

hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/AbstractCollectionMapper.java

@@ -318,10 +318,7 @@ public abstract class AbstractCollectionMapper<T> implements PropertyMapper {
 		final Collection newCollection = getNewCollectionContent( newColl );
 		final Collection oldCollection = getOldCollectionContent( oldColl );
 
-		final Set<Object> added = new HashSet<>();
+		final Set<Object> added = buildCollectionChangeSet( newColl, newCollection );
-		if ( newColl != null ) {
-			added.addAll( newCollection );
-		}
 		// Re-hashing the old collection as the hash codes of the elements there may have changed, and the
 		// removeAll in AbstractSet has an implementation that is hashcode-change sensitive (as opposed to addAll).
 		if ( oldColl != null ) {
@@ -329,10 +326,7 @@ public abstract class AbstractCollectionMapper<T> implements PropertyMapper {
 		}
 		addCollectionChanges( session, collectionChanges, added, RevisionType.ADD, id );
 
-		final Set<Object> deleted = new HashSet<>();
+		final Set<Object> deleted = buildCollectionChangeSet( oldColl, oldCollection );
-		if ( oldColl != null ) {
-			deleted.addAll( oldCollection );
-		}
 		// The same as above - re-hashing new collection.
 		if ( newColl != null ) {
 			deleted.removeAll( new HashSet( newCollection ) );
@@ -367,10 +361,7 @@ public abstract class AbstractCollectionMapper<T> implements PropertyMapper {
 
 		// take the new collection and remove any that exist in the old collection.
 		// take the resulting Set<> and generate ADD changes.
-		final Set<Object> added = new HashSet<>();
+		final Set<Object> added = buildCollectionChangeSet( newColl, newCollection );
-		if ( newColl != null ) {
-			added.addAll( newCollection );
-		}
 		if ( oldColl != null && collectionPersister != null ) {
 			for ( Object object : oldCollection ) {
 				for ( Iterator addedIt = added.iterator(); addedIt.hasNext(); ) {
@@ -386,10 +377,7 @@ public abstract class AbstractCollectionMapper<T> implements PropertyMapper {
 
 		// take the old collection and remove any that exist in the new collection.
 		// take the resulting Set<> and generate DEL changes.
-		final Set<Object> deleted = new HashSet<>();
+		final Set<Object> deleted = buildCollectionChangeSet( oldColl, oldCollection );
-		if ( oldColl != null ) {
-			deleted.addAll( oldCollection );
-		}
 		if ( newColl != null && collectionPersister != null ) {
 			for ( Object object : newCollection ) {
 				for ( Iterator deletedIt = deleted.iterator(); deletedIt.hasNext(); ) {
@@ -406,6 +394,8 @@ public abstract class AbstractCollectionMapper<T> implements PropertyMapper {
 		return collectionChanges;
 	}
 
+	protected abstract Set<Object> buildCollectionChangeSet(Object eventCollection, Collection collection);
+
 	@Override
 	public boolean hasPropertiesWithModifiedFlag() {
 		if ( commonCollectionMapperData != null ) {

hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/BasicCollectionMapper.java

@@ -8,7 +8,9 @@ package org.hibernate.envers.internal.entities.mapper.relation;
 
 import java.io.Serializable;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 
 import org.hibernate.collection.spi.PersistentCollection;
 import org.hibernate.engine.spi.SessionImplementor;
@@ -20,6 +22,7 @@ import org.hibernate.envers.internal.reader.AuditReaderImplementor;
 
 /**
  * @author Adam Warski (adam at warski dot org)
+ * @author Chris Cranford
  */
 public class BasicCollectionMapper<T extends Collection> extends AbstractCollectionMapper<T> implements PropertyMapper {
 	protected final MiddleComponentData elementComponentData;
@@ -80,4 +83,17 @@ public class BasicCollectionMapper<T extends Collection> extends AbstractCollect
 			Object changed) {
 		elementComponentData.getComponentMapper().mapToMapFromObject( session, idData, data, changed );
 	}
+
+	@Override
+	protected Set<Object> buildCollectionChangeSet(Object eventCollection, Collection collection) {
+		final Set<Object> changeSet = new HashSet<>();
+		if ( eventCollection != null ) {
+			for ( Object entry : collection ) {
+				if ( entry != null ) {
+					changeSet.add( entry );
+				}
+			}
+		}
+		return changeSet;
+	}
 }

hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/ListCollectionMapper.java

@@ -8,8 +8,10 @@ package org.hibernate.envers.internal.entities.mapper.relation;
 
 import java.io.Serializable;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import org.hibernate.collection.spi.PersistentCollection;
 import org.hibernate.engine.spi.SessionImplementor;
@@ -24,6 +26,7 @@ import org.hibernate.envers.tools.Pair;
 
 /**
  * @author Adam Warski (adam at warski dot org)
+ * @author Chris Cranford
  */
 public final class ListCollectionMapper extends AbstractCollectionMapper<List> implements PropertyMapper {
 	private final MiddleComponentData elementComponentData;
@@ -95,4 +98,21 @@ public final class ListCollectionMapper extends AbstractCollectionMapper<List> i
 		);
 		indexComponentData.getComponentMapper().mapToMapFromObject( session, idData, data, indexValuePair.getFirst() );
 	}
+
+	@Override
+	protected Set<Object> buildCollectionChangeSet(Object eventCollection, Collection collection) {
+		final Set<Object> changeSet = new HashSet<>();
+		if ( eventCollection != null ) {
+			for ( Object entry : collection ) {
+				if ( entry != null ) {
+					final Pair pair = Pair.class.cast( entry );
+					if ( pair.getSecond() == null ) {
+						continue;
+					}
+					changeSet.add( entry );
+				}
+			}
+		}
+		return changeSet;
+	}
 }

hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/MapCollectionMapper.java

@@ -8,7 +8,9 @@ package org.hibernate.envers.internal.entities.mapper.relation;
 
 import java.io.Serializable;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 
 import org.hibernate.collection.spi.PersistentCollection;
 import org.hibernate.engine.spi.SessionImplementor;
@@ -20,6 +22,7 @@ import org.hibernate.envers.internal.reader.AuditReaderImplementor;
 
 /**
  * @author Adam Warski (adam at warski dot org)
+ * @author Chris Cranford
  */
 public class MapCollectionMapper<T extends Map> extends AbstractCollectionMapper<T> implements PropertyMapper {
 	protected final MiddleComponentData elementComponentData;
@@ -94,4 +97,21 @@ public class MapCollectionMapper<T extends Map> extends AbstractCollectionMapper
 				( (Map.Entry) changed ).getKey()
 		);
 	}
+
+	@Override
+	protected Set<Object> buildCollectionChangeSet(Object eventCollection, Collection collection) {
+		final Set<Object> changeSet = new HashSet<>();
+		if ( eventCollection != null ) {
+			for ( Object entry : collection ) {
+				if ( entry != null ) {
+					final Map.Entry element = Map.Entry.class.cast( entry );
+					if ( element.getValue() == null ) {
+						continue;
+					}
+					changeSet.add( entry );
+				}
+			}
+		}
+		return changeSet;
+	}
 }

hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/lazy/initializor/ListCollectionInitializor.java

@@ -19,6 +19,7 @@ import org.hibernate.envers.internal.reader.AuditReaderImplementor;
  * Initializes a map.
  *
  * @author Adam Warski (adam at warski dot org)
+ * @author Chris Cranford
  */
 public class ListCollectionInitializor extends AbstractCollectionInitializor<List> {
 	private final MiddleComponentData elementComponentData;
@@ -42,11 +43,19 @@ public class ListCollectionInitializor extends AbstractCollectionInitializor<Lis
 	@Override
 	@SuppressWarnings({"unchecked"})
 	protected List initializeCollection(int size) {
-		// Creating a list of the given capacity with all elements null initially. This ensures that we can then
+		// There are two types of List collections that this class may generate
-		// fill the elements safely using the <code>List.set</code> method.
+		//
+		// 		1. Those which are not-indexed, thus the entries in the list are equal to the size argument passed.
+		//		2. Those which are indexed, thus the entries in the list are based on the highest result-set index value.
+		//		   In this use case, the supplied size value is irrelevant other than to minimize allocations.
+		//
+		// So what we're going to do is to build an ArrayList based on the supplied size as the best of the two
+		// worlds.  When adding elements to the collection, we cannot make any assumption that the slot for which
+		// we are inserting is valid, so we must continually expand the list as needed for (2); however we can
+		// avoid unnecessary memory allocations by using the size parameter as an optimization for both (1) and (2).
 		final List list = new ArrayList( size );
 		for ( int i = 0; i < size; i++ ) {
-			list.add( null );
+			list.add( i, null );
 		}
 		return list;
 	}
@@ -58,23 +67,40 @@ public class ListCollectionInitializor extends AbstractCollectionInitializor<Lis
 		// otherwise it will be a List
 		Object elementData = collectionRow;
 		Object indexData = collectionRow;
-		if ( collectionRow instanceof java.util.List ) {
+		if ( java.util.List.class.isInstance( collectionRow ) ) {
-			elementData = ( (List) collectionRow ).get( elementComponentData.getComponentIndex() );
+			final java.util.List row = java.util.List.class.cast( collectionRow );
-			indexData = ( (List) collectionRow ).get( indexComponentData.getComponentIndex() );
+			elementData = row.get( elementComponentData.getComponentIndex() );
+			indexData = row.get( indexComponentData.getComponentIndex() );
+		}
+
+		final Object element;
+		if ( Map.class.isInstance( elementData ) ) {
+			element = elementComponentData.getComponentMapper().mapToObjectFromFullMap(
+					entityInstantiator,
+					(Map<String, Object>) elementData,
+					null,
+					revision
+			);
+		}
+		else {
+			element = elementData;
 		}
-		final Object element = elementData instanceof Map
-				? elementComponentData.getComponentMapper().mapToObjectFromFullMap(
-						entityInstantiator,
-						(Map<String, Object>) elementData, null, revision
-				)
-				: elementData;
 
 		final Object indexObj = indexComponentData.getComponentMapper().mapToObjectFromFullMap(
 				entityInstantiator,
-				(Map<String, Object>) indexData, element, revision
+				(Map<String, Object>) indexData,
+				element,
+				revision
 		);
+
 		final int index = ( (Number) indexObj ).intValue();
 
+		// This is copied from PersistentList#readFrom
+		// For the indexed list use case to make sure the slot that we're going to set actually exists.
+		for ( int i = collection.size(); i <= index; ++i ) {
+			collection.add( i, null );
+		}
+
 		collection.set( index, element );
 	}
 }

hibernate-envers/src/test/java/org/hibernate/envers/test/integration/collection/CollectionNullValueTest.java

@@ -0,0 +1,138 @@
+/*
+ * 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.envers.test.integration.collection;
+
+import java.util.Arrays;
+import java.util.List;
+
+import org.hibernate.envers.test.BaseEnversJPAFunctionalTestCase;
+import org.hibernate.envers.test.Priority;
+import org.hibernate.envers.test.entities.collection.StringListEntity;
+import org.hibernate.envers.test.entities.collection.StringMapEntity;
+import org.hibernate.envers.test.entities.collection.StringSetEntity;
+import org.hibernate.envers.test.tools.TestTools;
+import org.junit.Test;
+
+import org.hibernate.testing.TestForIssue;
+
+import static org.hibernate.testing.transaction.TransactionUtil.doInJPA;
+import static org.junit.Assert.assertEquals;
+
+/**
+ * @author Chris Cranford
+ */
+@TestForIssue(jiraKey = "HHH-11901")
+public class CollectionNullValueTest extends BaseEnversJPAFunctionalTestCase {
+	private Integer mapId;
+	private Integer listId;
+	private Integer setId;
+
+	@Override
+	protected Class<?>[] getAnnotatedClasses() {
+		return new Class<?>[] { StringMapEntity.class, StringListEntity.class, StringSetEntity.class };
+	}
+
+	@Test
+	@Priority(10)
+	public void initData() {
+		// Persist map with null values
+		mapId = doInJPA( this::entityManagerFactory, entityManager -> {
+			final StringMapEntity sme = new StringMapEntity();
+			sme.getStrings().put( "A", "B" );
+			sme.getStrings().put( "B", null );
+			entityManager.persist( sme );
+
+			return sme.getId();
+		} );
+
+		// Update map with null values
+		doInJPA( this::entityManagerFactory, entityManager -> {
+			final StringMapEntity sme = entityManager.find( StringMapEntity.class, mapId );
+			sme.getStrings().put( "C", null );
+			sme.getStrings().put( "D", "E" );
+			sme.getStrings().remove( "A" );
+			entityManager.merge( sme );
+		} );
+
+		// Persist list with null values
+		listId = doInJPA( this::entityManagerFactory, entityManager -> {
+			final StringListEntity sle = new StringListEntity();
+			sle.getStrings().add( "A" );
+			sle.getStrings().add( null );
+			entityManager.persist( sle );
+
+			return sle.getId();
+		} );
+
+		// Update list with null values
+		doInJPA( this::entityManagerFactory, entityManager -> {
+			final StringListEntity sle = entityManager.find( StringListEntity.class, listId );
+			sle.getStrings().add( null );
+			sle.getStrings().add( "D" );
+			sle.getStrings().remove( "A" );
+			entityManager.merge( sle );
+		} );
+
+		// Persist set with null values
+		setId = doInJPA( this::entityManagerFactory, entityManager -> {
+			final StringSetEntity sse = new StringSetEntity();
+			sse.getStrings().add( "A" );
+			sse.getStrings().add( null );
+			entityManager.persist( sse );
+
+			return sse.getId();
+		} );
+
+		// Update set with null values
+		doInJPA( this::entityManagerFactory, entityManager -> {
+			final StringSetEntity sse = entityManager.find( StringSetEntity.class, setId );
+			sse.getStrings().add( null );
+			sse.getStrings().add( "D" );
+			sse.getStrings().remove( "A" );
+			entityManager.merge( sse );
+		} );
+	}
+
+	@Test
+	public void testStringMapHistory() {
+		final List<Number> revisions = getAuditReader().getRevisions( StringMapEntity.class, mapId );
+		assertEquals( Arrays.asList( 1, 2 ), revisions );
+
+		final StringMapEntity rev1 = getAuditReader().find( StringMapEntity.class, mapId, 1 );
+		assertEquals( TestTools.makeMap( "A", "B" ), rev1.getStrings() );
+
+		final StringMapEntity rev2 = getAuditReader().find( StringMapEntity.class, mapId, 2 );
+		assertEquals( TestTools.makeMap( "D", "E" ), rev2.getStrings() );
+	}
+
+	@Test
+	public void testStringListHistory() {
+		final List<Number> revisions = getAuditReader().getRevisions( StringListEntity.class, listId );
+		assertEquals( Arrays.asList( 3, 4 ), revisions );
+
+		final StringListEntity rev3 = getAuditReader().find( StringListEntity.class, listId, 3 );
+		assertEquals( TestTools.makeList( "A" ), rev3.getStrings() );
+
+		// NOTE: the only reason this assertion expects a null element is because the collection is indexed.
+		// ORM will return a list that consists of { null, "D" } and Envers should effectively mimic that.
+		final StringListEntity rev4 = getAuditReader().find( StringListEntity.class, listId, 4 );
+		assertEquals( TestTools.makeList( null, "D" ), rev4.getStrings() );
+	}
+
+	@Test
+	public void testStringSetHistory() {
+		final List<Number> revisions = getAuditReader().getRevisions( StringSetEntity.class, setId );
+		assertEquals( Arrays.asList( 5, 6 ), revisions );
+
+		final StringSetEntity rev5 = getAuditReader().find( StringSetEntity.class, setId, 5 );
+		assertEquals( TestTools.makeSet( "A" ), rev5.getStrings() );
+
+		final StringSetEntity rev6 = getAuditReader().find( StringSetEntity.class, setId, 6 );
+		assertEquals( TestTools.makeSet( "D" ), rev6.getStrings() );
+	}
+
+}