Use safe in() lists for id collections in the variable queries

modules/flowable-engine/src/test/java/org/flowable/engine/test/impl/VariableInstanceQueryLargeIdCollectionTest.java

@@ -0,0 +1,199 @@
+/* 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.engine.test.impl;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Consumer;
+
+import org.flowable.common.engine.api.scope.ScopeTypes;
+import org.flowable.common.engine.impl.db.AbstractDataManager;
+import org.flowable.engine.impl.test.PluggableFlowableTestCase;
+import org.flowable.engine.runtime.ProcessInstance;
+import org.flowable.engine.test.Deployment;
+import org.flowable.variable.api.history.HistoricVariableInstance;
+import org.flowable.variable.api.persistence.entity.VariableInstance;
+import org.flowable.variable.service.InternalVariableInstanceQuery;
+import org.flowable.variable.service.VariableService;
+import org.flowable.variable.service.impl.persistence.entity.VariableInstanceEntity;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests that the variable queries split id collections which exceed {@link AbstractDataManager#MAX_ENTRIES_IN_CLAUSE}
+ * into multiple in() clauses, since not every database supports more entries in a single in() clause.
+ *
+ * @author Filip Hrisafov
+ */
+class VariableInstanceQueryLargeIdCollectionTest extends PluggableFlowableTestCase {
+
+    protected final List<String> variableIds = new ArrayList<>();
+
+    @AfterEach
+    void tearDown() {
+        managementService.executeCommand(commandContext -> {
+            VariableService variableService = processEngineConfiguration.getVariableServiceConfiguration().getVariableService();
+            for (String variableId : variableIds) {
+                VariableInstanceEntity variable = variableService.createInternalVariableInstanceQuery().id(variableId).singleResult();
+                if (variable != null) {
+                    variableService.deleteVariableInstance(variable);
+                }
+            }
+            return null;
+        });
+        variableIds.clear();
+    }
+
+    @Test
+    void internalQueryWithLargeTaskIdCollection() {
+        createVariable("taskVar", variable -> variable.setTaskId("task-1"));
+        createVariable("otherTaskVar", variable -> variable.setTaskId("task-2"));
+        createVariable("ignoredTaskVar", variable -> variable.setTaskId("task-3"));
+
+        assertThat(findVariables(query -> query.taskIds(idsWith("task-1", "task-2"))))
+                .extracting(VariableInstance::getName)
+                .containsExactlyInAnyOrder("taskVar", "otherTaskVar");
+    }
+
+    @Test
+    @Deployment(resources = "org/flowable/engine/test/api/oneTaskProcess.bpmn20.xml")
+    void internalQueryWithLargeExecutionIdCollection() {
+        // Variables are linked to an execution with a foreign key, so real process instances are needed here
+        ProcessInstance firstInstance = runtimeService.startProcessInstanceByKey("oneTaskProcess", Map.of("executionVar", "first"));
+        ProcessInstance secondInstance = runtimeService.startProcessInstanceByKey("oneTaskProcess", Map.of("otherExecutionVar", "second"));
+        runtimeService.startProcessInstanceByKey("oneTaskProcess", Map.of("ignoredExecutionVar", "third"));
+
+        assertThat(findVariables(query -> query.executionIds(idsWith(firstInstance.getId(), secondInstance.getId()))))
+                .extracting(VariableInstance::getName)
+                .containsExactlyInAnyOrder("executionVar", "otherExecutionVar");
+    }
+
+    @Test
+    void internalQueryWithLargeScopeIdCollection() {
+        createVariable("scopeVar", variable -> {
+            variable.setScopeId("scope-1");
+            variable.setScopeType(ScopeTypes.CMMN);
+        });
+        createVariable("otherScopeVar", variable -> {
+            variable.setScopeId("scope-2");
+            variable.setScopeType(ScopeTypes.CMMN);
+        });
+        createVariable("ignoredScopeVar", variable -> {
+            variable.setScopeId("scope-3");
+            variable.setScopeType(ScopeTypes.CMMN);
+        });
+
+        assertThat(findVariables(query -> query.scopeIds(idsWith("scope-1", "scope-2")).scopeType(ScopeTypes.CMMN)))
+                .extracting(VariableInstance::getName)
+                .containsExactlyInAnyOrder("scopeVar", "otherScopeVar");
+    }
+
+    @Test
+    void internalQueryWithLargeSubScopeIdCollection() {
+        createVariable("subScopeVar", variable -> {
+            variable.setSubScopeId("subScope-1");
+            variable.setScopeType(ScopeTypes.CMMN);
+        });
+        createVariable("otherSubScopeVar", variable -> {
+            variable.setSubScopeId("subScope-2");
+            variable.setScopeType(ScopeTypes.CMMN);
+        });
+        createVariable("ignoredSubScopeVar", variable -> {
+            variable.setSubScopeId("subScope-3");
+            variable.setScopeType(ScopeTypes.CMMN);
+        });
+
+        assertThat(findVariables(query -> query.subScopeIds(idsWith("subScope-1", "subScope-2")).scopeType(ScopeTypes.CMMN)))
+                .extracting(VariableInstance::getName)
+                .containsExactlyInAnyOrder("subScopeVar", "otherSubScopeVar");
+    }
+
+    @Test
+    void variableInstanceQueryWithLargeTaskIdCollection() {
+        createVariable("taskVar", variable -> variable.setTaskId("task-1"));
+        createVariable("otherTaskVar", variable -> variable.setTaskId("task-2"));
+        createVariable("ignoredTaskVar", variable -> variable.setTaskId("task-3"));
+
+        assertThat(runtimeService.createVariableInstanceQuery().taskIds(idsWith("task-1", "task-2")).list())
+                .extracting(VariableInstance::getName)
+                .containsExactlyInAnyOrder("taskVar", "otherTaskVar");
+    }
+
+    @Test
+    @Deployment(resources = "org/flowable/engine/test/api/oneTaskProcess.bpmn20.xml")
+    void variableInstanceQueryWithLargeExecutionIdCollection() {
+        ProcessInstance firstInstance = runtimeService.startProcessInstanceByKey("oneTaskProcess", Map.of("executionVar", "first"));
+        ProcessInstance secondInstance = runtimeService.startProcessInstanceByKey("oneTaskProcess", Map.of("otherExecutionVar", "second"));
+        runtimeService.startProcessInstanceByKey("oneTaskProcess", Map.of("ignoredExecutionVar", "third"));
+
+        assertThat(runtimeService.createVariableInstanceQuery().executionIds(idsWith(firstInstance.getId(), secondInstance.getId())).list())
+                .extracting(VariableInstance::getName)
+                .containsExactlyInAnyOrder("executionVar", "otherExecutionVar");
+    }
+
+    @Test
+    @Deployment(resources = "org/flowable/engine/test/api/oneTaskProcess.bpmn20.xml")
+    void historicVariableInstanceQueryWithLargeExecutionIdCollection() {
+        ProcessInstance firstInstance = runtimeService.startProcessInstanceByKey("oneTaskProcess", Map.of("executionVar", "first"));
+        ProcessInstance secondInstance = runtimeService.startProcessInstanceByKey("oneTaskProcess", Map.of("otherExecutionVar", "second"));
+        runtimeService.startProcessInstanceByKey("oneTaskProcess", Map.of("ignoredExecutionVar", "third"));
+
+        assertThat(historyService.createHistoricVariableInstanceQuery().executionIds(idsWith(firstInstance.getId(), secondInstance.getId())).list())
+                .extracting(HistoricVariableInstance::getVariableName)
+                .containsExactlyInAnyOrder("executionVar", "otherExecutionVar");
+    }
+
+    protected List<VariableInstanceEntity> findVariables(Consumer<InternalVariableInstanceQuery> queryCustomizer) {
+        return managementService.executeCommand(commandContext -> {
+            InternalVariableInstanceQuery query = processEngineConfiguration.getVariableServiceConfiguration()
+                    .getVariableService()
+                    .createInternalVariableInstanceQuery();
+            queryCustomizer.accept(query);
+            return query.list();
+        });
+    }
+
+    protected void createVariable(String name, Consumer<VariableInstanceEntity> variableCustomizer) {
+        String variableId = managementService.executeCommand(commandContext -> {
+            VariableService variableService = processEngineConfiguration.getVariableServiceConfiguration().getVariableService();
+            VariableInstanceEntity variable = variableService.createVariableInstance(name);
+            variableCustomizer.accept(variable);
+            variableService.insertVariableInstanceWithValue(variable, name + "-value", null);
+            return variable.getId();
+        });
+        variableIds.add(variableId);
+    }
+
+    /**
+     * Returns a set which contains the given ids, padded with dummy ids so the total exceeds the maximum number of entries in a single in() clause.
+     */
+    protected Set<String> idsWith(String... ids) {
+        Set<String> allIds = new LinkedHashSet<>(List.of(ids));
+        allIds.addAll(generateIds("dummy", AbstractDataManager.MAX_ENTRIES_IN_CLAUSE + 500));
+        return allIds;
+    }
+
+    protected Collection<String> generateIds(String prefix, int amount) {
+        List<String> ids = new ArrayList<>(amount);
+        for (int i = 0; i < amount; i++) {
+            ids.add(prefix + "-generated-" + i);
+        }
+        return ids;
+    }
+}

modules/flowable-variable-service/src/main/java/org/flowable/variable/service/impl/HistoricVariableInstanceQueryImpl.java

@@ -422,6 +422,10 @@ public Set<String> getExecutionIds() {
         return executionIds;
     }
 
+    public List<List<String>> getSafeExecutionIds() {
+        return getSafeList(executionIds);
+    }
+
     public boolean isExcludeTaskRelated() {
         return excludeTaskRelated;
     }

modules/flowable-variable-service/src/main/java/org/flowable/variable/service/impl/InternalVariableInstanceQueryImpl.java

@@ -17,9 +17,11 @@
 
 import org.apache.commons.lang3.StringUtils;
 import org.flowable.common.engine.api.FlowableIllegalArgumentException;
+import org.flowable.common.engine.impl.db.AbstractDataManager;
 import org.flowable.common.engine.impl.db.SingleCachedEntityMatcher;
 import org.flowable.common.engine.impl.persistence.cache.CachedEntity;
 import org.flowable.common.engine.impl.persistence.cache.CachedEntityMatcher;
+import org.flowable.common.engine.impl.util.CollectionUtil;
 import org.flowable.variable.service.InternalVariableInstanceQuery;
 import org.flowable.variable.service.impl.persistence.entity.VariableInstanceEntity;
 import org.flowable.variable.service.impl.persistence.entity.data.VariableInstanceDataManager;
@@ -225,6 +227,10 @@ public Collection<String> getTaskIds() {
         return taskIds;
     }
 
+    public List<List<String>> getSafeTaskIds() {
+        return getSafeList(taskIds);
+    }
+
     public String getProcessInstanceId() {
         return processInstanceId;
     }
@@ -237,6 +243,10 @@ public Collection<String> getExecutionIds() {
         return executionIds;
     }
 
+    public List<List<String>> getSafeExecutionIds() {
+        return getSafeList(executionIds);
+    }
+
     public boolean isWithoutTaskId() {
         return withoutTaskId;
     }
@@ -249,6 +259,10 @@ public Collection<String> getScopeIds() {
         return scopeIds;
     }
 
+    public List<List<String>> getSafeScopeIds() {
+        return getSafeList(scopeIds);
+    }
+
     public String getSubScopeId() {
         return subScopeId;
     }
@@ -257,6 +271,10 @@ public Collection<String> getSubScopeIds() {
         return subScopeIds;
     }
 
+    public List<List<String>> getSafeSubScopeIds() {
+        return getSafeList(subScopeIds);
+    }
+
     public boolean isWithoutSubScopeId() {
         return withoutSubScopeId;
     }
@@ -277,6 +295,11 @@ public Collection<String> getNames() {
         return names;
     }
 
+    protected List<List<String>> getSafeList(Collection<String> values) {
+        // need to split into different parts due to some dbs not supporting more than MAX_ENTRIES_IN_CLAUSE for in()
+        return CollectionUtil.partition(values, AbstractDataManager.MAX_ENTRIES_IN_CLAUSE);
+    }
+
     // This method is needed because we have a different way of querying list and single objects via MyBatis.
     // Querying lists wraps the object in a ListQueryParameterObject
     public InternalVariableInstanceQueryImpl getParameter() {
@@ -304,7 +327,7 @@ public boolean isRetained(VariableInstanceEntity entity, InternalVariableInstanc
             return false;
         }
 
-        if (param.scopeIds != null && !param.scopeIds.contains(entity.getScopeId())) {
+        if (param.scopeIds != null && !param.scopeIds.isEmpty() && !param.scopeIds.contains(entity.getScopeId())) {
             return false;
         }
 
@@ -324,7 +347,7 @@ public boolean isRetained(VariableInstanceEntity entity, InternalVariableInstanc
             return false;
         }
 
-        if (param.subScopeIds != null && !param.subScopeIds.contains(entity.getSubScopeId())) {
+        if (param.subScopeIds != null && !param.subScopeIds.isEmpty() && !param.subScopeIds.contains(entity.getSubScopeId())) {
             return false;
         }
 

modules/flowable-variable-service/src/main/java/org/flowable/variable/service/impl/VariableInstanceQueryImpl.java

@@ -372,6 +372,10 @@ public Set<String> getTaskIds() {
         return taskIds;
     }
 
+    public List<List<String>> getSafeTaskIds() {
+        return getSafeList(taskIds);
+    }
+
     public String getExecutionId() {
         return executionId;
     }
@@ -380,6 +384,10 @@ public Set<String> getExecutionIds() {
         return executionIds;
     }
 
+    public List<List<String>> getSafeExecutionIds() {
+        return getSafeList(executionIds);
+    }
+
     public boolean isExcludeTaskRelated() {
         return excludeTaskRelated;
     }

modules/flowable-variable-service/src/main/resources/org/flowable/variable/service/db/mapping/entity/HistoricVariableInstance.xml

@@ -421,10 +421,14 @@
                 and RES.EXECUTION_ID_ = #{executionId, jdbcType=NVARCHAR}
             </if>
             <if test="executionIds != null and !executionIds.empty">
-                and RES.EXECUTION_ID_ in
-                <foreach item="executionId" index="index" collection="executionIds" open="(" separator="," close=")">
-                    #{executionId, jdbcType=NVARCHAR}
+                and (
+                <foreach item="listItem" collection="safeExecutionIds" separator="OR">
+                    RES.EXECUTION_ID_ in
+                    <foreach item="executionId" collection="listItem" open="(" separator="," close=")">
+                        #{executionId, jdbcType=NVARCHAR}
+                    </foreach>
                 </foreach>
+                )
             </if>
             <if test="taskId != null">
                 and RES.TASK_ID_ = #{taskId, jdbcType=NVARCHAR}