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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

kmannuru
Copy link

@kmannuru kmannuru commented Jul 31, 2024 •
edited by joaquinfelici
Loading

Related to #3182

@joaquinfelici
Copy link
Contributor

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
Copy link
Author

kmannuru commented Aug 6, 2024 •
edited
Loading

@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
Copy link
Contributor

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

@joaquinfelici 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 yanavasileva mentioned this pull request Aug 21, 2024
Open
1 task
@joaquinfelici 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
@kmannuru
Copy link
Author

kmannuru commented Aug 30, 2024 •
edited
Loading

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
Copy link
Member

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

@yanavasileva
Copy link
Member

yanavasileva commented Sep 2, 2024 •
edited
Loading

@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
Copy link
Author

kmannuru commented Sep 4, 2024 •
edited
Loading

@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
Copy link
Author

@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
Copy link
Contributor

joaquinfelici commented Sep 18, 2024 •
edited
Loading

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
joaquinfelici requested changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@joaquinfelici joaquinfelici left a comment

Choose a reason for hiding this comment

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

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

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

@Test
public void shouldReturnHistoricProcInstWithMatchingState() {
// given
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

...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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@kmannuru
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joaquinfelici joaquinfelici joaquinfelici requested changes

Assignees

@kmannuru kmannuru

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:migration Runs the process instance migration builds. ci:rest-api Runs extra builds for the REST API (currently only WLS compatibility builds).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kmannuru @joaquinfelici @yanavasileva