From 1c9cad44e1001e32c3e86d005cab4d1e7c1d68f9 Mon Sep 17 00:00:00 2001 From: Ricardo Zanini Date: Mon, 27 May 2024 17:56:03 -0300 Subject: [PATCH 1/3] Fix #213 - Make a required property for Retry and verify RetryDef Signed-off-by: Ricardo Zanini --- .../utils/WorkflowUtils.java | 8 +- .../validation/WorkflowValidatorImpl.java | 96 +++++++++++++------ .../test/WorkflowValidationTest.java | 64 +++++++++++++ 3 files changed, 136 insertions(+), 32 deletions(-) diff --git a/utils/src/main/java/io/serverlessworkflow/utils/WorkflowUtils.java b/utils/src/main/java/io/serverlessworkflow/utils/WorkflowUtils.java index c2663553..671d3c50 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 static JsonNode mergeNodes(JsonNode mainNode, JsonNode updateNode) { 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 static JsonNode mergeNodes(JsonNode mainNode, JsonNode updateNode) { * @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 static JsonNode addNode(JsonNode mainNode, JsonNode toAddNode, String fie * @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 static JsonNode addArray(JsonNode mainNode, ArrayNode toAddArray, String */ 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 739f24ea..0fd91a39 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.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 List validate() { // 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 List validate() { "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 List validate() { 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 List validate() { for (OnEvents onEvents : eventsActionsList) { List 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 List validate() { 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 List validate() { } if (switchState.getEventConditions() != null - && switchState.getEventConditions().size() > 0) { + && !switchState.getEventConditions().isEmpty()) { List 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 List validate() { } if (switchState.getDataConditions() != null - && switchState.getDataConditions().size() > 0) { + && !switchState.getDataConditions().isEmpty()) { List dataConditions = switchState.getDataConditions(); for (DataCondition dc : dataConditions) { if (dc.getEnd() != null) { @@ -219,7 +233,7 @@ public List validate() { 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 List validate() { 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 @@ private void checkActionsDefinition( 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 @@ private void checkActionsDefinition( 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 @@ private void checkActionsDefinition( 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,61 @@ private void checkActionsDefinition( 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 functions) { + private boolean isMissingFunctionDefinition( + String functionName, List 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() + .filter(f -> f.getName().equals(functionName)) + .findFirst() + .orElse(null) + == null; } else { - return false; + return true; } } - private boolean haveEventsDefinition(String eventName, List events) { + private boolean isMissingEventsDefinition(String eventName, List 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().filter(e -> e.getName().equals(eventName)).findFirst().orElse(null) + == null; } else { - return false; + return true; + } + } + + private boolean isMissingRetryDefinition(String retryName, List retries) { + if (retries != null) { + return retries.stream() + .filter(f -> f.getName() != null && f.getName().equals(retryName)) + .findFirst() + .orElse(null) + == null; + } else { + return true; } } private static final Set 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)) { 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 0bc2f3e4..3e44a4c2 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,68 @@ void testActionDefForEach() { "State action 'callFn' functionRef does not reference an existing workflow function definition", validationErrors.get(0).getMessage()); } + + /** + * @see Retry definition validation doesn't work + */ + @Test + public void testValidateRetry() { + WorkflowValidator workflowValidator = new WorkflowValidatorImpl(); + List 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()); + } } From 044ceb7bbd0807e7f7acf6d39496f1ef7336caa6 Mon Sep 17 00:00:00 2001 From: Ricardo Zanini <1538000+ricardozanini@users.noreply.github.com> Date: Tue, 28 May 2024 11:48:56 -0300 Subject: [PATCH 2/3] Apply suggestions from Javi's code review Co-authored-by: Francisco Javier Tirado Sarti <65240126+fjtirado@users.noreply.github.com> --- .../validation/WorkflowValidatorImpl.java | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java b/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java index 0fd91a39..30c25887 100644 --- a/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java +++ b/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java @@ -373,11 +373,7 @@ && isMissingRetryDefinition( private boolean isMissingFunctionDefinition( String functionName, List functions) { if (functions != null) { - return functions.stream() - .filter(f -> f.getName().equals(functionName)) - .findFirst() - .orElse(null) - == null; + return !functions.stream().anyMatch(f -> f.getName().equals(functionName)); } else { return true; } @@ -388,23 +384,15 @@ private boolean isMissingEventsDefinition(String eventName, List e.getName().equals(eventName)).findFirst().orElse(null) - == null; + return !events.stream().anyMatch(e -> e.getName().equals(eventName)); } else { return true; } } private boolean isMissingRetryDefinition(String retryName, List retries) { - if (retries != null) { - return retries.stream() - .filter(f -> f.getName() != null && f.getName().equals(retryName)) - .findFirst() - .orElse(null) - == null; - } else { - return true; - } + return retries == null || ! retries.stream() + .anyMatch(f -> f.getName() != null && f.getName().equals(retryName)); } private static final Set skipMessages = From fea27038853ebebdf05e6996908569e245fabddc Mon Sep 17 00:00:00 2001 From: Ricardo Zanini Date: Tue, 28 May 2024 12:44:37 -0300 Subject: [PATCH 3/3] Rebase with latest changes Signed-off-by: Ricardo Zanini --- .../validation/WorkflowValidatorImpl.java | 15 ++- .../test/WorkflowValidationTest.java | 103 +++++++++--------- 2 files changed, 59 insertions(+), 59 deletions(-) diff --git a/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java b/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java index 30c25887..972593f4 100644 --- a/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java +++ b/validation/src/main/java/io/serverlessworkflow/validation/WorkflowValidatorImpl.java @@ -359,13 +359,12 @@ private void checkActionsDefinition( } if (action.getRetryRef() != null - && isMissingRetryDefinition( - action.getRetryRef(), workflow.getRetries().getRetryDefs())) { + && 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); + String.format( + "Operation State action '%s' retryRef does not reference an existing workflow retry definition", + action.getName()), + ValidationError.WORKFLOW_VALIDATION); } } } @@ -391,8 +390,8 @@ private boolean isMissingEventsDefinition(String eventName, List retries) { - return retries == null || ! retries.stream() - .anyMatch(f -> f.getName() != null && f.getName().equals(retryName)); + return retries == null + || !retries.stream().anyMatch(f -> f.getName() != null && f.getName().equals(retryName)); } private static final Set skipMessages = 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 3e44a4c2..81d21330 100644 --- a/validation/src/test/java/io/serverlessworkflow/validation/test/WorkflowValidationTest.java +++ b/validation/src/test/java/io/serverlessworkflow/validation/test/WorkflowValidationTest.java @@ -369,66 +369,67 @@ void testActionDefForEach() { } /** - * @see Retry definition validation doesn't work + * @see Retry definition + * validation doesn't work */ @Test public void testValidateRetry() { WorkflowValidator workflowValidator = new WorkflowValidatorImpl(); List 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(); + 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()); + "Operation State action 'null' retryRef does not reference an existing workflow retry definition", + validationErrors.get(1).getMessage()); } }