Add notification for process update + Add getExecution endpoint#99
Add notification for process update + Add getExecution endpoint#99carojeandat wants to merge 3 commits into
Conversation
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a GET ChangesExecution retrieval and notification refactor
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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. 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: 2
🧹 Nitpick comments (2)
monitor-server/src/test/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionServiceTest.java (1)
294-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a real
ProcessExecutioninstance over mocking the record.
ProcessExecutionis a record (implicitly final) with plain data fields and no behavior, so mocking it viaMockito.mockrelies on Mockito's inline mock maker (default only since Mockito 5) and yields an instance whose accessors all returnnullunless stubbed.assertThat(result).contains(processExecution)here only proves reference identity, not that the mapped/returned value has meaningful field data — weaker coverage than the siblingProcessExecutionTxServiceTesttests which build a realProcessExecutionvia constructor/builder.♻️ Suggested fix
`@Test` void getExecutionReturnsExecution() { - ProcessExecution processExecution = Mockito.mock(ProcessExecution.class); + ProcessExecution processExecution = ProcessExecution.builder() + .id(executionId) + .type(ProcessType.SECURITY_ANALYSIS.name()) + .caseUuid(caseUuid) + .processConfigId(UUID.randomUUID()) + .status(ProcessStatus.COMPLETED) + .executionEnvName("env1") + .userId(userId) + .build();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monitor-server/src/test/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionServiceTest.java` around lines 294 - 315, The test `getExecutionReturnsExecution` is mocking `ProcessExecution`, but this record should be exercised as a real data object instead of a Mockito mock. Update the test in `ProcessExecutionServiceTest` to construct a concrete `ProcessExecution` instance with representative fields, then assert that `processExecutionService.getExecution(...)` returns that same value via `processExecutionTxService.getExecution(...)`. Keep the existing `getExecutionNotFoundReturnsEmpty` case unchanged.monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java (1)
218-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a real
ProcessExecutioninstance over mocking the record for the 200 test.Same concern as in
ProcessExecutionServiceTest:ProcessExecutionis a record; mocking it produces an instance with all-null accessor values. The JSON body assertion (content().json(objectMapper.writeValueAsString(processExecution))) will trivially match since both sides serialize the exact same (mostly-null) mock, so the test doesn't validate real field serialization. Sibling tests likegetLaunchedProcesses(line 197) construct realProcessExecutioninstances for meaningful JSON assertions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java` around lines 218 - 232, The 200-response test for getExecution currently mocks ProcessExecution, which makes the JSON assertion meaningless because the record serializes to mostly null values. Update getExecutionShouldReturn in MonitorControllerTest to build a real ProcessExecution instance, similar to getLaunchedProcesses, and keep stubbing processExecutionService.getExecution(executionId) to return that instance so the response body assertion validates actual field serialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java`:
- Around line 82-89: Update the OpenAPI documentation on
MonitorController#getExecution so it matches the actual behavior: the endpoint
already returns ResponseEntity.notFound().build() when
processExecutionService.getExecution(executionId) is empty, so add a documented
404 response alongside the existing 200 ApiResponse, following the pattern used
by getExecutionReports and getExecutionResults.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/messaging/ConsumerService.java`:
- Around line 58-71: The notification publish in
ConsumerService.handleExecutionStatusUpdate is currently uncaught, so a failure
in notificationService.sendProcessUpdatedMessage can escape the consumer
callback after processExecutionService.updateExecutionStatus has already
succeeded. Wrap the sendProcessUpdatedMessage call in localized error handling
(or otherwise isolate it) so broker/publish failures are logged and do not
trigger requeueing or duplicate processing of the already-applied execution
status update.
---
Nitpick comments:
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.java`:
- Around line 218-232: The 200-response test for getExecution currently mocks
ProcessExecution, which makes the JSON assertion meaningless because the record
serializes to mostly null values. Update getExecutionShouldReturn in
MonitorControllerTest to build a real ProcessExecution instance, similar to
getLaunchedProcesses, and keep stubbing
processExecutionService.getExecution(executionId) to return that instance so the
response body assertion validates actual field serialization.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionServiceTest.java`:
- Around line 294-315: The test `getExecutionReturnsExecution` is mocking
`ProcessExecution`, but this record should be exercised as a real data object
instead of a Mockito mock. Update the test in `ProcessExecutionServiceTest` to
construct a concrete `ProcessExecution` instance with representative fields,
then assert that `processExecutionService.getExecution(...)` returns that same
value via `processExecutionTxService.getExecution(...)`. Keep the existing
`getExecutionNotFoundReturnsEmpty` case unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecdd56ab-96d3-43cd-aad1-7193291b34b9
📒 Files selected for processing (10)
monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.javamonitor-server/src/main/java/org/gridsuite/monitor/server/messaging/ConsumerService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/messaging/NotificationService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionTxService.javamonitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/messaging/ConsumerServiceTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/messaging/NotificationServiceTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionServiceTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionTxServiceTest.java
| switch (messageType) { | ||
| case EXECUTION_STATUS_UPDATE -> handleExecutionStatusUpdate(executionId, message); | ||
| case EXECUTION_STATUS_UPDATE -> handleExecutionStatusUpdate(executionId, message, messageType); | ||
| case STEP_STATUS_UPDATE -> handleStepStatusUpdate(executionId, message); | ||
| case STEPS_STATUSES_UPDATE -> handleStepsStatusesUpdate(executionId, message); | ||
| default -> LOGGER.warn("Unknown message type: {}", messageType); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| private void handleExecutionStatusUpdate(UUID executionId, Message<String> message) { | ||
| private void handleExecutionStatusUpdate(UUID executionId, Message<String> message, MessageType messageType) { | ||
| ProcessExecutionStatusUpdate payload = parsePayload(message.getPayload(), ProcessExecutionStatusUpdate.class); | ||
| processExecutionService.updateExecutionStatus(executionId, payload.getStatus(), payload.getExecutionEnvName(), payload.getStartedAt(), payload.getCompletedAt()); | ||
| notificationService.sendProcessUpdatedMessage(executionId, messageType, payload); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled notification failure can block/duplicate message processing.
sendProcessUpdatedMessage runs with no error handling right after updateExecutionStatus. If the publish fails (e.g. broker unavailable), the exception propagates out of the consumer callback, likely causing the inbound message to be requeued/retried — reprocessing (duplicating) the already-applied status update and potentially stalling this queue if the failure persists.
🛡️ Suggested fix to isolate notification failures
private void handleExecutionStatusUpdate(UUID executionId, Message<String> message, MessageType messageType) {
ProcessExecutionStatusUpdate payload = parsePayload(message.getPayload(), ProcessExecutionStatusUpdate.class);
processExecutionService.updateExecutionStatus(executionId, payload.getStatus(), payload.getExecutionEnvName(), payload.getStartedAt(), payload.getCompletedAt());
- notificationService.sendProcessUpdatedMessage(executionId, messageType, payload);
+ try {
+ notificationService.sendProcessUpdatedMessage(executionId, messageType, payload);
+ } catch (Exception e) {
+ LOGGER.warn("Failed to send process updated notification for execution {}", executionId, e);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (messageType) { | |
| case EXECUTION_STATUS_UPDATE -> handleExecutionStatusUpdate(executionId, message); | |
| case EXECUTION_STATUS_UPDATE -> handleExecutionStatusUpdate(executionId, message, messageType); | |
| case STEP_STATUS_UPDATE -> handleStepStatusUpdate(executionId, message); | |
| case STEPS_STATUSES_UPDATE -> handleStepsStatusesUpdate(executionId, message); | |
| default -> LOGGER.warn("Unknown message type: {}", messageType); | |
| } | |
| }; | |
| } | |
| private void handleExecutionStatusUpdate(UUID executionId, Message<String> message) { | |
| private void handleExecutionStatusUpdate(UUID executionId, Message<String> message, MessageType messageType) { | |
| ProcessExecutionStatusUpdate payload = parsePayload(message.getPayload(), ProcessExecutionStatusUpdate.class); | |
| processExecutionService.updateExecutionStatus(executionId, payload.getStatus(), payload.getExecutionEnvName(), payload.getStartedAt(), payload.getCompletedAt()); | |
| notificationService.sendProcessUpdatedMessage(executionId, messageType, payload); | |
| } | |
| switch (messageType) { | |
| case EXECUTION_STATUS_UPDATE -> handleExecutionStatusUpdate(executionId, message, messageType); | |
| case STEP_STATUS_UPDATE -> handleStepStatusUpdate(executionId, message); | |
| case STEPS_STATUSES_UPDATE -> handleStepsStatusesUpdate(executionId, message); | |
| default -> LOGGER.warn("Unknown message type: {}", messageType); | |
| } | |
| }; | |
| } | |
| private void handleExecutionStatusUpdate(UUID executionId, Message<String> message, MessageType messageType) { | |
| ProcessExecutionStatusUpdate payload = parsePayload(message.getPayload(), ProcessExecutionStatusUpdate.class); | |
| processExecutionService.updateExecutionStatus(executionId, payload.getStatus(), payload.getExecutionEnvName(), payload.getStartedAt(), payload.getCompletedAt()); | |
| try { | |
| notificationService.sendProcessUpdatedMessage(executionId, messageType, payload); | |
| } catch (Exception e) { | |
| LOGGER.warn("Failed to send process updated notification for execution {}", executionId, e); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/messaging/ConsumerService.java`
around lines 58 - 71, The notification publish in
ConsumerService.handleExecutionStatusUpdate is currently uncaught, so a failure
in notificationService.sendProcessUpdatedMessage can escape the consumer
callback after processExecutionService.updateExecutionStatus has already
succeeded. Wrap the sendProcessUpdatedMessage call in localized error handling
(or otherwise isolate it) so broker/publish failures are logged and do not
trigger requeueing or duplicate processing of the already-applied execution
status update.
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
|
1 participant
PR Summary