Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature(engine): add activityIdIn filter to HistoricProcessInstanceQuery #4619

Merged

Conversation

venetrius
Copy link
Member

related to: #4618

@venetrius venetrius self-assigned this Sep 17, 2024
@venetrius venetrius force-pushed the 4618-add-ActivityIdIn-filter-to-HistoricProcessInstanceQuery branch 2 times, most recently from a571bce to c088a99 Compare September 18, 2024 06:10
@yanavasileva yanavasileva added ci:rest-api Runs extra builds for the REST API (currently only WLS compatibility builds). ci:default-build Runs the builds that have no explicit trigger (e.g. different history levels). ci:all-db Runs the builds for all databases. labels Sep 18, 2024
Copy link
Member

@tasso94 tasso94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • You are only testing the use case with a single activityId but not with multiple ids.
  • Can we test multiple activities in different scopes with and without incidents?
  • ❓ Does it make sense to copy some of these tests over to HistoricProcessInstanceTest (please scroll in the code excerpt to see all the tests)
    @Test
    public void testQueryByNullActivityId() {
    try {
    runtimeService.createProcessInstanceQuery()
    .activityIdIn((String) null);
    fail("exception expected");
    }
    catch (NullValueException e) {
    assertThat(e.getMessage()).contains("activity ids contains null value");
    }
    }
    @Test
    public void testQueryByNullActivityIds() {
    try {
    runtimeService.createProcessInstanceQuery()
    .activityIdIn((String[]) null);
    fail("exception expected");
    }
    catch (NullValueException e) {
    assertThat(e.getMessage()).contains("activity ids is null");
    }
    }
    @Test
    public void testQueryByUnknownActivityId() {
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery()
    .activityIdIn("unknown");
    assertNoProcessInstancesReturned(query);
    }
    @Test
    public void testQueryByLeafActivityId() {
    // given
    ProcessDefinition oneTaskDefinition = testHelper.deployAndGetDefinition(ProcessModels.ONE_TASK_PROCESS);
    ProcessDefinition gatewaySubProcessDefinition = testHelper.deployAndGetDefinition(FORK_JOIN_SUB_PROCESS_MODEL);
    // when
    ProcessInstance oneTaskInstance1 = runtimeService.startProcessInstanceById(oneTaskDefinition.getId());
    ProcessInstance oneTaskInstance2 = runtimeService.startProcessInstanceById(oneTaskDefinition.getId());
    ProcessInstance gatewaySubProcessInstance1 = runtimeService.startProcessInstanceById(gatewaySubProcessDefinition.getId());
    ProcessInstance gatewaySubProcessInstance2 = runtimeService.startProcessInstanceById(gatewaySubProcessDefinition.getId());
    Task task = engineRule.getTaskService().createTaskQuery()
    .processInstanceId(gatewaySubProcessInstance2.getId())
    .taskName("completeMe")
    .singleResult();
    engineRule.getTaskService().complete(task.getId());
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("userTask");
    assertReturnedProcessInstances(query, oneTaskInstance1, oneTaskInstance2);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("userTask1", "userTask2");
    assertReturnedProcessInstances(query, gatewaySubProcessInstance1, gatewaySubProcessInstance2);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("userTask", "userTask1");
    assertReturnedProcessInstances(query, oneTaskInstance1, oneTaskInstance2, gatewaySubProcessInstance1);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("userTask", "userTask1", "userTask2");
    assertReturnedProcessInstances(query, oneTaskInstance1, oneTaskInstance2, gatewaySubProcessInstance1, gatewaySubProcessInstance2);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("join");
    assertReturnedProcessInstances(query, gatewaySubProcessInstance2);
    }
    @Test
    public void testQueryByNonLeafActivityId() {
    // given
    ProcessDefinition processDefinition = testHelper.deployAndGetDefinition(FORK_JOIN_SUB_PROCESS_MODEL);
    // when
    runtimeService.startProcessInstanceById(processDefinition.getId());
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("subProcess", "fork");
    assertNoProcessInstancesReturned(query);
    }
    @Test
    public void testQueryByAsyncBeforeActivityId() {
    // given
    ProcessDefinition testProcess = testHelper.deployAndGetDefinition(ProcessModels.newModel()
    .startEvent("start").camundaAsyncBefore()
    .subProcess("subProcess").camundaAsyncBefore()
    .embeddedSubProcess()
    .startEvent()
    .serviceTask("task").camundaAsyncBefore().camundaExpression("${true}")
    .endEvent()
    .subProcessDone()
    .endEvent("end").camundaAsyncBefore()
    .done()
    );
    // when
    ProcessInstance instanceBeforeStart = runtimeService.startProcessInstanceById(testProcess.getId());
    ProcessInstance instanceBeforeSubProcess = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceBeforeSubProcess);
    ProcessInstance instanceBeforeTask = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceBeforeTask);
    executeJobForProcessInstance(instanceBeforeTask);
    ProcessInstance instanceBeforeEnd = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceBeforeEnd);
    executeJobForProcessInstance(instanceBeforeEnd);
    executeJobForProcessInstance(instanceBeforeEnd);
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("start");
    assertReturnedProcessInstances(query, instanceBeforeStart);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("subProcess");
    assertReturnedProcessInstances(query, instanceBeforeSubProcess);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("task");
    assertReturnedProcessInstances(query, instanceBeforeTask);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("end");
    assertReturnedProcessInstances(query, instanceBeforeEnd);
    }
    @Test
    public void testQueryByAsyncAfterActivityId() {
    // given
    ProcessDefinition testProcess = testHelper.deployAndGetDefinition(ProcessModels.newModel()
    .startEvent("start").camundaAsyncAfter()
    .subProcess("subProcess").camundaAsyncAfter()
    .embeddedSubProcess()
    .startEvent()
    .serviceTask("task").camundaAsyncAfter().camundaExpression("${true}")
    .endEvent()
    .subProcessDone()
    .endEvent("end").camundaAsyncAfter()
    .done()
    );
    // when
    ProcessInstance instanceAfterStart = runtimeService.startProcessInstanceById(testProcess.getId());
    ProcessInstance instanceAfterTask = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceAfterTask);
    ProcessInstance instanceAfterSubProcess = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceAfterSubProcess);
    executeJobForProcessInstance(instanceAfterSubProcess);
    ProcessInstance instanceAfterEnd = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceAfterEnd);
    executeJobForProcessInstance(instanceAfterEnd);
    executeJobForProcessInstance(instanceAfterEnd);
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("start");
    assertReturnedProcessInstances(query, instanceAfterStart);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("task");
    assertReturnedProcessInstances(query, instanceAfterTask);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("subProcess");
    assertReturnedProcessInstances(query, instanceAfterSubProcess);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("end");
    assertReturnedProcessInstances(query, instanceAfterEnd);
    }
    @Test
    public void testQueryByActivityIdBeforeCompensation() {
    // given
    ProcessDefinition testProcess = testHelper.deployAndGetDefinition(CompensationModels.COMPENSATION_ONE_TASK_SUBPROCESS_MODEL);
    // when
    runtimeService.startProcessInstanceById(testProcess.getId());
    testHelper.completeTask("userTask1");
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("subProcess");
    assertNoProcessInstancesReturned(query);
    }
    @Test
    public void testQueryByActivityIdDuringCompensation() {
    // given
    ProcessDefinition testProcess = testHelper.deployAndGetDefinition(CompensationModels.COMPENSATION_ONE_TASK_SUBPROCESS_MODEL);
    // when
    ProcessInstance processInstance = runtimeService.startProcessInstanceById(testProcess.getId());
    testHelper.completeTask("userTask1");
    testHelper.completeTask("userTask2");
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("subProcess");
    assertReturnedProcessInstances(query, processInstance);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("compensationEvent");
    assertReturnedProcessInstances(query, processInstance);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("compensationHandler");
    assertReturnedProcessInstances(query, processInstance);
    }
    • One candidate would be passing null but also some others might make sense.

Copy link
Member

@yanavasileva yanavasileva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Do we need to test call activities? I mean an incident in a call activity.

@@ -626,6 +636,21 @@
)
</if>

<if test="query.activityIds != null and query.activityIds.length > 0">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Do we need to consider adding some indexes here? To ensure performance for the modification. Or the existing indexes will be good enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's be careful with indices on history tables. I would only add them when we get feedback. Adding a new index for existing databases with tables that have huge amounts of data is a costly operation, and it slows down process execution since history insertion is slower.

Copy link
Member

@yanavasileva yanavasileva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✍️ To add to Tassilo's suggestions further.
We need to have tests for filtering historic instances where some of them have been already completed/deleted.
Similar to what was done in #4624 test coverage. Since that will be the usual case to do modification.

@venetrius
Copy link
Member Author

  • You are only testing the use case with a single activityId but not with multiple ids.

  • Can we test multiple activities in different scopes with and without incidents?

  • ❓ Does it make sense to copy some of these tests over to HistoricProcessInstanceTest (please scroll in the code excerpt to see all the tests)

    @Test
    public void testQueryByNullActivityId() {
    try {
    runtimeService.createProcessInstanceQuery()
    .activityIdIn((String) null);
    fail("exception expected");
    }
    catch (NullValueException e) {
    assertThat(e.getMessage()).contains("activity ids contains null value");
    }
    }
    @Test
    public void testQueryByNullActivityIds() {
    try {
    runtimeService.createProcessInstanceQuery()
    .activityIdIn((String[]) null);
    fail("exception expected");
    }
    catch (NullValueException e) {
    assertThat(e.getMessage()).contains("activity ids is null");
    }
    }
    @Test
    public void testQueryByUnknownActivityId() {
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery()
    .activityIdIn("unknown");
    assertNoProcessInstancesReturned(query);
    }
    @Test
    public void testQueryByLeafActivityId() {
    // given
    ProcessDefinition oneTaskDefinition = testHelper.deployAndGetDefinition(ProcessModels.ONE_TASK_PROCESS);
    ProcessDefinition gatewaySubProcessDefinition = testHelper.deployAndGetDefinition(FORK_JOIN_SUB_PROCESS_MODEL);
    // when
    ProcessInstance oneTaskInstance1 = runtimeService.startProcessInstanceById(oneTaskDefinition.getId());
    ProcessInstance oneTaskInstance2 = runtimeService.startProcessInstanceById(oneTaskDefinition.getId());
    ProcessInstance gatewaySubProcessInstance1 = runtimeService.startProcessInstanceById(gatewaySubProcessDefinition.getId());
    ProcessInstance gatewaySubProcessInstance2 = runtimeService.startProcessInstanceById(gatewaySubProcessDefinition.getId());
    Task task = engineRule.getTaskService().createTaskQuery()
    .processInstanceId(gatewaySubProcessInstance2.getId())
    .taskName("completeMe")
    .singleResult();
    engineRule.getTaskService().complete(task.getId());
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("userTask");
    assertReturnedProcessInstances(query, oneTaskInstance1, oneTaskInstance2);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("userTask1", "userTask2");
    assertReturnedProcessInstances(query, gatewaySubProcessInstance1, gatewaySubProcessInstance2);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("userTask", "userTask1");
    assertReturnedProcessInstances(query, oneTaskInstance1, oneTaskInstance2, gatewaySubProcessInstance1);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("userTask", "userTask1", "userTask2");
    assertReturnedProcessInstances(query, oneTaskInstance1, oneTaskInstance2, gatewaySubProcessInstance1, gatewaySubProcessInstance2);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("join");
    assertReturnedProcessInstances(query, gatewaySubProcessInstance2);
    }
    @Test
    public void testQueryByNonLeafActivityId() {
    // given
    ProcessDefinition processDefinition = testHelper.deployAndGetDefinition(FORK_JOIN_SUB_PROCESS_MODEL);
    // when
    runtimeService.startProcessInstanceById(processDefinition.getId());
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("subProcess", "fork");
    assertNoProcessInstancesReturned(query);
    }
    @Test
    public void testQueryByAsyncBeforeActivityId() {
    // given
    ProcessDefinition testProcess = testHelper.deployAndGetDefinition(ProcessModels.newModel()
    .startEvent("start").camundaAsyncBefore()
    .subProcess("subProcess").camundaAsyncBefore()
    .embeddedSubProcess()
    .startEvent()
    .serviceTask("task").camundaAsyncBefore().camundaExpression("${true}")
    .endEvent()
    .subProcessDone()
    .endEvent("end").camundaAsyncBefore()
    .done()
    );
    // when
    ProcessInstance instanceBeforeStart = runtimeService.startProcessInstanceById(testProcess.getId());
    ProcessInstance instanceBeforeSubProcess = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceBeforeSubProcess);
    ProcessInstance instanceBeforeTask = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceBeforeTask);
    executeJobForProcessInstance(instanceBeforeTask);
    ProcessInstance instanceBeforeEnd = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceBeforeEnd);
    executeJobForProcessInstance(instanceBeforeEnd);
    executeJobForProcessInstance(instanceBeforeEnd);
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("start");
    assertReturnedProcessInstances(query, instanceBeforeStart);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("subProcess");
    assertReturnedProcessInstances(query, instanceBeforeSubProcess);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("task");
    assertReturnedProcessInstances(query, instanceBeforeTask);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("end");
    assertReturnedProcessInstances(query, instanceBeforeEnd);
    }
    @Test
    public void testQueryByAsyncAfterActivityId() {
    // given
    ProcessDefinition testProcess = testHelper.deployAndGetDefinition(ProcessModels.newModel()
    .startEvent("start").camundaAsyncAfter()
    .subProcess("subProcess").camundaAsyncAfter()
    .embeddedSubProcess()
    .startEvent()
    .serviceTask("task").camundaAsyncAfter().camundaExpression("${true}")
    .endEvent()
    .subProcessDone()
    .endEvent("end").camundaAsyncAfter()
    .done()
    );
    // when
    ProcessInstance instanceAfterStart = runtimeService.startProcessInstanceById(testProcess.getId());
    ProcessInstance instanceAfterTask = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceAfterTask);
    ProcessInstance instanceAfterSubProcess = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceAfterSubProcess);
    executeJobForProcessInstance(instanceAfterSubProcess);
    ProcessInstance instanceAfterEnd = runtimeService.startProcessInstanceById(testProcess.getId());
    executeJobForProcessInstance(instanceAfterEnd);
    executeJobForProcessInstance(instanceAfterEnd);
    executeJobForProcessInstance(instanceAfterEnd);
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("start");
    assertReturnedProcessInstances(query, instanceAfterStart);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("task");
    assertReturnedProcessInstances(query, instanceAfterTask);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("subProcess");
    assertReturnedProcessInstances(query, instanceAfterSubProcess);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("end");
    assertReturnedProcessInstances(query, instanceAfterEnd);
    }
    @Test
    public void testQueryByActivityIdBeforeCompensation() {
    // given
    ProcessDefinition testProcess = testHelper.deployAndGetDefinition(CompensationModels.COMPENSATION_ONE_TASK_SUBPROCESS_MODEL);
    // when
    runtimeService.startProcessInstanceById(testProcess.getId());
    testHelper.completeTask("userTask1");
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("subProcess");
    assertNoProcessInstancesReturned(query);
    }
    @Test
    public void testQueryByActivityIdDuringCompensation() {
    // given
    ProcessDefinition testProcess = testHelper.deployAndGetDefinition(CompensationModels.COMPENSATION_ONE_TASK_SUBPROCESS_MODEL);
    // when
    ProcessInstance processInstance = runtimeService.startProcessInstanceById(testProcess.getId());
    testHelper.completeTask("userTask1");
    testHelper.completeTask("userTask2");
    // then
    ProcessInstanceQuery query = runtimeService.createProcessInstanceQuery().activityIdIn("subProcess");
    assertReturnedProcessInstances(query, processInstance);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("compensationEvent");
    assertReturnedProcessInstances(query, processInstance);
    query = runtimeService.createProcessInstanceQuery().activityIdIn("compensationHandler");
    assertReturnedProcessInstances(query, processInstance);
    }

    • One candidate would be passing null but also some others might make sense.
  • Added test when ActivityIdIn has multiple entries
  • Added tests with multiple activities in different scopes with and without incidents
  • Copy pasted the tests that are testing activityIdIn filter, it makes sense as we expect the two filter work in the same way in the 2 query types. Three of the copied test fails as queries do not yield the expected results, I added @Ignore annotation to them, but will come back later to see. If you have any insight in the mean time that would be most welcomed.

@venetrius
Copy link
Member Author

✍️ To add to Tassilo's suggestions further. We need to have tests for filtering historic instances where some of them have been already completed/deleted. Similar to what was done in #4624 test coverage. Since that will be the usual case to do modification.

Added test when some instances are deleted and completed

@venetrius venetrius force-pushed the 4618-add-ActivityIdIn-filter-to-HistoricProcessInstanceQuery branch from 4168695 to 7fb55ec Compare September 20, 2024 06:50
@venetrius
Copy link
Member Author

ActivityIdIn filter returns different processInstances for runtimeQuery and HistoricQuery in special cases around AsyncBefore, AsyncAfter, and compensation.
Updated the OpenApi doc, HistoricProcessInstanceQuery.java and relevant unit tests (testQueryByAsyncBeforeActivityId, testQueryByAsyncAfterActivityId, testQueryByActivityIdDuringCompensation) to document it.

@venetrius venetrius force-pushed the 4618-add-ActivityIdIn-filter-to-HistoricProcessInstanceQuery branch from 48bac24 to 86488bb Compare September 20, 2024 10:34
@yanavasileva yanavasileva removed their request for review September 20, 2024 10:58
Copy link
Member

@tasso94 tasso94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@@ -626,6 +636,21 @@
)
</if>

<if test="query.activityIds != null and query.activityIds.length > 0">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's be careful with indices on history tables. I would only add them when we get feedback. Adding a new index for existing databases with tables that have huge amounts of data is a costly operation, and it slows down process execution since history insertion is slower.

@venetrius venetrius force-pushed the 4618-add-ActivityIdIn-filter-to-HistoricProcessInstanceQuery branch from 0b8956c to e6dc437 Compare September 20, 2024 13:19
@venetrius venetrius merged commit fe6775a into master Sep 20, 2024
1 of 2 checks passed
@venetrius venetrius deleted the 4618-add-ActivityIdIn-filter-to-HistoricProcessInstanceQuery branch September 20, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-db Runs the builds for all databases. ci:default-build Runs the builds that have no explicit trigger (e.g. different history levels). ci:rest-api Runs extra builds for the REST API (currently only WLS compatibility builds).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants