Fix bug in CmmnRuntimeService#getVariable when used in nested command…

… context, where during looping a wrongly cached variable instance is returned.

More specifically: the iteration order of the hashmap changes depending on the amount of variables stored during the same transaction.

modules/flowable-cmmn-engine-configurator/pom.xml

@@ -62,6 +62,11 @@
             <artifactId>mockito-core</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.apache.groovy</groupId>
+            <artifactId>groovy-jsr223</artifactId>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>net.javacrumbs.json-unit</groupId>
             <artifactId>json-unit-assertj</artifactId>

modules/flowable-cmmn-engine-configurator/src/test/java/org/flowable/cmmn/test/AbstractProcessEngineIntegrationTest.java

@@ -15,7 +15,9 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 import java.util.Date;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.stream.Collectors;
 
 import org.flowable.cmmn.api.CmmnHistoryService;
@@ -52,6 +54,8 @@ public abstract class AbstractProcessEngineIntegrationTest {
     protected static CmmnEngineConfiguration cmmnEngineConfiguration;
     protected static ProcessEngine processEngine;
 
+    protected static Map<Object, Object> beans = new HashMap<>();
+
     protected CmmnRepositoryService cmmnRepositoryService;
     protected CmmnRuntimeService cmmnRuntimeService;
     protected CmmnTaskService cmmnTaskService;
@@ -69,7 +73,9 @@ public abstract class AbstractProcessEngineIntegrationTest {
     @BeforeClass
     public static void bootProcessEngine() {
         if (processEngine == null) {
-            processEngine = ProcessEngineConfiguration.createProcessEngineConfigurationFromResource("flowable.cfg.xml").buildProcessEngine();
+            ProcessEngineConfiguration processEngineConfiguration = ProcessEngineConfiguration.createProcessEngineConfigurationFromResource("flowable.cfg.xml");
+            processEngineConfiguration.setBeans(beans);
+            processEngine = processEngineConfiguration.buildProcessEngine();
             cmmnEngineConfiguration = (CmmnEngineConfiguration) processEngine.getProcessEngineConfiguration()
                     .getEngineConfigurations().get(EngineConfigurationConstants.KEY_CMMN_ENGINE_CONFIG);
             CmmnTestRunner.setCmmnEngineConfiguration(cmmnEngineConfiguration);
@@ -91,6 +97,12 @@ public void setupServices() {
         this.processEngineHistoryService = processEngine.getHistoryService();
         this.processEngineConfiguration = processEngine.getProcessEngineConfiguration();
         this.processEngineDynamicBpmnService = processEngine.getDynamicBpmnService();
+
+        beans.put("cmmnRepositoryService", cmmnEngineConfiguration.getCmmnRepositoryService());
+        beans.put("cmmnRuntimeService", cmmnEngineConfiguration.getCmmnRuntimeService());
+        beans.put("cmmnTaskService", cmmnEngineConfiguration.getCmmnTaskService());
+        beans.put("cmmnHistoryService", cmmnEngineConfiguration.getCmmnHistoryService());
+        beans.put("cmmnManagementService", cmmnEngineConfiguration.getCmmnManagementService());
     }
 
     @After
@@ -102,6 +114,8 @@ public void cleanup() {
         for (CmmnDeployment deployment : cmmnRepositoryService.createDeploymentQuery().list()) {
             cmmnRepositoryService.deleteDeployment(deployment.getId(), true);
         }
+
+        beans.clear();
     }
 
     protected Date setCmmnClockFixedToCurrentTime() {

modules/flowable-cmmn-engine-configurator/src/test/java/org/flowable/cmmn/test/VariablesTest.java

@@ -0,0 +1,62 @@
+/* Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.flowable.cmmn.test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.List;
+
+import org.flowable.cmmn.api.runtime.CaseInstance;
+import org.flowable.cmmn.engine.test.CmmnDeployment;
+import org.flowable.cmmn.engine.test.impl.CmmnHistoryTestHelper;
+import org.flowable.common.engine.impl.history.HistoryLevel;
+import org.flowable.engine.test.Deployment;
+import org.flowable.variable.api.history.HistoricVariableInstance;
+import org.junit.Test;
+
+/**
+ * @author Joram Barrez
+ */
+public class VariablesTest extends AbstractProcessEngineIntegrationTest {
+
+    @Test
+    @Deployment
+    @CmmnDeployment
+    public void testSettingAndRemovingVariableThroughCmmnRuntimeService() {
+        processEngineRepositoryService.createDeployment()
+                .addClasspathResource("org/flowable/cmmn/test/VariablesTest.testSettingAndRemovingVariableThroughCmmnRuntimeService.bpmn20.xml")
+                .deploy();
+
+        CaseInstance caseInstance = cmmnRuntimeService.createCaseInstanceBuilder()
+                .caseDefinitionKey("varSyncTestCase")
+                .variable("loopNum", 100)
+                .variable("round", 5)
+                .start();
+
+        // The case instance ending means all variable lookups succeeded
+        assertCaseInstanceEnded(caseInstance);
+        assertThat(cmmnRuntimeService.getVariables(caseInstance.getId())).isEmpty();
+
+        if (CmmnHistoryTestHelper.isHistoryLevelAtLeast(HistoryLevel.ACTIVITY, cmmnEngineConfiguration)) {
+            List<HistoricVariableInstance> historicVariables = cmmnHistoryService.createHistoricVariableInstanceQuery()
+                    .caseInstanceId(caseInstance.getId()).list();
+
+            // The variables are recreated each loop with a non-null value
+            assertThat(historicVariables).hasSize(102); // 100 from the variables and 2 for round and loopNum
+            for (HistoricVariableInstance historicVariable : historicVariables) {
+                assertThat(historicVariable.getValue()).isNotNull();
+            }
+        }
+    }
+
+}