-
Notifications
You must be signed in to change notification settings - Fork 47
AAE-39413 Change relationship of IntegrationContextEntity and ServiceTaskEntity #2047
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
Conversation
This reverts commit ebffb67.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request changes the relationship between IntegrationContextEntity and ServiceTaskEntity from a one-to-one to a one-to-many relationship, allowing multiple integration contexts to be associated with a single service task. This supports cyclical service tasks and retry scenarios.
Key changes:
- Changed JPA relationship from
@OneToOneto@OneToManyonServiceTaskEntityand@ManyToOneonIntegrationContextEntity - Modified database unique constraint to allow multiple integration contexts per service task
- Added new REST endpoint
/admin/v1/service-tasks/{serviceTaskId}/integration-contextsto retrieve all integration contexts for a service task - Added
integrationContextCounterfield toCloudServiceTaskto track the number of integration contexts
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| IntegrationContextEntity.java | Changed relationship from @OnetoOne to @manytoone with composite foreign key join columns; removed unique constraint on process/client/execution combination; simplified setServiceTask method |
| ServiceTaskEntity.java | Changed relationship from @OnetoOne to @onetomany; added getIntegrationContextCounter method that counts associated integration contexts |
| IntegrationContextRepository.java | Updated findByProcessInstanceIdAndClientIdAndExecutionId to return Page instead of single entity and added Pageable parameter |
| ServiceTaskIntegrationContextAdminController.java | Refactored findById to findByServiceTaskId with string parsing logic; added new findAllByServiceTaskId endpoint; removed EntityFinder dependency |
| IntegrationContextRepresentationModelAssembler.java | Updated to call renamed controller method findByServiceTaskId |
| QueryRestControllersAutoConfiguration.java | Removed unused import TaskControllerAdvice |
| IntegrationRequestedEventHandler.java | Separated integration context ID from service task ID; now uses integrationContext.getId() directly |
| IntegrationErrorReceivedEventHandler.java | Added explicit service task ID construction using IdBuilderHelper |
| BaseIntegrationEventHandler.java | Changed to use integrationContext.getId() instead of IdBuilderHelper for finding integration contexts |
| CloudServiceTask.java | Added getIntegrationContextCounter() method to interface |
| CloudServiceTaskImpl.java | Implemented integrationContextCounter field with getter, setter, and updated toString method |
| 33-alter.pg.schema.8.8.0.sql | Database migration script for PostgreSQL to modify unique constraint |
| 33-alter.oracle.schema.8.8.0.sql | Database migration script for Oracle to modify unique constraint |
| h2.schema.sql | Removed unique constraint creation from H2 schema |
| master.xml | Added new Liquibase changesets for database migrations |
| swagger-expected.json | Updated OpenAPI specification with new endpoint, parameters, and schemas; renamed operation from findById to findByServiceTaskId |
| QueryAdminProcessServiceTasksIT.java | Added integration tests for multiple integration contexts functionality |
| ProcessQueryAdminSteps.java | Added step method to retrieve all integration contexts |
| ProcessQueryAdminService.java | Added service method to call new REST endpoint |
| process-instance-service-tasks-actions.story | Added acceptance test scenario for retrieving all integration contexts |
| ProcessInstanceServiceTasks.java | Implemented test steps for verifying multiple integration contexts retrieval |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...quibase/src/main/resources/config/query/liquibase/changelog/33-alter.oracle.schema.8.8.0.sql
Outdated
Show resolved
Hide resolved
...ava/org/activiti/cloud/services/query/rest/ServiceTaskIntegrationContextAdminController.java
Outdated
Show resolved
Hide resolved
...ces-query-model/src/main/java/org/activiti/cloud/services/query/model/ServiceTaskEntity.java
Show resolved
Hide resolved
...viti/cloud/services/query/rest/assembler/IntegrationContextRepresentationModelAssembler.java
Outdated
Show resolved
Hide resolved
...ti-cloud-query-service/activiti-cloud-starter-query/src/test/resources/swagger-expected.json
Outdated
Show resolved
Hide resolved
...ry-model/src/main/java/org/activiti/cloud/services/query/model/IntegrationContextEntity.java
Show resolved
Hide resolved
...ti-cloud-query-service/activiti-cloud-starter-query/src/test/resources/swagger-expected.json
Outdated
Show resolved
Hide resolved
...model-impl/src/main/java/org/activiti/cloud/api/process/model/impl/CloudServiceTaskImpl.java
Outdated
Show resolved
Hide resolved
...ava/org/activiti/cloud/services/query/rest/ServiceTaskIntegrationContextAdminController.java
Outdated
Show resolved
Hide resolved
...y-liquibase/src/main/resources/config/query/liquibase/changelog/33-alter.pg.schema.8.8.0.sql
Outdated
Show resolved
Hide resolved
# Conflicts: # activiti-cloud-query-service/activiti-cloud-services-query/activiti-cloud-services-query-liquibase/src/main/resources/config/query/liquibase/changelog/33-alter.oracle.schema.8.8.0.sql # activiti-cloud-query-service/activiti-cloud-services-query/activiti-cloud-services-query-liquibase/src/main/resources/config/query/liquibase/changelog/33-alter.pg.schema.8.8.0.sql
erdemedeiros
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! I've added some questions and suggestions
| .untilAsserted(() -> { | ||
| PagedModel<CloudServiceTask> tasks = processQueryAdminSteps.getServiceTasks(processId); | ||
|
|
||
| assertThat(tasks.getContent()).isNotEmpty().hasSize(1).as("Should have exactly one service task"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calling isNotEmpty() is redundant. This assertion will already fail if size is different from 1.
| .untilAsserted(() -> { | ||
| PagedModel<CloudServiceTask> tasks = processQueryAdminSteps.getServiceTasks(processId); | ||
|
|
||
| assertThat(tasks.getContent()).isNotEmpty().hasSize(1).as("Should have exactly one service task"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isNotEmpty() is redundant
| CloudServiceTask serviceTask = tasks.getContent().iterator().next(); | ||
|
|
||
| assertThat(serviceTask.getIntegrationContextCounter()) | ||
| .isNotNull() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isNotNull() is redundant
| @@ -0,0 +1,70 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to add coverage for multi-instances as well once https://hyland.atlassian.net/browse/AAE-41417 is fixed (in a follow up PR).
| name = "integration_context_processInstance_elementId_idx", | ||
| columnList = "processInstanceId,clientId,executionId", | ||
| unique = true | ||
| name = "integration_context_proc_client_exec_idx", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth testing how an existing integration context will behave during migration. I think you can test this by making a connector fail in the first attempt, then using replay after migration
| public EntityModel<CloudIntegrationContext> toModel(IntegrationContextEntity entity) { | ||
| Link selfRel = linkTo(methodOn(ServiceTaskIntegrationContextAdminController.class).findById(entity.getId())) | ||
| Link selfRel = linkTo( | ||
| methodOn(ServiceTaskIntegrationContextAdminController.class).findByServiceTaskId(entity.getId()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this? entity here is IntegrationContextEntity, so getId() will be returning the new id format, if you want to use service task Id you need to build it again using IdBuilderHelper.from(entity) as suggested by Copilot. However, this is not going to be selfRel, as it's not retuning the element itself. So I think either we remove the selfRel link here or we expose a new endpoint to be able to return a single IntegrationContext based on its id (/integration-contexts/{inegrationContextID}) and use this new end point for self link
| repository, | ||
| serviceTaskId, | ||
| "Unable to find integration context entity for the given id:'" + serviceTaskId + "'" | ||
| public EntityModel<CloudIntegrationContext> findByServiceTaskId(@PathVariable String serviceTaskId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we deprecated this endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we should deprecate this endpoint.
| assertThat(responseEntity.getStatusCode()).isEqualTo(HttpStatus.OK); | ||
| assertThat(responseEntity.getBody()).isNotNull(); | ||
| assertThat(responseEntity.getBody().getContent()) | ||
| .hasSize(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get ride of .hasSize(1) if you use containsExactly instead of contains
| ); | ||
| assertThat(cloudIntegrationContext) | ||
| .isNotEmpty() | ||
| .hasSize(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasSize is redundant. The assertion is already fail if it has a different size because containsExactly is used
| ); | ||
| assertThat(cloudIntegrationContext) | ||
| .isNotEmpty() | ||
| .hasSize(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasSize is redundant
erdemedeiros
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
igdianov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should update
to include IntegrationContextEntity in the optimization logic after service tasks before entityManager.load to avoid performance bottlenecks.| private List<IntegrationContextEntity> integrationContexts; | ||
|
|
||
| @Formula( | ||
| "(SELECT COUNT(*) FROM INTEGRATION_CONTEXT ic WHERE ic.process_instance_id = process_instance_id AND ic.client_id = element_id AND ic.execution_id = execution_id)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using @Forumula here will cause a performance bottleneck, i.e. N+1 query for each row. This would be solved by incrementing and storing this counter in the completed integration context Query handler.
...ces-query-model/src/main/java/org/activiti/cloud/services/query/model/ServiceTaskEntity.java
Show resolved
Hide resolved
| repository, | ||
| serviceTaskId, | ||
| "Unable to find integration context entity for the given id:'" + serviceTaskId + "'" | ||
| public EntityModel<CloudIntegrationContext> findByServiceTaskId(@PathVariable String serviceTaskId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we should deprecate this endpoint.
| @@ -0,0 +1,8 @@ | |||
| ALTER TABLE integration_context | |||
| DROP CONSTRAINT integration_context_bpmn_activity_idx; | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are going to be running service tasks during the database upgrade window that should be migrated before creating the new index.
igdianov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
|


No description provided.