[NAE-2199] Force delete of processes #354
Conversation
- Introduced the ability to forcefully delete cases, tasks, and PetriNets, bypassing event-driven actions through a new `force` parameter. - Updated `WorkflowService`, `TaskService`, and `PetriNetService` to support forced deletion logic. - Modified controller endpoints to allow optional forcing of deletions via request parameters. - Updated relevant service interfaces to include new method overloads for forced deletion. - Improved code readability by introducing logging enhancements with structured placeholders. This change provides greater flexibility for scenarios with strict deletion requirements, ensuring optional event handling during deletions.
WalkthroughAdds a force-aware deletion path across controller, services, and interfaces. Introduces overloads with a boolean force parameter for Petri net, case, and task deletions. Updates WorkflowService to conditionally skip pre/post actions and events when force=true. Propagates the flag through bulk deletions. Minor logging and Optional API adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant C as PetriNetController
participant P as PetriNetService
participant W as WorkflowService
Client->>C: DELETE /petrinets/{id}?force={bool}
C->>P: deletePetriNet(id, user, force)
P->>W: deleteInstancesOfPetriNet(petriNet, force)
loop For each Case of PetriNet
W->>W: deleteCase(case, params, force)
alt force == false
Note over W: PRE actions/evaluators/events
W->>W: perform deletion
Note over W: POST actions/evaluators/events
else force == true
Note over W: Skip pre/post, direct deletion
end
end
P->>P: evict + reload cache
alt force == false
Note over P: Publish PRE and POST ProcessDelete events
else force == true
Note over P: Publish PRE and POST as implemented
end
C-->>Client: MessageResource
sequenceDiagram
autonumber
participant W as WorkflowService
participant T as TaskService
W->>T: deleteTasksByCase(caseId, force)
T->>T: find tasks by case
alt force == false
T->>W: removeTasksFromCase(caseId, tasks)
else force == true
Note over T: Skip unassignment
end
T->>T: repository delete + elastic cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
added
improvement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
nae-object-library/pom.xml (2)
77-81: Jackson version skew: hardcoded 2.18.2 vs root property 2.17.1.Mixing Jackson minor versions risks NoSuchMethodError/ClassCastException at runtime. Align all Jackson artifacts to a single version.
Apply one of the following diffs (preferred: centralize via root property):
Option A — use the root property here and bump root to 2.18.2:
- <version>2.18.2</version> + <version>${jackson.version}</version>And in the root pom.xml properties:
- <jackson.version>2.17.1</jackson.version> + <jackson.version>2.18.2</jackson.version>Option B — if you must stay on 2.17.x, downgrade jsr310 here to
${jackson.version}instead of 2.18.2.
102-107: Pin Log4j bridge via a property or BOM for consistency.
log4j-to-slf4jis pinned to 2.21.1 while the rest may be managed differently. Consider a${log4j2.version}property at the root or delegate to a BOM to avoid silent drifts.pom.xml (2)
80-104: Extraneous repositories increase supply-chain risk.Unless required for artifacts, drop
maven.imagej.net,repo.spring.io/plugins-release, andmulesoft. Prefer Maven Central (and OSSRH snapshots only where needed).
211-229: Outdated maven-gpg-plugin (1.5).Consider upgrading to a maintained 3.x to avoid signing quirks and CVEs in toolchain dependencies.
nae-user-common/pom.xml (1)
85-101: Jackson modules mix managed and hardcoded versions.This module imports Spring Boot BOM; remove explicit Jackson versions and let the BOM manage them to avoid drift.
jackson-datatype-jsr310is currently hardcoded to 2.18.2 while root property is 2.17.1.Suggested diff:
<dependency> <groupId>com.fasterxml.jackson.core</groupId> <artifactId>jackson-annotations</artifactId> - <version>${jackson.version}</version> <scope>provided</scope> </dependency> <dependency> <groupId>com.fasterxml.jackson.core</groupId> <artifactId>jackson-databind</artifactId> - <version>${jackson.version}</version> <scope>provided</scope> </dependency> <dependency> <groupId>com.fasterxml.jackson.datatype</groupId> <artifactId>jackson-datatype-jsr310</artifactId> - <version>2.18.2</version> <scope>provided</scope> </dependency>If you prefer explicit pinning, at least set all to
${jackson.version}and align the root property.nae-user-ce/pom.xml (1)
140-145: Use Jakarta Annotations on Spring Boot 3+.
javax.annotation-api1.3.2 is legacy and conflicts with Boot 3’s Jakarta namespace. Switch tojakarta.annotation:jakarta.annotation-api(version managed by BOM; no explicit version needed).Apply:
- <dependency> - <groupId>javax.annotation</groupId> - <artifactId>javax.annotation-api</artifactId> - <version>1.3.2</version> - <scope>provided</scope> - </dependency> + <dependency> + <groupId>jakarta.annotation</groupId> + <artifactId>jakarta.annotation-api</artifactId> + <scope>provided</scope> + </dependency>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
application-engine/pom.xml(1 hunks)application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/configuration/groovy/GroovyShellConfiguration.java(3 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/ITaskService.java(1 hunks)application-engine/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/IWorkflowService.java(1 hunks)application-engine/src/main/resources/application.yaml(1 hunks)nae-object-library/pom.xml(1 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java(3 hunks)nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java(1 hunks)nae-spring-core-adapter/pom.xml(1 hunks)nae-user-ce/pom.xml(1 hunks)nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java(1 hunks)nae-user-common/pom.xml(1 hunks)pom.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
application-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/web/responsebodies/MessageResource.java (1)
MessageResource(10-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (25)
nae-object-library/pom.xml (1)
10-10: RC6.1 parent bump looks good.No functional change; aligns module to the new parent.
pom.xml (3)
9-9: Project version → 7.0.0-RC6.1: OK.Bump is consistent with module parents.
57-63: Confirm CI JDK and Spring Boot version alignment.
- pom.xml now sets Java 21 (java.version, maven.compiler.source/target); ensure all CI/build runners use JDK 21.
- pom.xml defines
<spring.boot.version>3.4.4</spring.boot.version>—update your PR description and test matrix to target 3.4.4 (not 3.2.5).
69-72: Align Jackson version with Spring Boot BOM
Root POM defines<jackson.version>2.17.1but no modules override Jackson. Spring Boot 3.4.4’s BOM manages Jackson 2.18.x. Remove the root<jackson.version>override to use the BOM-managed version or bump it to 2.18.2 to match Spring Boot.Likely an incorrect or invalid review comment.
nae-user-common/pom.xml (1)
9-9: Parent bump to RC6.1: OK.nae-user-ce/pom.xml (1)
9-9: Parent bump to RC6.1: OK.nae-spring-core-adapter/pom.xml (2)
10-10: Parent bump to RC6.1: OK.
97-100: Ensure Jackson property alignment.This module uses
jackson-module-jsonSchemawith${jackson.version}while other modules hardcode 2.18.2. Align rootjackson.versionor drop per-module pins to BOM-managed versions to prevent cross-module incompatibilities.nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (1)
596-596: Lombok-generated equals/hashCode found — verify identity fields used
@EqualsAndHashCode(callSuper=true) on AbstractUser and @EqualsAndHashCode(callSuper=false) on User generate equals()/hashCode(); confirm they rely solely on your business-key (e.g. id) and exclude mutable or auxiliary fields (email, displayName, etc.) to prevent unintended deduplication.application-engine/src/main/java/com/netgrif/application/engine/configuration/groovy/GroovyShellConfiguration.java (2)
11-12: No action — import noiseThe added import appears unused; deferring to linters/CI.
23-23: Default engine star-imports applied before user imports — LGTMEnsures core types are available in scripts by default.
nae-object-library/src/main/java/com/netgrif/application/engine/objects/plugin/domain/Plugin.java (2)
9-9: OK to import LinkedHashMap for deterministic iteration.
14-15: Javadoc formatting changes are fine.Also applies to: 40-41
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java (1)
395-396: Good: centralize default path to force=false.application-engine/src/main/resources/application.yaml (1)
162-163: Importing Nullable into actions is correct.No issues; aligns with the new utility and Groovy helpers.
application-engine/src/main/groovy/com/netgrif/application/engine/petrinet/domain/dataset/logic/action/ActionDelegate.groovy (3)
93-94: Add Nullable import — OKImport path looks correct and unambiguous in this module.
911-914: Nullable helper method — OKSimple, discoverable DSL helper. No side effects; good.
2771-2776: Use of String.formatted — OKOn Java 21 this is fine and improves readability.
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/TaskService.java (3)
927-928: Delegate to boolean overload — OKKeeps source compatibility while centralizing logic.
946-948: deleteTasksByCase(force) — OKSimple delegation; consistent with new overload.
931-938: Log force intent and size; verify force usage
Optional refactor:- log.info("[{}]: Tasks of case are being deleted", caseId); + log.info("[{}]: Deleting {} tasks (force={})", caseId, tasks != null ? tasks.size() : 0, force);Search found no literal calls to
delete(..., true)—manually audit all callers ofTaskService.delete(List<Task>, String, boolean)to ensure forced deletions are always logged.application-engine/src/main/java/com/netgrif/application/engine/petrinet/service/PetriNetService.java (2)
526-528: Overload delegation — OKPreserves existing behavior; routes through the force-aware path.
531-551: Include force flag and actual caller in logs; consider moving PRE event before destructive actions
- Update the initial log to
"Initiating [FORCED|graceful] deletion of Petri net … by user [<loggedUser>]"usingloggedUser.getStringId()and theforceflag.- If you need listeners to react before instances or roles are removed, move the
publisher.publishEvent(new ProcessDeleteEvent(petriNet, EventPhase.PRE))to immediately after that log and before anyuserService/workflowService/processRoleServicecalls—verify no external subscribers depend on the current order.- Leave the
POSTevent publication after cache eviction unchanged.nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java (2)
272-291: equals/hashCode implementation looks correct.Pattern matching and value-based equality are appropriate for this type.
1-1: Check project header/license requirements.If the repo mandates a license header on source files, add it here.
.../main/java/com/netgrif/application/engine/configuration/groovy/GroovyShellConfiguration.java
Show resolved
Hide resolved
...c/main/java/com/netgrif/application/engine/petrinet/service/interfaces/IPetriNetService.java
Show resolved
Hide resolved
...ion-engine/src/main/java/com/netgrif/application/engine/petrinet/web/PetriNetController.java
Show resolved
Hide resolved
...e/src/main/java/com/netgrif/application/engine/workflow/service/interfaces/ITaskService.java
Show resolved
Hide resolved
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java
Show resolved
Hide resolved
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java
Show resolved
Hide resolved
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java
Show resolved
Hide resolved
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java
Show resolved
Hide resolved
nae-object-library/src/main/java/com/netgrif/application/engine/objects/utils/Nullable.java
Show resolved
Hide resolved
- Updated deleteCase method to include force parameter in overloaded versions.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java (5)
425-427: Bug fix confirmed: force flag now forwarded.
This addresses the earlier oversight.
398-417: Force delete must not return null outcome.
Returning null breaks callers that expect a non-null DeleteCaseEventOutcome. Initialize an empty outcome even when force=true.Apply:
- public DeleteCaseEventOutcome deleteCase(Case useCase, Map<String, String> params, boolean force) { - DeleteCaseEventOutcome outcome = null; + public DeleteCaseEventOutcome deleteCase(Case useCase, Map<String, String> params, boolean force) { + DeleteCaseEventOutcome outcome = new DeleteCaseEventOutcome(useCase, Collections.emptyList()); if (!force) { - outcome = new DeleteCaseEventOutcome(useCase, eventService.runActions(useCase.getPetriNet().getPreDeleteActions(), useCase, Optional.empty(), params)); + outcome = new DeleteCaseEventOutcome(useCase, eventService.runActions(useCase.getPetriNet().getPreDeleteActions(), useCase, Optional.empty(), params)); publisher.publishEvent(new DeleteCaseEvent(outcome, EventPhase.PRE)); useCase = ((Evaluator<DeleteCaseEvent, Case>) evaluationService.getEvaluator("default")).apply(new DeleteCaseEvent(outcome, EventPhase.PRE)); } @@ - if (!force) { + if (!force) { outcome.addOutcomes(eventService.runActions(useCase.getPetriNet().getPostDeleteActions(), null, Optional.empty(), params)); addMessageToOutcome(useCase.getPetriNet(), CaseEventType.DELETE, outcome); ((Evaluator<DeleteCaseEvent, Case>) evaluationService.getEvaluator("noContext")).apply(new DeleteCaseEvent(outcome, EventPhase.POST)); publisher.publishEvent(new DeleteCaseEvent(outcome, EventPhase.POST)); } return outcome; }
404-405: Avoid magic strings for evaluator names.
Replace "default"/"noContext" with named constants to prevent typos.Apply within this method:
- useCase = ((Evaluator<DeleteCaseEvent, Case>) evaluationService.getEvaluator("default")).apply(new DeleteCaseEvent(outcome, EventPhase.PRE)); + useCase = ((Evaluator<DeleteCaseEvent, Case>) evaluationService.getEvaluator(EVALUATOR_DEFAULT)).apply(new DeleteCaseEvent(outcome, EventPhase.PRE)); @@ - ((Evaluator<DeleteCaseEvent, Case>) evaluationService.getEvaluator("noContext")).apply(new DeleteCaseEvent(outcome, EventPhase.POST)); + ((Evaluator<DeleteCaseEvent, Case>) evaluationService.getEvaluator(EVALUATOR_NO_CONTEXT)).apply(new DeleteCaseEvent(outcome, EventPhase.POST));Add near the logger (top of class):
private static final String EVALUATOR_DEFAULT = "default"; private static final String EVALUATOR_NO_CONTEXT = "noContext"; // TODO: Move to a central enum/constant in evaluation layer.Also applies to: 413-414
435-441: Bulk delete: page results and avoid per-item Map allocation.
Current code loads all cases and allocates a Map per item. Page through results and reuse an empty map to prevent OOM on large nets and reduce GC churn.- List<Case> cases = this.searchAll(QCase.case$.petriNetObjectId.eq(net.getObjectId())).getContent(); - if (!cases.isEmpty()) { - cases.forEach(aCase -> deleteCase(aCase, new HashMap<>(), force)); - } + final Predicate predicate = QCase.case$.petriNetObjectId.eq(net.getObjectId()); + final Map<String, String> emptyParams = Collections.emptyMap(); + Pageable pageable = PageRequest.of(0, 500); + Page<Case> page = repository.findAll(predicate, pageable); + while (true) { + if (!page.getContent().isEmpty()) { + page.getContent().forEach(aCase -> { + try { + deleteCase(aCase, emptyParams, force); + } catch (Exception e) { + log.error("Failed to delete case [{}] in bulk delete; continuing", aCase.getStringId(), e); + } + }); + } + if (!page.hasNext()) break; + pageable = page.nextPageable(); + page = repository.findAll(predicate, pageable); + }
425-427: Minor: avoid allocating a new HashMap.
Use an immutable empty map.- return deleteCase(useCase, new HashMap<>(), force); + return deleteCase(useCase, Collections.emptyMap(), force);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Test
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (3)
application-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java (3)
395-396: Good: non-force overload delegates correctly.
The delegation to the force-aware path with force=false is correct.
419-422: Good: overload to no-params delegates correctly.
Delegation to the boolean overload is correct.
430-432: Good: non-force Petri net deletion delegates to new overload.
Keeps behavior backward compatible.
...on-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
Show resolved
Hide resolved
...on-engine/src/main/java/com/netgrif/application/engine/workflow/service/WorkflowService.java
Show resolved
Hide resolved
Description
Implements NAE-2199
Dependencies
No new dependencies were introduced
Third party dependencies
No new dependencies were introduced
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Changes
Chores