Refactor of RenditionService internals.

RenditionNodeManager and calling code.


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

source/java/org/alfresco/repo/rendition/AllRenditionTests.java

@@ -19,6 +19,7 @@
 package org.alfresco.repo.rendition;
 
 import org.alfresco.repo.rendition.executer.AbstractRenderingEngineTest;
+import org.alfresco.repo.rendition.executer.HTMLRenderingEngineTest;
 import org.alfresco.repo.thumbnail.ThumbnailServiceImplParameterTest;
 import org.alfresco.repo.thumbnail.ThumbnailServiceImplTest;
 import org.junit.runner.RunWith;
@@ -39,7 +40,8 @@ import org.junit.runners.Suite;
         ThumbnailServiceImplTest.class,
         StandardRenditionLocationResolverTest.class,
         RenditionServiceIntegrationTest.class,
-        RenditionServicePermissionsTest.class
+        RenditionServicePermissionsTest.class,
+        HTMLRenderingEngineTest.class
 })
 public class AllRenditionTests
 {

source/java/org/alfresco/repo/rendition/RenditionNodeManager.java

@@ -19,8 +19,15 @@
 
 package org.alfresco.repo.rendition;
 
+import static org.alfresco.model.ContentModel.PROP_NODE_REF;
+import static org.alfresco.model.ContentModel.PROP_STORE_NAME;
+
 import java.io.Serializable;
+import java.util.Arrays;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
 
 import org.alfresco.model.ContentModel;
 import org.alfresco.model.RenditionModel;
@@ -31,89 +38,168 @@ import org.alfresco.service.cmr.rendition.RenditionServiceException;
 import org.alfresco.service.cmr.repository.ChildAssociationRef;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.repository.NodeService;
+import org.alfresco.service.namespace.NamespaceService;
 import org.alfresco.service.namespace.QName;
 import org.alfresco.service.namespace.QNamePattern;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 
 /**
  * This class is responsible for placing a rendition node in the correct
  * location given a temporary rendition, a source node, a rendition location and
  * optionally an old rendition. This manages the complex logic of deciding
- * whether to move and old rendition or orphan it and create a new one amongst
+ * whether to move an old rendition or orphan it and create a new one amongst
  * other things.
  * 
  * @author Nick Smith
- * 
  */
 public class RenditionNodeManager
 {
+    /** Logger */
+    private static Log logger = LogFactory.getLog(RenditionNodeManager.class);
+
+    private static final List<QName> unchangedProperties = Arrays.asList(PROP_NODE_REF, PROP_STORE_NAME);
+    private static final String LINE_BREAK = System.getProperty("line.separator", "\n");
+    
+
+    /**
+     * The source node being rendered.
+     */
     private final NodeRef sourceNode;
-    private final NodeRef oldRendition;
+    private final NodeRef tempRenditionNode;
     private final RenditionDefinition renditionDefinition;
     private final RenditionLocation location;
     private final NodeService nodeService;
+    private final RenditionService renditionService;
+    private final NodeRef oldRendition;
+    private ChildAssociationRef finalRenditionAssoc;
     
-    public RenditionNodeManager(NodeRef sourceNode, NodeRef oldRendition, RenditionLocation location,
+    /**
-                RenditionDefinition renditionDefinition, NodeService nodeService)
+     * 
+     * @param sourceNode the source node which is being rendered.
+     * @param tempRenditionNode the temporary rendition
+     * @param location the proposed location of the rendition node.
+     * @param renditionDefinition
+     * @param nodeService
+     * @param renditionService
+     */
+    public RenditionNodeManager(NodeRef sourceNode, NodeRef tempRenditionNode, RenditionLocation location,
+                RenditionDefinition renditionDefinition, NodeService nodeService, RenditionService renditionService)
     {
         this.sourceNode = sourceNode;
-        this.oldRendition = oldRendition;
+        this.tempRenditionNode = tempRenditionNode;
         this.location = location;
         this.renditionDefinition = renditionDefinition;
         this.nodeService = nodeService;
+        this.renditionService = renditionService;
+        
+        this.oldRendition = this.getOldRenditionIfExists(sourceNode, renditionDefinition);
+
+        if (logger.isDebugEnabled())
+        {
+            StringBuilder msg = new StringBuilder();
+            msg.append("Creating/updating rendition based on:").append(LINE_BREAK).append("    sourceNode: ").append(
+                        sourceNode).append(LINE_BREAK).append("    tempRendition: ").append(tempRenditionNode).append(
+                        LINE_BREAK).append("    parentNode: ").append(location.getParentRef()).append(LINE_BREAK).append(
+                        "    childName: ").append(location.getChildName()).append(LINE_BREAK).append(
+                        "    renditionDefinition.name: ").append(renditionDefinition.getRenditionName());
+            logger.debug(msg.toString());
+        }
     }
 
     /**
      * This method returns the {@link ChildAssociationRef} for the rendition node. In doing this
      * it may reuse an existing rendition node, move an existing rendition node or create a new rendition node
      * as appropriate.
-     * 
-     * @return the {@link ChildAssociationRef} of the rendition node.
      */
     public ChildAssociationRef findOrCreateRenditionNode()
     {
         QName renditionName = renditionDefinition.getRenditionName();
+        
         // If no rendition already exists create a new rendition node and
         // association.
         if (oldRendition == null)
         {
-            return getSpecifiedRenditionOrCreateNewRendition(renditionName);
+            if (logger.isDebugEnabled())
+            {
+                logger.debug("No old rendition was found.");
+            }
+            
+            finalRenditionAssoc = getSpecifiedRenditionOrCreateNewRendition(renditionName);
         }
-        // If a rendition exists and is already in the correct location then
+        else
-        // return that renditions primary parent association
-        if (renditionLocationMatches())
         {
-            return nodeService.getPrimaryParent(oldRendition);
+            // If a rendition exists and is already in the correct location then
+            // return that rendition's primary parent association
+            if (isOldRenditionInCorrectLocation())
+            {
+                finalRenditionAssoc = nodeService.getPrimaryParent(oldRendition);
+            }
+            else
+            {
+                // If the old rendition is in the wrong location and the 'orphan
+                // existing rendition' param is set to true or the RenditionLocation
+                // specifies a destination NodeRef then delete the old
+                // rendition association and create a new rendition node.
+                if (isOrphaningRequired())
+                {
+                    orphanOldRendition(renditionName);
+                    finalRenditionAssoc = getSpecifiedRenditionOrCreateNewRendition(renditionName);
+                }
+                
+                // If the old rendition is in the wrong place and the 'orphan existing
+                // rendition' param is not set to true then move the existing rendition
+                // to the correct location.
+                finalRenditionAssoc = moveOldRendition(renditionName);
+            }
         }
-        // If the old rendition is in the wrong location and the 'orphan
+        
-        // existing rendition' param is set to true or the RenditionLocation
+        return finalRenditionAssoc;
-        // specifies a destination NodeRef then delete the old
-        // rendition association and create a new rendition node.
-        if (orphanExistingRendition())
-        {
-            orphanRendition( renditionName);
-            return getSpecifiedRenditionOrCreateNewRendition(renditionName);
-        }
-        // If the old rendition is in the wrong place and the 'orphan existing
-        // rendition' param is not set to true then move the existing rendition
-        // to the correct location.
-        return moveRendition(renditionName);
     }
 
-    private ChildAssociationRef moveRendition(QName associationName)
+    /**
+     * This method moves the old rendition to the required location giving it the correct parent-assoc type and
+     * the specified association name.
+     * 
+     * @param associationName the name to put on the newly created association.
+     * @return the ChildAssociationRef of the moved nodeRef.
+     */
+    private ChildAssociationRef moveOldRendition(QName associationName)
     {
         NodeRef parent = location.getParentRef();
         QName assocType = sourceNode.equals(parent) ? RenditionModel.ASSOC_RENDITION : ContentModel.ASSOC_CONTAINS;
-        return nodeService.moveNode(oldRendition, parent, assocType, associationName);
+        ChildAssociationRef result = nodeService.moveNode(oldRendition, parent, assocType, associationName);
+        
+        if (logger.isDebugEnabled())
+        {
+            logger.debug("The old rendition was moved to " + result);
+        }
+        
+        return result;
     }
 
-    private void orphanRendition(QNamePattern renditionName)
+    /**
+     * This method performs the 'orphaning' of the oldRendition. It removes the rendition aspect(s) and removes
+     * the child-association linking the old rendition to its source node. The old rendition node is not deleted.
+     * 
+     * @param renditionName the name of the rendition.
+     * @throws RenditionServiceException if there was not exactly one parent assoc from the oldRendition having the specified renditionName
+     *                                   or if the matching parent assoc was not to the correct source node.
+     */
+    private void orphanOldRendition(QNamePattern renditionName)
     {
+        // Get all parent assocs from the old rendition of the specified renditionName.
         List<ChildAssociationRef> parents = nodeService.getParentAssocs(oldRendition, RenditionModel.ASSOC_RENDITION, renditionName);
-        if(parents.size() ==1)
+        // There should only be one matching assoc.
+        if(parents.size() == 1)
         {
             ChildAssociationRef parentAssoc = parents.get(0);
             if(parentAssoc.getParentRef().equals(sourceNode))
             {
+                if (logger.isDebugEnabled())
+                {
+                    logger.debug("Orphaning old rendition node " + oldRendition);
+                }
                 nodeService.removeAspect(oldRendition, RenditionModel.ASPECT_HIDDEN_RENDITION);
                 nodeService.removeAspect(oldRendition, RenditionModel.ASPECT_VISIBLE_RENDITION);
                 nodeService.removeChildAssociation(parentAssoc);
@@ -123,50 +209,107 @@ public class RenditionNodeManager
         String msg = "Node: " + oldRendition 
             + " is not a rendition of type: " + renditionName 
             + " for source node: " + sourceNode;
+        if (logger.isDebugEnabled())
+        {
+            logger.debug(msg);
+        }
         throw new RenditionServiceException(msg);
     }
 
-    private boolean orphanExistingRendition()
+    /**
+     * This method determines whether or not orphaning of the old rendition is required.
+     * 
+     * @return <code>true</code> if orphaning is required, else <code>false</code>.
+     */
+    private boolean isOrphaningRequired()
     {
+        boolean result;
+        // Orphaning is required if the old rendition is in the wrong location and the 'orphan
+        // existing rendition' param is set to true or the RenditionLocation specifies a destination NodeRef.
         if (location.getChildRef() != null)
-            return true;
+            result = true;
         else
-            return AbstractRenderingEngine.getParamWithDefault(RenditionService.PARAM_ORPHAN_EXISTING_RENDITION,
+            result = AbstractRenderingEngine.getParamWithDefault(RenditionService.PARAM_ORPHAN_EXISTING_RENDITION,
                         Boolean.FALSE, renditionDefinition);
+        
+        if (logger.isDebugEnabled())
+        {
+            StringBuilder msg = new StringBuilder();
+            msg.append("The old rendition does ");
+            if (result == false)
+            {
+                msg.append("not ");
+            }
+            msg.append("require orphaning.");
+            logger.debug(msg.toString());
+        }
+        return result;
     }
 
-    private boolean renditionLocationMatches()
+    /**
+     * This method determines whether or not the old rendition is already in the correct location.
+     */
+    private boolean isOldRenditionInCorrectLocation()
     {
+        boolean result;
         NodeRef destination = location.getChildRef();
         if (destination != null)
         {
-            return destination.equals(oldRendition);
+            result = destination.equals(oldRendition);
         }
-        ChildAssociationRef oldParentAssoc = nodeService.getPrimaryParent(oldRendition);
+        else
-        NodeRef oldParent = oldParentAssoc.getParentRef();
-        if (oldParent.equals(location.getParentRef()))
         {
-            String childName = location.getChildName();
+            ChildAssociationRef oldParentAssoc = nodeService.getPrimaryParent(oldRendition);
-            if (childName == null)
+            NodeRef oldParent = oldParentAssoc.getParentRef();
-                return true;
+            if (oldParent.equals(location.getParentRef()))
-            else
             {
-                Serializable oldName = nodeService.getProperty(oldRendition, ContentModel.PROP_NAME);
+                String childName = location.getChildName();
-                return childName.equals(oldName);
+                if (childName == null)
+                    result = true;
+                else
+                {
+                    Serializable oldName = nodeService.getProperty(oldRendition, ContentModel.PROP_NAME);
+                    result = childName.equals(oldName);
+                }
             }
+            result = false;
         }
-        return false;
+        
+        if (logger.isDebugEnabled())
+        {
+            StringBuilder msg = new StringBuilder();
+            msg.append("The old rendition was ");
+            if (result == false)
+            {
+                msg.append("not ");
+            }
+            msg.append("in the correct location");
+            logger.debug(msg.toString());
+        }
+        
+        return result;
     }
 
     private ChildAssociationRef getSpecifiedRenditionOrCreateNewRendition(QName renditionName)
     {
+        ChildAssociationRef result;
         NodeRef destination = location.getChildRef();
         if (destination != null)
-            return nodeService.getPrimaryParent(destination);
+            result = nodeService.getPrimaryParent(destination);
         else
-            return createNewRendition(renditionName);
+            result = createNewRendition(renditionName);
+        
+        return result;
     }
 
+    /**
+     * This method creates a new rendition node. If the source node for this rendition is not
+     * the primary parent of the newly created rendition node, the rendition node is added as
+     * a child of the source node.
+     * 
+     * @param renditionName
+     * @return the primary parent association of the newly created rendition node.
+     */
     private ChildAssociationRef createNewRendition(QName renditionName)
     {
         NodeRef parentRef = location.getParentRef();
@@ -174,17 +317,92 @@ public class RenditionNodeManager
         QName renditionType = RenditionModel.ASSOC_RENDITION;
         QName assocTypeQName = parentIsSource ? renditionType : ContentModel.ASSOC_CONTAINS;
         QName nodeTypeQName = ContentModel.TYPE_CONTENT;
+
         ChildAssociationRef primaryAssoc = nodeService.createNode(parentRef, assocTypeQName, renditionName,
                     nodeTypeQName);
 
+        if (logger.isDebugEnabled())
+        {
+            StringBuilder msg = new StringBuilder();
+            msg.append("Created new rendition node ").append(primaryAssoc);
+            logger.debug(msg.toString());
+        }
+
         // If the new rendition is not directly under the source node then add
         // the rendition association.
         if (parentIsSource == false)
         {
             NodeRef rendition = primaryAssoc.getChildRef();
-            nodeService.addChild(sourceNode, rendition, renditionType, renditionName);
+            ChildAssociationRef newChild = nodeService.addChild(sourceNode, rendition, renditionType, renditionName);
+
+            if (logger.isDebugEnabled())
+            {
+                StringBuilder msg = new StringBuilder();
+                msg.append("Added new rendition node as child of source node ").append(newChild);
+                logger.debug(msg.toString());
+            }
         }
         return primaryAssoc;
     }
 
+    /**
+     * This method returns the rendition on the given sourceNode with the given renditionDefinition, if such
+     * a rendition exists.
+     * 
+     * @param sourceNode
+     * @param renditionDefinition
+     * @return the rendition node if one exists, else null.
+     */
+    private NodeRef getOldRenditionIfExists(NodeRef sourceNode, RenditionDefinition renditionDefinition)
+    {
+        QName renditionName=renditionDefinition.getRenditionName();
+        ChildAssociationRef renditionAssoc = renditionService.getRenditionByName(sourceNode, renditionName);
+        
+        NodeRef result = (renditionAssoc == null) ? null : renditionAssoc.getChildRef();
+        if (logger.isDebugEnabled())
+        {
+            StringBuilder msg = new StringBuilder();
+            msg.append("Existing rendition with name ")
+               .append(renditionName)
+               .append(": ")
+               .append(result);
+            logger.debug(msg.toString());
+        }
+        
+        return result;
+    }
+    
+    /**
+     * This method copies properties from the temporary rendition node onto the targetNode. It also sets the node type.
+     * {@link #unchangedProperties Some properties} are not copied.
+     */
+    public void transferNodeProperties()
+    {
+        NodeRef targetNode = finalRenditionAssoc.getChildRef();
+        if (logger.isDebugEnabled())
+        {
+            StringBuilder msg = new StringBuilder();
+            msg.append("Transferring some properties from ").append(tempRenditionNode).append(" to ").append(targetNode);
+            logger.debug(msg.toString());
+        }
+
+        // Copy the type from the temporary rendition to the real rendition
+        QName type = nodeService.getType(tempRenditionNode);
+        nodeService.setType(targetNode, type);
+        
+        // Copy over all regular properties from the temporary rendition
+        Map<QName, Serializable> newProps = new HashMap<QName, Serializable>();
+        for(Entry<QName,Serializable> entry : nodeService.getProperties(tempRenditionNode).entrySet())
+        {
+            QName propKey = entry.getKey();
+            if(unchangedProperties.contains(propKey) ||
+               NamespaceService.SYSTEM_MODEL_1_0_URI.equals(propKey.getNamespaceURI()))
+            {
+                // These shouldn't be copied over
+                continue;
+            }
+            newProps.put(propKey, entry.getValue());
+        }
+        nodeService.setProperties(targetNode, newProps);
+    }
 }

source/java/org/alfresco/repo/rendition/RenditionNodeManagerTest.java

@@ -19,12 +19,17 @@
 
 package org.alfresco.repo.rendition;
 
-import static org.mockito.Mockito.*;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import junit.framework.TestCase;
 
 import org.alfresco.model.ContentModel;
 import org.alfresco.model.RenditionModel;
-import org.alfresco.repo.security.authentication.AuthenticationUtil;
 import org.alfresco.service.cmr.rendition.RenditionDefinition;
+import org.alfresco.service.cmr.rendition.RenditionService;
 import org.alfresco.service.cmr.repository.ChildAssociationRef;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.repository.NodeService;
@@ -32,16 +37,13 @@ import org.alfresco.service.cmr.security.AuthenticationService;
 import org.alfresco.service.namespace.NamespaceService;
 import org.alfresco.service.namespace.QName;
 
-import com.sun.tools.xjc.reader.gbind.SourceNode;
-
-import junit.framework.TestCase;
-
 /**
  * @author Nick Smith
  */
 public class RenditionNodeManagerTest extends TestCase
 {
     private final NodeService nodeService = mock(NodeService.class);
+    private final RenditionService renditionService = mock(RenditionService.class);
     
     private final NodeRef source = new NodeRef("http://test/sourceId");
     private final NodeRef destination = new NodeRef("http://test/destinationId");
@@ -57,7 +59,7 @@ public class RenditionNodeManagerTest extends TestCase
         ChildAssociationRef parentAssoc = makeAssoc(source, destination, true);
         when(nodeService.getPrimaryParent(any(NodeRef.class))).thenReturn(parentAssoc);
         RenditionLocation location = new RenditionLocationImpl(source, destination, "destinationName");
-        RenditionNodeManager manager = new RenditionNodeManager(source, null, location, definition, nodeService);
+        RenditionNodeManager manager = new RenditionNodeManager(source, null, location, definition, nodeService, renditionService);
         ChildAssociationRef result = manager.findOrCreateRenditionNode();
         assertEquals(parentAssoc, result);
     }
@@ -72,7 +74,7 @@ public class RenditionNodeManagerTest extends TestCase
             .thenReturn(parentAssoc);
         
         RenditionLocation location = new RenditionLocationImpl(source, null, renditionName.getLocalName());
-        RenditionNodeManager manager = new RenditionNodeManager(source, null, location, definition, nodeService);
+        RenditionNodeManager manager = new RenditionNodeManager(source, null, location, definition, nodeService, renditionService);
         ChildAssociationRef result = manager.findOrCreateRenditionNode();
         assertEquals(parentAssoc, result);
     }
@@ -88,7 +90,7 @@ public class RenditionNodeManagerTest extends TestCase
             .thenReturn(parentAssoc);
         
         RenditionLocation location = new RenditionLocationImpl(parent, null, renditionName.getLocalName());
-        RenditionNodeManager manager = new RenditionNodeManager(source, null, location, definition, nodeService);
+        RenditionNodeManager manager = new RenditionNodeManager(source, null, location, definition, nodeService, renditionService);
         ChildAssociationRef result = manager.findOrCreateRenditionNode();
         assertEquals(parentAssoc, result);
         // Check the rendition association is created.
@@ -101,7 +103,7 @@ public class RenditionNodeManagerTest extends TestCase
     public void testHasOldRenditionMatchesSpecifiedDestinationNode()
     {
         RenditionLocation location = new RenditionLocationImpl(source, oldRendition, renditionName.getLocalName());
-        RenditionNodeManager manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService);
+        RenditionNodeManager manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService, renditionService);
         manager.findOrCreateRenditionNode();
         verify(nodeService).getPrimaryParent(oldRendition);
     }
@@ -116,7 +118,7 @@ public class RenditionNodeManagerTest extends TestCase
         when(nodeService.getPrimaryParent(oldRendition)).thenReturn(parentAssoc);
   
         RenditionLocation location = new RenditionLocationImpl(parent, null, null);
-        RenditionNodeManager manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService);
+        RenditionNodeManager manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService, renditionService);
         ChildAssociationRef result = manager.findOrCreateRenditionNode();
         assertEquals(parentAssoc, result);
         verify(nodeService, times(2)).getPrimaryParent(oldRendition);
@@ -135,7 +137,7 @@ public class RenditionNodeManagerTest extends TestCase
             .thenReturn(rendName);
 
         RenditionLocationImpl location = new RenditionLocationImpl(parent, null, rendName);
-        RenditionNodeManager manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService);
+        RenditionNodeManager manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService, renditionService);
         ChildAssociationRef result = manager.findOrCreateRenditionNode();
         assertEquals(parentAssoc, result);
         verify(nodeService, times(2)).getPrimaryParent(oldRendition);
@@ -154,7 +156,7 @@ public class RenditionNodeManagerTest extends TestCase
         when(nodeService.moveNode(oldRendition, parent, ContentModel.ASSOC_CONTAINS, renditionName))
             .thenReturn(parentAssoc);
         RenditionLocationImpl location = new RenditionLocationImpl(parent, null, null);
-        RenditionNodeManager manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService);
+        RenditionNodeManager manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService, renditionService);
         ChildAssociationRef result = manager.findOrCreateRenditionNode();
         assertEquals(parentAssoc, result);
         verify(nodeService).moveNode(oldRendition, parent, ContentModel.ASSOC_CONTAINS, renditionName);
@@ -164,7 +166,7 @@ public class RenditionNodeManagerTest extends TestCase
         when(nodeService.moveNode(oldRendition, source, RenditionModel.ASSOC_RENDITION, renditionName))
             .thenReturn(sourceAssoc);
         location = new RenditionLocationImpl(source, null, null);
-        manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService);
+        manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService, renditionService);
         result = manager.findOrCreateRenditionNode();
         assertEquals(sourceAssoc, result);
         verify(nodeService).moveNode(oldRendition, source, RenditionModel.ASSOC_RENDITION, renditionName);
@@ -178,7 +180,7 @@ public class RenditionNodeManagerTest extends TestCase
         when(nodeService.moveNode(oldRendition, newParent, ContentModel.ASSOC_CONTAINS, renditionName))
             .thenReturn(newParentAssoc);
         location = new RenditionLocationImpl(newParent, null, null);
-        manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService);
+        manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService, renditionService);
         result = manager.findOrCreateRenditionNode();
         assertEquals(newParentAssoc, result);
         verify(nodeService).moveNode(oldRendition, newParent, ContentModel.ASSOC_CONTAINS, renditionName);
@@ -200,7 +202,7 @@ public class RenditionNodeManagerTest extends TestCase
         
         String newName = "newName";
         RenditionLocationImpl location = new RenditionLocationImpl(parent, null, newName);
-        RenditionNodeManager manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService);
+        RenditionNodeManager manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService, renditionService);
         ChildAssociationRef result = manager.findOrCreateRenditionNode();
         assertEquals(parentAssoc, result);
         verify(nodeService).moveNode(oldRendition, parent, ContentModel.ASSOC_CONTAINS, renditionName);

source/java/org/alfresco/repo/rendition/executer/AbstractRenderingEngine.java

@@ -18,12 +18,6 @@
  */
 package org.alfresco.repo.rendition.executer;
 
-import static org.alfresco.model.ContentModel.PROP_NODE_DBID;
-import static org.alfresco.model.ContentModel.PROP_NODE_REF;
-import static org.alfresco.model.ContentModel.PROP_NODE_UUID;
-import static org.alfresco.model.ContentModel.PROP_STORE_IDENTIFIER;
-import static org.alfresco.model.ContentModel.PROP_STORE_NAME;
-import static org.alfresco.model.ContentModel.PROP_STORE_PROTOCOL;
 import static org.alfresco.service.cmr.rendition.RenditionService.PARAM_DESTINATION_PATH_TEMPLATE;
 import static org.alfresco.service.cmr.rendition.RenditionService.PARAM_IS_COMPONENT_RENDITION;
 import static org.alfresco.service.cmr.rendition.RenditionService.PARAM_ORPHAN_EXISTING_RENDITION;
@@ -31,7 +25,6 @@ import static org.alfresco.service.cmr.rendition.RenditionService.PARAM_RENDITIO
 
 import java.io.Serializable;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
@@ -88,7 +81,6 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
 
     /** Logger */
     private static Log logger = LogFactory.getLog(AbstractRenderingEngine.class);
-    private static final String LINE_BREAK = System.getProperty("line.separator", "\n");
 
     protected static final String CONTENT_READER_NOT_FOUND_MESSAGE = "Cannot find Content Reader for document. Operation can't be performed";
     private static final String DEFAULT_RUN_AS_NAME = AuthenticationUtil.getSystemUserName();
@@ -194,8 +186,6 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
      */
     public static final String PARAM_ENCODING = "encoding";
 
-    private static final List<QName> unchangedProperties = Arrays.asList(PROP_NODE_DBID, PROP_NODE_REF, PROP_NODE_UUID,
-            PROP_STORE_IDENTIFIER, PROP_STORE_NAME, PROP_STORE_PROTOCOL);
 	/**
 	 * Default {@link NodeLocator} simply returns the source node.
 	 */
@@ -380,8 +370,11 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
     @Override
     protected void executeImpl(final Action action, final NodeRef sourceNode)
     {
-    	final RenditionDefinition renditionDef = (RenditionDefinition)action;
+    	executeImpl( (RenditionDefinition)action, sourceNode );
+    }
     	
+    protected void executeImpl(final RenditionDefinition renditionDef, final NodeRef sourceNode)
+    {
         if (logger.isDebugEnabled())
         {
             StringBuilder msg = new StringBuilder();
@@ -402,7 +395,7 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
             logger.debug(msg.toString());
         }
 
-        Serializable runAsParam = action.getParameterValue(AbstractRenderingEngine.PARAM_RUN_AS);
+        Serializable runAsParam = renditionDef.getParameterValue(AbstractRenderingEngine.PARAM_RUN_AS);
         String runAsName = runAsParam == null ? DEFAULT_RUN_AS_NAME : (String) runAsParam;
 
         // Renditions should all be created by system by default.
@@ -420,19 +413,34 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
             	ChildAssociationRef result = null;
             	try
             	{
-					boolean isComponentRendition = isComponentRendition(action);
+            	    // Check whether this rendition is a component of a larger CompositeRendition
+					boolean isComponentRendition = isComponentRendition(renditionDef);
 					if (isComponentRendition == false)
 					{
-						preRendition(renditionDef, sourceNode);
+					    // Request that the rendition is initially created
+					    //  as a child of the source node
+				        setTemporaryRenditionProperties(sourceNode, renditionDef);					    
+					    
+					    // Add renditioned aspect to the source node
+				        tagSourceNodeAsRenditioned(renditionDef, sourceNode);
 					}
 
-					executeRenditionImpl(action, sourceNode);
+				    // Have the concrete implementation do the actual rendition
-					result = (ChildAssociationRef)action.getParameterValue(PARAM_RESULT);
+					executeRenditionImpl(renditionDef, sourceNode);
 
-					if (isComponentRendition == false)
+					// 
-					{
+                    if (isComponentRendition == false)
-						postRendition(renditionDef, sourceNode, result);
+                    {
-					}
+                        // Currently the rendition is on a temporary node, which may
+                        //  have the wrong name on it, and for path based renditions is
+                        //  in the wrong place
+                        // So, have the correct node created, and switch everything to use it
+                        switchToFinalRenditionNode(renditionDef, sourceNode);
+                    }
+					
+					// Grab a link to the rendition node - it's been saved as a parameter for us
+                    // (Wait until now to fetch in case it was moved)
+					result = (ChildAssociationRef)renditionDef.getParameterValue(PARAM_RESULT);
 				} catch (Throwable t)
 	            {
 	                notifyCallbackOfException(renditionDef, t);
@@ -447,6 +455,16 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
         }, runAsName);
     }
 
+    /**
+     * Is this a standalone rendition, or is it a sub-component of
+     *  a composite rendition?
+     * This is false for standalone renditions, AND ALSO false for
+     *  the main part of a composite rendition.
+     * This only returns true if we're currently processing a
+     *  component of a composite rendition.
+     * @param action
+     * @return
+     */
 	private boolean isComponentRendition(Action action) {
 		Serializable s = action.getParameterValue(PARAM_IS_COMPONENT_RENDITION);
 		boolean result = s == null ? false : (Boolean)s;
@@ -469,10 +487,7 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
         checkSourceNodeExists(sourceNode);
 
         ChildAssociationRef renditionAssoc = createRenditionNodeAssoc(sourceNode, renditionDefinition);
-        if (logger.isDebugEnabled())
+
-        {
-            logger.debug("Created rendition assoc: " + renditionAssoc);
-        }
         QName targetContentProp = getRenditionContentProperty(renditionDefinition);
         NodeRef destinationNode = renditionAssoc.getChildRef();
         RenderingContext context = new RenderingContext(sourceNode,//
@@ -640,6 +655,10 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
         try
         {
             childAssoc = nodeService.createNode(parentNode, assocType, assocName, nodeType, nodeProps);
+            if (logger.isDebugEnabled())
+            {
+                logger.debug("Created node " + childAssoc);
+            }
         }
         finally
         {
@@ -822,14 +841,10 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
     }
     
     
-    
+    protected void tagSourceNodeAsRenditioned(final RenditionDefinition renditionDef, final NodeRef actionedUponNodeRef)
-    
-    protected void preRendition(final RenditionDefinition renditionDef, final NodeRef actionedUponNodeRef)
     {
-        setTemporaryRenditionProperties(actionedUponNodeRef, renditionDef);
-
         // Adds the 'Renditioned' aspect to the source node if it
-        // doesn't exist.
+        // doesn't already have it.
         if (!nodeService.hasAspect(actionedUponNodeRef, RenditionModel.ASPECT_RENDITIONED))
         {
             // Ensure we do not update the 'modifier' due to thumbnail addition
@@ -845,8 +860,9 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
         }
     }
 
-    protected void postRendition(final RenditionDefinition renditionDef, final NodeRef actionedUponNodeRef, ChildAssociationRef tempRendAssoc)
+    protected void switchToFinalRenditionNode(final RenditionDefinition renditionDef, final NodeRef actionedUponNodeRef)
     {
+        ChildAssociationRef tempRendAssoc = (ChildAssociationRef)renditionDef.getParameterValue(PARAM_RESULT);
         ChildAssociationRef result = createOrUpdateRendition(actionedUponNodeRef, tempRendAssoc, renditionDef);
         renditionDef.setParameterValue(PARAM_RESULT, result);
     }
@@ -905,43 +921,42 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
         definition.setRenditionAssociationType(temporaryRenditionLinkType);
     }
 
+    /**
+     * 
+     * @param sourceNode The node that has been rendered
+     * @param tempRendition The relationship between the node and its rendition
+     * @param renditionDefinition The definition of the rendition that has just been performed.
+     *                            In the case of a composite rendition, this parameter refers
+     *                            to that CompositeRendition and not to any of its component renditions.
+     * @return
+     */
     private ChildAssociationRef createOrUpdateRendition(NodeRef sourceNode, ChildAssociationRef tempRendition,
                 RenditionDefinition renditionDefinition)
     {
         NodeRef tempRenditionNode = tempRendition.getChildRef();
         RenditionLocation renditionLocation = resolveRenditionLocation(sourceNode, renditionDefinition, tempRenditionNode);
         QName renditionQName = renditionDefinition.getRenditionName();
-        if (logger.isDebugEnabled())
-        {
-            StringBuilder msg = new StringBuilder();
-            msg.append("Creating/updating rendition based on:").append(LINE_BREAK).append("    sourceNode: ").append(
-                        sourceNode).append(LINE_BREAK).append("    tempRendition: ").append(tempRendition).append(
-                        LINE_BREAK).append("    parentNode: ").append(renditionLocation.getParentRef()).append(LINE_BREAK).append(
-                        "    childName: ").append(renditionLocation.getChildName()).append(LINE_BREAK).append(
-                        "    renditionDefinition.name: ").append(renditionQName);
-            logger.debug(msg.toString());
-        }
-        NodeRef oldRendition=getOldRenditionIfExists(sourceNode, renditionDefinition);
 
-        RenditionNodeManager renditionNodeManager = new RenditionNodeManager(sourceNode, oldRendition,
+        RenditionNodeManager renditionNodeManager = new RenditionNodeManager(sourceNode, tempRenditionNode,
-                renditionLocation, renditionDefinition, nodeService);
+                renditionLocation, renditionDefinition, nodeService, renditionService);
-        ChildAssociationRef primaryAssoc = renditionNodeManager.findOrCreateRenditionNode();
+        ChildAssociationRef renditionNode = renditionNodeManager.findOrCreateRenditionNode();
 
         // Copy relevant properties from the temporary node to the new rendition
         // node.
-        NodeRef renditionNode = primaryAssoc.getChildRef();
+        renditionNodeManager.transferNodeProperties();
-        transferNodeProperties(tempRenditionNode, renditionNode);
 
         // Set the name property on the rendition if it has not already been
         // set.
         String renditionName = getRenditionName(tempRenditionNode, renditionLocation, renditionDefinition);
-        nodeService.setProperty(renditionNode, ContentModel.PROP_NAME, renditionName);
+        nodeService.setProperty(renditionNode.getChildRef(), ContentModel.PROP_NAME, renditionName); // to manager
 
         // Delete the temporary rendition.
         nodeService.removeChildAssociation(tempRendition);
 
         // Handle the rendition aspects
-        manageRenditionAspects(sourceNode, primaryAssoc);
+        manageRenditionAspects(sourceNode, renditionNode);
+        
+        // Verify that everything has gone to plan, and nothing got lost on the way!
         ChildAssociationRef renditionAssoc = renditionService.getRenditionByName(sourceNode, renditionQName);
         if (renditionAssoc == null)
         {
@@ -949,36 +964,10 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
                         + sourceNode;
             throw new RenditionServiceException(msg);
         }
+        // Return the link between the source and the new, final rendition
         return renditionAssoc;
     }
 
-    /**
-     * This method returns the rendition on the given sourceNode with the given renditionDefinition, if such
-     * a rendition exists.
-     * 
-     * @param sourceNode
-     * @param renditionDefinition
-     * @return the rendition node if one exists, else null.
-     */
-    private NodeRef getOldRenditionIfExists(NodeRef sourceNode, RenditionDefinition renditionDefinition)
-    {
-        QName renditionName=renditionDefinition.getRenditionName();
-        ChildAssociationRef renditionAssoc = renditionService.getRenditionByName(sourceNode, renditionName);
-        
-        NodeRef result = (renditionAssoc == null) ? null : renditionAssoc.getChildRef();
-        if (logger.isDebugEnabled())
-        {
-        	StringBuilder msg = new StringBuilder();
-        	msg.append("Existing rendition with name ")
-        	   .append(renditionName)
-        	   .append(": ")
-        	   .append(result);
-        	logger.debug(msg.toString());
-        }
-        
-        return result;
-    }
-
     /**
      * This method manages the <code>rn:rendition</code> aspects on the rendition node. It applies the
      * correct rendition aspect based on the rendition node's location and removes any out-of-date rendition
@@ -1040,32 +1029,6 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase
         return renditionDefinition.getRenditionName().getLocalName();
     }
 
-    /**
-     * This method copies properties from the sourceNode onto the targetNode as well as the node type.
-     * {@link NewPerformRenditionActionExecuter#unchangedProperties Some properties} are not copied.
-     * 
-     * @param sourceNode the node from which the type and properties should be copied.
-     * @param targetNode the node to which the type and properties are copied.
-     */
-    private void transferNodeProperties(NodeRef sourceNode, NodeRef targetNode)
-    {
-        if (logger.isDebugEnabled())
-        {
-            StringBuilder msg = new StringBuilder();
-            msg.append("Transferring some properties from ").append(sourceNode).append(" to ").append(targetNode);
-            logger.debug(msg.toString());
-        }
-
-        QName type = nodeService.getType(sourceNode);
-        nodeService.setType(targetNode, type);
-        Map<QName, Serializable> newProps = nodeService.getProperties(sourceNode);
-        for (QName propKey : unchangedProperties)
-        {
-            newProps.remove(propKey);
-        }
-        nodeService.setProperties(targetNode, newProps);
-    }
-
     /**
      * Given a rendition definition, a source node and a temporary rendition node, this method uses a
      * {@link RenditionLocationResolver} to calculate the {@link RenditionLocation} of the rendition.