From 6e1d7b4face600d414396d96740af692ec96f7ff Mon Sep 17 00:00:00 2001 From: "yinan.liu" Date: Mon, 3 Oct 2022 09:22:54 +0200 Subject: [PATCH] Fix if condition issue in form reply sub process --- workflow-bot-app/build.gradle | 20 ++++--- .../engine/WorkflowDirectGraphBuilder.java | 11 ++-- .../camunda/WorkflowEventToCamundaEvent.java | 5 +- .../camunda/bpmn/CamundaBpmnBuilder.java | 23 ++++---- .../bpmn/builder/SignalNodeBuilder.java | 2 +- .../executor/message/SendMessageExecutor.java | 3 +- .../workflow/BranchingIntegrationTest.java | 26 ++++++++++ .../workflow/FormReplyIntegrationTest.java | 52 ++++++++++++++++++- .../bdk/workflow/IntegrationTest.java | 1 + .../custom/assertion/WorkflowAssert.java | 24 ++++++--- .../src/test/resources/application-test.yaml | 4 +- .../if-intermediate-event.swadl.yaml | 20 +++++++ .../send-form-reply-conditional.swadl.yaml | 38 ++++++++++++++ 13 files changed, 187 insertions(+), 42 deletions(-) create mode 100644 workflow-bot-app/src/test/resources/branching/if-intermediate-event.swadl.yaml create mode 100644 workflow-bot-app/src/test/resources/form/send-form-reply-conditional.swadl.yaml diff --git a/workflow-bot-app/build.gradle b/workflow-bot-app/build.gradle index e2c8c81e..7fcf8153 100644 --- a/workflow-bot-app/build.gradle +++ b/workflow-bot-app/build.gradle @@ -12,11 +12,15 @@ javadoc { dependencies { implementation project(':workflow-language') - implementation platform('org.finos.symphony.bdk:symphony-bdk-bom:2.9.0') + implementation platform('org.finos.symphony.bdk:symphony-bdk-bom:2.9.0') { + exclude group: 'org.slf4j', module: 'slf4j-api' + } implementation platform('com.fasterxml.jackson:jackson-bom:2.13.4') implementation platform('org.springframework.boot:spring-boot-dependencies:2.6.6') - implementation 'org.apache.httpcomponents.client5:httpclient5-fluent:5.1.3' + implementation ('org.apache.httpcomponents.client5:httpclient5-fluent:5.1.3') { + exclude group: 'org.slf4j', module: 'slf4j-api' + } implementation 'org.finos.symphony.bdk:symphony-bdk-core-spring-boot-starter' implementation 'org.finos.symphony.bdk.ext:symphony-group-extension' @@ -28,8 +32,8 @@ dependencies { runtimeOnly 'io.micrometer:micrometer-registry-prometheus' runtimeOnly 'com.h2database:h2' - implementation 'org.slf4j:slf4j-api' - runtimeOnly 'ch.qos.logback:logback-classic' + implementation 'org.slf4j:slf4j-api:1.7.36' + implementation 'ch.qos.logback:logback-classic:1.2.11' implementation 'org.aspectj:aspectjrt:1.9.9.1' implementation 'org.aspectj:aspectjweaver:1.9.9.1' @@ -63,10 +67,14 @@ dependencies { testImplementation 'org.junit.jupiter:junit-jupiter-params' testImplementation 'org.assertj:assertj-core' testImplementation 'org.awaitility:awaitility' - testImplementation 'com.github.tomakehurst:wiremock-jre8:2.34.0' + testImplementation ('com.github.tomakehurst:wiremock-jre8:2.34.0') { + exclude group: 'org.slf4j', module: 'slf4j-api' + } testImplementation 'io.rest-assured:rest-assured:4.5.1' testImplementation 'org.hamcrest:hamcrest-all:1.3' - testImplementation("org.camunda.community.mockito:camunda-platform-7-mockito:6.17.1") + testImplementation("org.camunda.community.mockito:camunda-platform-7-mockito:6.17.1") { + exclude group: 'org.slf4j', module: 'slf4j-api' + } } bootBuildImage { diff --git a/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/WorkflowDirectGraphBuilder.java b/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/WorkflowDirectGraphBuilder.java index 005b24b6..05b8ecfb 100644 --- a/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/WorkflowDirectGraphBuilder.java +++ b/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/WorkflowDirectGraphBuilder.java @@ -120,13 +120,7 @@ private void computeEvents(int activityIndex, String activityId, List directGraph.readWorkflowNode(activityId) .addIfCondition(eventNodeId, activity.getActivity().getIfCondition()); } - - if (activityIndex == 0 || !directGraph.getChildren(eventNodeId) - .getChildren() - .contains(activities.get(activityIndex - 1).getActivity().getId())) { - computeActivity(activityIndex, activities, eventNodeId, event, onEvents, directGraph); - } - + computeActivity(activityIndex, activities, eventNodeId, event, onEvents, directGraph); computeSignal(directGraph, event, eventNodeId, activityIndex, activities); } else if (event.getActivityExpired() != null) { eventNodeId = computeExpiredActivity(event, activity.getActivity().getId(), directGraph); @@ -223,7 +217,8 @@ private void computeActivity(int activityIndex, List activities, Strin if (activityIndex == 0) { validateFirstActivity(activities.get(0).getActivity(), event, workflow.getId()); directGraph.addStartEvent(nodeId); - } else { + } else if (!directGraph.getParents(activities.get(activityIndex - 1).getActivity().getId()) + .contains(nodeId)) { // the current event node is not a parent of previous activity boolean isParallel = onEvents.isParallel(); String allOfEventParentId = onEvents.getParentId(); String parentId = isParallel && StringUtils.isNotEmpty(allOfEventParentId) ? allOfEventParentId diff --git a/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/WorkflowEventToCamundaEvent.java b/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/WorkflowEventToCamundaEvent.java index 46854587..4de9de47 100644 --- a/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/WorkflowEventToCamundaEvent.java +++ b/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/WorkflowEventToCamundaEvent.java @@ -156,9 +156,7 @@ public Optional toSignalName(Event event, Workflow workflow) { } @SuppressWarnings("unchecked") - public void dispatch(RealTimeEvent event) - throws PresentationMLParserException { - + public void dispatch(RealTimeEvent event) throws PresentationMLParserException { Map processVariables = new HashMap<>(); processVariables.put(ActivityExecutorContext.EVENT, new EventHolder<>(event.getInitiator(), event.getSource(), new HashMap<>())); @@ -283,6 +281,7 @@ private void messageSentToMessage(RealTimeEvent event, // Event's message cannot be null, this if statement is only added to fix Sonar warnings if (event.getSource().getMessage() != null) { log.debug("receive message [{}]", event.getSource().getMessage().getMessageId()); + log.trace("receive message [{}]", event.getSource().getMessage().getMessage()); String presentationMl = event.getSource().getMessage().getMessage(); String receivedContent = PresentationMLParser.getTextContent(presentationMl); diff --git a/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/bpmn/CamundaBpmnBuilder.java b/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/bpmn/CamundaBpmnBuilder.java index 180524c0..05b7a591 100644 --- a/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/bpmn/CamundaBpmnBuilder.java +++ b/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/bpmn/CamundaBpmnBuilder.java @@ -41,7 +41,7 @@ @Component public class CamundaBpmnBuilder { public static final String DEPLOYMENT_RESOURCE_TOKEN_KEY = "WORKFLOW_TOKEN"; - public static final String EXCLUSIVE_GATEWAY_SUFFIX = "_ex_g"; + public static final String EXCLUSIVE_GATEWAY_SUFFIX = "_exclusive_gateway"; public static final String EVENT_GATEWAY_SUFFIX = "_event_gateway"; public static final String FORK_GATEWAY = "_fork_gateway"; @@ -145,7 +145,7 @@ private void computeChildren(WorkflowNode currentNode, AbstractFlowNodeBuilder exclusiveSubTreeNodes(String currentNodeId, WorkflowNodeType currentNodeType, AbstractFlowNodeBuilder builder, BuildProcessContext context, NodeChildren currentNodeChildren) { - if (currentNodeType == WorkflowNodeType.FORM_REPLIED_EVENT || hasFormRepliedEvent(context, - currentNodeChildren)) { - + if (currentNodeType == WorkflowNodeType.FORM_REPLIED_EVENT) { + log.trace("the node [{}] itself is a form replied event", currentNodeId); boolean activities = hasActivitiesOnly(context, currentNodeChildren); boolean conditional = hasConditionalString(context, currentNodeChildren, currentNodeId); log.trace("are the children of the node [{}]'s all activities ? [{}], is there any condition in children ? [{}]", currentNodeId, activities, conditional); - builder = addGateway(currentNodeId, builder, activities, conditional); - - log.trace("the node [{}] itself or one of its children is a form replied event", currentNodeId); + builder = addGateway(currentNodeId, builder, activities, conditional, currentNodeChildren.getChildren().size()); + return builder; + } + if (hasFormRepliedEvent(context, currentNodeChildren)) { + log.trace("one of [{}] children is a form replied event", currentNodeId); return builder; } // in case of conditional loop, add a default end event @@ -198,17 +199,17 @@ private void computeChildren(WorkflowNode currentNode, AbstractFlowNodeBuilder addGateway(String currentNodeId, AbstractFlowNodeBuilder builder, - boolean activities, boolean conditional) { + boolean activities, boolean conditional, int childrenSize) { // determine the gateway type if (activities && conditional) { log.trace("an exclusive gateway is added follow the node [{}]", currentNodeId); builder = builder.exclusiveGateway((currentNodeId.replace("/", "") + EXCLUSIVE_GATEWAY_SUFFIX)); - } else if (!activities && conditional) { + } else if (!activities && (conditional || childrenSize > 1)) { log.trace("an event gateway is added follow the node [{}]", currentNodeId); builder = builder.eventBasedGateway().id(currentNodeId + EVENT_GATEWAY_SUFFIX); } diff --git a/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/bpmn/builder/SignalNodeBuilder.java b/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/bpmn/builder/SignalNodeBuilder.java index 34776810..c78b9cd7 100644 --- a/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/bpmn/builder/SignalNodeBuilder.java +++ b/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/camunda/bpmn/builder/SignalNodeBuilder.java @@ -33,7 +33,7 @@ public class SignalNodeBuilder extends AbstractNodeBpmnBuilder { builder = ((AbstractCatchEventBuilder) builder).camundaAsyncBefore() .signal(element.getId()) .name(element.getId()); - } else if (builder instanceof AbstractGatewayBuilder) { + } else { builder = builder.intermediateCatchEvent().camundaAsyncBefore().signal(element.getId()).name(element.getId()); } return builder; diff --git a/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/executor/message/SendMessageExecutor.java b/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/executor/message/SendMessageExecutor.java index 32226148..cfcae231 100644 --- a/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/executor/message/SendMessageExecutor.java +++ b/workflow-bot-app/src/main/java/com/symphony/bdk/workflow/engine/executor/message/SendMessageExecutor.java @@ -44,11 +44,12 @@ public class SendMessageExecutor extends OboExecutor @Override public void execute(ActivityExecutorContext execution) throws IOException { + log.debug("Sending message..."); SendMessage activity = execution.getActivity(); List streamIds = resolveStreamId(execution, activity, execution.bdk().streams()); log.debug("Sending message to rooms {}", streamIds); - Message messageToSend = this.buildMessage(execution); + log.trace("message content \n {}", messageToSend.getContent()); V4Message message; if (streamIds.isEmpty()) { diff --git a/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/BranchingIntegrationTest.java b/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/BranchingIntegrationTest.java index 46add06a..01e983e3 100644 --- a/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/BranchingIntegrationTest.java +++ b/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/BranchingIntegrationTest.java @@ -3,20 +3,28 @@ import static com.symphony.bdk.workflow.custom.assertion.WorkflowAssert.assertThat; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.awaitility.Awaitility.await; import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.when; +import com.symphony.bdk.core.service.message.model.Message; +import com.symphony.bdk.gen.api.model.V4Message; import com.symphony.bdk.workflow.swadl.SwadlParser; import com.symphony.bdk.workflow.swadl.exception.ActivityNotFoundException; import com.symphony.bdk.workflow.swadl.exception.InvalidActivityException; import com.symphony.bdk.workflow.swadl.v1.Workflow; import com.github.fge.jsonschema.core.exceptions.ProcessingException; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import java.io.IOException; import java.util.List; +import java.util.concurrent.TimeUnit; import java.util.stream.Stream; class BranchingIntegrationTest extends IntegrationTest { @@ -67,6 +75,24 @@ void branching(String workflowFile, List activities) throws IOException, assertThat(workflow).isExecutedWithProcessAndActivities(lastProcess(), activities); } + @Test + void branching_intermediate() throws IOException, ProcessingException, InterruptedException { + final Workflow workflow = + SwadlParser.fromYaml(getClass().getResourceAsStream("/branching/if-intermediate-event.swadl.yaml")); + final V4Message message = message("Hello!"); + when(messageService.send(anyString(), any(Message.class))).thenReturn(message); + + engine.deploy(workflow); + + engine.onEvent(messageReceived("/execute")); + Thread.sleep(1000); + engine.onEvent(messageReceived("/execute2")); + await().atMost(5, TimeUnit.SECONDS).until(() -> { + assertThat(workflow).executed("act1", "act2"); + return true; + }); + } + @ParameterizedTest @MethodSource("expectedErrors") void branchingWithError(String workflowFile, Class expectedExceptionType, String error) diff --git a/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/FormReplyIntegrationTest.java b/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/FormReplyIntegrationTest.java index 7442c920..7477e20b 100644 --- a/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/FormReplyIntegrationTest.java +++ b/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/FormReplyIntegrationTest.java @@ -276,8 +276,7 @@ void sendFormOutputsArePreserved() throws Exception { return true; }); - assertThat(workflow) - .executed("init", "check"); + assertThat(workflow).executed("init", "check"); } @Test @@ -323,4 +322,53 @@ void sendMessageUpdateMessage() throws Exception { assertThat(workflow).executed("init", "message-received_/hey", "update"); }); } + + @Test + void formRepliedSendMessageOnConditionIf() throws Exception { + Workflow workflow = + SwadlParser.fromYaml(getClass().getResourceAsStream("/form/send-form-reply-conditional.swadl.yaml")); + + when(messageService.send(anyString(), any(Message.class))).thenReturn(message("msgId")); + + engine.deploy(workflow); + + // trigger workflow execution + engine.onEvent(messageReceived("/test")); + verify(messageService, timeout(5000)).send(anyString(), contains("form")); + + await().atMost(5, TimeUnit.SECONDS).ignoreExceptions().until(() -> { + engine.onEvent(form("msgId", "testForm", Collections.singletonMap("action", "create"))); + return true; + }); + verify(messageService, timeout(5000)).send(anyString(), contains("Create")); + + Thread.sleep(1000); + assertThat(workflow).executed(workflow, "testForm", "resCreate"); + } + + @Test + void formRepliedSendMessageOnConditionElse() throws Exception { + Workflow workflow = + SwadlParser.fromYaml(getClass().getResourceAsStream("/form/send-form-reply-conditional.swadl.yaml")); + + when(messageService.send(anyString(), any(Message.class))).thenReturn(message("msgId")); + + engine.deploy(workflow); + + // trigger workflow execution + engine.onEvent(messageReceived("/test")); + verify(messageService, timeout(5000)).send(anyString(), contains("form")); + + await().atMost(5, TimeUnit.SECONDS).ignoreExceptions().until(() -> { + engine.onEvent(form("msgId", "testForm", Collections.singletonMap("action", "menu"))); + return true; + }); + verify(messageService, timeout(5000)).send(anyString(), contains("Menu")); + + sleepToTimeout(1000); + engine.onEvent(messageReceived("/continue")); + verify(messageService, timeout(5000)).send(anyString(), contains("DONE")); + + assertThat(workflow).executed(workflow, "testForm", "resMenu", "finish"); + } } diff --git a/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/IntegrationTest.java b/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/IntegrationTest.java index a583748c..8dee13ab 100644 --- a/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/IntegrationTest.java +++ b/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/IntegrationTest.java @@ -219,6 +219,7 @@ public static RealTimeEvent messageReceived(String content) { V4MessageSent messageSent = new V4MessageSent(); V4Message message = new V4Message(); + message.setMessageId("msgId"); message.setMessage("" + content + ""); messageSent.setMessage(message); V4Stream stream = new V4Stream(); diff --git a/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/custom/assertion/WorkflowAssert.java b/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/custom/assertion/WorkflowAssert.java index 5db0a398..fe06ad1f 100644 --- a/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/custom/assertion/WorkflowAssert.java +++ b/workflow-bot-app/src/test/java/com/symphony/bdk/workflow/custom/assertion/WorkflowAssert.java @@ -88,7 +88,13 @@ public WorkflowAssert isExecuted() { public WorkflowAssert executed(String... activities) { isNotNull(); - assertExecuted(activities); + assertExecuted(Optional.empty(), activities); + return this; + } + + public WorkflowAssert executed(Workflow workflow, String... activities) { + isNotNull(); + assertExecuted(Optional.of(workflow), activities); return this; } @@ -113,7 +119,7 @@ public WorkflowAssert hasOutput(String key, Object value) { public static Optional lastProcess(Workflow workflow) { List processes = IntegrationTest.historyService.createHistoricProcessInstanceQuery() .processDefinitionName(workflow.getId()) - .orderByProcessInstanceStartTime().desc() + .orderByProcessInstanceStartTime().asc() .list(); if (processes.isEmpty()) { return Optional.empty(); @@ -150,7 +156,7 @@ public static void assertExecuted(Workflow workflow) { .map(Activity::getActivity) .map(BaseActivity::getId) .toArray(String[]::new); - assertExecuted(activityIds); + assertExecuted(Optional.empty(), activityIds); } public static void assertExecuted(Optional process, List activities) { @@ -171,12 +177,14 @@ public static void assertExecuted(Optional process, List activit } // activityIds represent all successfully executed activities and not only a subset - private static void assertExecuted(String... activityIds) { - Assertions.assertThat(listExecutedActivities()).containsExactly(activityIds); + private static void assertExecuted(Optional optionalWorkflow, String... activityIds) { + Assertions.assertThat(listExecutedActivities(optionalWorkflow)).containsExactly(activityIds); } - private static List listExecutedActivities() { - String process = lastProcess().orElseThrow(); + private static List listExecutedActivities(Optional optionalWorkflow) { + final String process = optionalWorkflow.map(workflow -> lastProcess(workflow).orElseThrow()) + .orElseGet(() -> lastProcess().orElseThrow()); + await().atMost(20, SECONDS).until(() -> processIsCompleted(process)); List processes = @@ -199,7 +207,7 @@ private static List listExecutedActivities() { private static void assertNotExecuted(String... activityIds) { Assertions.assertThat(Arrays.stream(activityIds) - .anyMatch(listExecutedActivities()::contains)) + .anyMatch(listExecutedActivities(Optional.empty())::contains)) .isFalse(); } diff --git a/workflow-bot-app/src/test/resources/application-test.yaml b/workflow-bot-app/src/test/resources/application-test.yaml index e6479c69..1aad62f9 100644 --- a/workflow-bot-app/src/test/resources/application-test.yaml +++ b/workflow-bot-app/src/test/resources/application-test.yaml @@ -19,8 +19,8 @@ logging: org.camunda.bpm.engine.pvm: DEBUG org.camunda.bpm.dmn.feel: DEBUG org.camunda.bpm.engine.script: DEBUG - com.symphony.bdk.workflow.engine.camunda.bpmn.CamundaBpmnBuilder: DEBUG - com.symphony.bdk.workflow: DEBUG + com.symphony.bdk.workflow.engine.camunda.bpmn.CamundaBpmnBuilder: TRACE + com.symphony.bdk.workflow: TRACE camunda: bpm: diff --git a/workflow-bot-app/src/test/resources/branching/if-intermediate-event.swadl.yaml b/workflow-bot-app/src/test/resources/branching/if-intermediate-event.swadl.yaml new file mode 100644 index 00000000..d3a03837 --- /dev/null +++ b/workflow-bot-app/src/test/resources/branching/if-intermediate-event.swadl.yaml @@ -0,0 +1,20 @@ +id: if-intermediate-event +variables: + foo: foo +activities: + - execute-script: + id: act1 + on: + message-received: + content: /execute + script: | + println "act1" + - send-message: + id: act2 + on: + message-received: + content: /execute2 + if: ${variables.foo == 'foo'} + to: + stream-id: abc + content: act2 diff --git a/workflow-bot-app/src/test/resources/form/send-form-reply-conditional.swadl.yaml b/workflow-bot-app/src/test/resources/form/send-form-reply-conditional.swadl.yaml new file mode 100644 index 00000000..bb8de613 --- /dev/null +++ b/workflow-bot-app/src/test/resources/form/send-form-reply-conditional.swadl.yaml @@ -0,0 +1,38 @@ +id: form-reply-conditional-message +activities: + - send-message: + id: testForm + on: + message-received: + content: /test + content: | + +

Test

+
+ Hi, what can I do for you? + + +
+
+ - send-message: + id: resCreate + if: ${testForm.action=='create'} + on: + form-replied: + form-id: testForm + exclusive: true + content: Create + - send-message: + id: resMenu + else: {} + on: + form-replied: + form-id: testForm + exclusive: true + content: Menu + - send-message: + id: finish + on: + message-received: + content: /continue + content: DONE