Launch process using config UUID

[FranckLecuyer]

PR Summary

Launch a security analysis process using an existing configuration :

To be merged together with PRs :
gridsuite/actions#7
gridsuite/actions-server#160
gridsuite/gridexplore-app#822

[coderabbitai]

♻️ Duplicate comments (1)

monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java (1)

58-64: ⚠️ Potential issue | 🟠 Major

Handle unsupported config types explicitly to avoid 500s.

Line 58 can throw IllegalArgumentException (from ProcessConfigService.getProcessConfig), and this path is currently unhandled, so clients may get an unintended 500 instead of a controlled 4xx.

🛠️ Proposed fix

@@
-    `@ApiResponses`(value = {`@ApiResponse`(responseCode = "200", description = "The security analysis execution has been started"),
-                           `@ApiResponse`(responseCode = "404", description = "Process config was not found")})
+    `@ApiResponses`(value = {`@ApiResponse`(responseCode = "200", description = "The security analysis execution has been started"),
+                           `@ApiResponse`(responseCode = "400", description = "Unsupported process config type"),
+                           `@ApiResponse`(responseCode = "404", description = "Process config was not found")})
@@
-        Optional<PersistedProcessConfig> processConfig = processConfigService.getProcessConfig(processConfigUuid);
-        if (processConfig.isPresent()) {
-            UUID executionId = monitorService.executeProcess(caseUuid, userId, processConfig.get().processConfig(), processConfig.get().id(), isDebug);
-            return ResponseEntity.ok(executionId);
-        } else {
-            return ResponseEntity.notFound().build();
-        }
+        try {
+            Optional<PersistedProcessConfig> processConfig = processConfigService.getProcessConfig(processConfigUuid);
+            if (processConfig.isPresent()) {
+                UUID executionId = monitorService.executeProcess(
+                    caseUuid,
+                    userId,
+                    processConfig.get().processConfig(),
+                    processConfig.get().id(),
+                    isDebug
+                );
+                return ResponseEntity.ok(executionId);
+            } else {
+                return ResponseEntity.notFound().build();
+            }
+        } catch (IllegalArgumentException e) {
+            return ResponseEntity.badRequest().build();
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java`
around lines 58 - 64, Wrap the call to
processConfigService.getProcessConfig(...) in a try/catch for
IllegalArgumentException thrown by that service (or catch around the whole block
before calling monitorService.executeProcess) and return an appropriate 4xx
ResponseEntity instead of letting the exception propagate; for example, catch
IllegalArgumentException and return
ResponseEntity.badRequest().body(errorMessage) (and optionally log the error) so
MonitorController no longer returns a 500 for unsupported config types while
still calling monitorService.executeProcess(...) only when a valid
PersistedProcessConfig is present.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java`:
- Around line 58-64: Wrap the call to processConfigService.getProcessConfig(...)
in a try/catch for IllegalArgumentException thrown by that service (or catch
around the whole block before calling monitorService.executeProcess) and return
an appropriate 4xx ResponseEntity instead of letting the exception propagate;
for example, catch IllegalArgumentException and return
ResponseEntity.badRequest().body(errorMessage) (and optionally log the error) so
MonitorController no longer returns a 500 for unsupported config types while
still calling monitorService.executeProcess(...) only when a valid
PersistedProcessConfig is present.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ed102793-4ff4-458b-93ce-ee2df2e21a13

📥 Commits

Reviewing files that changed from the base of the PR and between 4f3cd3c and 7e19290.

📒 Files selected for processing (1)

• monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (4)

monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ActionsRestService.java (1)

40-43: ⚠️ Potential issue | 🟠 Major

Normalize Actions server URL construction and set explicit HTTP timeouts.

Line 40’s trailing-slash default combined with Line 51 path concatenation can produce //v1/contingency-lists. Also, Line 42 builds a client with no explicit connect/read timeout for an external dependency.

Suggested fix

+import java.time.Duration;
 import java.util.List;
 import java.util.UUID;
@@
-    public ActionsRestService(`@Value`("${gridsuite.services.actions-server.base-uri:http://actions-server/}") String actionsServerBaseUri,
+    public ActionsRestService(`@Value`("${gridsuite.services.actions-server.base-uri:http://actions-server}") String actionsServerBaseUri,
                               RestTemplateBuilder restTemplateBuilder) {
-        this.actionsServerRest = restTemplateBuilder.build();
+        this.actionsServerRest = restTemplateBuilder
+            .setConnectTimeout(Duration.ofSeconds(5))
+            .setReadTimeout(Duration.ofSeconds(30))
+            .build();
         this.actionsServerBaseUri = actionsServerBaseUri;
     }
@@
-        String path = this.actionsServerBaseUri + UriComponentsBuilder
-            .fromPath(DELIMITER + ACTIONS_SERVER_API_VERSION + DELIMITER + "contingency-lists")
-            .buildAndExpand()
-            .toUriString();
+        String path = UriComponentsBuilder.fromUriString(actionsServerBaseUri)
+            .pathSegment(ACTIONS_SERVER_API_VERSION, "contingency-lists")
+            .toUriString();

Also applies to: 51-54

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ActionsRestService.java`
around lines 40 - 43, The constructor for ActionsRestService currently sets
actionsServerBaseUri with a trailing-slash default and builds actionsServerRest
without timeouts; update the constructor to normalize actionsServerBaseUri
(remove any trailing slash) and ensure all later concatenations (e.g., where
"/v1/contingency-lists" is appended) produce a single slash, and build the
RestTemplate via restTemplateBuilder with explicit connect and read timeouts
(set sensible defaults) so actionsServerRest has timeouts configured; adjust the
constructor method, the actionsServerBaseUri field usage, and the RestTemplate
creation (references: ActionsRestService constructor, actionsServerBaseUri,
actionsServerRest, restTemplateBuilder) accordingly.

monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/securityanalysis/steps/SecurityAnalysisRunComputationStep.java (1)

74-76: ⚠️ Potential issue | 🟠 Major

Make contingency UUID extraction null-safe.

Line 75 assumes non-null ContingencyListsInfos, nested lists, and items. A partial payload can throw NullPointerException and fail the whole step.

Suggested fix

 import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.UUID;
@@
-            List<ContingencyListsInfos> contingencyListInfos = securityAnalysisParametersValues.getContingencyListsInfos();
-            List<UUID> contingenciesListUuids = contingencyListInfos != null ? contingencyListInfos.stream().flatMap(contingencyListsInfos -> contingencyListsInfos.getContingencyLists().stream().map(IdNameInfos::getId)).toList() : List.of();
+            List<UUID> contingenciesListUuids = Optional.ofNullable(securityAnalysisParametersValues.getContingencyListsInfos())
+                .orElseGet(List::of)
+                .stream()
+                .filter(Objects::nonNull)
+                .flatMap(contingencyListsInfos -> Optional.ofNullable(contingencyListsInfos.getContingencyLists())
+                    .orElseGet(List::of)
+                    .stream())
+                .filter(Objects::nonNull)
+                .map(IdNameInfos::getId)
+                .filter(Objects::nonNull)
+                .distinct()
+                .toList();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/securityanalysis/steps/SecurityAnalysisRunComputationStep.java`
around lines 74 - 76, The extraction of contingency UUIDs from
securityAnalysisParametersValues is not null-safe and can NPE when
ContingencyListsInfos, its getContingencyLists() result, or IdNameInfos::getId
are null; update the pipeline around contingencyListInfos (the variable produced
from securityAnalysisParametersValues.getContingencyListsInfos()) to use
null-safe streaming: treat the outer list with Stream.ofNullable or a null
check, filter out null ContingencyListsInfos, call getContingencyLists() with
Stream.ofNullable(), filter out null IdNameInfos, map to IdNameInfos.getId while
filtering null ids, collect toList, then pass that safely to
actionsRestService.getPersistentContingencyLists so persistentContingencyLists
is computed only from non-null UUIDs. Ensure variable names referenced:
contingencyListInfos, ContingencyListsInfos, getContingencyLists(),
IdNameInfos::getId, and actionsRestService.getPersistentContingencyLists.

monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.java (1)

26-31: ⚠️ Potential issue | 🟠 Major

Handle StreamBridge.send failure explicitly.

Line 31 ignores the boolean returned by publisher.send(...). If publish fails, execution can remain scheduled without a run message.

Suggested fix

+import com.powsybl.commons.PowsyblException;
 import lombok.RequiredArgsConstructor;
@@
-        publisher.send(bindingName, message);
+        boolean sent = publisher.send(bindingName, message);
+        if (!sent) {
+            throw new PowsyblException("Failed to publish process run message for execution " + executionId);
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.java`
around lines 26 - 31, In sendProcessRunMessage, check the boolean result of
publisher.send(bindingName, message) and handle failures instead of ignoring it:
after building bindingName and ProcessRunMessage, capture the return value from
publisher.send and if false, log an error with
executionId/caseUuid/processConfig and throw an unchecked exception (e.g.,
IllegalStateException) or invoke the existing retry/reschedule path so the
execution isn't left scheduled without a run message; update
sendProcessRunMessage to perform this conditional handling and use the existing
logger to record details.

monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java (1)

58-64: ⚠️ Potential issue | 🟠 Major

Handle IllegalArgumentException from ProcessConfigService.getProcessConfig() to avoid 500 responses.

Per the ProcessConfigService.getProcessConfig() implementation (context snippet 2), the switch statement throws IllegalArgumentException for unsupported entity types. This exception path will surface as a 500 Internal Server Error rather than a controlled client response.

🛠️ Suggested fix to handle the exception

     public ResponseEntity<UUID> executeSecurityAnalysis(
             `@Parameter`(description = "Case uuid") `@RequestParam`(name = "caseUuid") UUID caseUuid,
             `@Parameter`(description = "Process config uuid") `@RequestParam`(name = "processConfigUuid") UUID processConfigUuid,
             `@RequestParam`(required = false, defaultValue = "false") boolean isDebug,
             `@RequestHeader`(HEADER_USER_ID) String userId) {
-        Optional<PersistedProcessConfig> processConfig = processConfigService.getProcessConfig(processConfigUuid);
-        if (processConfig.isPresent()) {
-            UUID executionId = monitorService.executeProcess(caseUuid, userId, processConfig.get().processConfig(), processConfig.get().id(), isDebug);
-            return ResponseEntity.ok(executionId);
-        } else {
-            return ResponseEntity.notFound().build();
+        try {
+            Optional<PersistedProcessConfig> processConfig = processConfigService.getProcessConfig(processConfigUuid);
+            if (processConfig.isPresent()) {
+                UUID executionId = monitorService.executeProcess(caseUuid, userId, processConfig.get().processConfig(), processConfig.get().id(), isDebug);
+                return ResponseEntity.ok(executionId);
+            } else {
+                return ResponseEntity.notFound().build();
+            }
+        } catch (IllegalArgumentException e) {
+            return ResponseEntity.badRequest().build();
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java`
around lines 58 - 64, Wrap the call to
processConfigService.getProcessConfig(processConfigUuid) in a try/catch that
catches IllegalArgumentException and returns a controlled client response (e.g.,
ResponseEntity.badRequest().body(...) or .build()) instead of letting the
exception propagate; update the logic in MonitorController around the existing
Optional handling so that if getProcessConfig throws IllegalArgumentException
you return the chosen client error response, otherwise proceed to call
monitorService.executeProcess(...) and return the executionId as before.

🧹 Nitpick comments (9)

monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessExecutionContextTest.java (1)

63-77: Add explicit assertion for step-context userId propagation.

Given the new constructor parameter, this test should also assert stepContext.getUserId() to protect the new contract.

Suggested test addition

         assertThat(stepContext.getConfig()).isEqualTo(config);
+        assertThat(stepContext.getUserId()).isEqualTo("user1");
         assertThat(stepContext.getProcessStepType()).isEqualTo(stepType);
         assertThat(stepContext.getStepOrder()).isEqualTo(stepOrder);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessExecutionContextTest.java`
around lines 63 - 77, The test currently validates fields from
ProcessExecutionContext.createStepContext but misses asserting user propagation;
add an assertion that the created ProcessStepExecutionContext's userId matches
the original ProcessExecutionContext user (e.g., assert
stepContext.getUserId().equals(processContext.getUserId()) or equals the "user1"
value used in the constructor) to ensure
ProcessExecutionContext.createStepContext preserves the userId contract (check
ProcessExecutionContext, createStepContext, ProcessStepExecutionContext, and
getUserId).

monitor-commons/src/main/java/org/gridsuite/monitor/commons/ProcessRunMessage.java (1)

25-27: Consider fail-fast validation for userId in the record.

userId is now part of the execution contract and consumed downstream; validating it at message construction avoids late failures deeper in execution.

Suggested refactor

 import java.util.UUID;
+import java.util.Objects;
@@
 public record ProcessRunMessage<T extends ProcessConfig>(
@@
     String debugFileLocation,
     String userId
 ) {
+    public ProcessRunMessage {
+        Objects.requireNonNull(config, "config must not be null");
+        Objects.requireNonNull(userId, "userId must not be null");
+    }
+
     public ProcessType processType() {
         return config.processType();
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/ProcessRunMessage.java`
around lines 25 - 27, ProcessRunMessage should validate userId at construction
to fail fast; add a compact constructor for the ProcessRunMessage record that
checks userId (e.g., Objects.requireNonNull(userId, "userId must not be null")
and also validate non-blank with userId.isBlank() and throw
IllegalArgumentException with a clear message) so invalid messages are rejected
immediately when the record is created.

monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestService.java (1)

34-35: Avoid a mutable public base-URI setter on a singleton service.

Lines 34-35 expose runtime mutation of the endpoint. Prefer constructor-injected immutable configuration for safer behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestService.java`
around lines 34 - 35, The service exposes a mutable runtime setter for
loadFlowServerBaseUri via the `@Setter` on the LoadFlowRestService class; remove
the public setter and make the URI an immutable dependency by turning
loadFlowServerBaseUri into a final field injected via the constructor (or use
Lombok's `@RequiredArgsConstructor`) so the value is set at construction time and
cannot be changed at runtime, updating any instantiation places to pass the base
URI accordingly and removing references to the removed setter.

monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java (1)

87-98: Assert processConfigId propagation in the executeProcess test.

Line 87 passes a config UUID, but Lines 90-98 do not verify it was persisted on ProcessExecutionEntity.

Suggested test adjustment

-        UUID result = monitorService.executeProcess(caseUuid, userId, securityAnalysisConfig, UUID.randomUUID(), true);
+        UUID processConfigId = UUID.randomUUID();
+        UUID result = monitorService.executeProcess(caseUuid, userId, securityAnalysisConfig, processConfigId, true);
@@
         verify(executionRepository).save(argThat(execution ->
                         execution.getId() != null &&
                         ProcessType.SECURITY_ANALYSIS.name().equals(execution.getType()) &&
                         caseUuid.equals(execution.getCaseUuid()) &&
+                        processConfigId.equals(execution.getProcessConfigId()) &&
                         userId.equals(execution.getUserId()) &&
                         ProcessStatus.SCHEDULED.equals(execution.getStatus()) &&
                         execution.getScheduledAt() != null &&
                         execution.getStartedAt() == null
         ));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java`
around lines 87 - 98, The test calls monitorService.executeProcess with an
inline config UUID but doesn't assert it was stored; create a local UUID
variable (e.g. processConfigId = UUID.randomUUID()), pass that to
monitorService.executeProcess, and extend the
verify(executionRepository).save(argThat(...)) predicate to assert
execution.getProcessConfigId() != null &&
processConfigId.equals(execution.getProcessConfigId()) so the
ProcessExecutionEntity persists the config ID.

monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ActionsRestServiceTest.java (1)

89-91: Strengthen the success-path assertion.

Line 90 only checks non-null; this can pass even if deserialization drops items. Assert expected count/content from the mocked response.

Minimal improvement

-        assertThat(result).isNotNull();
+        assertThat(result).hasSize(2);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ActionsRestServiceTest.java`
around lines 89 - 91, The test in ActionsRestServiceTest currently only asserts
result is not null after calling
actionsRestService.getPersistentContingencyLists(requestUuids); strengthen this
by asserting the expected number of items and at least one or two key properties
from the mocked response (e.g., compare result.size() to the expected count and
assert that result.get(0).getUuid() or getName() equals the known UUID/name from
the mock). Locate the mock setup for requestUuids/response in this test and use
those known values to assert contents so deserialization drops would fail the
test.

monitor-server/src/test/java/org/gridsuite/monitor/server/MonitorIntegrationTest.java (1)

239-244: Minor: Inconsistent variable naming for loadflow parameters UUID.

The variable loadflowparametersUuid at line 239 uses different casing than similar variables elsewhere in the same file (loadFlowParametersUuid at lines 255, 276, 287). Consider using consistent camelCase naming for readability.

Also, there appears to be an extra blank line at line 244.

✏️ Suggested fix for naming consistency

-        UUID loadflowparametersUuid = UUID.randomUUID();
+        UUID loadFlowParametersUuid = UUID.randomUUID();
         SecurityAnalysisConfig securityAnalysisConfig = new SecurityAnalysisConfig(
                 parametersUuid,
                 List.of(modificationUuid),
-                loadflowparametersUuid
-
+                loadFlowParametersUuid
         );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/MonitorIntegrationTest.java`
around lines 239 - 244, Rename the variable loadflowparametersUuid to use
consistent camelCase (loadFlowParametersUuid) where it's declared and passed
into the SecurityAnalysisConfig constructor (alongside parametersUuid and
modificationUuid) and update any subsequent uses to match; also remove the extra
blank line after the constructor call to match surrounding formatting.

monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestServiceTest.java (2)

83-92: Simplify exception handling in test setup.

The try-catch block wrapping objectMapper.writeValueAsString is unusual placement. Consider declaring throws JsonProcessingException on the test method instead for cleaner code.

✏️ Suggested simplification

     `@Test`
-    void getParameters() {
+    void getParameters() throws JsonProcessingException {
         SecurityAnalysisParametersValues expectedParameters = SecurityAnalysisParametersValues.builder()
             .flowProportionalThreshold(0.1)
             .lowVoltageProportionalThreshold(0.05)
             .highVoltageProportionalThreshold(0.05)
             .lowVoltageAbsoluteThreshold(10.0)
             .highVoltageAbsoluteThreshold(10.0)
             .build();

-        try {
-            server.expect(MockRestRequestMatchers.method(HttpMethod.GET))
-                .andExpect(MockRestRequestMatchers.requestTo(
-                    "http://security-analysis-server/v1/parameters/" + PARAMETERS_UUID))
-                .andRespond(MockRestResponseCreators.withSuccess()
-                    .contentType(MediaType.APPLICATION_JSON)
-                    .body(objectMapper.writeValueAsString(expectedParameters)));
-        } catch (JsonProcessingException e) {
-            throw new RuntimeException(e);
-        }
+        server.expect(MockRestRequestMatchers.method(HttpMethod.GET))
+            .andExpect(MockRestRequestMatchers.requestTo(
+                "http://security-analysis-server/v1/parameters/" + PARAMETERS_UUID))
+            .andRespond(MockRestResponseCreators.withSuccess()
+                .contentType(MediaType.APPLICATION_JSON)
+                .body(objectMapper.writeValueAsString(expectedParameters)));

         SecurityAnalysisParametersValues result = securityAnalysisRestService.getParameters(PARAMETERS_UUID, "user1");

         assertThat(result).usingRecursiveComparison().isEqualTo(expectedParameters);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestServiceTest.java`
around lines 83 - 92, Remove the try-catch around
objectMapper.writeValueAsString in the test setup and let the test method throw
JsonProcessingException instead; locate the block using
server.expect(...).andRespond(...).body(objectMapper.writeValueAsString(expectedParameters))
in SecurityAnalysisRestServiceTest and change the test method signature to
declare throws JsonProcessingException so the checked exception is propagated
rather than caught locally.

---

99-109: Consider renaming test to reflect the actual scenario being tested.

The test name getParametersNotFound is misleading since it tests a server error (500) scenario, not a 404 Not Found response. A more accurate name would be getParametersShouldThrowOnServerError or getParametersServerError.

✏️ Suggested rename

     `@Test`
-    void getParametersNotFound() {
+    void getParametersShouldThrowOnServerError() {
         server.expect(MockRestRequestMatchers.method(HttpMethod.GET))
             .andExpect(MockRestRequestMatchers.requestTo(
                 "http://security-analysis-server/v1/parameters/" + PARAMETERS_ERROR_UUID))
             .andRespond(MockRestResponseCreators.withServerError());

         assertThatThrownBy(() -> securityAnalysisRestService.getParameters(PARAMETERS_ERROR_UUID, "user1"))
             .isInstanceOf(PowsyblException.class)
             .hasMessageContaining("Error retrieving security analysis parameters");
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestServiceTest.java`
around lines 99 - 109, Rename the misleading test method getParametersNotFound
in SecurityAnalysisRestServiceTest to a name reflecting a 500 server error
scenario (e.g., getParametersShouldThrowOnServerError or
getParametersServerError); update the method declaration and any
references/annotations in the class so the test name matches the behavior
exercised by the MockRestResponseCreators.withServerError() and the assertion on
securityAnalysisRestService.getParameters(PARAMETERS_ERROR_UUID, "user1")
throwing PowsyblException.

monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestService.java (1)

76-78: Inconsistent URI construction between methods.

getParameters constructs the URI by directly concatenating securityAnalysisServerBaseUri with the path (line 76), while saveResult (line 64) uses getSecurityAnalysisServerBaseUri() which already appends the API version. This inconsistency can lead to maintenance issues and potential bugs if the base URI format changes.

Additionally, this still uses manual string concatenation which can produce malformed URIs if the base URI has a trailing slash.

♻️ Suggested fix using consistent URI builder pattern

-        var path = securityAnalysisServerBaseUri + UriComponentsBuilder.fromPath(DELIMITER + SA_API_VERSION + DELIMITER + "parameters/{uuid}")
-            .buildAndExpand(parametersUuid)
-            .toUriString();
+        var path = UriComponentsBuilder.fromUriString(securityAnalysisServerBaseUri)
+            .pathSegment(SA_API_VERSION, "parameters", "{uuid}")
+            .buildAndExpand(parametersUuid)
+            .toUriString();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestService.java`
around lines 76 - 78, getParameters currently concatenates
securityAnalysisServerBaseUri with a path which is inconsistent with saveResult
(which uses getSecurityAnalysisServerBaseUri()) and can break when the base URI
has a trailing slash; change getParameters to build the URI using the same base
accessor and UriComponentsBuilder instead of manual concatenation — e.g., call
getSecurityAnalysisServerBaseUri() and then use
UriComponentsBuilder.fromUriString(...).path or .pathSegment with DELIMITER,
SA_API_VERSION and "parameters/{uuid}" and then
.buildAndExpand(parametersUuid).toUriString() so the API version is not
duplicated and trailing slashes are handled consistently.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java`:
- Around line 58-64: The method executeProcess currently allows a null
processConfigId to be persisted into ProcessExecutionEntity, which can create
untraceable scheduled executions; update executeProcess to validate
processConfigId at the start (in the executeProcess method) and throw an
IllegalArgumentException (or another appropriate runtime exception) if
processConfigId is null, before building or saving the ProcessExecutionEntity,
referencing the executeProcess method and ProcessExecutionEntity.builder() usage
so the null check occurs prior to using processConfigId.

In `@monitor-worker-server/pom.xml`:
- Line 17: The build fails because the property actions.version is set to the
unavailable snapshot 0.4.0-SNAPSHOT which is used for the gridsuite-actions
dependency in compile scope; change the actions.version property to a published
release (set actions.version to 0.3.0) or alternatively configure a valid
snapshot repository that hosts 0.4.0-SNAPSHOT so the gridsuite-actions artifact
can be resolved; update the actions.version property (and verify the dependency
coordinates for gridsuite-actions) accordingly.

In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestService.java`:
- Around line 39-41: The LoadFlowRestService currently stores
loadFlowServerBaseUri with a trailing slash and builds the RestTemplate without
timeouts, causing double-slash URLs and no network timeouts; in the constructor
for LoadFlowRestService remove any trailing slash from the injected
loadFlowServerBaseUri (e.g., trim trailing '/') and store the normalized base
URI, build REST call URLs using a safe join (use
UriComponentsBuilder.fromHttpUrl(loadFlowServerBaseUri).path("/v1/...") or
similar) instead of simple string concatenation, and configure timeouts on the
RestTemplate creation (use restTemplateBuilder.setConnectTimeout(...) and
setReadTimeout(...) or supply a ClientHttpRequestFactory with configured
connect/read timeouts before calling restTemplateBuilder.build()) so all
outgoing calls use the normalized base URI and have explicit connect/read
timeouts.

---

Duplicate comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java`:
- Around line 58-64: Wrap the call to
processConfigService.getProcessConfig(processConfigUuid) in a try/catch that
catches IllegalArgumentException and returns a controlled client response (e.g.,
ResponseEntity.badRequest().body(...) or .build()) instead of letting the
exception propagate; update the logic in MonitorController around the existing
Optional handling so that if getProcessConfig throws IllegalArgumentException
you return the chosen client error response, otherwise proceed to call
monitorService.executeProcess(...) and return the executionId as before.

In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.java`:
- Around line 26-31: In sendProcessRunMessage, check the boolean result of
publisher.send(bindingName, message) and handle failures instead of ignoring it:
after building bindingName and ProcessRunMessage, capture the return value from
publisher.send and if false, log an error with
executionId/caseUuid/processConfig and throw an unchecked exception (e.g.,
IllegalStateException) or invoke the existing retry/reschedule path so the
execution isn't left scheduled without a run message; update
sendProcessRunMessage to perform this conditional handling and use the existing
logger to record details.

In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/securityanalysis/steps/SecurityAnalysisRunComputationStep.java`:
- Around line 74-76: The extraction of contingency UUIDs from
securityAnalysisParametersValues is not null-safe and can NPE when
ContingencyListsInfos, its getContingencyLists() result, or IdNameInfos::getId
are null; update the pipeline around contingencyListInfos (the variable produced
from securityAnalysisParametersValues.getContingencyListsInfos()) to use
null-safe streaming: treat the outer list with Stream.ofNullable or a null
check, filter out null ContingencyListsInfos, call getContingencyLists() with
Stream.ofNullable(), filter out null IdNameInfos, map to IdNameInfos.getId while
filtering null ids, collect toList, then pass that safely to
actionsRestService.getPersistentContingencyLists so persistentContingencyLists
is computed only from non-null UUIDs. Ensure variable names referenced:
contingencyListInfos, ContingencyListsInfos, getContingencyLists(),
IdNameInfos::getId, and actionsRestService.getPersistentContingencyLists.

In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ActionsRestService.java`:
- Around line 40-43: The constructor for ActionsRestService currently sets
actionsServerBaseUri with a trailing-slash default and builds actionsServerRest
without timeouts; update the constructor to normalize actionsServerBaseUri
(remove any trailing slash) and ensure all later concatenations (e.g., where
"/v1/contingency-lists" is appended) produce a single slash, and build the
RestTemplate via restTemplateBuilder with explicit connect and read timeouts
(set sensible defaults) so actionsServerRest has timeouts configured; adjust the
constructor method, the actionsServerBaseUri field usage, and the RestTemplate
creation (references: ActionsRestService constructor, actionsServerBaseUri,
actionsServerRest, restTemplateBuilder) accordingly.

---

Nitpick comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/ProcessRunMessage.java`:
- Around line 25-27: ProcessRunMessage should validate userId at construction to
fail fast; add a compact constructor for the ProcessRunMessage record that
checks userId (e.g., Objects.requireNonNull(userId, "userId must not be null")
and also validate non-blank with userId.isBlank() and throw
IllegalArgumentException with a clear message) so invalid messages are rejected
immediately when the record is created.

In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/MonitorIntegrationTest.java`:
- Around line 239-244: Rename the variable loadflowparametersUuid to use
consistent camelCase (loadFlowParametersUuid) where it's declared and passed
into the SecurityAnalysisConfig constructor (alongside parametersUuid and
modificationUuid) and update any subsequent uses to match; also remove the extra
blank line after the constructor call to match surrounding formatting.

In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.java`:
- Around line 87-98: The test calls monitorService.executeProcess with an inline
config UUID but doesn't assert it was stored; create a local UUID variable (e.g.
processConfigId = UUID.randomUUID()), pass that to
monitorService.executeProcess, and extend the
verify(executionRepository).save(argThat(...)) predicate to assert
execution.getProcessConfigId() != null &&
processConfigId.equals(execution.getProcessConfigId()) so the
ProcessExecutionEntity persists the config ID.

In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestService.java`:
- Around line 34-35: The service exposes a mutable runtime setter for
loadFlowServerBaseUri via the `@Setter` on the LoadFlowRestService class; remove
the public setter and make the URI an immutable dependency by turning
loadFlowServerBaseUri into a final field injected via the constructor (or use
Lombok's `@RequiredArgsConstructor`) so the value is set at construction time and
cannot be changed at runtime, updating any instantiation places to pass the base
URI accordingly and removing references to the removed setter.

In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestService.java`:
- Around line 76-78: getParameters currently concatenates
securityAnalysisServerBaseUri with a path which is inconsistent with saveResult
(which uses getSecurityAnalysisServerBaseUri()) and can break when the base URI
has a trailing slash; change getParameters to build the URI using the same base
accessor and UriComponentsBuilder instead of manual concatenation — e.g., call
getSecurityAnalysisServerBaseUri() and then use
UriComponentsBuilder.fromUriString(...).path or .pathSegment with DELIMITER,
SA_API_VERSION and "parameters/{uuid}" and then
.buildAndExpand(parametersUuid).toUriString() so the API version is not
duplicated and trailing slashes are handled consistently.

In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessExecutionContextTest.java`:
- Around line 63-77: The test currently validates fields from
ProcessExecutionContext.createStepContext but misses asserting user propagation;
add an assertion that the created ProcessStepExecutionContext's userId matches
the original ProcessExecutionContext user (e.g., assert
stepContext.getUserId().equals(processContext.getUserId()) or equals the "user1"
value used in the constructor) to ensure
ProcessExecutionContext.createStepContext preserves the userId contract (check
ProcessExecutionContext, createStepContext, ProcessStepExecutionContext, and
getUserId).

In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ActionsRestServiceTest.java`:
- Around line 89-91: The test in ActionsRestServiceTest currently only asserts
result is not null after calling
actionsRestService.getPersistentContingencyLists(requestUuids); strengthen this
by asserting the expected number of items and at least one or two key properties
from the mocked response (e.g., compare result.size() to the expected count and
assert that result.get(0).getUuid() or getName() equals the known UUID/name from
the mock). Locate the mock setup for requestUuids/response in this test and use
those known values to assert contents so deserialization drops would fail the
test.

In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestServiceTest.java`:
- Around line 83-92: Remove the try-catch around objectMapper.writeValueAsString
in the test setup and let the test method throw JsonProcessingException instead;
locate the block using
server.expect(...).andRespond(...).body(objectMapper.writeValueAsString(expectedParameters))
in SecurityAnalysisRestServiceTest and change the test method signature to
declare throws JsonProcessingException so the checked exception is propagated
rather than caught locally.
- Around line 99-109: Rename the misleading test method getParametersNotFound in
SecurityAnalysisRestServiceTest to a name reflecting a 500 server error scenario
(e.g., getParametersShouldThrowOnServerError or getParametersServerError);
update the method declaration and any references/annotations in the class so the
test name matches the behavior exercised by the
MockRestResponseCreators.withServerError() and the assertion on
securityAnalysisRestService.getParameters(PARAMETERS_ERROR_UUID, "user1")
throwing PowsyblException.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9347b1e3-9e1b-45c1-b775-6cb1b95925ad

📥 Commits

Reviewing files that changed from the base of the PR and between 7e19290 and a3f0d73.

📒 Files selected for processing (32)

• monitor-commons/src/main/java/org/gridsuite/monitor/commons/ProcessRunMessage.java
• monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.java
• monitor-server/src/main/java/org/gridsuite/monitor/server/dto/ProcessExecution.java
• monitor-server/src/main/java/org/gridsuite/monitor/server/entities/ProcessExecutionEntity.java
• monitor-server/src/main/java/org/gridsuite/monitor/server/mapper/ProcessExecutionMapper.java
• monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.java
• monitor-server/src/main/java/org/gridsuite/monitor/server/services/NotificationService.java
• monitor-server/src/main/resources/db/changelog/changesets/changelog_20260227T101113Z.xml
• monitor-server/src/test/java/org/gridsuite/monitor/server/MonitorIntegrationTest.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
• monitor-server/src/test/java/org/gridsuite/monitor/server/services/NotificationServiceTest.java
• monitor-worker-server/pom.xml
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/config/MonitorWorkerConfig.java
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/ProcessExecutionContext.java
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/ProcessStepExecutionContext.java
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/securityanalysis/steps/SecurityAnalysisRunComputationStep.java
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ActionsRestService.java
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterRestService.java
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestService.java
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestService.java
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java
• monitor-worker-server/src/main/resources/org/gridsuite/monitor/worker/server/reports.properties
• monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessExecutionContextTest.java
• monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/securityanalysis/steps/SecurityAnalysisRunComputationStepTest.java
• monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ActionsRestServiceTest.java
• monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ConsumerServiceTest.java
• monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestServiceTest.java
• monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionServiceTest.java
• monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestServiceTest.java
• monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/StepExecutionServiceTest.java

🚧 Files skipped from review as they are similar to previous changes (9)

• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionService.java
• monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestServiceTest.java
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/ProcessExecutionContext.java
• monitor-server/src/main/resources/db/changelog/changesets/changelog_20260227T101113Z.xml
• monitor-server/src/main/java/org/gridsuite/monitor/server/dto/ProcessExecution.java
• monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/StepExecutionService.java
• monitor-server/src/test/java/org/gridsuite/monitor/server/services/NotificationServiceTest.java
• monitor-server/src/main/java/org/gridsuite/monitor/server/entities/ProcessExecutionEntity.java
• monitor-worker-server/src/main/resources/org/gridsuite/monitor/worker/server/reports.properties

[antoinebhs]

Looks good to me!

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
90.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud