Fix get historic process instance api orquery for mutiple statues and… #4515

kmannuru · 2024-07-31T22:57:41Z

Related to #3182

… withIncidents combined with orQuery

joaquinfelici · 2024-08-02T14:39:50Z

Hello @kmannuru,
Thanks for contributing. Before this PR is ready to be reviewed, could you please explain a bit about the context and the value that this fix would bring? Also please open a new issue by using one of our templates, fill necessary fields and link it in this PR.

kmannuru · 2024-08-06T15:50:43Z

@joaquinfelici This PR is to fix for the issue: #3182
and also the related issue discussed in camunda forum https://forum.camunda.io/t/rest-orqueries-historic-process-instance-statuses/26518/2.
Please help review.

joaquinfelici · 2024-08-07T15:33:13Z

Hello @kmannuru ,
Thanks for your reply. I'll link the issue in the PR and I'll review it on the following days.

kmannuru · 2024-08-30T17:12:34Z

Hello @kmannuru , Thanks for your reply. I'll link the issue in the PR and I'll review it on the following days.

Hello @joaquinfelici following up to see if you're able to review the PR.

yanavasileva · 2024-09-02T10:48:13Z

@kmannuru, the review is in progress but not completed yet.

yanavasileva · 2024-09-02T10:51:06Z

@kmannuru, Are you interested in checking if the issue occurs for the other "Or queries" too (runtime and tasks)? Docs: https://docs.camunda.org/manual/7.21/user-guide/process-engine/process-engine-api/#or-queries

kmannuru · 2024-09-04T14:54:26Z

@kmannuru, Are you interested in checking if the issue occurs for the other "Or queries" too (runtime and tasks)? Docs: https://docs.camunda.org/manual/7.21/user-guide/process-engine/process-engine-api/#or-queries

@yanavasileva Sure. I can look into this.

kmannuru · 2024-09-11T05:15:12Z

@yanavasileva I'm currently working on checking if the same " or Query" issue occurs for runtime and tasks. Meanwhile checking if my current fix can be pushed part of the upcoming camunda release. Thanks.

joaquinfelici · 2024-09-18T17:52:33Z

Hello @kmannuru ,

I'm sorry I couldn't review your PR earlier. I'm currently reviewing it and will provide feedback tomorrow.

In the meantime, I cloned this PR to launch CI and we have some failing tests here. These are audit tests and the failures are probably due to the history level (audit level doesn't include incidents, more info here). Even some tests that are passing might have an incorrect history level, please check this part when you have the chance.

Test failure

shouldReturnHistoricProcInstWithVarValue1OrVarValue21:

org.camunda.bpm.engine.test.history.HistoricProcessInstanceQueryOrTest<1s
Error
expected:<1> but was:<0>
Stacktrace
java.lang.AssertionError: expected:<1> but was:<0>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
	at org.camunda.bpm.engine.test.history.HistoricProcessInstanceQueryOrTest.shouldReturnHistoricProcInstWithVarValue1OrVarValue21(HistoricProcessInstanceQueryOrTest.java:951)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:61)
	at org.camunda.bpm.engine.test.ProcessEngineRule$1.evaluate(ProcessEngineRule.java:191)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:365)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:273)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:238)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:159)
	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)

joaquinfelici

Thanks for raising this, the changes look good overall, I added some comments.

joaquinfelici · 2024-09-19T13:34:11Z

...ne/src/test/java/org/camunda/bpm/engine/test/history/HistoricProcessInstanceQueryOrTest.java

+
+  @Test
+  public void shouldReturnHistoricProcInstWithMatchingState() {
+    // given


I think the whole given block of this test and the one above is exactly the same, please extract it into a method with a significant name and reuse the code.

joaquinfelici · 2024-09-19T13:47:38Z

engine/src/main/java/org/camunda/bpm/engine/impl/HistoricProcessInstanceQueryImpl.java

@@ -91,7 +91,7 @@ public class HistoricProcessInstanceQueryImpl extends AbstractVariableQueryImpl<
  protected boolean isTenantIdSet;
  protected String[] executedActivityIds;
  protected String[] activeActivityIds;
-  protected String state;
+  protected Set<String> state = new HashSet<>();


All changes related to multiple state handling don't seem to be described or explained in the issue. Only the unexpected behavior of the combination of withIncidents and orQueries is described there. Could you please document this as well?

joaquinfelici · 2024-09-19T14:44:41Z

...ne/src/test/java/org/camunda/bpm/engine/test/history/HistoricProcessInstanceQueryOrTest.java

@@ -817,4 +917,89 @@ public void shouldReturnByProcessDefinitionIdOrIncidentType() {
    assertThat(processInstances.size()).isEqualTo(2);
  }

+  @Test
+  @Deployment(resources={"org/camunda/bpm/engine/test/api/oneTaskProcess.bpmn20.xml"})
+  public void shouldReturnHistoricProcInstWithVarValue1OrVarValue21() {


As mentioned here, this and the test below require FULL history level (more info).

kmannuru · 2024-09-24T15:24:54Z

@joaquinfelici Thanks for your feedback. I'll work on the changes.

joaquinfelici · 2024-10-28T10:10:02Z

Hello @kmannuru , I'll be closing this pull request for now. You can re-open it whenever you get the chance to work on the changes. Thanks a lot!

Fix get historic process instance api orquery for mutiple statues and…

7fa42a5

… withIncidents combined with orQuery

joaquinfelici assigned kmannuru Aug 2, 2024

joaquinfelici added ci:rest-api Runs extra builds for the REST API (currently only WLS compatibility builds). and removed ci:rest-api Runs extra builds for the REST API (currently only WLS compatibility builds). labels Aug 7, 2024

yanavasileva mentioned this pull request Aug 21, 2024

Historic process instances query withIncidents combined with orQueries provides incorrect results #3182

Open

1 task

yanavasileva requested a review from joaquinfelici August 21, 2024 08:17

joaquinfelici added 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:migration Runs the process instance migration builds. labels Aug 21, 2024

joaquinfelici requested changes Sep 19, 2024

View reviewed changes

joaquinfelici closed this Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix get historic process instance api orquery for mutiple statues and… #4515

Fix get historic process instance api orquery for mutiple statues and… #4515

kmannuru commented Jul 31, 2024 •

edited by joaquinfelici

Loading

joaquinfelici commented Aug 2, 2024

kmannuru commented Aug 6, 2024 •

edited

Loading

joaquinfelici commented Aug 7, 2024

kmannuru commented Aug 30, 2024 •

edited

Loading

yanavasileva commented Sep 2, 2024

yanavasileva commented Sep 2, 2024 •

edited

Loading

kmannuru commented Sep 4, 2024 •

edited

Loading

kmannuru commented Sep 11, 2024

joaquinfelici commented Sep 18, 2024 •

edited

Loading

joaquinfelici left a comment

joaquinfelici Sep 19, 2024

joaquinfelici Sep 19, 2024

joaquinfelici Sep 19, 2024

kmannuru commented Sep 24, 2024

joaquinfelici commented Oct 28, 2024

Fix get historic process instance api orquery for mutiple statues and… #4515

Fix get historic process instance api orquery for mutiple statues and… #4515

Conversation

kmannuru commented Jul 31, 2024 • edited by joaquinfelici Loading

joaquinfelici commented Aug 2, 2024

kmannuru commented Aug 6, 2024 • edited Loading

joaquinfelici commented Aug 7, 2024

kmannuru commented Aug 30, 2024 • edited Loading

yanavasileva commented Sep 2, 2024

yanavasileva commented Sep 2, 2024 • edited Loading

kmannuru commented Sep 4, 2024 • edited Loading

kmannuru commented Sep 11, 2024

joaquinfelici commented Sep 18, 2024 • edited Loading

joaquinfelici left a comment

Choose a reason for hiding this comment

joaquinfelici Sep 19, 2024

Choose a reason for hiding this comment

joaquinfelici Sep 19, 2024

Choose a reason for hiding this comment

joaquinfelici Sep 19, 2024

Choose a reason for hiding this comment

kmannuru commented Sep 24, 2024

joaquinfelici commented Oct 28, 2024

kmannuru commented Jul 31, 2024 •

edited by joaquinfelici

Loading

kmannuru commented Aug 6, 2024 •

edited

Loading

kmannuru commented Aug 30, 2024 •

edited

Loading

yanavasileva commented Sep 2, 2024 •

edited

Loading

kmannuru commented Sep 4, 2024 •

edited

Loading

joaquinfelici commented Sep 18, 2024 •

edited

Loading