MNT-22780: fix rule behavior (#950)

* MNT-22780 Remove properties if node has aspect

* MNT-22780 Add remove features test

* MNT-22780 Remove wildcard imports + fix test

* MNT-22780 Remove aspect when only node has aspect

* MNT-22780 Fix if statement

repository/src/main/java/org/alfresco/repo/node/db/DbNodeServiceImpl.java

@@ -2,7 +2,7 @@
  * #%L
  * Alfresco Repository
  * %%
- * Copyright (C) 2005 - 2016 Alfresco Software Limited
+ * Copyright (C) 2005 - 2022 Alfresco Software Limited
  * %%
  * This file is part of the Alfresco software. 
  * If the software was purchased under a paid Alfresco license, the terms of 
@@ -845,22 +845,21 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl implements Extens
         final Pair<Long, NodeRef> nodePair = getNodePairNotNull(nodeRef);
         final Long nodeId = nodePair.getFirst();
 
-        boolean hadAspect = nodeDAO.hasNodeAspect(nodeId, aspectTypeQName);
+        if (!nodeDAO.hasNodeAspect(nodeId, aspectTypeQName))
-        
+        {
+            return;
+        }
         // Invoke policy behaviours
         invokeBeforeUpdateNode(nodeRef);
-        if (hadAspect)
-        {
         invokeBeforeRemoveAspect(nodeRef, aspectTypeQName);
         nodeDAO.removeNodeAspects(nodeId, Collections.singleton(aspectTypeQName));
-        }
 
         AspectDefinition aspectDef = dictionaryService.getAspect(aspectTypeQName);
         boolean updated = false;
         if (aspectDef != null)
         {
             // Remove default properties
-            Map<QName,PropertyDefinition> propertyDefs = aspectDef.getProperties();
+            Map<QName, PropertyDefinition> propertyDefs = aspectDef.getProperties();
             Set<QName> propertyToRemoveQNames = propertyDefs.keySet();
             nodeDAO.removeNodeProperties(nodeId, propertyToRemoveQNames);
 
@@ -875,25 +874,20 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl implements Extens
                     return true;
                 }
 
-                @Override
+                @Override public boolean orderResults()
-                public boolean orderResults()
                 {
                     return false;
                 }
 
-                public boolean handle(
+                public boolean handle(Pair<Long, ChildAssociationRef> childAssocPair, Pair<Long, NodeRef> parentNodePair,
-                        Pair<Long, ChildAssociationRef> childAssocPair,
+                            Pair<Long, NodeRef> childNodePair)
-                        Pair<Long, NodeRef> parentNodePair,
-                        Pair<Long, NodeRef> childNodePair
-                        )
                 {
                     if (isPendingDelete(parentNodePair.getSecond()) || isPendingDelete(childNodePair.getSecond()))
                     {
                         if (logger.isTraceEnabled())
                         {
-                            logger.trace(
+                            logger.trace("Aspect-triggered association removal: "
-                                    "Aspect-triggered association removal: " +
+                                        + "Ignoring child associations where one of the nodes is pending delete: " + childAssocPair);
-                                    "Ignoring child associations where one of the nodes is pending delete: " + childAssocPair);
                         }
                         return true;
                     }
@@ -949,8 +943,8 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl implements Extens
                     if (logger.isTraceEnabled())
                     {
                         logger.trace(
-                                "Aspect-triggered association removal: " +
+                                    "Aspect-triggered association removal: " + "Ignoring peer associations where one of the nodes is pending delete: "
-                                "Ignoring peer associations where one of the nodes is pending delete: " + nodeRef);
+                                                + nodeRef);
                     }
                     continue;
                 }
@@ -967,9 +961,8 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl implements Extens
                     {
                         if (logger.isTraceEnabled())
                         {
-                            logger.trace(
+                            logger.trace("Aspect-triggered association removal: "
-                                    "Aspect-triggered association removal: " +
+                                        + "Ignoring peer associations where one of the nodes is pending delete: " + assocPair);
-                                    "Ignoring peer associations where one of the nodes is pending delete: " + assocPair);
                         }
                         continue;
                     }
@@ -996,10 +989,9 @@ public class DbNodeServiceImpl extends AbstractNodeServiceImpl implements Extens
         {
             invokeOnUpdateNode(nodeRef);
         }
-        if (hadAspect)
+
-        {
         invokeOnRemoveAspect(nodeRef, aspectTypeQName);
-        }
+
     }
 
     /**

repository/src/test/java/org/alfresco/repo/action/executer/RemoveFeaturesActionExecuterTest.java

@@ -31,10 +31,12 @@ import org.alfresco.repo.security.authentication.AuthenticationComponent;
 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.namespace.NamespaceService;
 import org.alfresco.service.namespace.QName;
 import org.alfresco.test_category.BaseSpringTestsCategory;
 import org.alfresco.util.BaseSpringTest;
 import org.alfresco.util.GUID;
+import org.alfresco.util.PropertyMap;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
@@ -130,4 +132,56 @@ public class RemoveFeaturesActionExecuterTest extends BaseSpringTest
         action2.setParameterValue(RemoveFeaturesActionExecuter.PARAM_ASPECT_NAME, ContentModel.ASPECT_VERSIONABLE);
         this.executer.execute(action2, this.nodeRef);
     }
+
+    /**
+     * Test removing aspect properties
+     */
+    @Test
+    public void testRemovingAspectPropertiesAfterExecution()
+    {
+        QName QNAME_PUBLISHER = QName.createQName(NamespaceService.CONTENT_MODEL_1_0_URI, "publisher");
+        QName QNAME_SUBJECT = QName.createQName(NamespaceService.CONTENT_MODEL_1_0_URI, "subject");
+
+        // Execute the action
+        PropertyMap dublinCoreProperties = new PropertyMap(2);
+        dublinCoreProperties.put(QNAME_PUBLISHER, "publisher");
+        dublinCoreProperties.put(QNAME_SUBJECT, "subject");
+        nodeService.addAspect(nodeRef, ContentModel.ASPECT_DUBLINCORE, dublinCoreProperties);
+
+        // Check that the node has aspect properties
+        assertTrue(this.nodeService.hasAspect(this.nodeRef, ContentModel.ASPECT_DUBLINCORE));
+        assertTrue(this.nodeService.getProperties(this.nodeRef).containsKey(QNAME_PUBLISHER));
+        assertTrue(this.nodeService.getProperties(this.nodeRef).containsKey(QNAME_SUBJECT));
+
+        // Remove the aspect
+        ActionImpl action = new ActionImpl(null, ID, RemoveFeaturesActionExecuter.NAME, null);
+        action.setParameterValue(RemoveFeaturesActionExecuter.PARAM_ASPECT_NAME, ContentModel.ASPECT_DUBLINCORE);
+        this.executer.execute(action, this.nodeRef);
+
+        // Check that the node now no longer has aspect properties
+        assertFalse(this.nodeService.getProperties(this.nodeRef).containsKey(QNAME_PUBLISHER));
+        assertFalse(this.nodeService.getProperties(this.nodeRef).containsKey(QNAME_SUBJECT));
+    }
+
+    /**
+     * Test removing not added child aspect
+     */
+    @Test
+    public void testRemovingNotAddedChildAspect()
+    {
+        QName QNAME_TITLE = QName.createQName(NamespaceService.CONTENT_MODEL_1_0_URI, "title");
+
+        // Execute the action
+        PropertyMap titledProperties = new PropertyMap(1);
+        titledProperties.put(QNAME_TITLE, "title");
+        nodeService.addAspect(nodeRef, ContentModel.ASPECT_TITLED, titledProperties);
+
+        // Remove the child aspect which has not been added to the node
+        ActionImpl action = new ActionImpl(null, ID, RemoveFeaturesActionExecuter.NAME, null);
+        action.setParameterValue(RemoveFeaturesActionExecuter.PARAM_ASPECT_NAME, ContentModel.ASPECT_DUBLINCORE);
+        this.executer.execute(action, this.nodeRef);
+
+        // Now check that the node has parent aspect properties
+        assertTrue(this.nodeService.getProperties(this.nodeRef).containsKey(QNAME_TITLE));
+    }
 }