HHH-9186 - ORM wrongly assumes that an element of a set has a primary key

hibernate-core/src/main/java/org/hibernate/mapping/Set.java

@@ -68,17 +68,20 @@ public class Set extends Collection {
 				if ( selectable instanceof Column ) {
 					Column col = (Column) selectable;
 					if ( !col.isNullable() ) {
-						pk.addColumn(col);
+						pk.addColumn( col );
+					}
+					else {
+						return;
 					}
 				}
 			}
-			if ( pk.getColumnSpan()==getKey().getColumnSpan() ) { 
-				//for backward compatibility, allow a set with no not-null 
+			if ( pk.getColumnSpan() == getKey().getColumnSpan() ) {
+				//for backward compatibility, allow a set with no not-null
 				//element columns, using all columns in the row locater SQL
 				//TODO: create an implicit not null constraint on all cols?
 			}
 			else {
-				getCollectionTable().setPrimaryKey(pk);
+				getCollectionTable().setPrimaryKey( pk );
 			}
 		}
 		else {

hibernate-core/src/test/java/org/hibernate/test/compositeusertype/CompositeUserTypeTest.java

@@ -0,0 +1,72 @@
+/**
+ *
+ */
+package org.hibernate.test.compositeusertype;
+
+import java.util.Arrays;
+import java.util.HashSet;
+
+import org.hibernate.testing.TestForIssue;
+import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
+import org.junit.Test;
+
+import static org.hibernate.testing.transaction.TransactionUtil.doInHibernate;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+/**
+ * @author Felix Feisst (feisst dot felix at gmail dot com)
+ */
+public class CompositeUserTypeTest extends BaseCoreFunctionalTestCase {
+
+	@Override
+	protected String[] getMappings() {
+		return new String[] { "compositeusertype/TestEntity.hbm.xml" };
+	}
+
+	@Test
+	@TestForIssue(jiraKey = "HHH-9186")
+	public void testRemovalWithNullableFields() {
+		final Unit unit1 = Percent.INSTANCE;
+		final Unit unit2 = new Currency( "EUR" );
+		final Unit unit3 = new Currency( "USD" );
+
+		final Integer id = 1;
+
+		doInHibernate( this::sessionFactory, session -> {
+			TestEntity entity = new TestEntity();
+			entity.setId( id );
+
+			session.persist( entity );
+		} );
+
+		doInHibernate( this::sessionFactory, session -> {
+			TestEntity entity = session.find( TestEntity.class, id );
+			assertNotNull( "Expected an entity to be returned", entity );
+			assertTrue( "Expected no units", entity.getUnits().isEmpty() );
+
+			entity.getUnits().add( unit1 );
+			entity.getUnits().add( unit2 );
+			entity.getUnits().add( unit3 );
+		} );
+
+		doInHibernate( this::sessionFactory, session -> {
+			TestEntity entity = session.get( TestEntity.class, id );
+			assertNotNull( "Expected an entity to be returned", entity );
+			assertEquals(
+					"Unexpected units",
+					new HashSet<>( Arrays.asList( unit1, unit2, unit3 ) ),
+					entity.getUnits()
+			);
+
+			entity.getUnits().remove( unit2 );
+		} );
+
+		doInHibernate( this::sessionFactory, session -> {
+			TestEntity entity = session.get( TestEntity.class, id );
+			assertNotNull( "Expected an entity to be returned", entity );
+			assertEquals( "Unexpected units", new HashSet<>( Arrays.asList( unit1, unit3 ) ), entity.getUnits() );
+		} );
+	}
+}

hibernate-core/src/test/java/org/hibernate/test/compositeusertype/Currency.java

@@ -0,0 +1,35 @@
+/**
+ * 
+ */
+package org.hibernate.test.compositeusertype;
+
+/**
+ * @author Felix Feisst (feisst dot felix at gmail dot com)
+ */
+public class Currency implements Unit {
+
+	private final String name;
+
+	public Currency(final String name) {
+		this.name = name;
+	}
+
+	public String getName() {
+		return name;
+	}
+
+	@Override
+	public boolean equals(Object obj) {
+		boolean result = false;
+		if ( obj instanceof Currency ) {
+			result = getName().equals( ( (Currency) obj ).getName() );
+		}
+		return result;
+	}
+
+	@Override
+	public int hashCode() {
+		return getName().hashCode();
+	}
+
+}

hibernate-core/src/test/java/org/hibernate/test/compositeusertype/Percent.java

@@ -0,0 +1,17 @@
+/**
+ * 
+ */
+package org.hibernate.test.compositeusertype;
+
+/**
+ * @author Felix Feisst (feisst dot felix at gmail dot com)
+ */
+public enum Percent implements Unit {
+
+	INSTANCE;
+
+	public String getName() {
+		return "%";
+	}
+
+}

hibernate-core/src/test/java/org/hibernate/test/compositeusertype/TestEntity.hbm.xml

@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE hibernate-mapping PUBLIC "-//Hibernate/Hibernate Mapping DTD 3.0//EN" "http://www.hibernate.org/dtd/hibernate-mapping-3.0.dtd">
+
+<hibernate-mapping>
+  <class name="org.hibernate.test.compositeusertype.TestEntity"
+        table="TestTable">
+        <id name="id" column="ID" type="integer" />
+		
+		<set name="units">
+		  <key column="entity_id" />
+		  <element type="org.hibernate.test.compositeusertype.UnitCompositeUserType">
+		    <column name="unit_type" not-null="true" />
+            <column name="currency_name" not-null="false" />
+		  </element>
+		</set>
+    </class>
+</hibernate-mapping>

hibernate-core/src/test/java/org/hibernate/test/compositeusertype/TestEntity.java

@@ -0,0 +1,28 @@
+package org.hibernate.test.compositeusertype;
+
+import java.util.Set;
+
+/**
+ * @author Felix Feisst (feisst dot felix at gmail dot com)
+ */
+public class TestEntity {
+
+	private int id;
+	private Set<Unit> units;
+
+	public void setId(int id) {
+		this.id = id;
+	}
+
+	public int getId() {
+		return id;
+	}
+
+	public void setUnits(Set<Unit> units) {
+		this.units = units;
+	}
+
+	public Set<Unit> getUnits() {
+		return units;
+	}
+}

hibernate-core/src/test/java/org/hibernate/test/compositeusertype/Unit.java

@@ -0,0 +1,13 @@
+/**
+ * 
+ */
+package org.hibernate.test.compositeusertype;
+
+/**
+ * @author Felix Feisst (feisst dot felix at gmail dot com)
+ */
+public interface Unit {
+
+	String getName();
+
+}

hibernate-core/src/test/java/org/hibernate/test/compositeusertype/UnitCompositeUserType.java

@@ -0,0 +1,145 @@
+/**
+ * 
+ */
+package org.hibernate.test.compositeusertype;
+
+import java.io.Serializable;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.sql.Types;
+
+import org.hibernate.HibernateException;
+import org.hibernate.engine.spi.SessionImplementor;
+import org.hibernate.engine.spi.SharedSessionContractImplementor;
+import org.hibernate.type.StringType;
+import org.hibernate.type.Type;
+import org.hibernate.usertype.CompositeUserType;
+
+/**
+ * @author Felix Feisst (feisst dot felix at gmail dot com)
+ */
+public class UnitCompositeUserType implements CompositeUserType {
+
+	@Override
+	public String[] getPropertyNames() {
+		return new String[] { "unit", "currency_name" };
+	}
+
+	@Override
+	public Type[] getPropertyTypes() {
+		return new Type[] { StringType.INSTANCE, StringType.INSTANCE };
+	}
+
+	@Override
+	public Object getPropertyValue(Object component, int property) throws HibernateException {
+		Object result = null;
+		if ( component == Percent.INSTANCE ) {
+			if ( property == 0 ) {
+				result = "PERCENT";
+			}
+		}
+		else if ( component instanceof Currency ) {
+			if ( property == 0 ) {
+				result = "CURRENCY";
+			}
+			else if ( property == 1 ) {
+				Currency curr = (Currency) component;
+				result = curr.getName();
+			}
+		}
+		return result;
+	}
+
+	@Override
+	public void setPropertyValue(Object component, int property, Object value) throws HibernateException {
+		throw new UnsupportedOperationException( "Units are not mutable" );
+	}
+
+	@Override
+	public Class<?> returnedClass() {
+		return Unit.class;
+	}
+
+	@Override
+	public boolean equals(Object x, Object y) throws HibernateException {
+		return ( x == y ) || ( x != null && x.equals( y ) );
+	}
+
+	@Override
+	public int hashCode(Object x) throws HibernateException {
+		return x == null ? 0 : x.hashCode();
+	}
+
+	@Override
+	public Object nullSafeGet(ResultSet rs, String[] names, SharedSessionContractImplementor session, Object owner) throws HibernateException, SQLException {
+		Object result = null;
+		String unitType = rs.getString( names[0] );
+		String currencyName = rs.getString( names[1] );
+		if ( "PERCENT".equals( unitType ) ) {
+			result = Percent.INSTANCE;
+		}
+		else if ( "CURRENCY".equals( unitType ) ) {
+			result = new Currency( currencyName );
+		}
+		return result;
+	}
+
+	@Override
+	public void nullSafeSet(PreparedStatement st, Object value, int index, SharedSessionContractImplementor session) throws HibernateException, SQLException {
+		if ( value == null ) {
+			st.setNull( index, Types.VARCHAR );
+			st.setNull( index + 1, Types.VARCHAR );
+		}
+		else if ( value == Percent.INSTANCE ) {
+			st.setString( index, "PERCENT" );
+			st.setNull( index + 1, Types.VARCHAR );
+		}
+		else if ( value instanceof Currency ) {
+			st.setString( index, "CURRENCY" );
+			st.setString( index + 1, ( (Currency) value ).getName() );
+		}
+	}
+
+	@Override
+	public Object deepCopy(Object value) throws HibernateException {
+		return value;
+	}
+
+	@Override
+	public boolean isMutable() {
+		return false;
+	}
+
+	@Override
+	public Serializable disassemble(Object value, SharedSessionContractImplementor session) throws HibernateException {
+		final Serializable[] result = new Serializable[2];
+		if ( value == Percent.INSTANCE ) {
+			result[0] = "PERCENT";
+		}
+		else if ( value instanceof Currency ) {
+			result[0] = "CURRENCY";
+			result[1] = ( (Currency) value ).getName();
+		}
+		return result;
+	}
+
+	@Override
+	public Object assemble(Serializable cached, SharedSessionContractImplementor session, Object owner) throws HibernateException {
+		Object result = null;
+		Serializable[] fields = (Serializable[]) cached;
+		if ( "PERCENT".equals( fields[0] ) ) {
+			result = Percent.INSTANCE;
+		}
+		else if ( "CURRENCY".equals( fields[0] ) ) {
+			result = new Currency( (String) fields[1] );
+		}
+		return result;
+	}
+
+	@Override
+	public Object replace(Object original, Object target, SharedSessionContractImplementor session, Object owner) throws HibernateException {
+		return original;
+	}
+
+}