From 5f9bcb8c7f2ba40f89ac93722d044ae6642bbc92 Mon Sep 17 00:00:00 2001 From: Chris Cranford Date: Thu, 8 Feb 2018 11:29:12 -0500 Subject: [PATCH] HHH-11901 - Fix audited collections that contain null values. (cherry picked from commit 2977d8f) --- .../relation/AbstractCollectionMapper.java | 22 +-- .../relation/BasicCollectionMapper.java | 16 ++ .../mapper/relation/ListCollectionMapper.java | 20 +++ .../mapper/relation/MapCollectionMapper.java | 20 +++ .../ListCollectionInitializor.java | 52 +++++-- .../collection/CollectionNullValueTest.java | 138 ++++++++++++++++++ 6 files changed, 239 insertions(+), 29 deletions(-) create mode 100644 hibernate-envers/src/test/java/org/hibernate/envers/test/integration/collection/CollectionNullValueTest.java diff --git a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/AbstractCollectionMapper.java b/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/AbstractCollectionMapper.java index f658d6e921..083222d52a 100644 --- a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/AbstractCollectionMapper.java +++ b/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/AbstractCollectionMapper.java @@ -318,10 +318,7 @@ public abstract class AbstractCollectionMapper implements PropertyMapper { final Collection newCollection = getNewCollectionContent( newColl ); final Collection oldCollection = getOldCollectionContent( oldColl ); - final Set added = new HashSet<>(); - if ( newColl != null ) { - added.addAll( newCollection ); - } + final Set added = buildCollectionChangeSet( newColl, 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 implements PropertyMapper { } addCollectionChanges( session, collectionChanges, added, RevisionType.ADD, id ); - final Set deleted = new HashSet<>(); - if ( oldColl != null ) { - deleted.addAll( oldCollection ); - } + final Set deleted = buildCollectionChangeSet( oldColl, 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 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 added = new HashSet<>(); - if ( newColl != null ) { - added.addAll( newCollection ); - } + final Set added = buildCollectionChangeSet( newColl, 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 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 deleted = new HashSet<>(); - if ( oldColl != null ) { - deleted.addAll( oldCollection ); - } + final Set deleted = buildCollectionChangeSet( oldColl, 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 implements PropertyMapper { return collectionChanges; } + protected abstract Set buildCollectionChangeSet(Object eventCollection, Collection collection); + @Override public boolean hasPropertiesWithModifiedFlag() { if ( commonCollectionMapperData != null ) { diff --git a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/BasicCollectionMapper.java b/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/BasicCollectionMapper.java index 43f30dcdf6..152a9380f6 100644 --- a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/BasicCollectionMapper.java +++ b/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 extends AbstractCollectionMapper implements PropertyMapper { protected final MiddleComponentData elementComponentData; @@ -80,4 +83,17 @@ public class BasicCollectionMapper extends AbstractCollect Object changed) { elementComponentData.getComponentMapper().mapToMapFromObject( session, idData, data, changed ); } + + @Override + protected Set buildCollectionChangeSet(Object eventCollection, Collection collection) { + final Set changeSet = new HashSet<>(); + if ( eventCollection != null ) { + for ( Object entry : collection ) { + if ( entry != null ) { + changeSet.add( entry ); + } + } + } + return changeSet; + } } diff --git a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/ListCollectionMapper.java b/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/ListCollectionMapper.java index 02dad90df9..6cc5477c05 100644 --- a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/ListCollectionMapper.java +++ b/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 implements PropertyMapper { private final MiddleComponentData elementComponentData; @@ -95,4 +98,21 @@ public final class ListCollectionMapper extends AbstractCollectionMapper i ); indexComponentData.getComponentMapper().mapToMapFromObject( session, idData, data, indexValuePair.getFirst() ); } + + @Override + protected Set buildCollectionChangeSet(Object eventCollection, Collection collection) { + final Set 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; + } } diff --git a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/MapCollectionMapper.java b/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/MapCollectionMapper.java index fa1bee0c94..a719f4b575 100644 --- a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/MapCollectionMapper.java +++ b/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 extends AbstractCollectionMapper implements PropertyMapper { protected final MiddleComponentData elementComponentData; @@ -94,4 +97,21 @@ public class MapCollectionMapper extends AbstractCollectionMapper ( (Map.Entry) changed ).getKey() ); } + + @Override + protected Set buildCollectionChangeSet(Object eventCollection, Collection collection) { + final Set 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; + } } diff --git a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/lazy/initializor/ListCollectionInitializor.java b/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/lazy/initializor/ListCollectionInitializor.java index 8b83dba292..455ee8057a 100644 --- a/hibernate-envers/src/main/java/org/hibernate/envers/internal/entities/mapper/relation/lazy/initializor/ListCollectionInitializor.java +++ b/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 { private final MiddleComponentData elementComponentData; @@ -42,11 +43,19 @@ public class ListCollectionInitializor extends AbstractCollectionInitializorList.set method. + // There are two types of List collections that this class may generate + // + // 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) elementData, + null, + revision + ); + } + else { + element = elementData; } - final Object element = elementData instanceof Map - ? elementComponentData.getComponentMapper().mapToObjectFromFullMap( - entityInstantiator, - (Map) elementData, null, revision - ) - : elementData; final Object indexObj = indexComponentData.getComponentMapper().mapToObjectFromFullMap( entityInstantiator, - (Map) indexData, element, revision + (Map) 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 ); } } diff --git a/hibernate-envers/src/test/java/org/hibernate/envers/test/integration/collection/CollectionNullValueTest.java b/hibernate-envers/src/test/java/org/hibernate/envers/test/integration/collection/CollectionNullValueTest.java new file mode 100644 index 0000000000..897d7980cb --- /dev/null +++ b/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 . + */ +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 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 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 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() ); + } + +}