Delete an execution #27
Conversation
📝 WalkthroughWalkthroughAdds DELETE /v1/executions/{executionId} to cascade-delete execution-associated results and reports; introduces ResultProvider.deleteResult and corresponding service/provider implementations; adds ReportService.deleteReport and SecurityAnalysisService.deleteResult; replaces Lombok constructor in MonitorService; updates JPA table names; adds unit tests. Changes
Sequence DiagramsequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
end
rect rgba(200,255,200,0.5)
participant Controller as MonitorController
participant Service as MonitorService
participant ResultSvc as ResultService
participant ReportSvc as ReportService
end
rect rgba(255,200,200,0.5)
participant Provider as ResultProvider
participant SecAnalysis as SecurityAnalysisServer
participant ReportServer as ReportServer
end
Client->>Controller: DELETE /v1/executions/{executionId}
Controller->>Service: deleteExecution(executionId)
Service->>Service: load execution & steps
alt step has result
Service->>ResultSvc: deleteResult(resultInfos)
ResultSvc->>Provider: deleteResult(resultId)
Provider->>SecAnalysis: DELETE /v1/results?resultsUuids={id}
end
alt step has report
Service->>ReportSvc: deleteReport(reportId)
ReportSvc->>ReportServer: DELETE /v1/reports/{id}
end
Service->>Service: delete execution entity from DB
Service-->>Controller: boolean deleted
Controller-->>Client: 200 OK / 404 Not Found
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java`:
- Around line 138-157: The current deleteExecution method performs external HTTP
deletions (resultService.deleteResult and reportService.deleteReport) inside the
`@Transactional` block; refactor deleteExecution to remove the
ProcessExecutionEntity first (executionRepository.deleteById or delete entity)
and then register a post-commit callback via
TransactionSynchronizationManager.registerSynchronization() to perform all
external deletions after commit; when scheduling deletions, iterate the saved
execution's steps and only call ResultService.deleteResult when
step.getResultId() != null AND step.getResultType() != null (avoid creating
ResultInfos with a null resultType), and pass each step's reportId to
reportService.deleteReport only if non-null.
🧹 Nitpick comments (2)
monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java (1)
301-335: Align test data with production expectations (resultType).Both steps include result IDs but omit resultType, which can mask real‑world behavior. Add resultType to each step so the test reflects actual data contracts.
♻️ Proposed tweak
ProcessExecutionStepEntity step0 = ProcessExecutionStepEntity.builder() .id(UUID.randomUUID()) .stepOrder(0) .reportId(reportId1) .resultId(resultId1) + .resultType(ResultType.SECURITY_ANALYSIS) .build(); @@ ProcessExecutionStepEntity step1 = ProcessExecutionStepEntity.builder() .id(UUID.randomUUID()) .stepOrder(1) .reportId(reportId2) .resultId(resultId2) + .resultType(ResultType.SECURITY_ANALYSIS) .build();monitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisService.java (1)
57-66: Implementation looks good and follows existing patterns.The method correctly:
- Logs the deletion action consistently with
getResult- Constructs the URI with query params (the batch-style
resultsUuidsparameter aligns with the API design)- Uses
restTemplate.delete()which will propagate HTTP errors as exceptionsOne consideration for resilience: external calls without timeouts or explicit error handling can cause cascading failures if the security-analysis-server becomes slow or unavailable. Since
getResultalso lacks this, it might be worth addressing both in a follow-up for improved operational resilience.
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java`:
- Around line 171-179: The current deleteExecution flow removes the DB row via
self.deleteExecutionEntity before calling resultService.deleteResult and
reportService.deleteReport, which makes external-resource failures
unrecoverable; change the flow in deleteExecution to first fetch the
resultIds/reportIds (call the same logic used by self.deleteExecutionEntity to
collect IDs or add a new helper to collect them), attempt to delete external
resources (resultService.deleteResult and reportService.deleteReport) treating
404/not-found responses as success and aggregating transient failures, and only
after successful external deletions remove the execution row (or if external
deletions fail persist a cleanup/outbox task for retry instead of deleting the
DB row); refer to deleteExecution, self.deleteExecutionEntity,
resultService.deleteResult, and reportService.deleteReport when making the
change.
🧹 Nitpick comments (1)
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java (1)
151-168: Avoid mutable out-parameters for deletion targets.
deleteExecutionEntitymutates caller-provided lists, which makes the public API easy to misuse (e.g., passingList.of(...)will throw) and hides side effects. Consider returning a small value object (e.g., a record with result/report IDs) or narrowing visibility to reduce accidental misuse.
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
FranckLecuyer
force-pushed
the
delete_an_execution
branch
from
d0e0612 to
0748f14
Compare
antoinebhs
left a comment
antoinebhs
left a comment
There was a problem hiding this comment.
Let's avoid self referencing and @lazy it makes tests difficult and code hard to understand
monitor-server/src/main/java/org/gridsuite/monitor/server/services/ResultService.java
Outdated
Show resolved
Hide resolved
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
Outdated
Show resolved
Hide resolved
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
Outdated
Show resolved
Hide resolved
monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java
Outdated
Show resolved
Hide resolved
monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@monitor-server/src/main/resources/db/changelog/changesets/changelog_20260130T160426Z.xml`:
- Around line 44-51: The changelog is adding started_at and user_id to table
"process_execution" but the JPA entity ProcessExecutionEntity is annotated with
`@Table`(name = "processExecution"), causing a mismatch; either change the
entity's `@Table` to `@Table`(name = "process_execution") or update the Liquibase
changeSet to target "processExecution" so both use the same exact table name;
update references to the two added columns (started_at, user_id) accordingly and
run schema validation/migrations to confirm alignment.
🧹 Nitpick comments (15)
monitor-server/src/test/java/org/gridsuite/monitor/server/MonitorIntegrationTest.java (1)
86-113: Add a userId persistence assertion.
The test now passes a userId but doesn’t verify it was stored on the execution entity.🧪 Suggested assertion
assertThat(execution.getStatus()).isEqualTo(ProcessStatus.SCHEDULED); assertThat(execution.getSteps()).isEmpty(); + assertThat(execution.getUserId()).isEqualTo(userId);monitor-server/src/main/resources/db/changelog/changesets/changelog_20260130T160426Z.xml (1)
44-60: Consider an index for launched‑process queries.
findByTypeAndStartedAtIsNotNullOrderByStartedAtDescwill scan on(type, started_at)as data grows. Adding an index on those columns would keep this query fast.monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java (3)
161-164: Add@Transactional(readOnly = true)for read consistency.This method queries the repository and maps entities to DTOs. For consistency with similar read methods like
getReports(Line 127) andgetResults(Line 144), consider adding the@Transactional(readOnly = true)annotation.♻️ Suggested fix
+ `@Transactional`(readOnly = true) public List<ProcessExecution> getLaunchedProcesses(ProcessType processType) { return executionRepository.findByTypeAndStartedAtIsNotNullOrderByStartedAtDesc(processType.name()).stream() .map(processExecutionMapper::toDto).toList(); }
166-182: Consider returning a result object instead of mutating parameter lists.The method uses out-parameters (
resultIds,reportIds) which is an unusual pattern in Java. Returning a dedicated result object or record would be more idiomatic and easier to reason about.♻️ Example refactor using a record
+ public record ExecutionRelatedIds(List<ResultInfos> resultIds, List<UUID> reportIds) {} + `@Transactional`(readOnly = true) - public boolean getReportsAndResultsUuids(UUID executionId, List<ResultInfos> resultIds, List<UUID> reportIds) { + public Optional<ExecutionRelatedIds> getReportsAndResultsUuids(UUID executionId) { Optional<ProcessExecutionEntity> executionEntity = executionRepository.findById(executionId); - if (executionEntity.isPresent()) { - executionEntity.get().getSteps().forEach(step -> { - if (step.getResultId() != null && step.getResultType() != null) { - resultIds.add(new ResultInfos(step.getResultId(), step.getResultType())); - } - if (step.getReportId() != null) { - reportIds.add(step.getReportId()); - } - }); - return true; - } else { - return false; - } + return executionEntity.map(entity -> { + List<ResultInfos> results = new ArrayList<>(); + List<UUID> reports = new ArrayList<>(); + entity.getSteps().forEach(step -> { + if (step.getResultId() != null && step.getResultType() != null) { + results.add(new ResultInfos(step.getResultId(), step.getResultType())); + } + if (step.getReportId() != null) { + reports.add(step.getReportId()); + } + }); + return new ExecutionRelatedIds(results, reports); + }); }
189-200: External deletion failures leave the execution intact but resources partially deleted.The current flow deletes external resources (results, reports) before the DB entity. If an external deletion fails mid-way, some resources may be deleted while others remain, and the execution entity is not removed. Consider:
- Treating 404 responses from external services as success (resource already gone).
- Aggregating failures and continuing to attempt all deletions before throwing.
- Using a cleanup/outbox pattern for reliability.
This is a reliability concern but may be acceptable depending on operational requirements.
monitor-server/src/test/java/org/gridsuite/monitor/server/services/ProcessConfigServiceTest.java (1)
109-130: Test relies on mocked entity state rather than verifying repository save.In
updateSecurityAnalysisConfig, after callingupdateProcessConfig, the test callsgetProcessConfigwhich returns the same mocked entity. This works because the@Spymapper mutates the entity in-place, but it doesn't verify thatprocessConfigRepository.save()was called. Consider adding a verification if the service is expected to explicitly save updates.monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/ProcessConfigController.java (1)
45-51: Consider returning HTTP 201 Created for resource creation.The
createProcessConfigendpoint returns HTTP 200, but RESTful conventions suggest using HTTP 201 (Created) when a new resource is successfully created. You could also add aLocationheader pointing to the new resource.♻️ Suggested fix
`@PostMapping`(value = "", consumes = MediaType.APPLICATION_JSON_VALUE, produces = MediaType.APPLICATION_JSON_VALUE) `@Operation`(summary = "Create process config") `@ApiResponses`(value = { - `@ApiResponse`(responseCode = "200", description = "process config was created")}) + `@ApiResponse`(responseCode = "201", description = "process config was created")}) public ResponseEntity<UUID> createProcessConfig(`@RequestBody` ProcessConfig processConfig) { - return ResponseEntity.ok().body(processConfigService.createProcessConfig(processConfig)); + UUID id = processConfigService.createProcessConfig(processConfig); + return ResponseEntity.status(HttpStatus.CREATED).body(id); }Note: You'll need to import
org.springframework.http.HttpStatus.monitor-server/src/main/java/org/gridsuite/monitor/server/services/ProcessConfigService.java (2)
44-51: Exception inflatMapfor unknown entity types may be unexpected.If an entity exists in the database with an unsupported type,
getProcessConfigthrowsIllegalArgumentExceptioninstead of returningOptional.empty(). This is reasonable if unknown types indicate a bug, but consider whether returning empty and logging a warning might be more graceful for forward compatibility.
70-77: Minor:existsById+deleteByIdresults in two DB round-trips.This could be optimized to a single operation, though the current approach is clearer and the performance impact is likely negligible for this use case.
♻️ Alternative using repository's delete count
If your repository extends
JpaRepository, you could use a custom method that returns the delete count, or simply calldeleteByIdand handleEmptyResultDataAccessException(though this is deprecated in recent Spring Data). The current approach is acceptable.monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java (1)
122-139: UnnecessaryuserIdheader ingetLaunchedProcessestest.The
GET /executionsendpoint doesn't require or use theuserIdheader based on the controller code. Including.header("userId", "user1,user2,user3")in the test is misleading and unnecessary.♻️ Suggested fix
- mockMvc.perform(get("/v1/executions?processType=SECURITY_ANALYSIS").accept(MediaType.APPLICATION_JSON_VALUE).header("userId", "user1,user2,user3")) + mockMvc.perform(get("/v1/executions?processType=SECURITY_ANALYSIS").accept(MediaType.APPLICATION_JSON_VALUE)) .andExpect(status().isOk())monitor-server/src/main/java/org/gridsuite/monitor/server/entities/AbstractProcessConfigEntity.java (1)
52-58: Consider lazy loading formodificationUuidsif the collection can grow large.
FetchType.EAGERon@ElementCollectionloads all modification UUIDs whenever the entity is fetched. If this list remains small (a few items), EAGER is fine. However, if it could grow to hundreds of entries, switching toFetchType.LAZYand fetching on demand would reduce memory pressure and improve query performance.monitor-server/src/main/java/org/gridsuite/monitor/server/entities/SecurityAnalysisConfigEntity.java (2)
36-40:@AllArgsConstructoronly covers fields in this class, not inherited ones.Lombok's
@AllArgsConstructorgenerates a constructor with onlyparametersUuidandcontingencies, excludingidandmodificationUuidsfrom the parentAbstractProcessConfigEntity. If you need to construct an entity with all fields (including inherited ones), consider using a builder pattern with@SuperBuilderon both parent and child, or a manual constructor.♻️ Suggested approach using `@SuperBuilder`
In
AbstractProcessConfigEntity:+import lombok.experimental.SuperBuilder; + `@Entity` `@Table`(name = "process_config") `@Inheritance`(strategy = InheritanceType.JOINED) `@DiscriminatorColumn`(name = "process_type", discriminatorType = STRING) `@NoArgsConstructor` -@AllArgsConstructor +@SuperBuilder `@Getter` `@Setter` public abstract class AbstractProcessConfigEntity {In
SecurityAnalysisConfigEntity:+import lombok.experimental.SuperBuilder; + `@Entity` `@Table`(name = "security_analysis_config") `@DiscriminatorValue`("SECURITY_ANALYSIS") `@PrimaryKeyJoinColumn`(foreignKey = `@ForeignKey`(name = "securityAnalysisConfig_id_fk_constraint")) `@NoArgsConstructor` -@AllArgsConstructor +@SuperBuilder `@Getter` `@Setter` public class SecurityAnalysisConfigEntity extends AbstractProcessConfigEntity {
44-50: Same EAGER fetch consideration as the parent class.If
contingenciescan grow to a large number of items, considerFetchType.LAZYto avoid loading all contingencies on every entity fetch.monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java (2)
56-57: Unused@SpyforProcessExecutionMapper.
processExecutionMapperis declared as a@Spybut is never referenced in any test method. If it's intended for future use, consider adding a TODO comment. Otherwise, remove it to avoid confusion.♻️ Remove unused spy
- `@Spy` - private ProcessExecutionMapper processExecutionMapper; -
372-384: Test data inconsistency:step0hasresultIdbut noresultType.
step0is configured withresultIdbut withoutresultType, whilestep1has both. This leads todeleteResultbeing called only once (Line 406). While this may correctly reflect the implementation's behavior (skip deletion whenresultTypeis null), the test fixture is ambiguous.Consider either:
- Removing
resultIdfromstep0if it's intentionally incomplete, or- Adding
resultTypetostep0and updating the verification totimes(2)if both should have deletable results.♻️ Option 1: Remove resultId from step0 for clarity
ProcessExecutionStepEntity step0 = ProcessExecutionStepEntity.builder() .id(UUID.randomUUID()) .stepOrder(0) .reportId(reportId1) - .resultId(resultId1) .build();And update:
- UUID resultId1 = UUID.randomUUID();
monitor-server/src/main/resources/db/changelog/changesets/changelog_20260130T160426Z.xml
Show resolved
Hide resolved
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com> # Conflicts: # monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java # monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java # monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java # monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/entities/ProcessExecutionEntity.java`:
- Line 26: ProcessExecutionStepEntity currently uses `@Table`(name =
"processExecutionStep") which mismatches the actual DB table; update the
annotation on the ProcessExecutionStepEntity class to `@Table`(name =
"process_execution_step") so it aligns with the database changelog and the
snake_case convention used by ProcessExecutionEntity and other entities.
🧹 Nitpick comments (1)
monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java (1)
371-376: Test implicitly validates nullresultTypefiltering—consider making it explicit.
step0lacks aresultType, so the implementation correctly skips its result deletion. Thetimes(1)verification on line 403 passes because onlystep1(withresultType) triggersdeleteResult.For clarity, consider verifying the exact
ResultInfosargument:verify(resultService).deleteResult(new ResultInfos(resultId2, ResultType.SECURITY_ANALYSIS));This makes the test's intent explicit and guards against accidentally deleting the wrong result.
monitor-server/src/main/java/org/gridsuite/monitor/server/entities/ProcessExecutionEntity.java
Show resolved
Hide resolved
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com> # Conflicts: # monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java # monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java # monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java`:
- Around line 200-207: The code iterates executionEntity.get().getSteps()
directly which can be null and cause an NPE; change the iteration to defensively
handle null like other methods (see getReportIds/getResultInfos) by using
Optional.ofNullable(executionEntity.get().getSteps()).orElse(List.of()) (or an
explicit null check) before calling forEach, and keep the inner logic that
builds resultIds and reportIds intact.
🧹 Nitpick comments (1)
monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java (1)
86-94: Minor inconsistency: missing@Parameterannotation onexecutionId.Other endpoints in this controller (e.g.,
getExecutionReports,getExecutionResults,getStepsInfos) include@Parameter(description = "Execution UUID")on theexecutionIdpath variable. Consider adding it here for consistent API documentation.📝 Suggested fix
`@DeleteMapping`("/executions/{executionId}") `@Operation`(summary = "Delete an execution") `@ApiResponses`(value = {`@ApiResponse`(responseCode = "200", description = "Execution was deleted"), `@ApiResponse`(responseCode = "404", description = "Execution was not found")}) - public ResponseEntity<Void> deleteExecution(`@PathVariable` UUID executionId) { + public ResponseEntity<Void> deleteExecution(`@Parameter`(description = "Execution UUID") `@PathVariable` UUID executionId) { return monitorService.deleteExecution(executionId) ? ResponseEntity.ok().build() : ResponseEntity.notFound().build(); }
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
|
| Optional.ofNullable(executionEntity.get().getSteps()).orElse(List.of()).forEach(step -> { | ||
| if (step.getResultId() != null && step.getResultType() != null) { |
There was a problem hiding this comment.
If you choose to rely on Optional to check for null values here, you must ensure that the same checks are applied consistently throughout the codebase.
In fact, Hibernate always initializes a collection when loading a relationship of a collection type. However, to be extra safe, we can explicitly initialize an empty ArrayList in the parent entity, i.e. ProcessExecutionEntity
3 participants
PR Summary
Delete an execution, given its id (delete the execution entity, its steps, reports and results)