Skip to content

Commit 2977d8f

Browse files
committed
HHH-11901 - Fix audited collections that contain null values.
1 parent b0ca1c5 commit 2977d8f

File tree

6 files changed

+239
-29
lines changed

6 files changed

+239
-29
lines changed

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

+6-16
Original file line numberDiff line numberDiff line change
@@ -318,21 +318,15 @@ private List<PersistentCollectionChangeData> mapCollectionChanges(
318318
final Collection newCollection = getNewCollectionContent( newColl );
319319
final Collection oldCollection = getOldCollectionContent( oldColl );
320320

321-
final Set<Object> added = new HashSet<>();
322-
if ( newColl != null ) {
323-
added.addAll( newCollection );
324-
}
321+
final Set<Object> added = buildCollectionChangeSet( newColl, newCollection );
325322
// Re-hashing the old collection as the hash codes of the elements there may have changed, and the
326323
// removeAll in AbstractSet has an implementation that is hashcode-change sensitive (as opposed to addAll).
327324
if ( oldColl != null ) {
328325
added.removeAll( new HashSet( oldCollection ) );
329326
}
330327
addCollectionChanges( session, collectionChanges, added, RevisionType.ADD, id );
331328

332-
final Set<Object> deleted = new HashSet<>();
333-
if ( oldColl != null ) {
334-
deleted.addAll( oldCollection );
335-
}
329+
final Set<Object> deleted = buildCollectionChangeSet( oldColl, oldCollection );
336330
// The same as above - re-hashing new collection.
337331
if ( newColl != null ) {
338332
deleted.removeAll( new HashSet( newCollection ) );
@@ -367,10 +361,7 @@ private List<PersistentCollectionChangeData> mapCollectionChanges(
367361

368362
// take the new collection and remove any that exist in the old collection.
369363
// take the resulting Set<> and generate ADD changes.
370-
final Set<Object> added = new HashSet<>();
371-
if ( newColl != null ) {
372-
added.addAll( newCollection );
373-
}
364+
final Set<Object> added = buildCollectionChangeSet( newColl, newCollection );
374365
if ( oldColl != null && collectionPersister != null ) {
375366
for ( Object object : oldCollection ) {
376367
for ( Iterator addedIt = added.iterator(); addedIt.hasNext(); ) {
@@ -386,10 +377,7 @@ private List<PersistentCollectionChangeData> mapCollectionChanges(
386377

387378
// take the old collection and remove any that exist in the new collection.
388379
// take the resulting Set<> and generate DEL changes.
389-
final Set<Object> deleted = new HashSet<>();
390-
if ( oldColl != null ) {
391-
deleted.addAll( oldCollection );
392-
}
380+
final Set<Object> deleted = buildCollectionChangeSet( oldColl, oldCollection );
393381
if ( newColl != null && collectionPersister != null ) {
394382
for ( Object object : newCollection ) {
395383
for ( Iterator deletedIt = deleted.iterator(); deletedIt.hasNext(); ) {
@@ -406,6 +394,8 @@ private List<PersistentCollectionChangeData> mapCollectionChanges(
406394
return collectionChanges;
407395
}
408396

397+
protected abstract Set<Object> buildCollectionChangeSet(Object eventCollection, Collection collection);
398+
409399
@Override
410400
public boolean hasPropertiesWithModifiedFlag() {
411401
if ( commonCollectionMapperData != null ) {

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

+16
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@
88

99
import java.io.Serializable;
1010
import java.util.Collection;
11+
import java.util.HashSet;
1112
import java.util.Map;
13+
import java.util.Set;
1214

1315
import org.hibernate.collection.spi.PersistentCollection;
1416
import org.hibernate.engine.spi.SessionImplementor;
@@ -20,6 +22,7 @@
2022

2123
/**
2224
* @author Adam Warski (adam at warski dot org)
25+
* @author Chris Cranford
2326
*/
2427
public class BasicCollectionMapper<T extends Collection> extends AbstractCollectionMapper<T> implements PropertyMapper {
2528
protected final MiddleComponentData elementComponentData;
@@ -80,4 +83,17 @@ protected void mapToMapFromObject(
8083
Object changed) {
8184
elementComponentData.getComponentMapper().mapToMapFromObject( session, idData, data, changed );
8285
}
86+
87+
@Override
88+
protected Set<Object> buildCollectionChangeSet(Object eventCollection, Collection collection) {
89+
final Set<Object> changeSet = new HashSet<>();
90+
if ( eventCollection != null ) {
91+
for ( Object entry : collection ) {
92+
if ( entry != null ) {
93+
changeSet.add( entry );
94+
}
95+
}
96+
}
97+
return changeSet;
98+
}
8399
}

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

+20
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,10 @@
88

99
import java.io.Serializable;
1010
import java.util.Collection;
11+
import java.util.HashSet;
1112
import java.util.List;
1213
import java.util.Map;
14+
import java.util.Set;
1315

1416
import org.hibernate.collection.spi.PersistentCollection;
1517
import org.hibernate.engine.spi.SessionImplementor;
@@ -24,6 +26,7 @@
2426

2527
/**
2628
* @author Adam Warski (adam at warski dot org)
29+
* @author Chris Cranford
2730
*/
2831
public final class ListCollectionMapper extends AbstractCollectionMapper<List> implements PropertyMapper {
2932
private final MiddleComponentData elementComponentData;
@@ -95,4 +98,21 @@ protected void mapToMapFromObject(
9598
);
9699
indexComponentData.getComponentMapper().mapToMapFromObject( session, idData, data, indexValuePair.getFirst() );
97100
}
101+
102+
@Override
103+
protected Set<Object> buildCollectionChangeSet(Object eventCollection, Collection collection) {
104+
final Set<Object> changeSet = new HashSet<>();
105+
if ( eventCollection != null ) {
106+
for ( Object entry : collection ) {
107+
if ( entry != null ) {
108+
final Pair pair = Pair.class.cast( entry );
109+
if ( pair.getSecond() == null ) {
110+
continue;
111+
}
112+
changeSet.add( entry );
113+
}
114+
}
115+
}
116+
return changeSet;
117+
}
98118
}

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

+20
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@
88

99
import java.io.Serializable;
1010
import java.util.Collection;
11+
import java.util.HashSet;
1112
import java.util.Map;
13+
import java.util.Set;
1214

1315
import org.hibernate.collection.spi.PersistentCollection;
1416
import org.hibernate.engine.spi.SessionImplementor;
@@ -20,6 +22,7 @@
2022

2123
/**
2224
* @author Adam Warski (adam at warski dot org)
25+
* @author Chris Cranford
2326
*/
2427
public class MapCollectionMapper<T extends Map> extends AbstractCollectionMapper<T> implements PropertyMapper {
2528
protected final MiddleComponentData elementComponentData;
@@ -94,4 +97,21 @@ protected void mapToMapFromObject(
9497
( (Map.Entry) changed ).getKey()
9598
);
9699
}
100+
101+
@Override
102+
protected Set<Object> buildCollectionChangeSet(Object eventCollection, Collection collection) {
103+
final Set<Object> changeSet = new HashSet<>();
104+
if ( eventCollection != null ) {
105+
for ( Object entry : collection ) {
106+
if ( entry != null ) {
107+
final Map.Entry element = Map.Entry.class.cast( entry );
108+
if ( element.getValue() == null ) {
109+
continue;
110+
}
111+
changeSet.add( entry );
112+
}
113+
}
114+
}
115+
return changeSet;
116+
}
97117
}

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

+39-13
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
* Initializes a map.
2020
*
2121
* @author Adam Warski (adam at warski dot org)
22+
* @author Chris Cranford
2223
*/
2324
public class ListCollectionInitializor extends AbstractCollectionInitializor<List> {
2425
private final MiddleComponentData elementComponentData;
@@ -42,11 +43,19 @@ public ListCollectionInitializor(
4243
@Override
4344
@SuppressWarnings({"unchecked"})
4445
protected List initializeCollection(int size) {
45-
// Creating a list of the given capacity with all elements null initially. This ensures that we can then
46-
// fill the elements safely using the <code>List.set</code> method.
46+
// There are two types of List collections that this class may generate
47+
//
48+
// 1. Those which are not-indexed, thus the entries in the list are equal to the size argument passed.
49+
// 2. Those which are indexed, thus the entries in the list are based on the highest result-set index value.
50+
// In this use case, the supplied size value is irrelevant other than to minimize allocations.
51+
//
52+
// So what we're going to do is to build an ArrayList based on the supplied size as the best of the two
53+
// worlds. When adding elements to the collection, we cannot make any assumption that the slot for which
54+
// we are inserting is valid, so we must continually expand the list as needed for (2); however we can
55+
// avoid unnecessary memory allocations by using the size parameter as an optimization for both (1) and (2).
4756
final List list = new ArrayList( size );
4857
for ( int i = 0; i < size; i++ ) {
49-
list.add( null );
58+
list.add( i, null );
5059
}
5160
return list;
5261
}
@@ -58,23 +67,40 @@ protected void addToCollection(List collection, Object collectionRow) {
5867
// otherwise it will be a List
5968
Object elementData = collectionRow;
6069
Object indexData = collectionRow;
61-
if ( collectionRow instanceof java.util.List ) {
62-
elementData = ( (List) collectionRow ).get( elementComponentData.getComponentIndex() );
63-
indexData = ( (List) collectionRow ).get( indexComponentData.getComponentIndex() );
70+
if ( java.util.List.class.isInstance( collectionRow ) ) {
71+
final java.util.List row = java.util.List.class.cast( collectionRow );
72+
elementData = row.get( elementComponentData.getComponentIndex() );
73+
indexData = row.get( indexComponentData.getComponentIndex() );
74+
}
75+
76+
final Object element;
77+
if ( Map.class.isInstance( elementData ) ) {
78+
element = elementComponentData.getComponentMapper().mapToObjectFromFullMap(
79+
entityInstantiator,
80+
(Map<String, Object>) elementData,
81+
null,
82+
revision
83+
);
84+
}
85+
else {
86+
element = elementData;
6487
}
65-
final Object element = elementData instanceof Map
66-
? elementComponentData.getComponentMapper().mapToObjectFromFullMap(
67-
entityInstantiator,
68-
(Map<String, Object>) elementData, null, revision
69-
)
70-
: elementData;
7188

7289
final Object indexObj = indexComponentData.getComponentMapper().mapToObjectFromFullMap(
7390
entityInstantiator,
74-
(Map<String, Object>) indexData, element, revision
91+
(Map<String, Object>) indexData,
92+
element,
93+
revision
7594
);
95+
7696
final int index = ( (Number) indexObj ).intValue();
7797

98+
// This is copied from PersistentList#readFrom
99+
// For the indexed list use case to make sure the slot that we're going to set actually exists.
100+
for ( int i = collection.size(); i <= index; ++i ) {
101+
collection.add( i, null );
102+
}
103+
78104
collection.set( index, element );
79105
}
80106
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
/*
2+
* Hibernate, Relational Persistence for Idiomatic Java
3+
*
4+
* License: GNU Lesser General Public License (LGPL), version 2.1 or later.
5+
* See the lgpl.txt file in the root directory or <http://www.gnu.org/licenses/lgpl-2.1.html>.
6+
*/
7+
package org.hibernate.envers.test.integration.collection;
8+
9+
import java.util.Arrays;
10+
import java.util.List;
11+
12+
import org.hibernate.envers.test.BaseEnversJPAFunctionalTestCase;
13+
import org.hibernate.envers.test.Priority;
14+
import org.hibernate.envers.test.entities.collection.StringListEntity;
15+
import org.hibernate.envers.test.entities.collection.StringMapEntity;
16+
import org.hibernate.envers.test.entities.collection.StringSetEntity;
17+
import org.hibernate.envers.test.tools.TestTools;
18+
import org.junit.Test;
19+
20+
import org.hibernate.testing.TestForIssue;
21+
22+
import static org.hibernate.testing.transaction.TransactionUtil.doInJPA;
23+
import static org.junit.Assert.assertEquals;
24+
25+
/**
26+
* @author Chris Cranford
27+
*/
28+
@TestForIssue(jiraKey = "HHH-11901")
29+
public class CollectionNullValueTest extends BaseEnversJPAFunctionalTestCase {
30+
private Integer mapId;
31+
private Integer listId;
32+
private Integer setId;
33+
34+
@Override
35+
protected Class<?>[] getAnnotatedClasses() {
36+
return new Class<?>[] { StringMapEntity.class, StringListEntity.class, StringSetEntity.class };
37+
}
38+
39+
@Test
40+
@Priority(10)
41+
public void initData() {
42+
// Persist map with null values
43+
mapId = doInJPA( this::entityManagerFactory, entityManager -> {
44+
final StringMapEntity sme = new StringMapEntity();
45+
sme.getStrings().put( "A", "B" );
46+
sme.getStrings().put( "B", null );
47+
entityManager.persist( sme );
48+
49+
return sme.getId();
50+
} );
51+
52+
// Update map with null values
53+
doInJPA( this::entityManagerFactory, entityManager -> {
54+
final StringMapEntity sme = entityManager.find( StringMapEntity.class, mapId );
55+
sme.getStrings().put( "C", null );
56+
sme.getStrings().put( "D", "E" );
57+
sme.getStrings().remove( "A" );
58+
entityManager.merge( sme );
59+
} );
60+
61+
// Persist list with null values
62+
listId = doInJPA( this::entityManagerFactory, entityManager -> {
63+
final StringListEntity sle = new StringListEntity();
64+
sle.getStrings().add( "A" );
65+
sle.getStrings().add( null );
66+
entityManager.persist( sle );
67+
68+
return sle.getId();
69+
} );
70+
71+
// Update list with null values
72+
doInJPA( this::entityManagerFactory, entityManager -> {
73+
final StringListEntity sle = entityManager.find( StringListEntity.class, listId );
74+
sle.getStrings().add( null );
75+
sle.getStrings().add( "D" );
76+
sle.getStrings().remove( "A" );
77+
entityManager.merge( sle );
78+
} );
79+
80+
// Persist set with null values
81+
setId = doInJPA( this::entityManagerFactory, entityManager -> {
82+
final StringSetEntity sse = new StringSetEntity();
83+
sse.getStrings().add( "A" );
84+
sse.getStrings().add( null );
85+
entityManager.persist( sse );
86+
87+
return sse.getId();
88+
} );
89+
90+
// Update set with null values
91+
doInJPA( this::entityManagerFactory, entityManager -> {
92+
final StringSetEntity sse = entityManager.find( StringSetEntity.class, setId );
93+
sse.getStrings().add( null );
94+
sse.getStrings().add( "D" );
95+
sse.getStrings().remove( "A" );
96+
entityManager.merge( sse );
97+
} );
98+
}
99+
100+
@Test
101+
public void testStringMapHistory() {
102+
final List<Number> revisions = getAuditReader().getRevisions( StringMapEntity.class, mapId );
103+
assertEquals( Arrays.asList( 1, 2 ), revisions );
104+
105+
final StringMapEntity rev1 = getAuditReader().find( StringMapEntity.class, mapId, 1 );
106+
assertEquals( TestTools.makeMap( "A", "B" ), rev1.getStrings() );
107+
108+
final StringMapEntity rev2 = getAuditReader().find( StringMapEntity.class, mapId, 2 );
109+
assertEquals( TestTools.makeMap( "D", "E" ), rev2.getStrings() );
110+
}
111+
112+
@Test
113+
public void testStringListHistory() {
114+
final List<Number> revisions = getAuditReader().getRevisions( StringListEntity.class, listId );
115+
assertEquals( Arrays.asList( 3, 4 ), revisions );
116+
117+
final StringListEntity rev3 = getAuditReader().find( StringListEntity.class, listId, 3 );
118+
assertEquals( TestTools.makeList( "A" ), rev3.getStrings() );
119+
120+
// NOTE: the only reason this assertion expects a null element is because the collection is indexed.
121+
// ORM will return a list that consists of { null, "D" } and Envers should effectively mimic that.
122+
final StringListEntity rev4 = getAuditReader().find( StringListEntity.class, listId, 4 );
123+
assertEquals( TestTools.makeList( null, "D" ), rev4.getStrings() );
124+
}
125+
126+
@Test
127+
public void testStringSetHistory() {
128+
final List<Number> revisions = getAuditReader().getRevisions( StringSetEntity.class, setId );
129+
assertEquals( Arrays.asList( 5, 6 ), revisions );
130+
131+
final StringSetEntity rev5 = getAuditReader().find( StringSetEntity.class, setId, 5 );
132+
assertEquals( TestTools.makeSet( "A" ), rev5.getStrings() );
133+
134+
final StringSetEntity rev6 = getAuditReader().find( StringSetEntity.class, setId, 6 );
135+
assertEquals( TestTools.makeSet( "D" ), rev6.getStrings() );
136+
}
137+
138+
}

0 commit comments

Comments
 (0)