Fix ALF-2210 - incorrect version history (version label incremented twice, in certain cases)

git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@19897 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

source/java/org/alfresco/repo/rule/RuleServiceCoverageTest.java

@@ -82,6 +82,7 @@ import org.alfresco.service.cmr.rule.Rule;
 import org.alfresco.service.cmr.rule.RuleService;
 import org.alfresco.service.cmr.rule.RuleServiceException;
 import org.alfresco.service.cmr.rule.RuleType;
+import org.alfresco.service.cmr.version.VersionService;
 import org.alfresco.service.namespace.NamespaceService;
 import org.alfresco.service.namespace.QName;
 import org.alfresco.service.namespace.RegexQNamePattern;
@@ -119,6 +120,7 @@ public class RuleServiceCoverageTest extends TestCase
     private CopyService copyService;
     private AuthenticationComponent authenticationComponent;
     private FileFolderService fileFolderService;
+    private VersionService versionService;
     
     /**
      * Category related values
@@ -159,6 +161,7 @@ public class RuleServiceCoverageTest extends TestCase
         this.transformerRegistry = (ContentTransformerRegistry)applicationContext.getBean("contentTransformerRegistry");
         this.authenticationComponent = (AuthenticationComponent)applicationContext.getBean("authenticationComponent");
         this.fileFolderService = serviceRegistry.getFileFolderService();
+        this.versionService = serviceRegistry.getVersionService();
         
         //authenticationComponent.setCurrentUser(authenticationComponent.getSystemUserName());
         //authenticationComponent.setSystemUserAsCurrentUser();
@@ -336,6 +339,7 @@ public class RuleServiceCoverageTest extends TestCase
                 getContentProperties()).getChildRef();
         addContentToNode(newNodeRef);
         assertTrue(this.nodeService.hasAspect(newNodeRef, ContentModel.ASPECT_VERSIONABLE));
+        assertEquals(1, versionService.getVersionHistory(newNodeRef).getAllVersions().size());
         
         Map<String, Serializable> params2 = new HashMap<String, Serializable>(2);
         params2.put(AddFeaturesActionExecuter.PARAM_ASPECT_NAME, ApplicationModel.ASPECT_SIMPLE_WORKFLOW);
@@ -480,6 +484,7 @@ public class RuleServiceCoverageTest extends TestCase
                 getContentProperties()).getChildRef();
         addContentToNode(newNodeRef2);
         assertTrue(this.nodeService.hasAspect(newNodeRef2, ContentModel.ASPECT_VERSIONABLE));
+        assertEquals(1, versionService.getVersionHistory(newNodeRef2).getAllVersions().size());
         
     }
     
@@ -668,6 +673,7 @@ public class RuleServiceCoverageTest extends TestCase
             };
             NodeRef newNodeRef = transactionService.getRetryingTransactionHelper().doInTransaction(callback1);
             assertTrue(this.nodeService.hasAspect(newNodeRef, ContentModel.ASPECT_VERSIONABLE));
+            assertEquals(1, versionService.getVersionHistory(newNodeRef).getAllVersions().size());
             
             // Check rule does not get fired when the node has the incorrect category value
             RetryingTransactionCallback<NodeRef> callback3 = new RetryingTransactionCallback<NodeRef>()
@@ -1121,7 +1127,6 @@ public class RuleServiceCoverageTest extends TestCase
      *          condition:  no-condition()
      *          action:     checkin()
      */
-    @SuppressWarnings("unchecked")
     public void testCheckInAction()
     {
         Map<String, Serializable> params = new HashMap<String, Serializable>(1);
@@ -1211,6 +1216,7 @@ public class RuleServiceCoverageTest extends TestCase
                 getContentProperties()).getChildRef();
         addContentToNode(newNodeRef2);
         assertTrue(this.nodeService.hasAspect(newNodeRef2, ContentModel.ASPECT_VERSIONABLE));
+        assertEquals(1, versionService.getVersionHistory(newNodeRef2).getAllVersions().size());
     }
     
     /**
@@ -1429,6 +1435,7 @@ public class RuleServiceCoverageTest extends TestCase
                 QName.createQName(TEST_NAMESPACE, "children"),
                 ContentModel.TYPE_CONTAINER).getChildRef();
         assertTrue(this.nodeService.hasAspect(newNodeRef, ContentModel.ASPECT_VERSIONABLE));
+        assertEquals(1, versionService.getVersionHistory(newNodeRef).getAllVersions().size());
         
         // Create a content node
         NodeRef contentNodeRef = this.nodeService.createNode(
@@ -1439,6 +1446,26 @@ public class RuleServiceCoverageTest extends TestCase
         assertFalse(this.nodeService.hasAspect(contentNodeRef, ContentModel.ASPECT_VERSIONABLE));
         addContentToNode(contentNodeRef);
         assertTrue(this.nodeService.hasAspect(contentNodeRef, ContentModel.ASPECT_VERSIONABLE));
+        assertEquals(1, versionService.getVersionHistory(contentNodeRef).getAllVersions().size());
+        
+        // Create a content node in a single txn
+        RetryingTransactionCallback<NodeRef> callback = new RetryingTransactionCallback<NodeRef>()
+        {
+            public NodeRef execute() throws Throwable
+            {
+                NodeRef contentNodeRef = nodeService.createNode(
+                        nodeRef,
+                        ContentModel.ASSOC_CHILDREN,
+                        QName.createQName(TEST_NAMESPACE, "children"),
+                        ContentModel.TYPE_CONTENT).getChildRef();
+                addContentToNode(contentNodeRef);
+                return contentNodeRef;
+            }
+        };
+        NodeRef contentNodeRef2 = transactionService.getRetryingTransactionHelper().doInTransaction(callback);
+        
+        assertTrue(this.nodeService.hasAspect(contentNodeRef2, ContentModel.ASPECT_VERSIONABLE));
+        assertEquals(1, versionService.getVersionHistory(contentNodeRef2).getAllVersions().size());
         
         // Create a node to be moved
         NodeRef moveNode = this.nodeService.createNode(
@@ -1448,14 +1475,16 @@ public class RuleServiceCoverageTest extends TestCase
                 ContentModel.TYPE_CONTENT).getChildRef();
         addContentToNode(moveNode);
         assertFalse(this.nodeService.hasAspect(moveNode, ContentModel.ASPECT_VERSIONABLE));
+        
         this.nodeService.moveNode(
                 moveNode,
                 this.nodeRef,
                 ContentModel.ASSOC_CHILDREN,
                 QName.createQName(TEST_NAMESPACE, "children"));
         assertTrue(this.nodeService.hasAspect(moveNode, ContentModel.ASPECT_VERSIONABLE));
+        assertEquals(1, versionService.getVersionHistory(moveNode).getAllVersions().size());
         
-        // Enusre the rule type does not get fired when the node is updated
+        // Ensure the rule type does not get fired when the node is updated
         this.nodeService.removeAspect(contentNodeRef, ContentModel.ASPECT_VERSIONABLE);
         assertFalse(this.nodeService.hasAspect(contentNodeRef, ContentModel.ASPECT_VERSIONABLE)); 
         this.nodeService.setProperty(contentNodeRef, ContentModel.PROP_NAME, "name.txt");
@@ -1488,6 +1517,7 @@ public class RuleServiceCoverageTest extends TestCase
         // Update the non-content node
         this.nodeService.setProperty(newNodeRef, ContentModel.PROP_NAME, "testName");
         assertTrue(this.nodeService.hasAspect(newNodeRef, ContentModel.ASPECT_VERSIONABLE));
+        assertEquals(1, versionService.getVersionHistory(newNodeRef).getAllVersions().size());
         
         // Create a content node
         NodeRef contentNodeRef = this.nodeService.createNode(
@@ -1500,6 +1530,7 @@ public class RuleServiceCoverageTest extends TestCase
         assertFalse(this.nodeService.hasAspect(contentNodeRef, ContentModel.ASPECT_VERSIONABLE));
         addContentToNode(contentNodeRef);
         assertTrue(this.nodeService.hasAspect(contentNodeRef, ContentModel.ASPECT_VERSIONABLE));
+        assertEquals(1, versionService.getVersionHistory(contentNodeRef).getAllVersions().size());
         
         // Create a non content node, setting a property at the same time
         Map<QName, Serializable> props = new HashMap<QName, Serializable>(1);
@@ -1515,6 +1546,7 @@ public class RuleServiceCoverageTest extends TestCase
         assertFalse(this.nodeService.hasAspect(nodeRef2, ContentModel.ASPECT_VERSIONABLE));
         this.nodeService.setProperty(nodeRef2, ContentModel.PROP_NAME, "testName2");
         assertTrue(this.nodeService.hasAspect(nodeRef2, ContentModel.ASPECT_VERSIONABLE));
+        assertEquals(1, versionService.getVersionHistory(nodeRef2).getAllVersions().size());
         
         transactionService.getRetryingTransactionHelper().doInTransaction(new RetryingTransactionCallback<Object>()
         {
@@ -1575,6 +1607,7 @@ public class RuleServiceCoverageTest extends TestCase
                 ContentModel.ASSOC_CHILDREN,
                 QName.createQName(TEST_NAMESPACE, "children"));
         assertTrue(this.nodeService.hasAspect(newNodeRef, ContentModel.ASPECT_VERSIONABLE));
+        assertEquals(1, versionService.getVersionHistory(newNodeRef).getAllVersions().size());
         
         // Check the deletion of a node
         
@@ -1653,6 +1686,7 @@ public class RuleServiceCoverageTest extends TestCase
 			for (NodeRef ref : nodeRefs) 
 			{
 				assertTrue(this.nodeService.hasAspect(ref, ContentModel.ASPECT_VERSIONABLE));
+				assertEquals(1, versionService.getVersionHistory(ref).getAllVersions().size());
 			}
 	        
 	        System.out.println(sw.prettyPrint());

source/java/org/alfresco/repo/version/VersionableAspect.java

@@ -251,7 +251,8 @@ public class VersionableAspect implements ContentServicePolicies.OnContentUpdate
                     // Create the initial-version
                     Map<String, Serializable> versionProperties = new HashMap<String, Serializable>(1);
                     versionProperties.put(Version.PROP_DESCRIPTION, I18NUtil.getMessage(MSG_INITIAL_VERSION));
-                    this.versionService.createVersion(nodeRef, versionProperties);
+                    
+                    createVersionImpl(nodeRef, versionProperties);
                 }
             }
         }
@@ -296,7 +297,8 @@ public class VersionableAspect implements ContentServicePolicies.OnContentUpdate
                     // Create the auto-version
                     Map<String, Serializable> versionProperties = new HashMap<String, Serializable>(1);
                     versionProperties.put(Version.PROP_DESCRIPTION, I18NUtil.getMessage(MSG_AUTO_VERSION));
-                    this.versionService.createVersion(nodeRef, versionProperties);
+                    
+                    createVersionImpl(nodeRef, versionProperties);
                 }
             }
         }
@@ -382,17 +384,29 @@ public class VersionableAspect implements ContentServicePolicies.OnContentUpdate
                     // Create the auto-version
                     Map<String, Serializable> versionProperties = new HashMap<String, Serializable>(1);
                     versionProperties.put(Version.PROP_DESCRIPTION, I18NUtil.getMessage(MSG_AUTO_VERSION_PROPS));
-                    this.versionService.createVersion(nodeRef, versionProperties);
+                    
+                    createVersionImpl(nodeRef, versionProperties);
                 }
             }
         }
     }
     
+    private void createVersionImpl(NodeRef nodeRef, Map<String, Serializable> versionProperties)
+    {
+        recordCreateVersion(nodeRef, null);
+        this.versionService.createVersion(nodeRef, versionProperties);
+    }
+    
     /**
      * @see org.alfresco.repo.version.VersionServicePolicies.OnCreateVersionPolicy#onCreateVersion(org.alfresco.service.namespace.QName, org.alfresco.service.cmr.repository.NodeRef, java.util.Map, org.alfresco.repo.policy.PolicyScope)
      */
-    @SuppressWarnings("unchecked")
     public void afterCreateVersion(NodeRef versionableNode, Version version) 
+    {
+        recordCreateVersion(versionableNode, version);
+    }
+    
+    @SuppressWarnings("unchecked")
+    private void recordCreateVersion(NodeRef versionableNode, Version version) 
     {
         Map<NodeRef, NodeRef> versionedNodeRefs = (Map<NodeRef, NodeRef>)AlfrescoTransactionSupport.getResource(KEY_VERSIONED_NODEREFS);
         if (versionedNodeRefs == null)