IWF-174: Incorrect overriding stateOptions when reusing a static valu… (#279)

Author: steven-rg-harrison

src/main/java/io/iworkflow/core/Client.java

@@ -172,7 +172,7 @@ public String startWorkflow(
                 throw new WorkflowDefinitionException(String.format("input cannot be assigned to the starting state, input type: %s, starting state input type: %s", input.getClass(), registeredInputType));
             }
 
-            WorkflowStateOptions stateOptions = StateMovementMapper.validateAndGetStateOptions(stateDef);
+            WorkflowStateOptions stateOptions = StateMovementMapper.validateAndGetStateOptionsCopy(stateDef);
             if (shouldSkipWaitUntil(stateDef.getWorkflowState())) {
                 if (stateOptions == null) {
                     stateOptions = new WorkflowStateOptions().skipWaitUntil(true);

src/main/java/io/iworkflow/core/WorkflowStateOptionsExtension.java

@@ -4,6 +4,8 @@
 import io.iworkflow.gen.models.WaitUntilApiFailurePolicy;
 import io.iworkflow.gen.models.WorkflowStateOptions;
 
+import java.util.Objects;
+
 /**
  * WorkflowStateOptionsExtension provides extension to WorkflowStateOptions
  * to make it easier to build some fields of the stateOptions.
@@ -21,6 +23,7 @@
  *     }
  */
 public class WorkflowStateOptionsExtension extends WorkflowStateOptions {
+    private static final JacksonJsonObjectEncoder JSON_ENCODER = new JacksonJsonObjectEncoder();
 
     /**
      * By default, workflow would fail after execute API retry exhausted.
@@ -96,4 +99,23 @@ public WorkflowStateOptionsExtension setProceedOnExecuteFailure(final Class<? ex
         this.executeApiFailureProceedStateOptions(stateOptionsOverride);
         return this;
     }
+
+    // TODO: This is a workaround due to openapi-generator's "generateBuilders" config not working.
+    //  https://openapi-generator.tech/docs/generators/java/#config-options
+    //  I have opened a ticket with them (https://github.com/OpenAPITools/openapi-generator/issues/20320).
+    /**
+     * Uses JSON serialization to deep copy WorkflowStateOptions.
+     * @param stateOptions the state options to deep copy.
+     * @return the newly created copy.
+     */
+    public static WorkflowStateOptions deepCopyStateOptions(WorkflowStateOptions stateOptions) {
+        final WorkflowStateOptions deepCopy =
+                stateOptions == null ? null : JSON_ENCODER.decode(JSON_ENCODER.encode(stateOptions), stateOptions.getClass());
+
+        if (!Objects.deepEquals(stateOptions, deepCopy)) {
+            throw new ObjectEncoderException("Deep copy of WorkflowStateOptions did not match.");
+        }
+
+        return deepCopy;
+    }
 }

src/main/java/io/iworkflow/core/mapper/StateMovementMapper.java

@@ -13,6 +13,7 @@
 
 import static io.iworkflow.core.StateMovement.RESERVED_STATE_ID_PREFIX;
 import static io.iworkflow.core.WorkflowState.shouldSkipWaitUntil;
+import static io.iworkflow.core.WorkflowStateOptionsExtension.deepCopyStateOptions;
 
 public class StateMovementMapper {
 
@@ -28,9 +29,12 @@ public static StateMovement toGenerated(final io.iworkflow.core.StateMovement st
             }
 
             // Try to get the overrode stateOptions, if it's null, get the stateOptions from stateDef
-            WorkflowStateOptions stateOptions = stateMovement.getStateOptionsOverride().orElse(null);
-            if (stateOptions == null) {
-                stateOptions = StateMovementMapper.validateAndGetStateOptions(stateDef);
+            WorkflowStateOptions stateOptions;
+            if (stateMovement.getStateOptionsOverride().isPresent()) {
+                // Always deep copy the state options so we don't modify the original
+                stateOptions = deepCopyStateOptions(stateMovement.getStateOptionsOverride().get());
+            } else {
+                stateOptions = StateMovementMapper.validateAndGetStateOptionsCopy(stateDef);
             }
 
             if (shouldSkipWaitUntil(stateDef.getWorkflowState())) {
@@ -62,7 +66,7 @@ public static void autoFillFailureProceedingStateOptions(WorkflowStateOptions st
             // fill the state options for the proceeding state
             String proceedStateId = stateOptions.getExecuteApiFailureProceedStateId();
             final StateDef proceedStatDef = registry.getWorkflowState(workflowType, proceedStateId);
-            WorkflowStateOptions proceedStateOptions = StateMovementMapper.validateAndGetStateOptions(proceedStatDef);
+            WorkflowStateOptions proceedStateOptions = StateMovementMapper.validateAndGetStateOptionsCopy(proceedStatDef);
             if (proceedStateOptions != null &&
                     proceedStateOptions.getExecuteApiFailurePolicy() == ExecuteApiFailurePolicy.PROCEED_TO_CONFIGURED_STATE) {
                 throw new WorkflowDefinitionException("nested failure handling is not supported. You cannot set a failure proceeding state on top of another failure proceeding state.");
@@ -80,9 +84,10 @@ public static void autoFillFailureProceedingStateOptions(WorkflowStateOptions st
         }
     }
 
-    public static WorkflowStateOptions validateAndGetStateOptions(final StateDef stateDef){
+    public static WorkflowStateOptions validateAndGetStateOptionsCopy(final StateDef stateDef){
         final WorkflowState state = stateDef.getWorkflowState();
-        WorkflowStateOptions stateOptions = state.getStateOptions();
+        // Always deep copy the state options so we don't modify the original
+        WorkflowStateOptions stateOptions = deepCopyStateOptions(state.getStateOptions());
         if (stateOptions == null){
             return null;
         }

src/test/java/io/iworkflow/integ/BasicTest.java

@@ -14,27 +14,33 @@
 import io.iworkflow.gen.models.Context;
 import io.iworkflow.gen.models.ErrorSubStatus;
 import io.iworkflow.gen.models.IDReusePolicy;
+import io.iworkflow.gen.models.PersistenceLoadingPolicy;
+import io.iworkflow.gen.models.PersistenceLoadingType;
+import io.iworkflow.gen.models.RetryPolicy;
 import io.iworkflow.gen.models.WorkflowConfig;
+import io.iworkflow.gen.models.WorkflowStateOptions;
 import io.iworkflow.gen.models.WorkflowStatus;
 import io.iworkflow.integ.basic.AbnormalExitWorkflow;
 import io.iworkflow.integ.basic.BasicWorkflow;
-import io.iworkflow.integ.basic.BasicWorkflowState1;
 import io.iworkflow.integ.basic.BasicWorkflowState2;
 import io.iworkflow.integ.basic.EmptyInputWorkflow;
 import io.iworkflow.integ.basic.EmptyInputWorkflowState1;
 import io.iworkflow.integ.basic.FakContextImpl;
+import io.iworkflow.integ.basic.MixOfWithWaitUntilAndSkipWaitUntilWorkflow;
 import io.iworkflow.integ.basic.ModelInputWorkflow;
 import io.iworkflow.integ.basic.ProceedOnStateStartFailWorkflow;
-import io.iworkflow.integ.timer.BasicTimerWorkflow;
-import io.iworkflow.integ.timer.BasicTimerWorkflowState1;
+import io.iworkflow.integ.stateoptions.StateOptionsWorkflow;
 import io.iworkflow.spring.TestSingletonWorkerService;
 import io.iworkflow.spring.controller.WorkflowRegistry;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import java.util.Collections;
 import java.util.concurrent.ExecutionException;
 
+import static io.iworkflow.core.WorkflowStateOptionsExtension.deepCopyStateOptions;
+
 public class BasicTest {
 
     @BeforeEach
@@ -226,4 +232,33 @@ public void testWorkflowWaitForStateCompletionWithWaitForKey() {
         final WorkflowInfo childWorkflowInfo = client.describeWorkflow(childWfId);
         Assertions.assertEquals(WorkflowStatus.COMPLETED, childWorkflowInfo.getWorkflowStatus());
     }
+
+    @Test
+    public void testMixOfWithWaitUntilAndSkipWaitUntilWorkflow() {
+        final Client client = new Client(WorkflowRegistry.registry, ClientOptions.localDefault);
+        final long startTs = System.currentTimeMillis();
+        final String wfId = "wf-mix-of-with-wait-until-and-skip-wait-until-workflow-test-id" + startTs / 1000;
+        final Integer input = 5;
+
+        client.startWorkflow(MixOfWithWaitUntilAndSkipWaitUntilWorkflow.class, wfId, 10, input);
+        client.waitForWorkflowCompletion(wfId);
+    }
+
+    @Test
+    public void deepCopyWorkflowStateOptionsTest() {
+        final WorkflowStateOptions origOptions =
+                new WorkflowStateOptions().executeApiRetryPolicy(new RetryPolicy().maximumAttempts(3));
+        origOptions.setSkipWaitUntil(true);
+        origOptions.setExecuteApiFailureProceedStateId("execute-api-failure-proceed-state-id");
+        origOptions.setSearchAttributesLoadingPolicy(new PersistenceLoadingPolicy().persistenceLoadingType(
+                        PersistenceLoadingType.PARTIAL_WITH_EXCLUSIVE_LOCK)
+                .partialLoadingKeys(Collections.singletonList(StateOptionsWorkflow.DA_WAIT_UNTIL)));
+
+        final WorkflowStateOptions deepCopyOptions = deepCopyStateOptions(origOptions);
+        Assertions.assertEquals(origOptions, deepCopyOptions);
+
+        // Verify changing a value in one object doesn't update both by reference
+        origOptions.setSkipWaitUntil(false);
+        Assertions.assertNotEquals(origOptions, deepCopyOptions);
+    }
 }

src/test/java/io/iworkflow/integ/basic/MixOfWithWaitUntilAndSkipWaitUntilState1.java

@@ -0,0 +1,30 @@
+package io.iworkflow.integ.basic;
+
+import io.iworkflow.core.Context;
+import io.iworkflow.core.StateDecision;
+import io.iworkflow.core.WorkflowState;
+import io.iworkflow.core.command.CommandResults;
+import io.iworkflow.core.communication.Communication;
+import io.iworkflow.core.persistence.Persistence;
+import io.iworkflow.gen.models.WorkflowStateOptions;
+
+import static io.iworkflow.integ.basic.MixOfWithWaitUntilAndSkipWaitUntilWorkflow.SHARED_STATE_OPTIONS;
+
+public class MixOfWithWaitUntilAndSkipWaitUntilState1 implements WorkflowState<Integer> {
+
+    @Override
+    public Class<Integer> getInputType() {
+        return Integer.class;
+    }
+
+    @Override
+    public StateDecision execute(final Context context, final Integer input, final CommandResults commandResults, Persistence persistence, final Communication communication) {
+        final int output = input + 1;
+        return StateDecision.singleNextState(MixOfWithWaitUntilAndSkipWaitUntilState2.class, output);
+    }
+
+    @Override
+    public WorkflowStateOptions getStateOptions() {
+        return SHARED_STATE_OPTIONS;
+    }
+}

src/test/java/io/iworkflow/integ/basic/MixOfWithWaitUntilAndSkipWaitUntilState2.java

@@ -0,0 +1,49 @@
+package io.iworkflow.integ.basic;
+
+import io.iworkflow.core.Context;
+import io.iworkflow.core.StateDecision;
+import io.iworkflow.core.WorkflowState;
+import io.iworkflow.core.command.CommandRequest;
+import io.iworkflow.core.command.CommandResults;
+import io.iworkflow.core.command.TimerCommand;
+import io.iworkflow.core.communication.Communication;
+import io.iworkflow.core.persistence.Persistence;
+import io.iworkflow.gen.models.WorkflowStateOptions;
+
+import java.time.Duration;
+
+import static io.iworkflow.integ.basic.MixOfWithWaitUntilAndSkipWaitUntilWorkflow.SHARED_STATE_OPTIONS;
+
+public class MixOfWithWaitUntilAndSkipWaitUntilState2 implements WorkflowState<Integer> {
+
+    @Override
+    public Class<Integer> getInputType() {
+        return Integer.class;
+    }
+
+    @Override
+    public CommandRequest waitUntil(
+            final Context context,
+            final Integer input,
+            Persistence persistence,
+            final Communication communication) {
+        return CommandRequest.forAllCommandCompleted(TimerCommand.createByDuration(Duration.ofSeconds(1)));
+    }
+
+    @Override
+    public StateDecision execute(
+            final Context context,
+            final Integer input,
+            final CommandResults commandResults,
+            Persistence persistence,
+            final Communication communication) {
+        final int output = input + 1;
+        commandResults.getAllTimerCommandResults().get(0).getTimerStatus();
+        return StateDecision.gracefulCompleteWorkflow(output);
+    }
+
+    @Override
+    public WorkflowStateOptions getStateOptions() {
+        return SHARED_STATE_OPTIONS;
+    }
+}

src/test/java/io/iworkflow/integ/basic/MixOfWithWaitUntilAndSkipWaitUntilWorkflow.java

@@ -0,0 +1,25 @@
+package io.iworkflow.integ.basic;
+
+import io.iworkflow.core.ObjectWorkflow;
+import io.iworkflow.core.StateDef;
+import io.iworkflow.gen.models.RetryPolicy;
+import io.iworkflow.gen.models.WorkflowStateOptions;
+import org.springframework.stereotype.Component;
+
+import java.util.Arrays;
+import java.util.List;
+
+@Component
+public class MixOfWithWaitUntilAndSkipWaitUntilWorkflow implements ObjectWorkflow {
+
+    public static WorkflowStateOptions SHARED_STATE_OPTIONS =
+            new WorkflowStateOptions().executeApiRetryPolicy(new RetryPolicy().maximumAttempts(3));
+
+    @Override
+    public List<StateDef> getWorkflowStates() {
+        return Arrays.asList(
+                StateDef.startingState(new MixOfWithWaitUntilAndSkipWaitUntilState1()),
+                StateDef.nonStartingState(new MixOfWithWaitUntilAndSkipWaitUntilState2())
+        );
+    }
+}