Get steps statuses of an execution #32
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
📝 WalkthroughWalkthroughAdds per-step UUIDs and a SCHEDULED step status; emits SCHEDULED notifications before execution and supports batch step-status update messaging; introduces step-info mapping, controller and service endpoints; updates core interfaces, contexts, and tests to propagate step IDs. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client as Client
participant PES as ProcessExecutionService
participant Proc as Process
participant Notif as NotificationService
participant Broker as MessageBroker
participant Consumer as ConsumerService
participant Monitor as MonitorService
participant Repo as Repository
Client->>PES: execute(processConfig)
PES->>Proc: defineSteps()
Proc-->>PES: List<ProcessStep {id, type, order}>
loop per step
PES->>PES: createStepContext(parent, type, step.id, order)
PES->>Notif: updateStepsStatuses(executionId, [ProcessExecutionStep(id,type,order,SCHEDULED)])
Notif->>Broker: publish(STEPS_STATUSES_UPDATE, payload)
Broker->>Consumer: deliver(message)
Consumer->>Monitor: updateStepsStatuses(executionId, payload)
Monitor->>Repo: apply/update step entities
end
PES->>Notif: send RUNNING(executionId)
PES->>Proc: execute(processExecutionContext)
Proc-->>PES: result
PES->>Notif: send COMPLETED/FAILED(executionId)
Notif->>Repo: persist execution status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 5
🤖 Fix all issues with AI agents
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java`:
- Around line 137-146: getStepsInfos currently assumes execution.getSteps() is
non-null and will NPE for newly created ProcessExecutionEntity instances; modify
the mapping in getStepsInfos to null-check the steps list (e.g., use
Optional.ofNullable(execution.getSteps()) or an if-null fallback) before
streaming so that when steps is null the method returns an empty List rather
than throwing. Update the transformation that constructs new
ProcessExecutionStep(...) to run only when the steps list is present.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/AbstractProcessStep.java`:
- Around line 20-22: The UUID id field on the singleton AbstractProcessStep is
generated at bean construction and thus reused across executions; move UUID
creation out of the singleton by removing the id field initialization from
AbstractProcessStep and instead generate and supply a new UUID per execution
(e.g., create the UUID inside ProcessExecutionContext.createStepContext() and
pass it into the step when creating ProcessExecutionStepEntity or via a
method/parameter on AbstractProcessStep used at invocation time); update places
that referenced AbstractProcessStep.id to accept the execution-specific id (or
read it from the newly created ProcessExecutionStepEntity) so each
ProcessExecutionStepEntity gets a unique primary key per run.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java`:
- Around line 61-69: The scheduling code calling process.defineSteps() and
notificationService.updateStepStatus(...) can throw and currently sits before
the try that sets ProcessStatus.RUNNING, so move the entire scheduling loop (the
call to defineSteps() and the for-loop creating ProcessExecutionStep entries)
inside the existing try block in ProcessExecutionService or add a surrounding
try-catch that catches Throwable from defineSteps()/updateStepStatus, calls
updateExecutionStatus(context.getExecutionId(), context.getExecutionEnvName(),
ProcessStatus.FAILED) and logs the error, and optionally mark any partially
scheduled steps as FAILED via notificationService.updateStepStatus to ensure
consistent state; refer to the methods defineSteps(),
notificationService.updateStepStatus(...), updateExecutionStatus(...),
ProcessExecutionStep and ProcessStatus.RUNNING/FAILED to locate where to apply
the change.
- Around line 61-66: The scheduling loop currently calls process.defineSteps()
and schedules each ProcessStep without handling exceptions; consolidate the
defineSteps() call into a local List<ProcessStep<T>> steps =
process.defineSteps() (reuse that list) and wrap the for-loop that calls
notificationService.updateStepStatus(...) in a try-catch; on any exception catch
(Exception e) send a final
notificationService.updateStepStatus(context.getExecutionId(), new
ProcessExecutionStep(..., StepStatus.FAILED, /* include error message or
e.getMessage() in the appropriate field */)) to mark the execution failed,
include the exception message/details in the ProcessExecutionStep payload, and
rethrow or handle as appropriate to preserve current error semantics.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessStepExecutionContextTest.java`:
- Around line 72-74: In the shouldCreateStepContext test, mock step.getId() and
assert the step ID was propagated: add when(step.getId()).thenReturn(stepId)
before calling createStepContext (the method under test) and add an assertion
asserting ProcessStepExecutionContext.getStepExecutionId() equals stepId; refer
to the step.getId(), createStepContext, ProcessStepExecutionContext, and
getStepExecutionId symbols to locate where to add the mock and assertion.
🧹 Nitpick comments (2)
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionServiceTest.java (2)
99-135: Keep a singleInOrderto assert scheduling happens before execution status.
Line 128 reinitializesInOrder, so ordering between step scheduling and execution status updates isn’t asserted. If scheduling-before-execution is part of the contract, reuse the sameInOrder.♻️ Suggested tweak to enforce full ordering
- inOrder = inOrder(notificationService); + // Reuse the same InOrder to enforce scheduling before execution status updates
45-95: Align test generics from ProcessConfig to SecurityAnalysisConfig to eliminate the unchecked cast.The test mixes generics inconsistently:
Process<ProcessConfig>,LoadNetworkStep<ProcessConfig>, andProcessConfig processConfig, while the actualSecurityAnalysisProcessusesSecurityAnalysisConfigthroughout. This causes the raw(List)cast on line 95.Since
SecurityAnalysisConfigimplementsProcessConfig, change:
Process<ProcessConfig> process→Process<SecurityAnalysisConfig>ProcessConfig processConfig→SecurityAnalysisConfig processConfigLoadNetworkStep<ProcessConfig> loadNetworkStep→LoadNetworkStep<SecurityAnalysisConfig>ProcessRunMessage<ProcessConfig> runMessage→ProcessRunMessage<SecurityAnalysisConfig>(line 94)- Remove the raw
(List)cast on line 95♻️ Suggested changes
- private Process<ProcessConfig> process; + private Process<SecurityAnalysisConfig> process; - private ProcessConfig processConfig; + private SecurityAnalysisConfig processConfig; - private LoadNetworkStep<ProcessConfig> loadNetworkStep; + private LoadNetworkStep<SecurityAnalysisConfig> loadNetworkStep;ProcessRunMessage<ProcessConfig> runMessage = new ProcessRunMessage<>(executionId, caseUuid, processConfig); - when(process.defineSteps()).thenReturn((List) List.of(loadNetworkStep, applyModificationsStep, runComputationStep)); + ProcessRunMessage<SecurityAnalysisConfig> runMessage = new ProcessRunMessage<>(executionId, caseUuid, processConfig); + when(process.defineSteps()).thenReturn(List.of(loadNetworkStep, applyModificationsStep, runComputationStep));
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
Outdated
Show resolved
Hide resolved
...orker-server/src/main/java/org/gridsuite/monitor/worker/server/core/AbstractProcessStep.java
Show resolved
Hide resolved
...rver/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java
Outdated
Show resolved
Hide resolved
...rver/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java
Outdated
Show resolved
Hide resolved
.../src/test/java/org/gridsuite/monitor/worker/server/core/ProcessStepExecutionContextTest.java
Show resolved
Hide resolved
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
FranckLecuyer
changed the title
FranckLecuyer
changed the title
…an_execution 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>
FranckLecuyer
changed the title
klesaulnier
reviewed
Feb 3, 2026
Contributor
klesaulnier
left a comment
klesaulnier
left a comment
There was a problem hiding this comment.
Tests: OK
Small remarks
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
Outdated
Show resolved
Hide resolved
...rver/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java
Outdated
Show resolved
Hide resolved
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>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerService.java`:
- Around line 73-75: The handleStepsStatusesUpdate method currently calls
parsePayload(message.getPayload(), List.class) which loses generic type info and
yields List<LinkedHashMap> at runtime; change this to use a TypeReference to
preserve element types: add import
com.fasterxml.jackson.core.type.TypeReference, add an overloaded
parsePayload(TypeReference<T>) in the class (or where parsePayload lives) that
forwards to objectMapper.readValue(payload, typeRef), and update
ConsumerService.handleStepsStatusesUpdate to call
parsePayload(message.getPayload(), new
TypeReference<List<ProcessExecutionStep>>() {}) so
monitorService.updateStepsStatuses receives a List<ProcessExecutionStep> instead
of raw maps.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java`:
- Around line 77-93: The updateStep method can NPE because execution.getSteps()
may be null; before streaming or modifying steps, ensure the steps collection on
ProcessExecutionEntity is initialized (e.g., check if execution.getSteps() ==
null and set a new List), then proceed to find & update or add the
ProcessExecutionStepEntity; updateStep, ProcessExecutionEntity.getSteps(), and
ProcessExecutionStepEntity are the symbols to change so new executions have a
non-null steps list before the stream/add logic runs.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionServiceTest.java`:
- Line 94: The test uses a raw cast on when(process.defineSteps()) because the
mocked Process is typed as Process<ProcessConfig> but the steps returned are
SecurityAnalysisConfig-specific; update the mock in ProcessExecutionServiceTest
to be Process<SecurityAnalysisConfig> and change loadNetworkStep to
LoadNetworkStep<SecurityAnalysisConfig> (so it matches
ApplyModificationsStep<SecurityAnalysisConfig> and runComputationStep), then
remove the unchecked (List) cast from the when(...).defineSteps() stubbing so
the generics align and the raw cast is no longer needed.
monitor-server/src/main/java/org/gridsuite/monitor/server/services/ConsumerService.java
Show resolved
Hide resolved
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
Show resolved
Hide resolved
.../src/test/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionServiceTest.java
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/test/java/org/gridsuite/monitor/server/services/ConsumerServiceTest.java`:
- Around line 150-153: The batch-update test in ConsumerServiceTest currently
verifies monitorService.updateStepsStatuses(...) is called and
monitorService.updateExecutionStatus(...) is not, but it must also assert the
single-step method is never invoked; add a verification that
monitorService.updateStepStatus(...) is never called (use verify(monitorService,
never()).updateStepStatus(eq(executionId), any(), any(), any()) or similar
signature matching the existing updateStepStatus method) alongside the existing
verify calls to ensure the STEPS_STATUSES_UPDATE handler doesn't call the
singular update.
🧹 Nitpick comments (2)
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/NotificationServiceTest.java (1)
80-96: Minor naming inconsistency in test method name.The method is named
updateStepsStatusesShouldSendStepStatusUpdateMessage(singular "StepStatusUpdateMessage") but verifiesMessageType.STEPS_STATUSES_UPDATE(plural). Consider renaming for consistency with the actual message type:✏️ Suggested rename
`@Test` - void updateStepsStatusesShouldSendStepStatusUpdateMessage() { + void updateStepsStatusesShouldSendStepsStatusesUpdateMessage() {monitor-server/src/test/java/org/gridsuite/monitor/server/services/ConsumerServiceTest.java (1)
130-141: Consider removingstartedAtfor SCHEDULED steps or using a more appropriate status.Steps with
StepStatus.SCHEDULEDsemantically haven't started execution yet, so settingstartedAt(Instant.now())may be misleading. Consider either removingstartedAtfor these test fixtures or using a status likeRUNNINGif you need the timestamp field populated.This is a minor test data clarity issue and doesn't affect functionality.
monitor-server/src/test/java/org/gridsuite/monitor/server/services/ConsumerServiceTest.java
Show resolved
Hide resolved
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java (2)
142-149:⚠️ Potential issue | 🟡 MinorPotential NPE:
getReportIdslacks null-safe guard forsteps.The same null-safety pattern applied in
updateStepandgetStepsInfosshould be applied here. For newly created executions wherestepsis null,execution.getSteps().stream()will throw an NPE.🛡️ Proposed fix
private List<UUID> getReportIds(UUID executionId) { return executionRepository.findById(executionId) - .map(execution -> execution.getSteps().stream() + .map(execution -> Optional.ofNullable(execution.getSteps()).orElse(List.of()).stream() .map(ProcessExecutionStepEntity::getReportId) .filter(java.util.Objects::nonNull) .toList()) .orElse(List.of()); }
159-166:⚠️ Potential issue | 🟡 MinorPotential NPE:
getResultInfoslacks null-safe guard forsteps.Same issue as
getReportIds- callingexecution.getSteps().stream()without a null check can throw NPE for executions with uninitialized steps.🛡️ Proposed fix
private List<ResultInfos> getResultInfos(UUID executionId) { return executionRepository.findById(executionId) - .map(execution -> execution.getSteps().stream() + .map(execution -> Optional.ofNullable(execution.getSteps()).orElse(List.of()).stream() .filter(step -> step.getResultId() != null) .map(step -> new ResultInfos(step.getResultId(), step.getResultType())) .toList()) .orElse(List.of()); }
🧹 Nitpick comments (1)
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java (1)
174-184: Consider simplifying the Optional chain.The
if (entity.isPresent())check is redundant sinceentity.map()already returnsOptional.empty()when the entity is absent.♻️ Proposed simplification
`@Transactional`(readOnly = true) public Optional<List<ProcessExecutionStep>> getStepsInfos(UUID executionId) { - Optional<ProcessExecutionEntity> entity = executionRepository.findById(executionId); - if (entity.isPresent()) { - return entity.map(execution -> Optional.ofNullable(execution.getSteps()).orElse(List.of()).stream() - .map(processExecutionStepMapper::toDto) - .toList()); - } else { - return Optional.empty(); - } + return executionRepository.findById(executionId) + .map(execution -> Optional.ofNullable(execution.getSteps()).orElse(List.of()).stream() + .map(processExecutionStepMapper::toDto) + .toList()); }
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java (2)
142-148:⚠️ Potential issue | 🟡 MinorPotential NPE:
getReportIdslacks null-safe handling for steps.Unlike
updateStepandgetStepsInfos, this method callsexecution.getSteps().stream()directly without a null guard. Ifstepsis null, this will throw an NPE.Suggested fix
private List<UUID> getReportIds(UUID executionId) { return executionRepository.findById(executionId) - .map(execution -> execution.getSteps().stream() + .map(execution -> Optional.ofNullable(execution.getSteps()).orElse(List.of()).stream() .map(ProcessExecutionStepEntity::getReportId) .filter(java.util.Objects::nonNull) .toList()) .orElse(List.of()); }
159-166:⚠️ Potential issue | 🟡 MinorPotential NPE:
getResultInfoslacks null-safe handling for steps.Same issue as
getReportIds- direct call toexecution.getSteps().stream()without null guard.Suggested fix
private List<ResultInfos> getResultInfos(UUID executionId) { return executionRepository.findById(executionId) - .map(execution -> execution.getSteps().stream() + .map(execution -> Optional.ofNullable(execution.getSteps()).orElse(List.of()).stream() .filter(step -> step.getResultId() != null) .map(step -> new ResultInfos(step.getResultId(), step.getResultType())) .toList()) .orElse(List.of()); }
🧹 Nitpick comments (2)
monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java (1)
157-174: Test covers happy path well; consider adding 404 case.The test correctly verifies the successful response with step data. However, there's no test for the scenario when
getStepsInfosreturnsOptional.empty()(execution not found), which should result in a 404 response based on the controller behavior.Suggested additional test
`@Test` void getStepsInfosShouldReturn404WhenExecutionNotFound() throws Exception { UUID executionId = UUID.randomUUID(); when(monitorService.getStepsInfos(executionId)).thenReturn(Optional.empty()); mockMvc.perform(get("/v1/executions/{executionId}/step-infos", executionId)) .andExpect(status().isNotFound()); verify(monitorService).getStepsInfos(executionId); }monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java (1)
458-481: Test verifies happy path; consider adding edge cases.The test correctly verifies step retrieval and mapping. Consider adding tests for:
- Execution not found (should return
Optional.empty())- Execution with null/empty steps (should return
Optional.of(List.of()))Suggested additional tests
`@Test` void getStepsInfosShouldReturnEmptyWhenExecutionNotFound() { UUID executionUuid = UUID.randomUUID(); when(executionRepository.findById(executionUuid)).thenReturn(Optional.empty()); Optional<List<ProcessExecutionStep>> result = monitorService.getStepsInfos(executionUuid); assertThat(result).isEmpty(); verify(executionRepository).findById(executionUuid); } `@Test` void getStepsInfosShouldReturnEmptyListWhenNoSteps() { UUID executionUuid = UUID.randomUUID(); ProcessExecutionEntity execution = ProcessExecutionEntity.builder() .id(executionUuid) .type(ProcessType.SECURITY_ANALYSIS.name()) .steps(null) .build(); when(executionRepository.findById(executionUuid)).thenReturn(Optional.of(execution)); Optional<List<ProcessExecutionStep>> result = monitorService.getStepsInfos(executionUuid); assertThat(result).isPresent(); assertThat(result.get()).isEmpty(); }
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
klesaulnier
reviewed
Feb 4, 2026
monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java
Outdated
Show resolved
Hide resolved
...tor-server/src/main/java/org/gridsuite/monitor/server/mapper/ProcessExecutionStepMapper.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>
|
klesaulnier
approved these changes
Feb 4, 2026
This was referenced Feb 4, 2026
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Summary
Add endpoint ("executions/{executionId}/step-infos") to get all steps statuses of a given execution