ACS-3429: Better fitting exceptions on POST rule with actions (#1376)

* ACS-3429: Better fitting exceptions on POST rule with actions when missing or invalid fields.

* ACS-3429: Fixing and adding tests

packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/CreateRulesTests.java

@@ -393,6 +393,42 @@ public class CreateRulesTests extends RestTest
                 .assertThat().field("isShared").isNull();
     }
 
+    /**
+     * Check we get error when attempt to create a rule without any actions.
+     */
+    @Test(groups = {TestGroup.REST_API, TestGroup.RULES})
+    public void createRuleWithoutActionsShouldFail()
+    {
+        final RestRuleModel ruleModel = createRuleModelWithDefaultValues();
+        ruleModel.setActions(null);
+
+        restClient.authenticateUser(user).withCoreAPI().usingNode(ruleFolder).usingDefaultRuleSet()
+                .createSingleRule(ruleModel);
+
+        restClient.assertStatusCodeIs(BAD_REQUEST);
+        restClient.assertLastError().containsSummary("A rule must have at least one action");
+    }
+
+    /**
+     * Check we get error when attempt to create a rule with invalid action.
+     */
+    @Test(groups = {TestGroup.REST_API, TestGroup.RULES})
+    public void createRuleWithInvalidActionsShouldFail()
+    {
+        final RestRuleModel ruleModel = createRuleModelWithDefaultValues();
+        final RestActionBodyExecTemplateModel invalidAction = new RestActionBodyExecTemplateModel();
+        final String actionDefinitionId = "invalid-definition-value";
+        invalidAction.setActionDefinitionId(actionDefinitionId);
+        invalidAction.setParams(Map.of("dummy-key", "dummy-value"));;
+        ruleModel.setActions(List.of(invalidAction));
+
+        restClient.authenticateUser(user).withCoreAPI().usingNode(ruleFolder).usingDefaultRuleSet()
+                .createSingleRule(ruleModel);
+
+        restClient.assertStatusCodeIs(NOT_FOUND);
+        restClient.assertLastError().containsSummary(actionDefinitionId);
+    }
+
     /**
      * Check we can create a rule with multiple conditions
      */

packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/rules/UpdateRulesTests.java

@@ -27,6 +27,7 @@ package org.alfresco.rest.rules;
 
 import static org.alfresco.rest.rules.RulesTestsUtils.createDefaultActionModel;
 import static org.alfresco.rest.rules.RulesTestsUtils.createRuleModel;
+import static org.alfresco.rest.rules.RulesTestsUtils.createRuleModelWithDefaultValues;
 import static org.alfresco.utility.constants.UserRole.SiteCollaborator;
 import static org.alfresco.utility.report.log.Step.STEP;
 import static org.springframework.http.HttpStatus.BAD_REQUEST;
@@ -35,6 +36,7 @@ import static org.springframework.http.HttpStatus.NOT_FOUND;
 import static org.springframework.http.HttpStatus.OK;
 
 import java.util.List;
+import java.util.Map;
 
 import com.google.common.collect.ImmutableMap;
 
@@ -200,6 +202,46 @@ public class UpdateRulesTests extends RestTest
         updatedRule.assertThat().field("isShared").isNotNull();
     }
 
+    /**
+     * Check we get error when attempt to update a rule to one without any actions.
+     */
+    @Test(groups = {TestGroup.REST_API, TestGroup.RULES})
+    public void updateRuleWithoutActionsShouldFail()
+    {
+        RestRuleModel rule = createAndSaveRule("Rule name");
+
+        STEP("Try to update the rule - set no actions.");
+        rule.setActions(null);
+        restClient.authenticateUser(user).withCoreAPI().usingNode(ruleFolder).usingDefaultRuleSet()
+                .include("isShared")
+                .updateRule(rule.getId(), rule);
+
+        restClient.assertStatusCodeIs(BAD_REQUEST);
+        restClient.assertLastError().containsSummary("A rule must have at least one action");
+    }
+
+    /**
+     * Check we get error when attempt to update a rule to one with invalid action.
+     */
+    @Test(groups = {TestGroup.REST_API, TestGroup.RULES})
+    public void updateRuleWithInvalidActionsShouldFail()
+    {
+        RestRuleModel rule = createAndSaveRule("Rule name");
+
+        STEP("Try to update the rule - set no actions.");
+        final RestActionBodyExecTemplateModel invalidAction = new RestActionBodyExecTemplateModel();
+        final String actionDefinitionId = "invalid-definition-value";
+        invalidAction.setActionDefinitionId(actionDefinitionId);
+        invalidAction.setParams(Map.of("dummy-key", "dummy-value"));;
+        rule.setActions(List.of(invalidAction));
+        restClient.authenticateUser(user).withCoreAPI().usingNode(ruleFolder).usingDefaultRuleSet()
+                .include("isShared")
+                .updateRule(rule.getId(), rule);
+
+        restClient.assertStatusCodeIs(NOT_FOUND);
+        restClient.assertLastError().containsSummary(actionDefinitionId);
+    }
+
     /** Check we can use the POST response to create the new rule. */
     @Test (groups = { TestGroup.REST_API, TestGroup.RULES, TestGroup.SANITY })
     public void updateCopyRuleWithResponseFromPOST()

remote-api/src/main/java/org/alfresco/rest/api/impl/rules/ActionParameterConverter.java

@@ -48,6 +48,7 @@ import org.alfresco.service.namespace.NamespaceService;
 import org.alfresco.service.namespace.QName;
 import org.json.JSONArray;
 import org.json.JSONException;
+import org.springframework.beans.factory.NoSuchBeanDefinitionException;
 
 @Experimental
 public class ActionParameterConverter
@@ -67,9 +68,15 @@ public class ActionParameterConverter
     Map<String, Serializable> getConvertedParams(Map<String, Serializable> params, String name)
     {
         final Map<String, Serializable> parameters = new HashMap<>(params.size());
-        final ParameterizedItemDefinition definition = actionService.getActionDefinition(name);
+        final ParameterizedItemDefinition definition;
-        if (definition == null)
+        try
         {
+            definition = actionService.getActionDefinition(name);
+            if (definition == null)
+            {
+                throw new NotFoundException(NotFoundException.DEFAULT_MESSAGE_ID, new String[]{name});
+            }
+        } catch (NoSuchBeanDefinitionException e) {
             throw new NotFoundException(NotFoundException.DEFAULT_MESSAGE_ID, new String[]{name});
         }
 

remote-api/src/main/java/org/alfresco/rest/api/impl/rules/RulesImpl.java

@@ -34,6 +34,7 @@ import org.alfresco.rest.api.Nodes;
 import org.alfresco.rest.api.Rules;
 import org.alfresco.rest.api.model.rules.Rule;
 import org.alfresco.rest.api.model.rules.RuleSet;
+import org.alfresco.rest.framework.core.exceptions.InvalidArgumentException;
 import org.alfresco.rest.framework.resource.parameters.CollectionWithPagingInfo;
 import org.alfresco.rest.framework.resource.parameters.ListPage;
 import org.alfresco.rest.framework.resource.parameters.Paging;
@@ -41,6 +42,7 @@ import org.alfresco.service.Experimental;
 import org.alfresco.service.cmr.action.CompositeAction;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.rule.RuleService;
+import org.alfresco.util.collections.CollectionUtils;
 import org.alfresco.service.namespace.NamespaceService;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -49,6 +51,7 @@ import org.slf4j.LoggerFactory;
 public class RulesImpl implements Rules
 {
     private static final Logger LOGGER = LoggerFactory.getLogger(RulesImpl.class);
+    private static final String MUST_HAVE_AT_LEAST_ONE_ACTION = "A rule must have at least one action";
 
     private Nodes nodes;
     private RuleService ruleService;
@@ -126,6 +129,10 @@ public class RulesImpl implements Rules
 
     private org.alfresco.service.cmr.rule.Rule mapToServiceModelAndValidateActions(Rule rule)
     {
+        if (CollectionUtils.isEmpty(rule.getActions()))
+        {
+            throw new InvalidArgumentException(MUST_HAVE_AT_LEAST_ONE_ACTION);
+        }
         final org.alfresco.service.cmr.rule.Rule serviceModelRule = rule.toServiceModel(nodes, namespaceService);
         final CompositeAction compositeAction = (CompositeAction) serviceModelRule.getAction();
         compositeAction.getActions().forEach(action -> action.setParameterValues(

remote-api/src/test/java/org/alfresco/rest/api/impl/rules/ActionParameterConverterTest.java

@@ -29,6 +29,7 @@ package org.alfresco.rest.api.impl.rules;
 import static org.alfresco.service.cmr.repository.StoreRef.STORE_REF_WORKSPACE_SPACESSTORE;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.BDDMockito.given;
@@ -49,6 +50,7 @@ import org.alfresco.repo.action.executer.RemoveFeaturesActionExecuter;
 import org.alfresco.repo.action.executer.ScriptActionExecuter;
 import org.alfresco.repo.action.executer.SetPropertyValueActionExecuter;
 import org.alfresco.repo.action.executer.SimpleWorkflowActionExecuter;
+import org.alfresco.rest.framework.core.exceptions.NotFoundException;
 import org.alfresco.service.Experimental;
 import org.alfresco.service.cmr.action.ActionDefinition;
 import org.alfresco.service.cmr.action.ActionService;
@@ -553,6 +555,19 @@ public class ActionParameterConverterTest
         assertEquals(propType, convertedPropTypeParam);
     }
 
+    @Test
+    public void testInvalidActionDefinitionConversion() {
+        final String invalidName = "dummy-definition";
+        final Map<String, Serializable> params = Map.of("dummy-key", "dummy-value");
+        given(actionService.getActionDefinition(invalidName)).willReturn(null);
+        //when-then
+        assertThrows(NotFoundException.class, () ->objectUnderTest.getConvertedParams(params, invalidName));
+
+        given(actionService.getActionDefinition(invalidName)).willThrow(NotFoundException.class);
+        //when-then
+        assertThrows(NotFoundException.class, () ->objectUnderTest.getConvertedParams(params, invalidName));
+    }
+
     @Test
     public void testQnameServiceModelParamConversion() {
         given(namespaceService.getPrefixes(any())).willReturn(List.of("cm"));

remote-api/src/test/java/org/alfresco/rest/api/impl/rules/RulesImplTest.java

@@ -49,6 +49,7 @@ import junit.framework.TestCase;
 import org.alfresco.repo.action.ActionImpl;
 import org.alfresco.repo.action.CompositeActionImpl;
 import org.alfresco.rest.api.Nodes;
+import org.alfresco.rest.api.model.rules.Action;
 import org.alfresco.rest.api.model.rules.Rule;
 import org.alfresco.rest.framework.core.exceptions.EntityNotFoundException;
 import org.alfresco.rest.framework.core.exceptions.InvalidArgumentException;
@@ -103,6 +104,9 @@ public class RulesImplTest extends TestCase
     private org.alfresco.service.cmr.rule.Rule serviceRuleMock;
     @Mock
     private Rule ruleMock;
+    @Mock
+    private Action actionMock;
+
     private org.alfresco.service.cmr.rule.Rule ruleModel = createRule(RULE_ID);
     private CompositeAction compositeAction = new CompositeActionImpl(RULE_NODE_REF, "compositeActionId");
 
@@ -292,6 +296,7 @@ public class RulesImplTest extends TestCase
     {
         List<Rule> ruleList = List.of(ruleMock);
         given(ruleMock.toServiceModel(nodesMock, namespaceService)).willReturn(serviceRuleMock);
+        given(ruleMock.getActions()).willReturn(List.of(actionMock));
         given(serviceRuleMock.getAction()).willReturn(compositeAction);
         given(ruleServiceMock.saveRule(FOLDER_NODE_REF, serviceRuleMock)).willAnswer(arg -> arg.getArguments()[1]);
         given(ruleLoaderMock.loadRule(serviceRuleMock, INCLUDE)).willReturn(ruleMock);
@@ -321,6 +326,7 @@ public class RulesImplTest extends TestCase
     {
         List<Rule> ruleList = List.of(ruleMock);
         given(ruleMock.toServiceModel(nodesMock, namespaceService)).willReturn(serviceRuleMock);
+        given(ruleMock.getActions()).willReturn(List.of(actionMock));
         given(ruleServiceMock.saveRule(FOLDER_NODE_REF, serviceRuleMock)).willAnswer(arg -> arg.getArguments()[1]);
         given(ruleLoaderMock.loadRule(serviceRuleMock, INCLUDE)).willReturn(ruleMock);
         given(serviceRuleMock.getAction()).willReturn(compositeAction);
@@ -363,6 +369,7 @@ public class RulesImplTest extends TestCase
         List<Rule> expected = new ArrayList<>();
         IntStream.range(0, 3).forEach(i -> {
             Rule ruleBodyMock = mock(Rule.class);
+            given(ruleBodyMock.getActions()).willReturn(List.of(actionMock));
             ruleBodyList.add(ruleBodyMock);
             org.alfresco.service.cmr.rule.Rule serviceRuleMockInner = mock(org.alfresco.service.cmr.rule.Rule.class);
             given(ruleBodyMock.toServiceModel(nodesMock, namespaceService)).willReturn(serviceRuleMockInner);
@@ -432,6 +439,27 @@ public class RulesImplTest extends TestCase
         }
     }
 
+    /**
+     * Fail on create a rule without any actions.
+     */
+    @Test
+    public void testCreateRuleWithoutActionsShouldFail()
+    {
+        List<Rule> ruleList = List.of(ruleMock);
+        given(ruleMock.getActions()).willReturn(null);
+
+        // when
+        assertThatExceptionOfType(InvalidArgumentException.class)
+                .isThrownBy(() -> rules.createRules(FOLDER_NODE_REF.getId(), RULE_SET_NODE_REF.getId(), ruleList, INCLUDE));
+
+        then(nodeValidatorMock).should().validateFolderNode(FOLDER_NODE_ID, true);
+        then(nodeValidatorMock).should().validateRuleSetNode(RULE_SET_ID, FOLDER_NODE_REF);
+        then(nodeValidatorMock).shouldHaveNoMoreInteractions();
+        then(actionParameterConverterMock).shouldHaveNoInteractions();
+        then(actionPermissionValidatorMock).shouldHaveNoInteractions();
+        then(ruleServiceMock).shouldHaveNoInteractions();
+    }
+
     /**
      * Check that we can update a rule.
      */
@@ -439,6 +467,7 @@ public class RulesImplTest extends TestCase
     public void testUpdateRuleById()
     {
         given(ruleMock.toServiceModel(nodesMock, namespaceService)).willReturn(serviceRuleMock);
+        given(ruleMock.getActions()).willReturn(List.of(actionMock));
         given(ruleServiceMock.saveRule(FOLDER_NODE_REF, serviceRuleMock)).willAnswer(a -> a.getArguments()[1]);
         given(serviceRuleMock.getAction()).willReturn(compositeAction);
         given(ruleLoaderMock.loadRule(serviceRuleMock, INCLUDE)).willReturn(ruleMock);
@@ -520,6 +549,28 @@ public class RulesImplTest extends TestCase
         }
     }
 
+    /**
+     * Fail on update a rule without any actions.
+     */
+    @Test
+    public void testUpdateRuleWithoutActionShouldFail()
+    {
+        given(ruleMock.getActions()).willReturn(emptyList());
+
+        // when
+        assertThatExceptionOfType(InvalidArgumentException.class)
+                .isThrownBy(() -> rules.updateRuleById(FOLDER_NODE_REF.getId(), RULE_SET_NODE_REF.getId(), RULE_ID, ruleMock, INCLUDE));
+
+        then(nodeValidatorMock).should().validateFolderNode(FOLDER_NODE_ID, true);
+        then(nodeValidatorMock).should().validateRuleSetNode(RULE_SET_ID, FOLDER_NODE_REF);
+        then(nodeValidatorMock).should().validateRuleNode(RULE_ID, RULE_SET_NODE_REF);
+        then(nodeValidatorMock).shouldHaveNoMoreInteractions();
+
+        then(ruleServiceMock).shouldHaveNoInteractions();
+        then(actionParameterConverterMock).shouldHaveNoInteractions();
+        then(actionPermissionValidatorMock).shouldHaveNoInteractions();
+    }
+
     @Test
     public void testDeleteRuleById()
     {