HHH-5967: code cleanup, extending test

hibernate-envers/src/main/java/org/hibernate/envers/strategy/ValidityAuditStrategy.java

@@ -61,8 +61,7 @@ public class ValidityAuditStrategy implements AuditStrategy {
             IdMapper idMapper = auditCfg.getEntCfg().get(entityName).getIdMapper();
             idMapper.addIdEqualsToQuery(qb.getRootParameters(), id, auditCfg.getAuditEntCfg().getOriginalIdPropName(), true);
 
-            // e.end_rev is null
+            addEndRevisionNulLRestriction(auditCfg, qb);
-            qb.getRootParameters().addWhere(auditCfg.getAuditEntCfg().getRevisionEndFieldName(), true, "is", "null", false);
 
             @SuppressWarnings({"unchecked"})
             List<Object> l = qb.toQuery(session).list();
@@ -91,12 +90,13 @@ public class ValidityAuditStrategy implements AuditStrategy {
             }
         }
 
-        // e.end_rev is null
+        addEndRevisionNulLRestriction(auditCfg, qb);
-        qb.getRootParameters().addWhere(auditCfg.getAuditEntCfg().getRevisionEndFieldName(), true, "is", "null", false);
 
         final List<Object> l = qb.toQuery(session).list();
 
-        // Update the last revision if there exists such a last revision
+        // Update the last revision if one exists.
+        // HHH-5967: with collections, the same element can be added and removed multiple times. So even if it's an
+        // ADD, we may need to update the last revision.
         if (l.size() > 0) {
             updateLastRevision(session, auditCfg, l, originalId, persistentCollectionChangeData.getEntityName(), revision);
         }
@@ -105,7 +105,12 @@ public class ValidityAuditStrategy implements AuditStrategy {
         session.save(persistentCollectionChangeData.getEntityName(), persistentCollectionChangeData.getData());
     }
 
-	public void addEntityAtRevisionRestriction(GlobalConfiguration globalCfg, QueryBuilder rootQueryBuilder,
+    private void addEndRevisionNulLRestriction(AuditConfiguration auditCfg, QueryBuilder qb) {
+        // e.end_rev is null
+        qb.getRootParameters().addWhere(auditCfg.getAuditEntCfg().getRevisionEndFieldName(), true, "is", "null", false);
+    }
+
+    public void addEntityAtRevisionRestriction(GlobalConfiguration globalCfg, QueryBuilder rootQueryBuilder,
 			String revisionProperty,String revisionEndProperty, boolean addAlias,
             MiddleIdData idData, String revisionPropertyPath, String originalIdPropertyName,
             String alias1, String alias2) {

hibernate-envers/src/test/java/org/hibernate/envers/test/integration/strategy/ValidityAuditStrategyManyToManyTest.java

@@ -1,6 +1,9 @@
 package org.hibernate.envers.test.integration.strategy;
 
+import java.util.Arrays;
 import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
 
 import javax.persistence.EntityManager;
 
@@ -8,9 +11,15 @@ import org.hibernate.ejb.Ejb3Configuration;
 import org.hibernate.envers.test.AbstractEntityTest;
 import org.hibernate.envers.test.entities.manytomany.SetOwnedEntity;
 import org.hibernate.envers.test.entities.manytomany.SetOwningEntity;
+import org.hibernate.envers.test.entities.manytomany.sametable.Child1Entity;
+import org.hibernate.envers.test.entities.manytomany.sametable.Child2Entity;
+import org.hibernate.envers.test.entities.manytomany.sametable.ParentEntity;
+import org.testng.Assert;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.Test;
 
+import static org.testng.Assert.assertEquals;
+
 /**
  * Tests the ValidityAuditStrategy on many-to-many Sets.
  * It was first introduced because of a bug when adding and removing the same element
@@ -42,7 +51,7 @@ public class ValidityAuditStrategyManyToManyTest extends AbstractEntityTest {
         final SetOwningEntity setOwningEntity = new SetOwningEntity(1, "parent");
         final SetOwnedEntity setOwnedEntity = new SetOwnedEntity(2, "child");
 
-        // Initial persist
+        // Revision 1: Initial persist
         em.getTransaction().begin();
 
         em.persist(setOwningEntity);
@@ -59,7 +68,7 @@ public class ValidityAuditStrategyManyToManyTest extends AbstractEntityTest {
     public void testMultipleAddAndRemove() {
         final EntityManager em = getEntityManager();
 
-        // add child for first time
+        // Revision 2: add child for first time
         em.getTransaction().begin();
 
         SetOwningEntity owningEntity = getEntityManager().find(SetOwningEntity.class, ing_id);
@@ -71,7 +80,7 @@ public class ValidityAuditStrategyManyToManyTest extends AbstractEntityTest {
         em.getTransaction().commit();
         em.clear();
 
-        // remove child
+        // Revision 3: remove child
         em.getTransaction().begin();
 
         owningEntity = getEntityManager().find(SetOwningEntity.class, ing_id);
@@ -82,7 +91,7 @@ public class ValidityAuditStrategyManyToManyTest extends AbstractEntityTest {
         em.getTransaction().commit();
         em.clear();
 
-        // add child again
+        // Revision 4: add child again
         em.getTransaction().begin();
 
         owningEntity = getEntityManager().find(SetOwningEntity.class, ing_id);
@@ -93,7 +102,7 @@ public class ValidityAuditStrategyManyToManyTest extends AbstractEntityTest {
         em.getTransaction().commit();
         em.clear();
 
-        // remove child again
+        // Revision 5: remove child again
         em.getTransaction().begin();
 
         owningEntity = getEntityManager().find(SetOwningEntity.class, ing_id);
@@ -106,8 +115,56 @@ public class ValidityAuditStrategyManyToManyTest extends AbstractEntityTest {
 
         // now the set owning entity list should be empty again
         owningEntity = getEntityManager().find(SetOwningEntity.class, ing_id);
-        assert owningEntity.getReferences().size() == 0;
+        assertEquals(owningEntity.getReferences().size(), 0);
     }
 
+    @Test(enabled = true, dependsOnMethods = "testMultipleAddAndRemove")
+	public void testRevisionsCounts() {
+		assertEquals(getAuditReader().getRevisions(SetOwningEntity.class, ing_id), Arrays.asList(1, 2, 3, 4, 5));
+		assertEquals(getAuditReader().getRevisions(SetOwnedEntity.class, ed_id), Arrays.asList(1, 2, 3, 4, 5));
+	}
 
+	@Test(enabled = true, dependsOnMethods = "testMultipleAddAndRemove")
+	public void testHistoryOfIng1() {
+		SetOwningEntity ver_empty = createOwningEntity();
+		SetOwningEntity ver_child = createOwningEntity(new SetOwnedEntity(ed_id, "child"));
+
+        assertEquals(getAuditReader().find(SetOwningEntity.class, ing_id, 1), ver_empty);
+        assertEquals(getAuditReader().find(SetOwningEntity.class, ing_id, 2), ver_child);
+        assertEquals(getAuditReader().find(SetOwningEntity.class, ing_id, 3), ver_empty);
+        assertEquals(getAuditReader().find(SetOwningEntity.class, ing_id, 4), ver_child);
+        assertEquals(getAuditReader().find(SetOwningEntity.class, ing_id, 5), ver_empty);
+	}
+
+    @Test(enabled = true, dependsOnMethods = "testMultipleAddAndRemove")
+	public void testHistoryOfEd1() {
+		SetOwnedEntity ver_empty = createOwnedEntity();
+		SetOwnedEntity ver_child = createOwnedEntity(new SetOwningEntity(ing_id, "parent"));
+
+        assertEquals(getAuditReader().find(SetOwnedEntity.class, ed_id, 1), ver_empty);
+        assertEquals(getAuditReader().find(SetOwnedEntity.class, ed_id, 2), ver_child);
+        assertEquals(getAuditReader().find(SetOwnedEntity.class, ed_id, 3), ver_empty);
+        assertEquals(getAuditReader().find(SetOwnedEntity.class, ed_id, 4), ver_child);
+        assertEquals(getAuditReader().find(SetOwnedEntity.class, ed_id, 5), ver_empty);
+	}
+
+    private SetOwningEntity createOwningEntity(SetOwnedEntity... owned) {
+        SetOwningEntity result = new SetOwningEntity(ing_id, "parent");
+        result.setReferences(new HashSet<SetOwnedEntity>());
+        for (SetOwnedEntity setOwnedEntity : owned) {
+            result.getReferences().add(setOwnedEntity);
+        }
+
+        return result;
+    }
+
+    private SetOwnedEntity createOwnedEntity(SetOwningEntity... owning) {
+        SetOwnedEntity result = new SetOwnedEntity(ed_id, "child");
+        result.setReferencing(new HashSet<SetOwningEntity>());
+        for (SetOwningEntity setOwningEntity : owning) {
+            result.getReferencing().add(setOwningEntity);
+        }
+
+        return result;
+    }
 }