search

camunda / camunda-bpm-platform

Flexible framework for workflow and decision automation with BPMN and DMN. Integration with Quarkus, Spring, Spring Boot, CDI.
https://camunda.com/
Apache License 2.0
4.11k stars 1.55k forks source link

Feature/git issue 4049 process instance api enhancement #4222

Open Nandanrshenoy opened 6 months ago

Nandanrshenoy commented 6 months ago

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

image-2024-3-8_14-41-18

Ticket: https://github.com/camunda/camunda-bpm-platform/issues/4049

CLAassistant commented 6 months ago

CLA assistant check
All committers have signed the CLA.

yanavasileva commented 6 months ago

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 commented 6 months ago

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.

image

Nandanrshenoy commented 6 months ago

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

Thanks and Regards, Nandan Shenoy

yanavasileva commented 6 months ago

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: https://github.com/cla-assistant/cla-assistant/issues/352

Nandanrshenoy commented 6 months ago

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

Regards, Nandan Shenoy

Nandanrshenoy commented 6 months ago

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

Regards, Nandan Shenoy

yanavasileva commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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

Nandanrshenoy commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

Nandanrshenoy commented 3 months ago

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

Thanks and Regards, Nandan Shenoy

yanavasileva commented 3 months ago

@Nandanrshenoy, PR is reopened.

Nandanrshenoy commented 3 months ago

@Nandanrshenoy, PR is reopened.

Thanks @yanavasileva for all your support.

Nandanrshenoy commented 2 months ago

@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 commented 2 months ago

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

Thanks and Regards, Nandan Shenoy

yanavasileva commented 1 month ago

I will work on the review in the next days.

yanavasileva commented 1 month ago

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 https://github.com/camunda/camunda-bpm-platform/pull/4222#discussion_r1730879655, 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.