Feature/git issue 4049 process instance api enhancement

[Nandanrshenoy]

Feature Description:
The Get Process Instance API does not return the Process Definition Key in the API response.

How it is currently being handled

To fetch the process definition key associated with a process instance, the end user needs to first make a call to the Get Process Instance API through which the definition ID can be retrieved and then subsequently make a call the Get Process definition API(https://docs.camunda.org/rest/camunda-bpm-platform/7.21-SNAPSHOT/{url}/process-definition/{id}) to retrieve the definition key.

Advantage of having this feature

This feature if added will avert making the additional call and the retrieval of process definition key can be supported in a single call using Get Process Instance API.

Test Screenshot

Ticket: #4049

[CLAassistant]

All committers have signed the CLA.

[yanavasileva]

Hi @Nandanrshenoy,

Thank you for raising this.
I will have a look at it and get back to you.
Please note before merge, you need to sign one time only the Contributor license agreement (CLA).

Best,
Yana

[Nandanrshenoy]

Hi Team ,
I have signed the license/cla but even then its showing as pending.Could you kindly help check this issue.Kindly check the below screenshot , the options are disabled for me.

[Nandanrshenoy]

@yanavasileva,
Could you please help with reviewing this PR.

Thanks and Regards,
Nandan Shenoy

[yanavasileva]

Hi @Nandanrshenoy,

I have signed the license/cla but even then its showing as pending.Could you kindly help check this issue.

We see that you have signed the CLA on 26th of March but we are not sure what is the issue.

I see that the PR is created with account @Nandanrshenoy but the commits are pushed with account @Nandan-shenoy.
It looks like to be related to your github account(s). Could you try again to sign after cleaning your browser cache for example?
Another option is that you face the same issue as reported here: cla-assistant/cla-assistant#352

[Nandanrshenoy]

@yanavasileva ,
Thanks for your inputs.I have resolved the CLA issue.Could you please help me with my PR approval.

Regards,
Nandan Shenoy

[Nandanrshenoy]

@yanavasileva ,
A gentle reminder to help me with the approval of this PR.Thanks for your support.

Regards,
Nandan Shenoy

[yanavasileva]

I am looking into the code. I need do some research before make up my mind. (Such as: are we concerned for performance as the REST API will do additional query, do we want to expose the field to the Java API (pd id is not)) And I need to read a bit for the association element in mybatis.
First feedback, that I can share is that tests are missing in the contribution, for both REST API and Java. After go over the mentioned above I will get back to you with further feedback.
Thank you for your patience with this.

[yanavasileva]

Note to myself:

Details

• Do we need the association in the mybatis?
  • with this implementation, it's not used.
  • Also the executionResultMap is used for more queries where there's no PD join.
• In REST API, we do sometime additional queries as prerequisite for some endpoints (fetching PD by key in startForm by PD key)
• If we don't want to have the additional pd query, we can introduce a flag get PI "with PD key included". Otherwise the additional query will be executed all the time, every time.
• Failing test: ProcessInstanceRestServiceInteractionTest.testGetSingleInstance

java.lang.AssertionError: 
1 expectation failed.
JSON path definitionKey doesn't match.
Expected: aKey
  Actual: null

	at org.camunda.bpm.engine.rest.ProcessInstanceRestServiceInteractionTest.testGetSingleInstance(ProcessInstanceRestServiceInteractionTest.java:1090)

[Nandanrshenoy]

@yanavasileva ,
Thanks for sharing your insights. I will await for your response.
On the missing testcases for Rest and Java ,I had updated one of the test files to support my change which is in : engine-rest/engine-rest/src/test/java/org/camunda/bpm/engine/rest/ProcessInstanceRestServiceInteractionTest.java.
Could you please share some additional information on which module/package I am supposed to look in to for missing REST/Java test cases.

Thanks and Regards,
Nandan Shenoy

[yanavasileva]

Hi @Nandanrshenoy,

I evaluated the proposed changes.
❌My suggestion is to add a new process definition key column to the Execution table instead of relying on an association and having additional queries to the database. So similar to processDefintionId, persist the processDefinitionKey as well. We already have the same in other tables (job, job def).

🔧 Please remove the GIT Issue comments from the code.

🔧 I found the following test cases that can be extended with the new API:

• https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/test/java/org/camunda/bpm/engine/test/standalone/entity/ExecutionEntityTest.java#L110-L111
• https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/test/java/org/camunda/bpm/engine/test/api/runtime/ExecutionQueryTest.java#L96-L101

[yanavasileva]

❌ Failing test (the mocks are not updated): ProcessInstanceRestServiceInteractionTest.testGetSingleInstance with

java.lang.AssertionError: 
1 expectation failed.
JSON path definitionKey doesn't match.
Expected: aKey
  Actual: null
	at org.camunda.bpm.engine.rest.ProcessInstanceRestServiceInteractionTest.testGetSingleInstance(ProcessInstanceRestServiceInteractionTest.java:1090)

How to run the REST API tests (after the engine and sql-script modules are built)?:
mvn clean install -Pjersey2 -f engine-rest/engine-rest/pom.xml

[yanavasileva]

👍 Changes in OpenAPI look good.

[Nandanrshenoy]

Hi @yanavasileva ,
Thanks for taking your precious timeout and helping me with your validation on this PR.
I am good with all your comments but needed some slight clarity on your proposal for adding a new Process definition Key Column on the Execution table.
Initially my idea was on similar lines but here is why I reverted against it.
Process definition Key is a static attribute of Process definition and hence its available rightly under ACT_RE_PROCDEF.
The reason we had PROC Definition ID in Execution table was mainly to have a linkage between ACT_RE_PROCDEF and ACT_RU_EXECUTION.
My main intension to go ahead with association methodology was that the JOIN already existed between the above two tables and we could programmatically achieve adding PDK to the response than bloating up the execution table with new columns just to have enhanced responses.
I am sure you may have other thoughts with this regard and I am happy to understand/learn with your point of view as well.

Thanks and Regards,
Nandan Shenoy

[yanavasileva]

Hi @Nandanrshenoy,

Yes, you're right, the key is somehow static. (That's why it was not already in the execution table.)

1. I am not so familiar with the association concept but I tried out the current changes without the association and the rest of the changes in mybatis script; and it turned out that the association was not used at all.
2. The current implementation does an additional database query to fetch the process definition of it's not previously cached. (Via #getProcessDefinition() -> #ensureProcessDefinitionInitialized())
3. The extended result map (executionResultMap) is used in more queries and not all of them have the mentioned join. So it could happen to request the execution's process definition key but due to missing JOIN it won't be returned. That might create inconsistencies and/or cause failures.
4. As mentioned, it's not precedence to add both process definition id and key to a database table.

Considering above, my suggestion is still the same to introduce a new column to the execution table.

Best,
Yana

[Nandanrshenoy]

Thanks @yanavasileva for sharing your point of view.
In a nutshell, you wish to have the additional column of Process definition Key added in the execution table and have new queries defined in execution xml file to retrieve all values from the execution table and not use the existing queries. Kindly correct if my understanding is wrong.

Thanks and Regards,
Nandan Shenoy

[yanavasileva]

My suggestion is add new column in the execution table and add new result to the executionResultMap in the Execution.xml file. The queries will stay the same.

[yanavasileva]

Closing due to inactivity. The PR can be reopen at again when you get back to the topic.

[Nandanrshenoy]

@yanavasileva,I have a PR that's lined up for a contribution. Could you please open this PR.

Thanks and Regards,
Nandan Shenoy

[yanavasileva]

@Nandanrshenoy, PR is reopened.

[Nandanrshenoy]

@Nandanrshenoy, PR is reopened.

Thanks @yanavasileva for all your support.

[Nandanrshenoy]

@yanavasileva ,Could you please help me with a review for this PR.
A kind note : As per your suggestion, I added the PDK in the execution table. When I did an impact analysis ,I noticed that PDK was getting populated on instantiation when the start event was none but In case of conditional start event, Message start event and timer event I could see this column was empty. If you can verify this change associated with none event and give me a go ahead ,I will start working on the other three changes mentioned changes and deliver them in one go.Thanks.

Regards,
Nandan Shenoy

[Nandanrshenoy]

@yanavasileva ,
Request you to kindly help me with this review.

Thanks and Regards,
Nandan Shenoy

[yanavasileva]

I will work on the review in the next days.

[yanavasileva]

@Nandanrshenoy, thank you for considering my feedback. You are on a good track, however, more work is required.

✍️ Please check the places where the OpenAPI needs to be adjusted. They are good indication where tests for the REST API and Java API should be added.

[yanavasileva]

When I did an impact analysis ,I noticed that PDK was getting populated on instantiation when the start event was none but In case of conditional start event, Message start event and timer event I could see this column was empty. If you can verify this change associated with none event and give me a go ahead

Please see #4222 (comment), with the current changes the observed behaviour is expected. I suggest to set the key where the process definition id is set, that way we keep the code consistent and you won't need to look for how to cover the mentioned cases.

[yanavasileva]

I will continue the review.

[yanavasileva]

Closing as it doesn't contain the latest changes.