Skip to content

Commit

Permalink
HHH-11901 - Fix audited collections that contain null values.
Browse files Browse the repository at this point in the history
(cherry picked from commit 2977d8f)
  • Loading branch information
Naros committed Feb 19, 2018
1 parent 2749065 commit 5f9bcb8
Show file tree
Hide file tree
Showing 6 changed files with 239 additions and 29 deletions.
Expand Up @@ -318,21 +318,15 @@ private List<PersistentCollectionChangeData> mapCollectionChanges(
final Collection newCollection = getNewCollectionContent( newColl );
final Collection oldCollection = getOldCollectionContent( oldColl );

final Set<Object> added = new HashSet<>();
if ( newColl != null ) {
added.addAll( newCollection );
}
final Set<Object> 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 ) {
added.removeAll( new HashSet( oldCollection ) );
}
addCollectionChanges( session, collectionChanges, added, RevisionType.ADD, id );

final Set<Object> deleted = new HashSet<>();
if ( oldColl != null ) {
deleted.addAll( oldCollection );
}
final Set<Object> deleted = buildCollectionChangeSet( oldColl, oldCollection );
// The same as above - re-hashing new collection.
if ( newColl != null ) {
deleted.removeAll( new HashSet( newCollection ) );
Expand Down Expand Up @@ -367,10 +361,7 @@ private List<PersistentCollectionChangeData> mapCollectionChanges(

// 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<>();
if ( newColl != null ) {
added.addAll( newCollection );
}
final Set<Object> added = buildCollectionChangeSet( newColl, newCollection );
if ( oldColl != null && collectionPersister != null ) {
for ( Object object : oldCollection ) {
for ( Iterator addedIt = added.iterator(); addedIt.hasNext(); ) {
Expand All @@ -386,10 +377,7 @@ private List<PersistentCollectionChangeData> mapCollectionChanges(

// 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<>();
if ( oldColl != null ) {
deleted.addAll( oldCollection );
}
final Set<Object> deleted = buildCollectionChangeSet( oldColl, oldCollection );
if ( newColl != null && collectionPersister != null ) {
for ( Object object : newCollection ) {
for ( Iterator deletedIt = deleted.iterator(); deletedIt.hasNext(); ) {
Expand All @@ -406,6 +394,8 @@ private List<PersistentCollectionChangeData> mapCollectionChanges(
return collectionChanges;
}

protected abstract Set<Object> buildCollectionChangeSet(Object eventCollection, Collection collection);

@Override
public boolean hasPropertiesWithModifiedFlag() {
if ( commonCollectionMapperData != null ) {
Expand Down
Expand Up @@ -8,7 +8,9 @@

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;
Expand All @@ -20,6 +22,7 @@

/**
* @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;
Expand Down Expand Up @@ -80,4 +83,17 @@ protected void mapToMapFromObject(
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;
}
}
Expand Up @@ -8,8 +8,10 @@

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;
Expand All @@ -24,6 +26,7 @@

/**
* @author Adam Warski (adam at warski dot org)
* @author Chris Cranford
*/
public final class ListCollectionMapper extends AbstractCollectionMapper<List> implements PropertyMapper {
private final MiddleComponentData elementComponentData;
Expand Down Expand Up @@ -95,4 +98,21 @@ protected void mapToMapFromObject(
);
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;
}
}
Expand Up @@ -8,7 +8,9 @@

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;
Expand All @@ -20,6 +22,7 @@

/**
* @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;
Expand Down Expand Up @@ -94,4 +97,21 @@ protected void mapToMapFromObject(
( (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;
}
}
Expand Up @@ -19,6 +19,7 @@
* Initializes a map.
*
* @author Adam Warski (adam at warski dot org)
* @author Chris Cranford
*/
public class ListCollectionInitializor extends AbstractCollectionInitializor<List> {
private final MiddleComponentData elementComponentData;
Expand All @@ -42,11 +43,19 @@ public ListCollectionInitializor(
@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
// fill the elements safely using the <code>List.set</code> 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;
}
Expand All @@ -58,23 +67,40 @@ protected void addToCollection(List collection, Object collectionRow) {
// otherwise it will be a List
Object elementData = collectionRow;
Object indexData = collectionRow;
if ( collectionRow instanceof java.util.List ) {
elementData = ( (List) collectionRow ).get( elementComponentData.getComponentIndex() );
indexData = ( (List) collectionRow ).get( indexComponentData.getComponentIndex() );
if ( java.util.List.class.isInstance( collectionRow ) ) {
final java.util.List row = java.util.List.class.cast( collectionRow );
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 );
}
}
@@ -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() );
}

}

0 comments on commit 5f9bcb8

Please sign in to comment.