HHH-8171 - Cleanup

Author: lukasz-antoniak

documentation/src/main/docbook/devguide/en-US/Envers.xml

@@ -323,7 +323,7 @@
                             SETORDINAL
                         </entry>
                         <entry>
-                            The name of the column used for storing the ordinal of the change in sets of embeddables. 
+                            Name of column used for storing ordinal of the change in sets of embeddable elements.
                         </entry>
                     </row>
                 </tbody>

hibernate-envers/src/main/java/org/hibernate/envers/configuration/AuditEntitiesConfiguration.java

@@ -115,9 +115,10 @@ public AuditEntitiesConfiguration(Properties properties, String revisionInfoEnti
         revisionNumberPath = originalIdPropName + "." + revisionFieldName + ".id";
         revisionPropBasePath = originalIdPropName + "." + revisionFieldName + ".";
 
-		embeddableSetOrdinalPropertyName = getProperty( properties,
-				"org.hibernate.envers.embeddable_set_ordinal_field_name",
-				"org.hibernate.envers.embeddable_set_ordinal_field_name", "SETORDINAL" );
+		embeddableSetOrdinalPropertyName = getProperty(
+				properties, "org.hibernate.envers.embeddable_set_ordinal_field_name",
+				"org.hibernate.envers.embeddable_set_ordinal_field_name", "SETORDINAL"
+		);
     }
 
     public String getOriginalIdPropName() {

hibernate-envers/src/main/java/org/hibernate/envers/configuration/metadata/CollectionMetadataGenerator.java

@@ -87,7 +87,6 @@
 import org.hibernate.mapping.OneToMany;
 import org.hibernate.mapping.PersistentClass;
 import org.hibernate.mapping.Property;
-import org.hibernate.mapping.SimpleValue;
 import org.hibernate.mapping.Table;
 import org.hibernate.mapping.Value;
 import org.hibernate.type.BagType;
@@ -506,20 +505,16 @@ private MiddleComponentData addValueToMiddleTable(Value value, Element xmlMappin
 				);
 			}
 
-			// Add an additional column holding a number to make each entry unique within the set,
-			// since embeddable properties can be null
-			if ( propertyValue.getCollectionType() instanceof SetType ) {
-				final String auditedEmbeddableSetOrdinalPropertyName = mainGenerator.getVerEntCfg()
-						.getEmbeddableSetOrdinalPropertyName();
-				final String auditedEmbeddableSetOrdinalPropertyType = "integer";
-
-				final SimpleValue simpleValue = new SimpleValue( component.getMappings(), component.getTable() );
-				simpleValue.setTypeName( auditedEmbeddableSetOrdinalPropertyType );
-
-				final Element idProperty = MetadataTools.addProperty( xmlMapping,
-						auditedEmbeddableSetOrdinalPropertyName, auditedEmbeddableSetOrdinalPropertyType, true, true );
-				MetadataTools.addColumn( idProperty, auditedEmbeddableSetOrdinalPropertyName, null, 0, 0, null, null,
-						null, false );
+			// Add an additional column holding a number to make each entry unique within the set.
+			// Embeddable properties may contain null values, so cannot be stored within composite primary key.
+			if ( propertyValue.isSet() ) {
+				final String setOrdinalPropertyName = mainGenerator.getVerEntCfg().getEmbeddableSetOrdinalPropertyName();
+				final Element ordinalProperty = MetadataTools.addProperty(
+						xmlMapping, setOrdinalPropertyName, "integer", true, true
+				);
+				MetadataTools.addColumn(
+						ordinalProperty, setOrdinalPropertyName, null, null, null, null, null, null, false
+				);
 			}
 
 			return new MiddleComponentData( componentMapper, 0 );

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

@@ -87,27 +87,16 @@ protected AbstractCollectionMapper(CommonCollectionMapperData commonCollectionMa
     protected abstract void mapToMapFromObject(SessionImplementor session, Map<String, Object> idData, Map<String, Object> data, Object changed);
 
 	/**
-	 * Creates a Map for the id.
-	 * 
-	 * <p>
-	 * The ordinal parameter represents the iteration ordinal of the current element, used to add a synthetic id when
-	 * dealing with embeddables since embeddable fields can't be contained within the primary key since they might be
-	 * nullable.
-	 * </p>
-	 * 
-	 * @param ordinal
-	 *            The element iteration ordinal.
-	 * 
-	 * @return A Map for holding the ID information.
+	 * Creates map for storing identifier data. Ordinal parameter guarantees uniqueness of primary key.
+	 * Composite primary key cannot contain embeddable properties since they might be nullable.
+	 * @param ordinal Iteration ordinal.
+	 * @return Map for holding identifier data.
 	 */
 	protected Map<String, Object> createIdMap(int ordinal) {
-		final HashMap<String, Object> idMap = new HashMap<String, Object>();
-
+		final Map<String, Object> idMap = new HashMap<String, Object>();
 		if ( ordinalInId ) {
-			idMap.put( this.commonCollectionMapperData.getVerEntCfg().getEmbeddableSetOrdinalPropertyName(),
-					Integer.valueOf( ordinal ) );
+			idMap.put( commonCollectionMapperData.getVerEntCfg().getEmbeddableSetOrdinalPropertyName(), ordinal );
 		}
-
 		return idMap;
 	}
 

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

@@ -41,8 +41,7 @@ public SortedSetCollectionMapper(CommonCollectionMapperData commonCollectionMapp
 			Class<? extends SortedSet> collectionClass, Class<? extends SortedSet> proxyClass,
 			MiddleComponentData elementComponentData, Comparator comparator, boolean ordinalInId,
 			boolean revisionTypeInId) {
-		super( commonCollectionMapperData, collectionClass, proxyClass, elementComponentData, ordinalInId,
-				revisionTypeInId );
+		super( commonCollectionMapperData, collectionClass, proxyClass, elementComponentData, ordinalInId, revisionTypeInId );
 		this.comparator = comparator;
 	}
 

hibernate-envers/src/test/java/org/hibernate/envers/test/integration/collection/embeddable/EmbeddableSet.java

@@ -24,7 +24,6 @@
 package org.hibernate.envers.test.integration.collection.embeddable;
 
 import java.util.Arrays;
-import java.util.Collections;
 import javax.persistence.EntityManager;
 
 import org.junit.Test;
@@ -65,7 +64,7 @@ public void initData() {
 
 		EmbeddableSetEntity ese1 = new EmbeddableSetEntity();
 
-		// Revision 1 (ese1: initially 2 elements)
+		// Revision 1 (ese1: initially two elements)
 		em.getTransaction().begin();
 		ese1.getComponentSet().add( c3_1 );
 		ese1.getComponentSet().add( c3_3 );
@@ -84,33 +83,33 @@ public void initData() {
 		ese1.getComponentSet().add( c3_2 );
 		em.getTransaction().commit();
 
-		// Revision 3 (ese1: adding one existing element)
+		// Revision (still 2) (ese1: adding one existing element)
 		em.getTransaction().begin();
 		ese1 = em.find( EmbeddableSetEntity.class, ese1.getId() );
 		ese1.getComponentSet().add( c3_1 );
 		em.getTransaction().commit();
 
-		// Revision 4 (ese1: removing one existing element)
+		// Revision 3 (ese1: removing one existing element)
 		em.getTransaction().begin();
 		ese1 = em.find( EmbeddableSetEntity.class, ese1.getId() );
 		ese1.getComponentSet().remove( c3_2 );
 		em.getTransaction().commit();
 
-		// Revision 5 (ese1: adding two elements)
+		// Revision 4 (ese1: adding two elements)
 		em.getTransaction().begin();
 		ese1 = em.find( EmbeddableSetEntity.class, ese1.getId() );
 		ese1.getComponentSet().add( c3_2 );
 		ese1.getComponentSet().add( c3_4 );
 		em.getTransaction().commit();
 
-		// Revision 6 (ese1: removing two elements)
+		// Revision 5 (ese1: removing two elements)
 		em.getTransaction().begin();
 		ese1 = em.find( EmbeddableSetEntity.class, ese1.getId() );
 		ese1.getComponentSet().remove( c3_2 );
 		ese1.getComponentSet().remove( c3_4 );
 		em.getTransaction().commit();
 
-		// Revision 7 (ese1: removing and adding two elements)
+		// Revision 6 (ese1: removing and adding two elements)
 		em.getTransaction().begin();
 		ese1 = em.find( EmbeddableSetEntity.class, ese1.getId() );
 		ese1.getComponentSet().remove( c3_1 );
@@ -119,14 +118,20 @@ public void initData() {
 		ese1.getComponentSet().add( c3_4 );
 		em.getTransaction().commit();
 
+		// Revision 7 (ese1: adding one element)
+		em.getTransaction().begin();
+		ese1 = em.find( EmbeddableSetEntity.class, ese1.getId() );
+		ese1.getComponentSet().add( c3_1 );
+		em.getTransaction().commit();
+
 		ese1_id = ese1.getId();
 
 		em.close();
 	}
 
 	@Test
 	public void testRevisionsCounts() {
-		assertEquals( Arrays.asList( 1, 2, 3, 4, 5, 6 ), getAuditReader().getRevisions( EmbeddableSetEntity.class, ese1_id ) );
+		assertEquals( Arrays.asList( 1, 2, 3, 4, 5, 6, 7 ), getAuditReader().getRevisions( EmbeddableSetEntity.class, ese1_id ) );
 	}
 
 	@Test
@@ -137,12 +142,14 @@ public void testHistoryOfEse1() {
 		EmbeddableSetEntity rev4 = getAuditReader().find( EmbeddableSetEntity.class, ese1_id, 4 );
 		EmbeddableSetEntity rev5 = getAuditReader().find( EmbeddableSetEntity.class, ese1_id, 5 );
 		EmbeddableSetEntity rev6 = getAuditReader().find( EmbeddableSetEntity.class, ese1_id, 6 );
+		EmbeddableSetEntity rev7 = getAuditReader().find( EmbeddableSetEntity.class, ese1_id, 7 );
 
 		assertEquals( TestTools.makeSet( c3_1, c3_3 ), rev1.getComponentSet() );
 		assertEquals( TestTools.makeSet( c3_1, c3_2, c3_3 ), rev2.getComponentSet() );
 		assertEquals( TestTools.makeSet( c3_1, c3_3 ), rev3.getComponentSet() );
 		assertEquals( TestTools.makeSet( c3_1, c3_2, c3_3, c3_4 ), rev4.getComponentSet() );
 		assertEquals( TestTools.makeSet( c3_1, c3_3 ), rev5.getComponentSet() );
 		assertEquals( TestTools.makeSet( c3_2, c3_4 ), rev6.getComponentSet() );
+		assertEquals( TestTools.makeSet( c3_2, c3_4, c3_1 ), rev7.getComponentSet() );
 	}
 }