Repository: serverlessworkflow/sdk-java (java)

Issue:
Retry definition validation doesn't work 
**Given**:
Workflow definition to be validated

```json
{
  "id": "workflow_1",
  "name": "workflow_1",
  "description": "workflow_1",
  "version": "1.0",
  "specVersion": "0.8",
  "start": "Task1",
  "functions": [
    {
      "name": "increment",
      "type": "custom",
      "operation": "worker"
    }
  ],
  "retries": [
    {
      "maxAttempts": 3
    }
  ],
  "states": [
    {
      "name": "Task1",
      "type": "operation",
      "actionMode": "sequential",
      "actions": [
        {
          "functionRef": {
            "refName": "increment",
            "arguments": {
              "input": "some text"
            }
          },
          "retryRef": "const",
          "actionDataFilter": {
            "toStateData": "${ .result }"
          }
        }
      ],
      "end": true
    }
  ]
}
```

**Expected result:**
Validation failed (because of `name` of `RetryDefinition` missed)

**Actual result:**
Validation passed.

The same result(validation passed) if 'name' field is empty or/and `maxAttempts` not defined.
According to specification 'name ' is required and `maxAttempts` not.
But in the `retrydef.json` schema they are required both.

Could you please support us in this question? May be we miss something?


Pull request:
Fix #213 - Make  a required property for Retry and verify RetryDef
**Many thanks for submitting your Pull Request :heart:!**

**What this PR does / why we need it**:
Fixes #213 

**Special notes for reviewers**:
I've also removed a few deprecated usages (see put/set) and simplified a few methods in the class I was working on.

**Additional information (if needed):**

Patch:
diff --git a/utils/src/main/java/io/serverlessworkflow/utils/WorkflowUtils.java b/utils/src/main/java/io/serverlessworkflow/utils/WorkflowUtils.java
index c266355..671d3c5 100644
--- a/utils/src/main/java/io/serverlessworkflow/utils/WorkflowUtils.java
+++ b/utils/src/main/java/io/serverlessworkflow/utils/WorkflowUtils.java
@@ -584,7 +584,7 @@ public final class WorkflowUtils {
         if (mainNode instanceof ObjectNode) {
           // Overwrite field
           JsonNode value = updateNode.get(fieldName);
-          ((ObjectNode) mainNode).put(fieldName, value);
+          ((ObjectNode) mainNode).set(fieldName, value);
         }
       }
     }
@@ -601,7 +601,7 @@ public final class WorkflowUtils {
    * @return original, main node with field added
    */
   public static JsonNode addNode(JsonNode mainNode, JsonNode toAddNode, String fieldName) {
-    ((ObjectNode) mainNode).put(fieldName, toAddNode);
+    ((ObjectNode) mainNode).set(fieldName, toAddNode);
     return mainNode;
   }
 
@@ -614,7 +614,7 @@ public final class WorkflowUtils {
    * @return original, main node with array added
    */
   public static JsonNode addArray(JsonNode mainNode, ArrayNode toAddArray, String arrayName) {
-    ((ObjectNode) mainNode).put(arrayName, toAddArray);
+    ((ObjectNode) mainNode).set(arrayName, toAddArray);
     return mainNode;
   }
 
@@ -628,7 +628,7 @@ public final class WorkflowUtils {
    */
   public static JsonNode addFieldValue(JsonNode mainNode, Object toAddValue, String fieldName) {
     ObjectMapper mapper = new ObjectMapper();
-    ((ObjectNode) mainNode).put(fieldName, mapper.valueToTree(toAddValue));
+    ((ObjectNode) mainNode).set(fieldName, mapper.valueToTree(toAddValue));
     return mainNode;
   }
 
diff --git a/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java b/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java
index 739f24e..972593f 100644
--- a/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java
+++ b/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java
@@ -25,6 +25,7 @@ import io.serverlessworkflow.api.events.OnEvents;
 import io.serverlessworkflow.api.functions.FunctionDefinition;
 import io.serverlessworkflow.api.interfaces.State;
 import io.serverlessworkflow.api.interfaces.WorkflowValidator;
+import io.serverlessworkflow.api.retry.RetryDefinition;
 import io.serverlessworkflow.api.states.*;
 import io.serverlessworkflow.api.switchconditions.DataCondition;
 import io.serverlessworkflow.api.switchconditions.EventCondition;
@@ -78,7 +79,7 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
 
     // if there are schema validation errors
     // there is no point of doing the workflow validation
-    if (validationErrors.size() > 0) {
+    if (!validationErrors.isEmpty()) {
       return validationErrors;
     } else if (workflow == null) {
       workflow = Workflow.fromSource(source);
@@ -101,6 +102,19 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
           "Workflow version should not be empty", ValidationError.WORKFLOW_VALIDATION);
     }
 
+    if (workflow.getRetries() != null && workflow.getRetries().getRetryDefs() != null) {
+      workflow
+          .getRetries()
+          .getRetryDefs()
+          .forEach(
+              r -> {
+                if (r.getName() == null || r.getName().isEmpty()) {
+                  addValidationError(
+                      "Retry name should not be empty", ValidationError.WORKFLOW_VALIDATION);
+                }
+              });
+    }
+
     if (workflow.getStates() == null || workflow.getStates().isEmpty()) {
       addValidationError("No states found", ValidationError.WORKFLOW_VALIDATION);
     }
@@ -149,7 +163,7 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
 
                 if (s instanceof EventState) {
                   EventState eventState = (EventState) s;
-                  if (eventState.getOnEvents() == null || eventState.getOnEvents().size() < 1) {
+                  if (eventState.getOnEvents() == null || eventState.getOnEvents().isEmpty()) {
                     addValidationError(
                         "Event State has no eventActions defined",
                         ValidationError.WORKFLOW_VALIDATION);
@@ -158,13 +172,13 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
                   for (OnEvents onEvents : eventsActionsList) {
 
                     List<String> eventRefs = onEvents.getEventRefs();
-                    if (eventRefs == null || eventRefs.size() < 1) {
+                    if (eventRefs == null || eventRefs.isEmpty()) {
                       addValidationError(
                           "Event State eventsActions has no event refs",
                           ValidationError.WORKFLOW_VALIDATION);
                     } else {
                       for (String eventRef : eventRefs) {
-                        if (!haveEventsDefinition(eventRef, events)) {
+                        if (isMissingEventsDefinition(eventRef, events)) {
                           addValidationError(
                               "Event State eventsActions eventRef does not match a declared workflow event definition",
                               ValidationError.WORKFLOW_VALIDATION);
@@ -177,9 +191,9 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
                 if (s instanceof SwitchState) {
                   SwitchState switchState = (SwitchState) s;
                   if ((switchState.getDataConditions() == null
-                          || switchState.getDataConditions().size() < 1)
+                          || switchState.getDataConditions().isEmpty())
                       && (switchState.getEventConditions() == null
-                          || switchState.getEventConditions().size() < 1)) {
+                          || switchState.getEventConditions().isEmpty())) {
                     addValidationError(
                         "Switch state should define either data or event conditions",
                         ValidationError.WORKFLOW_VALIDATION);
@@ -192,10 +206,10 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
                   }
 
                   if (switchState.getEventConditions() != null
-                      && switchState.getEventConditions().size() > 0) {
+                      && !switchState.getEventConditions().isEmpty()) {
                     List<EventCondition> eventConditions = switchState.getEventConditions();
                     for (EventCondition ec : eventConditions) {
-                      if (!haveEventsDefinition(ec.getEventRef(), events)) {
+                      if (isMissingEventsDefinition(ec.getEventRef(), events)) {
                         addValidationError(
                             "Switch state event condition eventRef does not reference a defined workflow event",
                             ValidationError.WORKFLOW_VALIDATION);
@@ -207,7 +221,7 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
                   }
 
                   if (switchState.getDataConditions() != null
-                      && switchState.getDataConditions().size() > 0) {
+                      && !switchState.getDataConditions().isEmpty()) {
                     List<DataCondition> dataConditions = switchState.getDataConditions();
                     for (DataCondition dc : dataConditions) {
                       if (dc.getEnd() != null) {
@@ -219,7 +233,7 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
 
                 if (s instanceof SleepState) {
                   SleepState sleepState = (SleepState) s;
-                  if (sleepState.getDuration() == null || sleepState.getDuration().length() < 1) {
+                  if (sleepState.getDuration() == null || sleepState.getDuration().isEmpty()) {
                     addValidationError(
                         "Sleep state should have a non-empty time delay",
                         ValidationError.WORKFLOW_VALIDATION);
@@ -260,13 +274,13 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
                 if (s instanceof CallbackState) {
                   CallbackState callbackState = (CallbackState) s;
 
-                  if (!haveEventsDefinition(callbackState.getEventRef(), events)) {
+                  if (isMissingEventsDefinition(callbackState.getEventRef(), events)) {
                     addValidationError(
                         "CallbackState event ref does not reference a defined workflow event definition",
                         ValidationError.WORKFLOW_VALIDATION);
                   }
 
-                  if (!haveFunctionDefinition(
+                  if (isMissingFunctionDefinition(
                       callbackState.getAction().getFunctionRef().getRefName(), functions)) {
                     addValidationError(
                         "CallbackState action function ref does not reference a defined workflow function definition",
@@ -316,7 +330,7 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
               ValidationError.WORKFLOW_VALIDATION);
         }
 
-        if (!haveFunctionDefinition(action.getFunctionRef().getRefName(), functions)) {
+        if (isMissingFunctionDefinition(action.getFunctionRef().getRefName(), functions)) {
           addValidationError(
               String.format(
                   "State action '%s' functionRef does not reference an existing workflow function definition",
@@ -327,7 +341,7 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
 
       if (action.getEventRef() != null) {
 
-        if (!haveEventsDefinition(action.getEventRef().getTriggerEventRef(), events)) {
+        if (isMissingEventsDefinition(action.getEventRef().getTriggerEventRef(), events)) {
           addValidationError(
               String.format(
                   "State action '%s' trigger event def does not reference an existing workflow event definition",
@@ -335,7 +349,7 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
               ValidationError.WORKFLOW_VALIDATION);
         }
 
-        if (!haveEventsDefinition(action.getEventRef().getResultEventRef(), events)) {
+        if (isMissingEventsDefinition(action.getEventRef().getResultEventRef(), events)) {
           addValidationError(
               String.format(
                   "State action '%s' results event def does not reference an existing workflow event definition",
@@ -343,35 +357,48 @@ public class WorkflowValidatorImpl implements WorkflowValidator {
               ValidationError.WORKFLOW_VALIDATION);
         }
       }
+
+      if (action.getRetryRef() != null
+          && isMissingRetryDefinition(action.getRetryRef(), workflow.getRetries().getRetryDefs())) {
+        addValidationError(
+            String.format(
+                "Operation State action '%s' retryRef does not reference an existing workflow retry definition",
+                action.getName()),
+            ValidationError.WORKFLOW_VALIDATION);
+      }
     }
   }
 
-  private boolean haveFunctionDefinition(String functionName, List<FunctionDefinition> functions) {
+  private boolean isMissingFunctionDefinition(
+      String functionName, List<FunctionDefinition> functions) {
     if (functions != null) {
-      FunctionDefinition fun =
-          functions.stream().filter(f -> f.getName().equals(functionName)).findFirst().orElse(null);
-
-      return fun == null ? false : true;
+      return !functions.stream().anyMatch(f -> f.getName().equals(functionName));
     } else {
-      return false;
+      return true;
     }
   }
 
-  private boolean haveEventsDefinition(String eventName, List<EventDefinition> events) {
+  private boolean isMissingEventsDefinition(String eventName, List<EventDefinition> events) {
     if (eventName == null) {
-      return true;
+      return false;
     }
     if (events != null) {
-      EventDefinition eve =
-          events.stream().filter(e -> e.getName().equals(eventName)).findFirst().orElse(null);
-      return eve == null ? false : true;
+      return !events.stream().anyMatch(e -> e.getName().equals(eventName));
     } else {
-      return false;
+      return true;
     }
   }
 
+  private boolean isMissingRetryDefinition(String retryName, List<RetryDefinition> retries) {
+    return retries == null
+        || !retries.stream().anyMatch(f -> f.getName() != null && f.getName().equals(retryName));
+  }
+
   private static final Set<String> skipMessages =
-      Set.of("$.start: string found, object expected", "$.functions: array found, object expected");
+      Set.of(
+          "$.start: string found, object expected",
+          "$.functions: array found, object expected",
+          "$.retries: array found, object expected");
 
   private void addValidationError(String message, String type) {
     if (skipMessages.contains(message)) {


Test patch:
diff --git a/validation/src/test/java/io/serverlessworkflow/validation/test/WorkflowValidationTest.java b/validation/src/test/java/io/serverlessworkflow/validation/test/WorkflowValidationTest.java
index 0bc2f3e..81d2133 100644
--- a/validation/src/test/java/io/serverlessworkflow/validation/test/WorkflowValidationTest.java
+++ b/validation/src/test/java/io/serverlessworkflow/validation/test/WorkflowValidationTest.java
@@ -367,4 +367,69 @@ public class WorkflowValidationTest {
         "State action 'callFn' functionRef does not reference an existing workflow function definition",
         validationErrors.get(0).getMessage());
   }
+
+  /**
+   * @see <a href="https://github.com/serverlessworkflow/sdk-java/issues/213">Retry definition
+   *     validation doesn't work</a>
+   */
+  @Test
+  public void testValidateRetry() {
+    WorkflowValidator workflowValidator = new WorkflowValidatorImpl();
+    List<ValidationError> validationErrors =
+        workflowValidator
+            .setSource(
+                "{\n"
+                    + "  \"id\": \"workflow_1\",\n"
+                    + "  \"name\": \"workflow_1\",\n"
+                    + "  \"description\": \"workflow_1\",\n"
+                    + "  \"version\": \"1.0\",\n"
+                    + "  \"specVersion\": \"0.8\",\n"
+                    + "  \"start\": \"Task1\",\n"
+                    + "  \"functions\": [\n"
+                    + "    {\n"
+                    + "      \"name\": \"increment\",\n"
+                    + "      \"type\": \"custom\",\n"
+                    + "      \"operation\": \"worker\"\n"
+                    + "    }\n"
+                    + "  ],\n"
+                    + "  \"retries\": [\n"
+                    + "    {\n"
+                    + "      \"maxAttempts\": 3\n"
+                    + "    },\n"
+                    + "    {\n"
+                    + "      \"name\": \"testRetry\" \n"
+                    + "    }\n"
+                    + "  ],\n"
+                    + "  \"states\": [\n"
+                    + "    {\n"
+                    + "      \"name\": \"Task1\",\n"
+                    + "      \"type\": \"operation\",\n"
+                    + "      \"actionMode\": \"sequential\",\n"
+                    + "      \"actions\": [\n"
+                    + "        {\n"
+                    + "          \"functionRef\": {\n"
+                    + "            \"refName\": \"increment\",\n"
+                    + "            \"arguments\": {\n"
+                    + "              \"input\": \"some text\"\n"
+                    + "            }\n"
+                    + "          },\n"
+                    + "          \"retryRef\": \"const\",\n"
+                    + "          \"actionDataFilter\": {\n"
+                    + "            \"toStateData\": \"${ .result }\"\n"
+                    + "          }\n"
+                    + "        }\n"
+                    + "      ],\n"
+                    + "      \"end\": true\n"
+                    + "    }\n"
+                    + "  ]\n"
+                    + "}")
+            .validate();
+
+    Assertions.assertNotNull(validationErrors);
+    Assertions.assertEquals(2, validationErrors.size());
+    Assertions.assertEquals("Retry name should not be empty", validationErrors.get(0).getMessage());
+    Assertions.assertEquals(
+        "Operation State action 'null' retryRef does not reference an existing workflow retry definition",
+        validationErrors.get(1).getMessage());
+  }
 }