diff --git a/source/java/org/alfresco/repo/node/BaseNodeServiceTest.java b/source/java/org/alfresco/repo/node/BaseNodeServiceTest.java index 681bbcf806..62af5cf676 100644 --- a/source/java/org/alfresco/repo/node/BaseNodeServiceTest.java +++ b/source/java/org/alfresco/repo/node/BaseNodeServiceTest.java @@ -1011,6 +1011,7 @@ public abstract class BaseNodeServiceTest extends BaseSpringTest private List beforeDeleteNodeRefs; private boolean onDeleteCreateChild = true; + private boolean beforeDeleteCreateChild = true; public BadOnDeleteNodePolicy(NodeService nodeService, List beforeDeleteNodeRefs, @@ -1028,8 +1029,9 @@ public abstract class BaseNodeServiceTest extends BaseSpringTest // add the child to the list beforeDeleteNodeRefs.add(nodeRef); - if(onDeleteCreateChild) + if(beforeDeleteCreateChild) { + System.out.println("before delete node - add child."); // add a new child to the child, i.e. just before it is deleted ChildAssociationRef assocRef = nodeService.createNode( nodeRef, @@ -1051,6 +1053,7 @@ public abstract class BaseNodeServiceTest extends BaseSpringTest if(onDeleteCreateChild) { + System.out.println("on delete node - add sibling."); // now perform some nasties on the node's parent, i.e. add a new child NodeRef parentRef = childAssocRef.getParentRef(); NodeRef childRef = childAssocRef.getChildRef(); @@ -1072,6 +1075,16 @@ public abstract class BaseNodeServiceTest extends BaseSpringTest return onDeleteCreateChild; } + private void setBeforeDeleteCreateChild(boolean beforeDeleteCreateChild) + { + this.beforeDeleteCreateChild = beforeDeleteCreateChild; + } + + private boolean isBeforeDeleteCreateChild() + { + return beforeDeleteCreateChild; + } + } public void testDelete() throws Exception @@ -1081,6 +1094,7 @@ public abstract class BaseNodeServiceTest extends BaseSpringTest BadOnDeleteNodePolicy nasty = new BadOnDeleteNodePolicy(nodeService, beforeDeleteNodeRefs, deletedNodeRefs); nasty.setOnDeleteCreateChild(false); + nasty.setBeforeDeleteCreateChild(false); NodeServicePolicies.OnDeleteNodePolicy policy = nasty; // bind to listen to the deletion of a node @@ -1127,67 +1141,122 @@ public abstract class BaseNodeServiceTest extends BaseSpringTest endTransaction(); } -// /** -// * This test is similar to the test above but the delete policies do nasty stuff such as -// * creating children of the soon to be deleted children. -// * -// * In particular, it verifies that we don't get stuck in an infinite loop. -// * @throws Exception -// */ -// public void testDeleteWithBadlyBehavedPolicies() throws Exception -// { -// try -// { -// final List beforeDeleteNodeRefs = new ArrayList(5); -// final List deletedNodeRefs = new ArrayList(5); -// -// BadOnDeleteNodePolicy nasty = new BadOnDeleteNodePolicy(nodeService, beforeDeleteNodeRefs, deletedNodeRefs); -// nasty.setOnDeleteCreateChild(true); -// NodeServicePolicies.OnDeleteNodePolicy policy = nasty; -// -// // bind to listen to the deletion of a node -// policyComponent.bindClassBehaviour( -// QName.createQName(NamespaceService.ALFRESCO_URI, "onDeleteNode"), -// policy, -// new JavaBehaviour(policy, "onDeleteNode")); -// -// policyComponent.bindClassBehaviour( -// QName.createQName(NamespaceService.ALFRESCO_URI, "beforeDeleteNode"), -// policy, -// new JavaBehaviour(policy, "beforeDeleteNode")); -// -// // build the node and commit the node graph -// Map assocRefs = buildNodeGraph(nodeService, rootNodeRef); -// NodeRef n1Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "root_p_n1")).getChildRef(); -// NodeRef n3Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n1_p_n3")).getChildRef(); -// NodeRef n4Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n2_p_n4")).getChildRef(); -// NodeRef n6Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n3_p_n6")).getChildRef(); -// NodeRef n8Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n6_p_n8")).getChildRef(); -// -// // delete n1 -// nodeService.deleteNode(n1Ref); -// -// // turn off nasty policy - may upset other tests -// nasty.setOnDeleteCreateChild(false); -// -// // Just a cut down set of tests to validate that something has happened, the real point of the test is to see how -// // the end of the transaction fails. -// -// assertEquals("Node not directly deleted", 0, countNodesByReference(n1Ref)); -// assertTrue("n1Ref before delete policy not called", beforeDeleteNodeRefs.contains(n1Ref)); -// assertTrue("n1Ref delete policy not called", deletedNodeRefs.contains(n1Ref)); -// -// // commit to check -// setComplete(); -// endTransaction(); -// fail("test has not detected orphan children"); -// } -// catch (Exception e) -// { -// // We expect to get here with this test. -// e.printStackTrace(); -// } -// } + /** + * This test is similar to the test above but onDelete does nasty stuff such as + * creating siblings of the soon to be deleted children. + * + * In particular, it verifies that we don't get stuck in an infinite loop. + * @throws Exception + */ + public void testDeleteWithBadlyBehavedOnDeletePolicies() throws Exception + { + final List beforeDeleteNodeRefs = new ArrayList(5); + final List deletedNodeRefs = new ArrayList(5); + BadOnDeleteNodePolicy nasty = new BadOnDeleteNodePolicy(nodeService, beforeDeleteNodeRefs, deletedNodeRefs); + + try + { + nasty.setOnDeleteCreateChild(true); + nasty.setBeforeDeleteCreateChild(false); + NodeServicePolicies.OnDeleteNodePolicy policy = nasty; + + // bind to listen to the deletion of a node + policyComponent.bindClassBehaviour( + QName.createQName(NamespaceService.ALFRESCO_URI, "onDeleteNode"), + policy, + new JavaBehaviour(policy, "onDeleteNode")); + + policyComponent.bindClassBehaviour( + QName.createQName(NamespaceService.ALFRESCO_URI, "beforeDeleteNode"), + policy, + new JavaBehaviour(policy, "beforeDeleteNode")); + + // build the node and commit the node graph + Map assocRefs = buildNodeGraph(nodeService, rootNodeRef); + NodeRef n1Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "root_p_n1")).getChildRef(); + NodeRef n3Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n1_p_n3")).getChildRef(); + NodeRef n4Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n2_p_n4")).getChildRef(); + NodeRef n6Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n3_p_n6")).getChildRef(); + NodeRef n8Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n6_p_n8")).getChildRef(); + + // delete n1 + nodeService.deleteNode(n1Ref); + + fail("test has not detected orphan children"); + + // commit to check + setComplete(); + endTransaction(); + + } + catch (Exception e) + { + // We expect to get here with this test. + //e.printStackTrace(); + } + finally + { + // turn off nasty policy - may upset other tests + nasty.setOnDeleteCreateChild(false); + nasty.setBeforeDeleteCreateChild(false); + } + } + /** + * This test is similar to the test above but beforeDelete does nasty stuff such as + * creating children of the soon to be deleted children. + * + * In particular, it verifies that we don't get stuck in an infinite loop. + * @throws Exception + */ + public void testDeleteWithBadlyBehavedBeforeDeletePolicies() throws Exception + { + final List beforeDeleteNodeRefs = new ArrayList(5); + final List deletedNodeRefs = new ArrayList(5); + BadOnDeleteNodePolicy nasty = new BadOnDeleteNodePolicy(nodeService, beforeDeleteNodeRefs, deletedNodeRefs); + + try + { + nasty.setOnDeleteCreateChild(false); + nasty.setBeforeDeleteCreateChild(true); + NodeServicePolicies.OnDeleteNodePolicy policy = nasty; + + // bind to listen to the deletion of a node + policyComponent.bindClassBehaviour( + QName.createQName(NamespaceService.ALFRESCO_URI, "onDeleteNode"), + policy, + new JavaBehaviour(policy, "onDeleteNode")); + + policyComponent.bindClassBehaviour( + QName.createQName(NamespaceService.ALFRESCO_URI, "beforeDeleteNode"), + policy, + new JavaBehaviour(policy, "beforeDeleteNode")); + + // build the node and commit the node graph + Map assocRefs = buildNodeGraph(nodeService, rootNodeRef); + NodeRef n1Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "root_p_n1")).getChildRef(); + NodeRef n3Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n1_p_n3")).getChildRef(); + NodeRef n4Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n2_p_n4")).getChildRef(); + NodeRef n6Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n3_p_n6")).getChildRef(); + NodeRef n8Ref = assocRefs.get(QName.createQName(BaseNodeServiceTest.NAMESPACE, "n6_p_n8")).getChildRef(); + + // delete n1 + nodeService.deleteNode(n1Ref); + + fail("test has not detected orphan children"); + + } + catch (Exception e) + { + // We expect to get here with this test. + //e.printStackTrace(); + } + finally + { + // turn off nasty policy - may upset other tests + nasty.setOnDeleteCreateChild(false); + nasty.setBeforeDeleteCreateChild(false); + } + } @SuppressWarnings("unchecked") diff --git a/source/java/org/alfresco/repo/node/db/DbNodeServiceImpl.java b/source/java/org/alfresco/repo/node/db/DbNodeServiceImpl.java index bc3689c24a..52e9524fb8 100644 --- a/source/java/org/alfresco/repo/node/db/DbNodeServiceImpl.java +++ b/source/java/org/alfresco/repo/node/db/DbNodeServiceImpl.java @@ -96,6 +96,7 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl private NodeService avmNodeService; private NodeIndexer nodeIndexer; private final static String KEY_PRE_COMMIT_ADD_NODE = "DbNodeServiceImpl.PreCommitAddNode"; + private final static String KEY_DELETED_NODES = "DbNodeServiceImpl.DeletedNodes"; public DbNodeServiceImpl() { @@ -274,6 +275,12 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl { properties = Collections.emptyMap(); } + + // check the parent node has not been deleted in this txn + if(isDeletedNodeRef(parentRef)) + { + throw new InvalidNodeRefException("The parent node has been deleted", parentRef); + } // get/generate an ID for the node String newUuid = generateGuid(properties); @@ -359,14 +366,37 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl */ private void trackNewNodeRef(NodeRef newNodeRef) { -// // bind a pre-commit listener to validate any new node associations -// Set newNodes = TransactionalResourceHelper.getSet(KEY_PRE_COMMIT_ADD_NODE); -// if (newNodes.size() == 0) -// { -// PreCommitNewNodeListener listener = new PreCommitNewNodeListener(); -// AlfrescoTransactionSupport.bindListener(listener); -// } -// newNodes.add(newNodeRef); + // bind a pre-commit listener to validate any new node associations + Set newNodes = TransactionalResourceHelper.getSet(KEY_PRE_COMMIT_ADD_NODE); + if (newNodes.size() == 0) + { + PreCommitNewNodeListener listener = new PreCommitNewNodeListener(); + AlfrescoTransactionSupport.bindListener(listener); + } + newNodes.add(newNodeRef); + } + + + + /** + * Track a deleted node + * + * The deleted node set is used to break an infinite loop which can happen when adding a new node into a path containing a + * deleted node. This transactional list is used to detect and prevent that from + * happening. + * + * @param nodeRef the deleted node to track + */ + private void trackDeletedNodeRef(NodeRef deletedNodeRef) + { + Set deletedNodes = TransactionalResourceHelper.getSet(KEY_DELETED_NODES); + deletedNodes.add(deletedNodeRef); + } + + private boolean isDeletedNodeRef(NodeRef deletedNodeRef) + { + Set deletedNodes = TransactionalResourceHelper.getSet(KEY_DELETED_NODES); + return deletedNodes.contains(deletedNodeRef); } /** @@ -375,18 +405,28 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl * for example if its been deleted or moved * @param nodeRef the node ref to untrack */ - private void untrackNodeRef(NodeRef nodeRef) + private void untrackNewNodeRef(NodeRef nodeRef) { -// Set newNodes = TransactionalResourceHelper.getSet(KEY_PRE_COMMIT_ADD_NODE); -// if (newNodes.size() > 0) -// { -// newNodes.remove(nodeRef); -// } + Set newNodes = TransactionalResourceHelper.getSet(KEY_PRE_COMMIT_ADD_NODE); + if (newNodes.size() > 0) + { + newNodes.remove(nodeRef); + } } private class PreCommitNewNodeListener extends TransactionListenerAdapter { - @Override + public void afterCommit() + { + // NO-OP + } + + public void afterRollback() + { + // NO-OP + + } + public void beforeCommit(boolean readOnly) { if (readOnly) @@ -394,19 +434,36 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl return; } Set nodeRefs = TransactionalResourceHelper.getSet(KEY_PRE_COMMIT_ADD_NODE); -// for (NodeRef nodeRef : nodeRefs) -// { -// // Need to check for exists the node may be created -// // and deleted within the same transaction -// if(exists(nodeRef)) -// { -// System.out.println("Checking bideRef " + nodeRef); -// // Check that the primary path is valid for this node -// getPaths(nodeRef, false); -// } -// } + for (NodeRef nodeRef : nodeRefs) + { + // Need to check for exists since the node may be created + // and deleted within the same transaction + if(exists(nodeRef)) + { + try + { + // Check that the primary path is valid for this node + getPaths(nodeRef, false); + } + catch (AlfrescoRuntimeException are) + { + throw new AlfrescoRuntimeException("Error while validating path:" + are.toString(), are); + } + } + } nodeRefs.clear(); } + + public void beforeCompletion() + { + // NO-OP + + } + + public void flush() + { + // NO-OP + } } @@ -858,6 +915,12 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl */ if (requiresDelete == null || requiresDelete) { + // remove the deleted node from the list of new nodes + untrackNewNodeRef(nodeRef); + + // track the deletion of this node - so we can prevent new associations to it. + trackDeletedNodeRef(nodeRef); + // Invoke policy behaviours invokeBeforeDeleteNode(nodeRef); @@ -881,10 +944,6 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl */ archiveNode(nodeRef, archiveStoreRef); } - - // remove the deleted node from the list of new nodes - untrackNodeRef(nodeRef); - } /** @@ -932,6 +991,12 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl Set childNodeQNames = nodeDaoService.getNodeAspects(childNodeId); ChildAssociationRef childParentAssocRef = childAssocRefsByChildId.get(childNodeId); + // remove the deleted node from the list of new nodes + untrackNewNodeRef(childNodeRef); + + // track the deletion of this node - so we can prevent new associations to it. + trackDeletedNodeRef(childNodeRef); + invokeBeforeDeleteNode(childNodeRef); // Cascade first @@ -943,7 +1008,7 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl invokeOnDeleteNode(childParentAssocRef, childNodeType, childNodeQNames, false); // loose interest in tracking this node ref - untrackNodeRef(childNodeRef); + untrackNewNodeRef(childNodeRef); } } @@ -2190,6 +2255,12 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl // Invoke "Before"policy behaviour if (movingStore) { + // remove the deleted node from the list of new nodes + untrackNewNodeRef(nodeToMoveRef); + + // track the deletion of this node - so we can prevent new associations to it. + trackDeletedNodeRef(nodeToMoveRef); + invokeBeforeDeleteNode(nodeToMoveRef); invokeBeforeCreateNode(newParentRef, assocTypeQName, assocQName, nodeToMoveTypeQName); } @@ -2239,9 +2310,10 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl // Ensure name uniqueness setChildNameUnique(newParentAssocPair, newNodeToMovePair); - + // Check that there is not a cyclic relationship getPaths(newNodeToMoveRef, false); + trackNewNodeRef(newNodeToMoveRef); // Call behaviours if (movingStore) @@ -2333,6 +2405,13 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl Pair newChildNodePair = oldChildNodePair; Pair newParentAssocPair = oldParentAssocPair; ChildAssociationRef newParentAssocRef = newParentAssocPair.getSecond(); + + // remove the deleted node from the list of new nodes + untrackNewNodeRef(childNodeRef); + + // track the deletion of this node - so we can prevent new associations to it. + trackDeletedNodeRef(childNodeRef); + // Fire node policies. This ensures that each node in the hierarchy gets a notification fired. invokeBeforeDeleteNode(childNodeRef); invokeBeforeCreateNode(