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

Implementation of a method subprocesses #2260

Merged
merged 11 commits into from Dec 21, 2018

Conversation

Projects
None yet
4 participants
@CTI777
Copy link
Contributor

CTI777 commented Dec 12, 2018

@CTI777 CTI777 self-assigned this Dec 12, 2018

@CTI777 CTI777 requested a review from erdemedeiros Dec 12, 2018

@CTI777 CTI777 added the in progress label Dec 12, 2018

processInstance.setProcessDefinitionVersion(internalProcessInstance.getProcessDefinitionVersion());

//To do: it is not the best way to search parentProcessId by this method, it will require extra queries!

This comment has been minimized.

@igdianov

igdianov Dec 13, 2018

Member

It will be a general problem for lazy loaded associations when using converters outside of Activiti engine transaction. This will also create N+1 additional queries to database if we need to resolve child entities for each record. We should be able to solve this by implementing or using a query to join associated entities in one query from the database.

This comment has been minimized.

@erdemedeiros

erdemedeiros Dec 13, 2018

Member

Another question: do we really need the parentProcessInstanceId or rootProcessInstanceId and callerId(not sure how to name this one) is enough? If we really need it, maybe we should consider to add an extra property to the internal process instance and store it in the database: similar to what is done for rootProcessInstanceId. @igdianov what do you think about that? @salaboy you may be interested in this conversation as well.

This comment has been minimized.

@CTI777

CTI777 Dec 13, 2018

Contributor

rootProcessInstanceId - is present but not clear if it is supported (filled) at all.
parentId (parentProcessInstanceId) we will need for sure to get list if subprocesses...
What is callerId and do we need it is a question...

This comment has been minimized.

@igdianov

igdianov Dec 13, 2018

Member

@erdemedeiros It makes sense to expose and provide parentProcessInstanceId attribute for query and audit services. This use case and provided hack exposes several problems. Problem 1 is that internal entity model converters need to run inside Activiti transaction in order to be able to resolve lazily associated superExecution entity, to get its parent process instance id. Problem 2 is that resolving lazy associations this way for each execution record will create additional queries to the engine database from many places when resolving execution context for aggregated events and api queries that use internal converters because of Problem 1.

The parent process instance seems to be a common property shared and used by model entities and event classes, so this particular use case could be solved by making changes to the execution entity manager query in Activiti engine to always eagerly load super execution entity with join query. This should have better performance overall especially for queries via Rest api.

@erdemedeiros

This comment has been minimized.

Copy link
Member

erdemedeiros commented Dec 13, 2018

Refs #2113

Pageable pageable) {

//Process Instance will check security policies on read
ProcessInstance processInstance = processInstance(getSubprocessesPayload.getProcessInstanceId());

This comment has been minimized.

@erdemedeiros

erdemedeiros Dec 13, 2018

Member

Based on my comments on Activiti/activiti-api#62 this extra method is not necessary you can update processInstances method instead.

@@ -334,4 +335,21 @@ public ProcessInstance update(UpdateProcessPayload updateProcessPayload) {
return updatedProcessInstance;

}

@Override
public Page<ProcessInstance> subprocesses(GetSubprocessesPayload getSubprocessesPayload,

This comment has been minimized.

@erdemedeiros

erdemedeiros Dec 13, 2018

Member

Same here, you can update processInstances method instead.

processInstance.setProcessDefinitionVersion(internalProcessInstance.getProcessDefinitionVersion());

//To do: it is not the best way to search parentProcessId by this method, it will require extra queries!

This comment has been minimized.

@erdemedeiros

erdemedeiros Dec 13, 2018

Member

Another question: do we really need the parentProcessInstanceId or rootProcessInstanceId and callerId(not sure how to name this one) is enough? If we really need it, maybe we should consider to add an extra property to the internal process instance and store it in the database: similar to what is done for rootProcessInstanceId. @igdianov what do you think about that? @salaboy you may be interested in this conversation as well.

CTI777 and others added some commits Dec 13, 2018

fix: add Mybatis superExecution parent entity mapping
Enable eager fetching of execution's superExecution entity via Mybatis association property mapping
} else {
Optional.ofNullable(executionEntity.getSuperExecution())
.ifPresent(superExecution -> processInstance.setParentId(superExecution.getProcessInstanceId()));
}

This comment has been minimized.

@igdianov

igdianov Dec 18, 2018

Member

@erdemedeiros @CTI777 I have removed the hack in APIProcessInstanceConverter.java with commit f354348 in favor of using Mybatis mapping to eagerly fetch superExecution entity via association property. It seems like we always need to resolve parent process instance attributes in event converters and queries, so I think it makes sense to always fetch it.

See Mybatis changes below: f354348#diff-b9f50b6ee46df36fb61073916d78de78 for details.

CTI777 and others added some commits Dec 20, 2018

Merge branch 'develop' into CTI777-2113-Create-new-method-to-expose-s…
…ubprocess-IDs-to-a-given-process-instance
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 21, 2018

Codecov Report

Merging #2260 into develop will decrease coverage by <.01%.
The diff coverage is 20%.

Impacted file tree graph

@@             Coverage Diff              @@
##             develop   #2260      +/-   ##
============================================
- Coverage       61.2%   61.2%   -0.01%     
  Complexity      1557    1557              
============================================
  Files           1137    1137              
  Lines          40152   40157       +5     
  Branches        5830    5832       +2     
============================================
+ Hits           24576   24579       +3     
- Misses         12836   12839       +3     
+ Partials        2740    2739       -1
Impacted Files Coverage Δ Complexity Δ
...viti/runtime/api/impl/ProcessAdminRuntimeImpl.java 6.45% <0%> (-0.15%) 0 <0> (ø)
.../activiti/runtime/api/impl/ProcessRuntimeImpl.java 11.67% <0%> (-0.18%) 0 <0> (ø)
...me/api/model/impl/APIProcessInstanceConverter.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...e/impl/persistence/entity/ExecutionEntityImpl.java 84.82% <0%> (-0.62%) 0% <0%> (ø)
.../entity/data/impl/MybatisExecutionDataManager.java 79.31% <0%> (+1.72%) 0% <0%> (ø) ⬇️
...xecutionsWithSameRootProcessInstanceIdMatcher.java 37.5% <0%> (+12.5%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 324c595...6245bca. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 21, 2018

Codecov Report

Merging #2260 into develop will decrease coverage by 0.11%.
The diff coverage is 20%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2260      +/-   ##
=============================================
- Coverage       61.2%   61.08%   -0.12%     
  Complexity      1557     1557              
=============================================
  Files           1137     1137              
  Lines          40152    40251      +99     
  Branches        5830     5862      +32     
=============================================
+ Hits           24576    24589      +13     
- Misses         12836    12917      +81     
- Partials        2740     2745       +5
Impacted Files Coverage Δ Complexity Δ
...viti/runtime/api/impl/ProcessAdminRuntimeImpl.java 6.45% <0%> (-0.15%) 0 <0> (ø)
.../activiti/runtime/api/impl/ProcessRuntimeImpl.java 11.67% <0%> (-0.18%) 0 <0> (ø)
...me/api/model/impl/APIProcessInstanceConverter.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...ctiviti/runtime/api/impl/TaskAdminRuntimeImpl.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...org/activiti/runtime/api/impl/TaskRuntimeImpl.java 13.67% <0%> (+0.92%) 4% <0%> (ø) ⬇️
...xecutionsWithSameRootProcessInstanceIdMatcher.java 37.5% <0%> (+12.5%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 324c595...6245bca. Read the comment docs.

@salaboy salaboy self-requested a review Dec 21, 2018

@salaboy
Copy link
Member

salaboy left a comment

this is ready to merge

@salaboy salaboy merged commit e229c4c into develop Dec 21, 2018

5 of 8 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
codecov/patch 20% of diff hit (target 61.2%)
Details
codecov/project 61.08% (-0.12%) compared to 324c595
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
security/snyk - pom.xml (Activiti) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment