Skip to content

HHH-13675 HHH-13651 (5.3) #3111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 7, 2020
Merged

HHH-13675 HHH-13651 (5.3) #3111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39ee25d
HHH-13651 Adding a regression test for the issue
Sanne Oct 24, 2019
6f873c5
HHH-13675 : Optimize PersistentBag.groupByEqualityHash()
dreis2211 Oct 17, 2019
bdd15c9
HHH-13651 NPE on flushing when ElementCollection field contains null …
yyona-panaya Oct 23, 2019
6a87dd1
HHH-13651 Applying hibernate codestyle to PersistentBag
yyona-panaya Oct 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 44 additions & 19 deletions hibernate-core/src/main/java/org/hibernate/collection/internal/PersistentBag.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.ListIterator;
import java.util.Map;
import java.util.stream.Collectors;

import org.hibernate.HibernateException;
import org.hibernate.engine.spi.SessionImplementor;
Expand Down Expand Up @@ -66,6 +67,7 @@ public PersistentBag(SharedSessionContractImplementor session) {
public PersistentBag(SessionImplementor session) {
this( (SharedSessionContractImplementor) session );
}

/**
* Constructs a PersistentBag
*
Expand Down Expand Up @@ -96,7 +98,7 @@ public PersistentBag(SharedSessionContractImplementor session, Collection coll)
* @param coll The base elements.
*
* @deprecated {@link #PersistentBag(SharedSessionContractImplementor, Collection)}
* should be used instead.
* should be used instead.
*/
@Deprecated
public PersistentBag(SessionImplementor session, Collection coll) {
Expand Down Expand Up @@ -129,7 +131,7 @@ public Object readFrom(ResultSet rs, CollectionPersister persister, CollectionAl
throws HibernateException, SQLException {
// note that if we load this collection from a cartesian product
// the multiplicity would be broken ... so use an idbag instead
final Object element = persister.readElement( rs, owner, descriptor.getSuffixedElementAliases(), getSession() ) ;
final Object element = persister.readElement( rs, owner, descriptor.getSuffixedElementAliases(), getSession() );
if ( element != null ) {
bag.add( element );
}
Expand All @@ -142,7 +144,7 @@ public void beforeInitialize(CollectionPersister persister, int anticipatedSize)
}

@Override
@SuppressWarnings( "unchecked" )
@SuppressWarnings("unchecked")
public boolean equalsSnapshot(CollectionPersister persister) throws HibernateException {
final Type elementType = persister.getElementType();
final List<Object> sn = (List<Object>) getSnapshot();
Expand Down Expand Up @@ -182,7 +184,8 @@ public boolean equalsSnapshot(CollectionPersister persister) throws HibernateExc
instance,
instancesBag,
elementType,
countOccurrences( instance, instancesSn, elementType ) ) ) {
countOccurrences( instance, instancesSn, elementType )
) ) {
return false;
}
}
Expand All @@ -196,7 +199,28 @@ public boolean equalsSnapshot(CollectionPersister persister) throws HibernateExc
* @return Map of "equality" hashCode to List of objects
*/
private Map<Integer, List<Object>> groupByEqualityHash(List<Object> searchedBag, Type elementType) {
return searchedBag.stream().collect( Collectors.groupingBy( elementType::getHashCode ) );
if ( searchedBag.isEmpty() ) {
return Collections.emptyMap();
}
Map<Integer, List<Object>> map = new HashMap<>();
for ( Object o : searchedBag ) {
map.computeIfAbsent( nullableHashCode( o, elementType ), k -> new ArrayList<>() ).add( o );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using LinkedList seems not a bad idea, btw. It would simulate the hashmap bucket implementation as much as possible and avoid ArrayList sizing dilemma.

}
return map;
}

/**
* @param o
* @param elementType
* @return the default elementType hashcode of the object o, or null if the object is null
*/
private Integer nullableHashCode(Object o, Type elementType) {
if ( o == null ) {
return null;
}
else {
return elementType.getHashCode( o );
}
}

@Override
Expand Down Expand Up @@ -248,7 +272,7 @@ public Serializable disassemble(CollectionPersister persister)
throws HibernateException {
final int length = bag.size();
final Serializable[] result = new Serializable[length];
for ( int i=0; i<length; i++ ) {
for ( int i = 0; i < length; i++ ) {
result[i] = persister.getElementType().disassemble( bag.get( i ), getSession(), null );
}
return result;
Expand Down Expand Up @@ -290,13 +314,13 @@ public Iterator getDeletes(CollectionPersister persister, boolean indexIsFormula
final ArrayList deletes = new ArrayList();
final List sn = (List) getSnapshot();
final Iterator olditer = sn.iterator();
int i=0;
int i = 0;
while ( olditer.hasNext() ) {
final Object old = olditer.next();
final Iterator newiter = bag.iterator();
boolean found = false;
if ( bag.size()>i && elementType.isSame( old, bag.get( i++ ) ) ) {
//a shortcut if its location didn't change!
if ( bag.size() > i && elementType.isSame( old, bag.get( i++ ) ) ) {
//a shortcut if its location didn't change!
found = true;
}
else {
Expand Down Expand Up @@ -352,7 +376,7 @@ public int size() {

@Override
public boolean isEmpty() {
return readSize() ? getCachedSize()==0 : bag.isEmpty();
return readSize() ? getCachedSize() == 0 : bag.isEmpty();
}

@Override
Expand Down Expand Up @@ -415,7 +439,7 @@ public boolean containsAll(Collection c) {
@Override
@SuppressWarnings("unchecked")
public boolean addAll(Collection values) {
if ( values.size()==0 ) {
if ( values.size() == 0 ) {
return false;
}
if ( !isOperationQueueEnabled() ) {
Expand All @@ -426,14 +450,14 @@ public boolean addAll(Collection values) {
for ( Object value : values ) {
queueOperation( new SimpleAdd( value ) );
}
return values.size()>0;
return values.size() > 0;
}
}

@Override
@SuppressWarnings("unchecked")
public boolean removeAll(Collection c) {
if ( c.size()>0 ) {
if ( c.size() > 0 ) {
initialize( true );
if ( bag.removeAll( c ) ) {
elementRemoved = true;
Expand Down Expand Up @@ -470,7 +494,7 @@ public void clear() {
}
else {
initialize( true );
if ( ! bag.isEmpty() ) {
if ( !bag.isEmpty() ) {
bag.clear();
dirty();
}
Expand All @@ -479,7 +503,7 @@ public void clear() {

@Override
public Object getIndex(Object entry, int i, CollectionPersister persister) {
throw new UnsupportedOperationException("Bags don't have indexes");
throw new UnsupportedOperationException( "Bags don't have indexes" );
}

@Override
Expand Down Expand Up @@ -592,7 +616,7 @@ public List subList(int start, int end) {

@Override
public boolean entryExists(Object entry, int i) {
return entry!=null;
return entry != null;
}

@Override
Expand All @@ -606,8 +630,9 @@ public String toString() {
* JVM instance comparison to do the equals.
* The semantic is broken not to have to initialize a
* collection for a simple equals() operation.
* @see java.lang.Object#equals(java.lang.Object)
*
* @see java.lang.Object#equals(java.lang.Object)
* <p>
* {@inheritDoc}
*/
@Override
Expand All @@ -633,7 +658,7 @@ public Object getAddedInstance() {

@Override
public Object getOrphan() {
throw new UnsupportedOperationException("queued clear cannot be used with orphan delete");
throw new UnsupportedOperationException( "queued clear cannot be used with orphan delete" );
}
}

Expand Down
33 changes: 32 additions & 1 deletion hibernate-core/src/test/java/org/hibernate/test/collection/bag/BagElementNullBasicTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import java.util.ArrayList;
import java.util.List;
import javax.persistence.CollectionTable;
import javax.persistence.Column;
import javax.persistence.ElementCollection;
import javax.persistence.Entity;
import javax.persistence.GeneratedValue;
Expand All @@ -17,6 +18,9 @@
import javax.persistence.OrderBy;
import javax.persistence.Table;

import org.hibernate.Session;
import org.hibernate.Transaction;
import org.hibernate.testing.TestForIssue;
import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
import org.junit.Test;

Expand All @@ -31,7 +35,8 @@ public class BagElementNullBasicTest extends BaseCoreFunctionalTestCase {
@Override
protected Class[] getAnnotatedClasses() {
return new Class[] {
AnEntity.class
AnEntity.class,
NullableElementsEntity.class
};
}

Expand Down Expand Up @@ -85,6 +90,19 @@ public void addNullValue() {
);
}

@Test
@TestForIssue(jiraKey = "HHH-13651")
public void addNullValueToNullableCollections() {
try (final Session s = sessionFactory().openSession()) {
final Transaction tx = s.beginTransaction();
NullableElementsEntity e = new NullableElementsEntity();
e.list.add( null );
s.persist( e );
s.flush();
tx.commit();
}
}

@Test
public void testUpdateNonNullValueToNull() {
int entityId = doInHibernate(
Expand Down Expand Up @@ -169,4 +187,17 @@ public static class AnEntity {
@OrderBy
private List<String> aCollection = new ArrayList<String>();
}

@Entity
@Table(name="NullableElementsEntity")
public static class NullableElementsEntity {
@Id
@GeneratedValue
private int id;

@ElementCollection
@CollectionTable(name="e_2_string", joinColumns=@JoinColumn(name="e_id"))
@Column(name="string_value", unique = false, nullable = true, insertable = true, updatable = true)
private List<String> list = new ArrayList<String>();
}
}