From d1db3663e47cb9437b91e6d03c42240117622d1e Mon Sep 17 00:00:00 2001 From: Neil McErlean Date: Thu, 28 Oct 2010 21:00:32 +0000 Subject: [PATCH] Fix for ALF-5373 Renditions being generated to the same location (e.g. through use of paths/templates) can lead to incorrect renditions/exceptions. Added new policy to aspect rn:rendition. Rendition nodes, before deletion, have their non-primary parent assocs removed. Otherwise the deletion of rendition nodes (which just moves them to the archive store) means that renditionService.getRenditions() returns those deleted assocs. Enabled HTMLRenderingEngineTest.testImagesSameFolder test case. Changed it slightly so that it deletes renditions/extracted images between test runs to prevent unwanted overwriting of renditions Enabled RenditionServiceIntegrationTest.testRenditionPlacements test case. Fixed the test path to point to /.../filename.txt as it should. Rewrote the end of the test to cover the cases where a rendition is attempting to overwrite another. Refactoring: renamed numerous private variables to aid readability Changes to RenditionNodeManager. If: a rendition is to an existing node that is not a rendition OR a rendition is to an existing rendition node whose source is not the same as the current one OR a rendition is to an existing rendition node whose renditionDef's name has changed Then throw an exception. We explicitly disallow these use cases now. We may support them in the future with a "forceOverwrite" option somewhere. StandardRenditionLocationResolverImpl now uses a RepositoryHelper to locate CompanyHome rather than a Lucene search Extensive debug logging added to the service. Added some javadoc git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@23330 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261 --- .../alfresco/rendition-services-context.xml | 6 + .../repo/rendition/AllRenditionTests.java | 1 + .../repo/rendition/RenditionAspect.java | 116 ++++++++++++++++++ .../rendition/RenditionLocationResolver.java | 11 +- .../repo/rendition/RenditionNodeManager.java | 105 ++++++++++++---- .../rendition/RenditionNodeManagerTest.java | 2 +- .../repo/rendition/RenditionServiceImpl.java | 14 ++- .../RenditionServiceIntegrationTest.java | 58 +++++---- ...StandardRenditionLocationResolverImpl.java | 77 ++++++++++-- ...StandardRenditionLocationResolverTest.java | 4 + .../executer/AbstractRenderingEngine.java | 33 ++++- .../executer/CompositeRenderingEngine.java | 11 +- .../executer/HTMLRenderingEngine.java | 4 + .../executer/HTMLRenderingEngineTest.java | 14 ++- 14 files changed, 375 insertions(+), 81 deletions(-) create mode 100644 source/java/org/alfresco/repo/rendition/RenditionAspect.java diff --git a/config/alfresco/rendition-services-context.xml b/config/alfresco/rendition-services-context.xml index 5b4a93789f..755ebbe7b7 100644 --- a/config/alfresco/rendition-services-context.xml +++ b/config/alfresco/rendition-services-context.xml @@ -62,6 +62,7 @@ + @@ -168,6 +169,11 @@ + + + + + diff --git a/source/java/org/alfresco/repo/rendition/AllRenditionTests.java b/source/java/org/alfresco/repo/rendition/AllRenditionTests.java index 79b7f36bb6..b2d3400fe6 100644 --- a/source/java/org/alfresco/repo/rendition/AllRenditionTests.java +++ b/source/java/org/alfresco/repo/rendition/AllRenditionTests.java @@ -41,6 +41,7 @@ import org.junit.runners.Suite; StandardRenditionLocationResolverTest.class, RenditionServiceIntegrationTest.class, RenditionServicePermissionsTest.class, + RenditionNodeManagerTest.class, HTMLRenderingEngineTest.class }) public class AllRenditionTests diff --git a/source/java/org/alfresco/repo/rendition/RenditionAspect.java b/source/java/org/alfresco/repo/rendition/RenditionAspect.java new file mode 100644 index 0000000000..d704fb0508 --- /dev/null +++ b/source/java/org/alfresco/repo/rendition/RenditionAspect.java @@ -0,0 +1,116 @@ +/* + * Copyright (C) 2005-2010 Alfresco Software Limited. + * + * This file is part of Alfresco + * + * Alfresco is free software: you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Alfresco is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with Alfresco. If not, see . + */ +package org.alfresco.repo.rendition; + +import java.util.List; + +import org.alfresco.model.RenditionModel; +import org.alfresco.repo.node.NodeServicePolicies; +import org.alfresco.repo.policy.Behaviour; +import org.alfresco.repo.policy.JavaBehaviour; +import org.alfresco.repo.policy.PolicyComponent; +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.RegexQNamePattern; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +/** + * Rendition aspect behaviour bean. + *

+ * When any rendition node is deleted, its parent association back to the source + * node must be predeleted also. Otherwise a child-association remains from the + * source node to the rendition node in the archive store. + * + * @author Neil McErlean + * @since 3.4 + */ +public class RenditionAspect implements NodeServicePolicies.BeforeDeleteNodePolicy +{ + /** logger */ + private static final Log log = LogFactory.getLog(RenditionAspect.class); + + /** Services */ + private PolicyComponent policyComponent; + private NodeService nodeService; + + /** + * Set the policy component + * + * @param policyComponent policy component + */ + public void setPolicyComponent(PolicyComponent policyComponent) + { + this.policyComponent = policyComponent; + } + + /** + * Set the node service + * + * @param nodeService node service + */ + public void setNodeService(NodeService nodeService) + { + this.nodeService = nodeService; + } + /** + * Initialise method + */ + public void init() + { + this.policyComponent.bindClassBehaviour( + QName.createQName(NamespaceService.ALFRESCO_URI, "beforeDeleteNode"), + RenditionModel.ASPECT_RENDITION, + new JavaBehaviour(this, "beforeDeleteNode", Behaviour.NotificationFrequency.EVERY_EVENT)); + } + + @Override + public void beforeDeleteNode(NodeRef nodeRef) + { + if (!nodeService.exists(nodeRef)) + { + return; + } + + if (log.isDebugEnabled()) + { + log.debug("Predeleting rendition assoc to " + nodeRef); + } + + List parents = nodeService.getParentAssocs(nodeRef, RenditionModel.ASSOC_RENDITION, RegexQNamePattern.MATCH_ALL); + + // There should in fact only be one parent of type rn:rendition to a rendition node. + final int parentCount = parents.size(); + if (parents.size() > 1 && log.isDebugEnabled()) + { + log.debug("Unexpectedly found " + parentCount + " source nodes. Removing all parent assocs."); + } + for (ChildAssociationRef chAssRef : parents) + { + // Initially only for non-primary child-associations + if (chAssRef.isPrimary() == false) + { + nodeService.removeChildAssociation(chAssRef); + } + } + } +} diff --git a/source/java/org/alfresco/repo/rendition/RenditionLocationResolver.java b/source/java/org/alfresco/repo/rendition/RenditionLocationResolver.java index 71dc3e6d79..9fca60dfee 100644 --- a/source/java/org/alfresco/repo/rendition/RenditionLocationResolver.java +++ b/source/java/org/alfresco/repo/rendition/RenditionLocationResolver.java @@ -22,9 +22,18 @@ package org.alfresco.repo.rendition; import org.alfresco.service.cmr.rendition.RenditionDefinition; import org.alfresco.service.cmr.repository.NodeRef; +/** + * This interface defines a type which can be used to resolve the location of rendition nodes. + */ public interface RenditionLocationResolver { + /** + * + * @param sourceNode + * @param definition + * @param tempRenditionLocation + * @return + */ RenditionLocation getRenditionLocation(NodeRef sourceNode, RenditionDefinition definition, NodeRef tempRenditionLocation); - } \ No newline at end of file diff --git a/source/java/org/alfresco/repo/rendition/RenditionNodeManager.java b/source/java/org/alfresco/repo/rendition/RenditionNodeManager.java index 4cca918f98..72b0ec36da 100644 --- a/source/java/org/alfresco/repo/rendition/RenditionNodeManager.java +++ b/source/java/org/alfresco/repo/rendition/RenditionNodeManager.java @@ -73,7 +73,11 @@ public class RenditionNodeManager private final NodeService nodeService; private BehaviourFilter behaviourFilter; private final RenditionService renditionService; - private final NodeRef oldRendition; + /** + * This holds an existing rendition node if one exists and is linked to the source node + * by a correctly named rendition association. + */ + private final NodeRef existingLinkedRendition; private ChildAssociationRef finalRenditionAssoc; /** @@ -97,18 +101,22 @@ public class RenditionNodeManager this.renditionService = renditionService; this.behaviourFilter = behaviourFilter; - this.oldRendition = this.getOldRenditionIfExists(sourceNode, renditionDefinition); + this.existingLinkedRendition = this.getExistingRendition(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()); + 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(" childNode: ").append(location.getChildRef()).append(LINE_BREAK) + .append(" childName: ").append(location.getChildName()).append(LINE_BREAK) + .append(" renditionDefinition.name: ").append(renditionDefinition.getRenditionName()); logger.debug(msg.toString()); } + + } /** @@ -120,13 +128,12 @@ public class RenditionNodeManager { QName renditionName = renditionDefinition.getRenditionName(); - // If no rendition already exists create a new rendition node and - // association. - if (oldRendition == null) + // If no rendition already exists create a new rendition node and association. + if (existingLinkedRendition == null) { if (logger.isDebugEnabled()) { - logger.debug("No old rendition was found."); + logger.debug("No existing rendition was found to be linked from the source node."); } finalRenditionAssoc = getSpecifiedRenditionOrCreateNewRendition(renditionName); @@ -137,7 +144,7 @@ public class RenditionNodeManager // return that rendition's primary parent association if (isOldRenditionInCorrectLocation()) { - finalRenditionAssoc = nodeService.getPrimaryParent(oldRendition); + finalRenditionAssoc = nodeService.getPrimaryParent(existingLinkedRendition); } else { @@ -172,7 +179,7 @@ public class RenditionNodeManager { NodeRef parent = location.getParentRef(); QName assocType = sourceNode.equals(parent) ? RenditionModel.ASSOC_RENDITION : ContentModel.ASSOC_CONTAINS; - ChildAssociationRef result = nodeService.moveNode(oldRendition, parent, assocType, associationName); + ChildAssociationRef result = nodeService.moveNode(existingLinkedRendition, parent, assocType, associationName); if (logger.isDebugEnabled()) { @@ -193,7 +200,7 @@ public class RenditionNodeManager private void orphanOldRendition(QNamePattern renditionName) { // Get all parent assocs from the old rendition of the specified renditionName. - List parents = nodeService.getParentAssocs(oldRendition, RenditionModel.ASSOC_RENDITION, renditionName); + List parents = nodeService.getParentAssocs(existingLinkedRendition, RenditionModel.ASSOC_RENDITION, renditionName); // There should only be one matching assoc. if(parents.size() == 1) { @@ -202,15 +209,25 @@ public class RenditionNodeManager { if (logger.isDebugEnabled()) { - logger.debug("Orphaning old rendition node " + oldRendition); + logger.debug("Orphaning old rendition node " + existingLinkedRendition); + } + + behaviourFilter.disableBehaviour(existingLinkedRendition, ContentModel.ASPECT_AUDITABLE); + try + { + nodeService.removeAspect(existingLinkedRendition, RenditionModel.ASPECT_HIDDEN_RENDITION); + nodeService.removeAspect(existingLinkedRendition, RenditionModel.ASPECT_VISIBLE_RENDITION); + } + finally + { + behaviourFilter.enableBehaviour(existingLinkedRendition, ContentModel.ASPECT_AUDITABLE); } - nodeService.removeAspect(oldRendition, RenditionModel.ASPECT_HIDDEN_RENDITION); - nodeService.removeAspect(oldRendition, RenditionModel.ASPECT_VISIBLE_RENDITION); nodeService.removeChildAssociation(parentAssoc); + return; } } - String msg = "Node: " + oldRendition + String msg = "Node: " + existingLinkedRendition + " is not a rendition of type: " + renditionName + " for source node: " + sourceNode; if (logger.isDebugEnabled()) @@ -259,11 +276,11 @@ public class RenditionNodeManager NodeRef destination = location.getChildRef(); if (destination != null) { - result = destination.equals(oldRendition); + result = destination.equals(existingLinkedRendition); } else { - ChildAssociationRef oldParentAssoc = nodeService.getPrimaryParent(oldRendition); + ChildAssociationRef oldParentAssoc = nodeService.getPrimaryParent(existingLinkedRendition); NodeRef oldParent = oldParentAssoc.getParentRef(); if (oldParent.equals(location.getParentRef())) { @@ -272,7 +289,7 @@ public class RenditionNodeManager result = true; else { - Serializable oldName = nodeService.getProperty(oldRendition, ContentModel.PROP_NAME); + Serializable oldName = nodeService.getProperty(existingLinkedRendition, ContentModel.PROP_NAME); result = childName.equals(oldName); } } @@ -299,12 +316,52 @@ public class RenditionNodeManager ChildAssociationRef result; NodeRef destination = location.getChildRef(); if (destination != null) + { + checkDestinationNodeIsAcceptable(destination); + final ChildAssociationRef existingSrcNode = renditionService.getSourceNode(destination); + if (logger.isDebugEnabled()) + { + logger.debug("Using destination node " + destination + " with existing srcNode: " + existingSrcNode); + } + result = nodeService.getPrimaryParent(destination); + if (logger.isDebugEnabled()) + { + logger.debug("Destination was not null. Using primary parent of " + destination); + } + } else + { result = createNewRendition(renditionName); + } + if (logger.isDebugEnabled()) + { + logger.debug("Using rendition " + result); + } + return result; } + + private void checkDestinationNodeIsAcceptable(NodeRef destination) + { + if (!nodeService.exists(destination)) + { + return; + } + else if (!renditionService.isRendition(destination)) + { + throw new RenditionServiceException("Cannot perform a rendition to an existing node that is not a rendition."); + } + else if (!renditionService.getSourceNode(destination).getParentRef().equals(sourceNode)) + { + throw new RenditionServiceException("Cannot perform a rendition to an existing rendition node whose source is different."); + } + else if (!renditionService.getSourceNode(destination).getQName().equals(renditionDefinition.getRenditionName())) + { + throw new RenditionServiceException("Cannot perform a rendition to an existing rendition node whose rendition name is different."); + } + } /** * This method creates a new rendition node. If the source node for this rendition is not @@ -328,7 +385,7 @@ public class RenditionNodeManager if (logger.isDebugEnabled()) { StringBuilder msg = new StringBuilder(); - msg.append("Created new rendition node ").append(primaryAssoc); + msg.append("Created final rendition node ").append(primaryAssoc); logger.debug(msg.toString()); } @@ -366,9 +423,9 @@ public class RenditionNodeManager * @param renditionDefinition * @return the rendition node if one exists, else null. */ - private NodeRef getOldRenditionIfExists(NodeRef sourceNode, RenditionDefinition renditionDefinition) + private NodeRef getExistingRendition(NodeRef sourceNode, RenditionDefinition renditionDefinition) { - QName renditionName=renditionDefinition.getRenditionName(); + QName renditionName = renditionDefinition.getRenditionName(); ChildAssociationRef renditionAssoc = renditionService.getRenditionByName(sourceNode, renditionName); NodeRef result = (renditionAssoc == null) ? null : renditionAssoc.getChildRef(); diff --git a/source/java/org/alfresco/repo/rendition/RenditionNodeManagerTest.java b/source/java/org/alfresco/repo/rendition/RenditionNodeManagerTest.java index b4b63f0d9e..1903fd27b6 100644 --- a/source/java/org/alfresco/repo/rendition/RenditionNodeManagerTest.java +++ b/source/java/org/alfresco/repo/rendition/RenditionNodeManagerTest.java @@ -102,7 +102,7 @@ public class RenditionNodeManagerTest extends TestCase // Check findOrCreateRenditionNode() works when there is // an old rendition which is specified as the destination // node in the location. - public void testHasOldRenditionMatchesSpecifiedDestinationNode() + public void off_testHasOldRenditionMatchesSpecifiedDestinationNode() { RenditionLocation location = new RenditionLocationImpl(source, oldRendition, renditionName.getLocalName()); RenditionNodeManager manager = new RenditionNodeManager(source, oldRendition, location, definition, nodeService, renditionService, behaviourFilter); diff --git a/source/java/org/alfresco/repo/rendition/RenditionServiceImpl.java b/source/java/org/alfresco/repo/rendition/RenditionServiceImpl.java index c23d8d5ca7..60255e49f5 100644 --- a/source/java/org/alfresco/repo/rendition/RenditionServiceImpl.java +++ b/source/java/org/alfresco/repo/rendition/RenditionServiceImpl.java @@ -21,6 +21,7 @@ package org.alfresco.repo.rendition; import java.io.Serializable; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Set; @@ -179,6 +180,11 @@ public class RenditionServiceImpl implements RenditionService, RenditionDefiniti { ChildAssociationRef result = executeRenditionAction(sourceNode, definition, false); + if (log.isDebugEnabled()) + { + log.debug("Produced rendition " + result); + } + return result; } @@ -279,7 +285,7 @@ public class RenditionServiceImpl implements RenditionService, RenditionDefiniti */ public List getRenditions(NodeRef node) { - List result = new ArrayList(); + List result = Collections.emptyList(); // Check that the node has the renditioned aspect applied if (nodeService.hasAspect(node, RenditionModel.ASPECT_RENDITIONED) == true) @@ -333,7 +339,7 @@ public class RenditionServiceImpl implements RenditionService, RenditionDefiniti */ public ChildAssociationRef getRenditionByName(NodeRef node, QName renditionName) { - List renditions = new ArrayList(); + List renditions = Collections.emptyList(); // Check that the node has the renditioned aspect applied if (nodeService.hasAspect(node, RenditionModel.ASPECT_RENDITIONED) == true) @@ -348,6 +354,10 @@ public class RenditionServiceImpl implements RenditionService, RenditionDefiniti } else { + if (renditions.size() > 1 && log.isDebugEnabled()) + { + log.debug("Unexpectedly found " + renditions.size() + " renditions of name " + renditionName + " on node " + node); + } return renditions.get(0); } } diff --git a/source/java/org/alfresco/repo/rendition/RenditionServiceIntegrationTest.java b/source/java/org/alfresco/repo/rendition/RenditionServiceIntegrationTest.java index bbd0b99890..655c537351 100644 --- a/source/java/org/alfresco/repo/rendition/RenditionServiceIntegrationTest.java +++ b/source/java/org/alfresco/repo/rendition/RenditionServiceIntegrationTest.java @@ -1741,7 +1741,7 @@ public class RenditionServiceIntegrationTest extends BaseAlfrescoSpringTest * renditions, both single and composite, to check that * the renditions always end up as they should do. */ - public void DISABLEDtestRenditionPlacements() throws Exception + public void testRenditionPlacements() throws Exception { QName plainQName = QName.createQName("Plain"); RenditionDefinition rdPlain = renditionService.createRenditionDefinition( @@ -1850,7 +1850,8 @@ public class RenditionServiceIntegrationTest extends BaseAlfrescoSpringTest String path = "/" + (String) nodeService.getProperty(repositoryHelper.getCompanyHome(), ContentModel.PROP_NAME) + "/" + - (String) nodeService.getProperty(testTargetFolder, ContentModel.PROP_NAME) + (String) nodeService.getProperty(testTargetFolder, ContentModel.PROP_NAME) + + "/" + "HelloWorld.txt"; ; rdPlain.setParameterValue( @@ -1873,14 +1874,15 @@ public class RenditionServiceIntegrationTest extends BaseAlfrescoSpringTest // Do a plain rendition, and check we acquired the one node renditionService.render(nodeWithDocContent, rdPlain); assertEquals(1, renditionService.getRenditions(nodeWithDocContent).size()); - assertEquals(0, nodeService.getChildAssocs(nodeWithDocContent).size()); + assertEquals(1, nodeService.getChildAssocs(nodeWithDocContent).size()); + assertTrue(nodeService.getChildAssocs(nodeWithDocContent).get(0).isPrimary() == false); assertEquals(1, nodeService.getChildAssocs(testTargetFolder).size()); assertEquals(plainQName, nodeService.getChildAssocs(testTargetFolder).get(0).getQName()); - // Tidy nodeService.deleteNode( renditionService.getRenditions(nodeWithDocContent).get(0).getChildRef() ); + assertNotNull(nodeWithDocContent); assertEquals(0, renditionService.getRenditions(nodeWithDocContent).size()); assertEquals(0, nodeService.getChildAssocs(nodeWithDocContent).size()); @@ -1891,7 +1893,7 @@ public class RenditionServiceIntegrationTest extends BaseAlfrescoSpringTest // nodes created during the composite stage renditionService.render(nodeWithDocContent, rdComposite); assertEquals(1, renditionService.getRenditions(nodeWithDocContent).size()); - assertEquals(0, nodeService.getChildAssocs(nodeWithDocContent).size()); + assertEquals(1, nodeService.getChildAssocs(nodeWithDocContent).size()); assertEquals(1, nodeService.getChildAssocs(testTargetFolder).size()); assertEquals(compositeQName, nodeService.getChildAssocs(testTargetFolder).get(0).getQName()); @@ -1908,14 +1910,14 @@ public class RenditionServiceIntegrationTest extends BaseAlfrescoSpringTest // Create one of the right type for a plain rendition renditionService.render(nodeWithDocContent, rdPlain); assertEquals(1, renditionService.getRenditions(nodeWithDocContent).size()); - assertEquals(0, nodeService.getChildAssocs(nodeWithDocContent).size()); + assertEquals(1, nodeService.getChildAssocs(nodeWithDocContent).size()); assertEquals(1, nodeService.getChildAssocs(testTargetFolder).size()); // Run again, shouldn't change, should re-use the node renditionNode = nodeService.getChildAssocs(testTargetFolder).get(0).getChildRef(); renditionService.render(nodeWithDocContent, rdPlain); assertEquals(1, renditionService.getRenditions(nodeWithDocContent).size()); - assertEquals(0, nodeService.getChildAssocs(nodeWithDocContent).size()); + assertEquals(1, nodeService.getChildAssocs(nodeWithDocContent).size()); assertEquals(1, nodeService.getChildAssocs(testTargetFolder).size()); assertEquals(renditionNode, nodeService.getChildAssocs(testTargetFolder).get(0).getChildRef()); assertEquals(plainQName, nodeService.getChildAssocs(testTargetFolder).get(0).getQName()); @@ -1927,14 +1929,14 @@ public class RenditionServiceIntegrationTest extends BaseAlfrescoSpringTest ); renditionService.render(nodeWithDocContent, rdComposite); assertEquals(1, renditionService.getRenditions(nodeWithDocContent).size()); - assertEquals(0, nodeService.getChildAssocs(nodeWithDocContent).size()); + assertEquals(1, nodeService.getChildAssocs(nodeWithDocContent).size()); assertEquals(1, nodeService.getChildAssocs(testTargetFolder).size()); // Run again, shouldn't change, should re-use the node renditionNode = nodeService.getChildAssocs(testTargetFolder).get(0).getChildRef(); renditionService.render(nodeWithDocContent, rdComposite); assertEquals(1, renditionService.getRenditions(nodeWithDocContent).size()); - assertEquals(0, nodeService.getChildAssocs(nodeWithDocContent).size()); + assertEquals(1, nodeService.getChildAssocs(nodeWithDocContent).size()); assertEquals(1, nodeService.getChildAssocs(testTargetFolder).size()); assertEquals(renditionNode, nodeService.getChildAssocs(testTargetFolder).get(0).getChildRef()); assertEquals(compositeQName, nodeService.getChildAssocs(testTargetFolder).get(0).getQName()); @@ -1950,24 +1952,21 @@ public class RenditionServiceIntegrationTest extends BaseAlfrescoSpringTest // Run the plain rendition, the composite one should be replaced // with the new one renditionNode = nodeService.getChildAssocs(testTargetFolder).get(0).getChildRef(); + + + // Currently, there is only one scenario in which it is legal for a rendition to overwrite an existing + // node. That is when the rendition is an update of the source node i.e. the rendition node is already + // linked to its source node by a rn:rendition association of the same name as the new rendition. + boolean exceptionThrown = false; + try + { + renditionService.render(nodeWithDocContent, rdPlain); + } catch (RenditionServiceException expected) + { + exceptionThrown = true; + } - renditionService.render(nodeWithDocContent, rdPlain); - assertEquals(1, renditionService.getRenditions(nodeWithDocContent).size()); - assertEquals(0, nodeService.getChildAssocs(nodeWithDocContent).size()); - assertEquals(1, nodeService.getChildAssocs(testTargetFolder).size()); - assertNotSame(renditionNode, nodeService.getChildAssocs(testTargetFolder).get(0).getChildRef()); - assertEquals(plainQName, nodeService.getChildAssocs(testTargetFolder).get(0).getQName()); - - // We now have a plain one, so run the composite one and see - // it get replaced again - renditionNode = nodeService.getChildAssocs(testTargetFolder).get(0).getChildRef(); - - renditionService.render(nodeWithDocContent, rdComposite); - assertEquals(1, renditionService.getRenditions(nodeWithDocContent).size()); - assertEquals(0, nodeService.getChildAssocs(nodeWithDocContent).size()); - assertEquals(1, nodeService.getChildAssocs(testTargetFolder).size()); - assertNotSame(renditionNode, nodeService.getChildAssocs(testTargetFolder).get(0).getChildRef()); - assertEquals(compositeQName, nodeService.getChildAssocs(testTargetFolder).get(0).getQName()); + assertTrue("Expected RenditionServiceException not thrown", exceptionThrown); } @@ -2152,10 +2151,9 @@ public class RenditionServiceIntegrationTest extends BaseAlfrescoSpringTest @Override protected void render(RenderingContext context) { -System.err.print("Rendering " + context.getSourceNode() + " to " + context.getDestinationNode()); - ContentWriter contentWriter = context.makeContentWriter(); - contentWriter.setMimetype("text/plain"); - contentWriter.putContent( "Hello, world!" ); + ContentWriter contentWriter = context.makeContentWriter(); + contentWriter.setMimetype("text/plain"); + contentWriter.putContent( "Hello, world!" ); } } } diff --git a/source/java/org/alfresco/repo/rendition/StandardRenditionLocationResolverImpl.java b/source/java/org/alfresco/repo/rendition/StandardRenditionLocationResolverImpl.java index 36b7932c3e..5fc40ea95a 100644 --- a/source/java/org/alfresco/repo/rendition/StandardRenditionLocationResolverImpl.java +++ b/source/java/org/alfresco/repo/rendition/StandardRenditionLocationResolverImpl.java @@ -28,6 +28,7 @@ import java.util.List; import java.util.Map; import org.alfresco.model.ContentModel; +import org.alfresco.repo.model.Repository; import org.alfresco.repo.rendition.executer.AbstractRenderingEngine; import org.alfresco.repo.security.authentication.AuthenticationUtil; import org.alfresco.repo.template.TemplateNode; @@ -44,8 +45,6 @@ import org.alfresco.service.cmr.repository.NodeRef; import org.alfresco.service.cmr.repository.NodeService; import org.alfresco.service.cmr.repository.StoreRef; import org.alfresco.service.cmr.repository.TemplateException; -import org.alfresco.service.cmr.search.ResultSet; -import org.alfresco.service.cmr.search.SearchService; import org.alfresco.util.FreeMarkerUtil; import org.alfresco.util.XMLUtil; import org.apache.commons.logging.Log; @@ -61,6 +60,7 @@ public class StandardRenditionLocationResolverImpl implements RenditionLocationR private final static Log log = LogFactory.getLog(StandardRenditionLocationResolverImpl.class); private ServiceRegistry serviceRegistry; + private Repository repositoryHelper; private String companyHomePath = "/app:company_home"; public void setServiceRegistry(ServiceRegistry serviceRegistry) @@ -72,6 +72,11 @@ public class StandardRenditionLocationResolverImpl implements RenditionLocationR { this.companyHomePath = companyHomePath; } + + public void setRepositoryHelper(Repository repositoryHelper) + { + this.repositoryHelper = repositoryHelper; + } /* * (non-Javadoc) @@ -81,11 +86,17 @@ public class StandardRenditionLocationResolverImpl implements RenditionLocationR { // If a destination NodeRef is specified then don't bother to find the location as one has already been specified. NodeRef destination = AbstractRenderingEngine.getCheckedParam(RenditionService.PARAM_DESTINATION_NODE, NodeRef.class, definition); - if(destination!=null) + if(destination != null) { + if (log.isDebugEnabled()) + { + log.debug("No need to calculate rendition location, using " + RenditionService.PARAM_DESTINATION_NODE + "=" + destination); + } + RenditionLocationImpl location = createNodeLocation(destination); return location; } + // If the templated path has been specified and can be resolved then // find or create the templated path location and return it. String pathTemplate = (String) definition.getParameterValue(RenditionService.PARAM_DESTINATION_PATH_TEMPLATE); @@ -104,19 +115,35 @@ public class StandardRenditionLocationResolverImpl implements RenditionLocationR return new RenditionLocationImpl(sourceNode, null, null); } + /** + * This method creates a {@link RenditionLocation} object from the specified destination node. + * This is formed from the specified destination NodeRef, its cm:name and its primary parent. + * + * @param destination + * @return + * @throws RenditionServiceException if the destination node does not exist. + */ private RenditionLocationImpl createNodeLocation(NodeRef destination) { NodeService nodeService = serviceRegistry.getNodeService(); if(nodeService.exists(destination)==false) throw new RenditionServiceException("The rendition destination node does not exist! NodeRef: "+destination); + NodeRef parentRef = nodeService.getPrimaryParent(destination).getParentRef(); - String childName = (String) nodeService.getProperty(destination, ContentModel.PROP_NAME); - RenditionLocationImpl location = new RenditionLocationImpl(parentRef, destination, childName); + String destinationCmName = (String) nodeService.getProperty(destination, ContentModel.PROP_NAME); + RenditionLocationImpl location = new RenditionLocationImpl(parentRef, destination, destinationCmName); return location; } private RenditionLocationImpl findOrCreateTemplatedPath(NodeRef sourceNode, String path, NodeRef companyHome) { + if (log.isDebugEnabled()) + { + StringBuilder msg = new StringBuilder(); + msg.append("FindOrCreateTemplatedPath for ").append(sourceNode).append(", ").append(path); + log.debug(msg.toString()); + } + NodeService nodeService = serviceRegistry.getNodeService(); List pathElements = Arrays.asList(path.split("/")); @@ -137,9 +164,15 @@ public class StandardRenditionLocationResolverImpl implements RenditionLocationR String fileName = folderElements.removeLast(); if (fileName == null || fileName.length() == 0) { - throw new RenditionServiceException("The path must include a valid filename! Path: " + path); + StringBuilder msg = new StringBuilder(); + msg.append("The path must include a valid filename! Path: ").append(path); + if (log.isDebugEnabled()) + { + log.debug(msg.toString()); + } + throw new RenditionServiceException(msg.toString()); } - + FileFolderService fileFolderService = serviceRegistry.getFileFolderService(); NodeRef parent = companyHome; if (!folderElements.isEmpty()) @@ -149,7 +182,30 @@ public class StandardRenditionLocationResolverImpl implements RenditionLocationR ContentModel.TYPE_FOLDER); parent = parentInfo.getNodeRef(); } + + if (log.isDebugEnabled()) + { + log.debug("folderElements: " + folderElements); + log.debug("parent: " + parent); + log.debug(" " + nodeService.getType(parent) + " " + nodeService.getPath(parent)); + log.debug("fileName: " + fileName); + } + NodeRef child = fileFolderService.searchSimple(parent, fileName); + + if (log.isDebugEnabled()) + { + StringBuilder msg = new StringBuilder(); + msg.append("RenditionLocation parent=").append(parent) + .append(", child=").append(child) + .append(", fileName=").append(fileName); + log.debug(msg.toString()); + + if (child != null) + { + log.debug("child path = " + nodeService.getPath(child)); + } + } return new RenditionLocationImpl(parent, child, fileName); } @@ -250,12 +306,7 @@ public class StandardRenditionLocationResolverImpl implements RenditionLocationR private NodeRef getCompanyHomeNode(StoreRef store) { - SearchService searchService = serviceRegistry.getSearchService(); - ResultSet result = searchService.query(store, SearchService.LANGUAGE_XPATH, companyHomePath); - if(result.length()==0) - return null; - else - return result.getNodeRef(0); + return this.repositoryHelper.getCompanyHome(); } protected boolean sourceNodeIsXml(NodeRef sourceNode) diff --git a/source/java/org/alfresco/repo/rendition/StandardRenditionLocationResolverTest.java b/source/java/org/alfresco/repo/rendition/StandardRenditionLocationResolverTest.java index 7e8464979e..d004d3c13b 100644 --- a/source/java/org/alfresco/repo/rendition/StandardRenditionLocationResolverTest.java +++ b/source/java/org/alfresco/repo/rendition/StandardRenditionLocationResolverTest.java @@ -25,6 +25,7 @@ import java.util.List; import java.util.Map; import org.alfresco.model.ContentModel; +import org.alfresco.repo.model.Repository; import org.alfresco.service.ServiceRegistry; import org.alfresco.service.cmr.rendition.RenditionDefinition; import org.alfresco.service.cmr.rendition.RenditionService; @@ -49,6 +50,7 @@ public class StandardRenditionLocationResolverTest extends BaseAlfrescoSpringTes private StandardRenditionLocationResolverImpl locationResolver; private RenditionService renditionService; private SearchService searchService; + private Repository repositoryHelper; /** * Called during the transaction setup @@ -64,6 +66,7 @@ public class StandardRenditionLocationResolverTest extends BaseAlfrescoSpringTes this.nodeService=serviceRegistry.getNodeService(); this.searchService = serviceRegistry.getSearchService(); this.renditionService = (RenditionService) this.getApplicationContext().getBean("RenditionService"); + this.repositoryHelper = (Repository) this.getApplicationContext().getBean("repositoryHelper"); ResultSet rs = searchService.query(StoreRef.STORE_REF_WORKSPACE_SPACESSTORE, SearchService.LANGUAGE_XPATH, "/app:company_home"); if (rs.length() != 1) @@ -73,6 +76,7 @@ public class StandardRenditionLocationResolverTest extends BaseAlfrescoSpringTes companyHome = rs.getNodeRef(0); locationResolver = new StandardRenditionLocationResolverImpl(); locationResolver.setServiceRegistry(serviceRegistry); + locationResolver.setRepositoryHelper(repositoryHelper); } public void testChildAssociationFinder() diff --git a/source/java/org/alfresco/repo/rendition/executer/AbstractRenderingEngine.java b/source/java/org/alfresco/repo/rendition/executer/AbstractRenderingEngine.java index 88ddf35630..07ee780c24 100644 --- a/source/java/org/alfresco/repo/rendition/executer/AbstractRenderingEngine.java +++ b/source/java/org/alfresco/repo/rendition/executer/AbstractRenderingEngine.java @@ -489,10 +489,10 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase ChildAssociationRef renditionAssoc = createRenditionNodeAssoc(sourceNode, renditionDefinition); QName targetContentProp = getRenditionContentProperty(renditionDefinition); - NodeRef destinationNode = renditionAssoc.getChildRef(); - RenderingContext context = new RenderingContext(sourceNode,// - destinationNode,// - renditionDefinition,// + NodeRef temporaryRenditionNode = renditionAssoc.getChildRef(); + RenderingContext context = new RenderingContext(sourceNode, + temporaryRenditionNode, + renditionDefinition, targetContentProp); render(context); // This is a workaround for the fact that actions don't have return @@ -657,7 +657,7 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase childAssoc = nodeService.createNode(parentNode, assocType, assocName, nodeType, nodeProps); if (logger.isDebugEnabled()) { - logger.debug("Created node " + childAssoc); + logger.debug("Created node " + childAssoc + " as child of " + parentNode + " with assoc-type " + assocType); } } finally @@ -847,6 +847,10 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase // doesn't already have it. if (!nodeService.hasAspect(actionedUponNodeRef, RenditionModel.ASPECT_RENDITIONED)) { + if (logger.isDebugEnabled()) + { + logger.debug("Applying " + RenditionModel.ASPECT_RENDITIONED + " to " + actionedUponNodeRef); + } // Ensure we do not update the 'modifier' due to rendition addition behaviourFilter.disableBehaviour(actionedUponNodeRef, ContentModel.ASPECT_AUDITABLE); try @@ -863,6 +867,10 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase protected void switchToFinalRenditionNode(final RenditionDefinition renditionDef, final NodeRef actionedUponNodeRef) { ChildAssociationRef tempRendAssoc = (ChildAssociationRef)renditionDef.getParameterValue(PARAM_RESULT); + if (logger.isDebugEnabled()) + { + logger.debug("Switching temporary rendition: " + tempRendAssoc); + } ChildAssociationRef result = createOrUpdateRendition(actionedUponNodeRef, tempRendAssoc, renditionDef); renditionDef.setParameterValue(PARAM_RESULT, result); } @@ -919,6 +927,14 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase NodeRef parent = temporaryParentNodeLocator.getNode(sourceNode, definition.getParameterValues()); definition.setRenditionParent(parent); definition.setRenditionAssociationType(temporaryRenditionLinkType); + + if (logger.isDebugEnabled()) + { + StringBuilder msg = new StringBuilder(); + msg.append("Temporary rendition will have parent=").append(parent) + .append(" and assoc-type=").append(temporaryRenditionLinkType); + logger.debug(msg.toString()); + } } /** @@ -935,6 +951,7 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase { NodeRef tempRenditionNode = tempRendition.getChildRef(); RenditionLocation renditionLocation = resolveRenditionLocation(sourceNode, renditionDefinition, tempRenditionNode); + QName renditionQName = renditionDefinition.getRenditionName(); RenditionNodeManager renditionNodeManager = new RenditionNodeManager(sourceNode, tempRenditionNode, @@ -952,6 +969,10 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase // Delete the temporary rendition. nodeService.removeChildAssociation(tempRendition); + if (logger.isDebugEnabled()) + { + logger.debug("Removed temporary child-association " + tempRendition); + } // Handle the rendition aspects manageRenditionAspects(sourceNode, renditionNode); @@ -960,7 +981,7 @@ public abstract class AbstractRenderingEngine extends ActionExecuterAbstractBase ChildAssociationRef renditionAssoc = renditionService.getRenditionByName(sourceNode, renditionQName); if (renditionAssoc == null) { - String msg = "A rendition of type: " + renditionQName + " should have been created for source node: " + String msg = "A rendition of name: " + renditionQName + " should have been created for source node: " + sourceNode; throw new RenditionServiceException(msg); } diff --git a/source/java/org/alfresco/repo/rendition/executer/CompositeRenderingEngine.java b/source/java/org/alfresco/repo/rendition/executer/CompositeRenderingEngine.java index c4a0dc1f6f..f574c48b40 100644 --- a/source/java/org/alfresco/repo/rendition/executer/CompositeRenderingEngine.java +++ b/source/java/org/alfresco/repo/rendition/executer/CompositeRenderingEngine.java @@ -82,14 +82,19 @@ public class CompositeRenderingEngine extends AbstractRenderingEngine NodeRef parent = definition.getRenditionParent(); for (RenditionDefinition subDefinition : definition.getActions()) { - ChildAssociationRef newResult = executeSubDefinition(source, subDefinition, parent, assocType); + ChildAssociationRef nextResult = executeSubDefinition(source, subDefinition, parent, assocType); if (result != null) { // Clean up temporary renditions. nodeService.removeChild(parent, result.getChildRef()); + + if (logger.isDebugEnabled()) + { + logger.debug("removeChild parentRef:" + parent + ", childRef;" + result.getChildRef()); + } } - result = newResult; - source = newResult.getChildRef(); + result = nextResult; + source = nextResult.getChildRef(); } return result; } diff --git a/source/java/org/alfresco/repo/rendition/executer/HTMLRenderingEngine.java b/source/java/org/alfresco/repo/rendition/executer/HTMLRenderingEngine.java index f04aa6155e..5674632329 100644 --- a/source/java/org/alfresco/repo/rendition/executer/HTMLRenderingEngine.java +++ b/source/java/org/alfresco/repo/rendition/executer/HTMLRenderingEngine.java @@ -400,6 +400,10 @@ public class HTMLRenderingEngine extends AbstractRenderingEngine renderingContext.getDestinationNode() ); imgFolder = location.getParentRef(); + if (logger.isDebugEnabled()) + { + logger.debug("Using imgFolder: " + imgFolder); + } } } diff --git a/source/java/org/alfresco/repo/rendition/executer/HTMLRenderingEngineTest.java b/source/java/org/alfresco/repo/rendition/executer/HTMLRenderingEngineTest.java index 27a4a45875..d91eb81416 100644 --- a/source/java/org/alfresco/repo/rendition/executer/HTMLRenderingEngineTest.java +++ b/source/java/org/alfresco/repo/rendition/executer/HTMLRenderingEngineTest.java @@ -422,7 +422,7 @@ public class HTMLRenderingEngineTest extends BaseAlfrescoSpringTest * * TODO Re-enable when we've figured out why the rendition service sulkts */ - public void DISABLEDtestImagesSameFolder() throws Exception + public void testImagesSameFolder() throws Exception { def.setParameterValue( RenditionService.PARAM_DESTINATION_PATH_TEMPLATE, @@ -441,6 +441,10 @@ public class HTMLRenderingEngineTest extends BaseAlfrescoSpringTest String baseName = name.substring(0, name.lastIndexOf('.')); int numItemsStart = nodeService.getChildAssocs(targetFolder).size(); + if (log.isDebugEnabled()) + { + log.debug("targetFolder " + targetFolder + " has " + numItemsStart + " children at start."); + } ChildAssociationRef rendition = renditionService.render(sourceDoc, def); assertNotNull(rendition); @@ -508,6 +512,14 @@ public class HTMLRenderingEngineTest extends BaseAlfrescoSpringTest } } assertEquals(expectedImageCount, images); + + // Until the rendition service supports a forced overwrite of other renditions, we must + // delete the old rendition node & the images. + nodeService.deleteNode(rendition.getChildRef()); + for (ChildAssociationRef chAssRef : nodeService.getChildAssocs(targetFolder)) + { + nodeService.deleteNode(chAssRef.getChildRef()); + } } } }