Merged
Conversation
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request restructures the codebase into a more modular architecture by reorganizing packages, renaming services to REST clients, introducing new orchestration interfaces, and refactoring dependency injection patterns across monitor-commons, monitor-server, and monitor-worker-server modules. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
monitor-server/src/test/java/org/gridsuite/monitor/server/MonitorIntegrationTest.java (1)
160-168:⚠️ Potential issue | 🟡 MinorUse
orElseThrow()instead oforElse(null)to avoid NPE-driven test failures.At lines 160 and 182,
executionis re-fetched with.orElse(null)but then accessed directly without a null guard (unlike the first fetch at line 121 which hasassertThat(execution).isNotNull()). If the entity is absent, the test aborts with an opaqueNullPointerExceptionrather than a meaningful assertion failure.🛡️ Proposed fix
- execution = executionRepository.findById(executionId).orElse(null); - assertThat(execution.getSteps()).hasSize(2); + execution = executionRepository.findById(executionId).orElseThrow(); + assertThat(execution.getSteps()).hasSize(2);Apply the same change at line 182:
- execution = executionRepository.findById(executionId).orElse(null); - assertThat(execution.getStatus()).isEqualTo(ProcessStatus.COMPLETED); + execution = executionRepository.findById(executionId).orElseThrow(); + assertThat(execution.getStatus()).isEqualTo(ProcessStatus.COMPLETED);🤖 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 160 - 168, Replace uses of executionRepository.findById(...).orElse(null) in MonitorIntegrationTest (the re-fetch at the second block where variable execution is used) with executionRepository.findById(...).orElseThrow() so the test fails with a clear NoSuchElementException instead of NPE; update the second retrieval that assigns to execution (the one followed by assertions on execution.getSteps()) to use orElseThrow() to ensure a meaningful failure and avoid null access when asserting on execution and its steps.monitor-server/src/main/java/org/gridsuite/monitor/server/client/ReportRestClient.java (2)
50-53:⚠️ Potential issue | 🟡 Minor
Content-Typeis wrong for a GET request; useAcceptinstead.
Content-Typedescribes the body of the request, which a GET has none. The correct header to declare expected response format isAccept: application/json. Some servers or proxies may reject/ignoreContent-Typeon a bodyless request.🛡️ Proposed fix
HttpHeaders headers = new HttpHeaders(); - headers.setContentType(MediaType.APPLICATION_JSON); + headers.setAccept(List.of(MediaType.APPLICATION_JSON)); return restTemplate.exchange(this.getReportServerURI() + path, HttpMethod.GET, new HttpEntity<>(headers), new ParameterizedTypeReference<ReportPage>() { }).getBody();🤖 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/client/ReportRestClient.java` around lines 50 - 53, The request currently sets Content-Type on a GET which is incorrect; in ReportRestClient where HttpHeaders headers are created and used in restTemplate.exchange, replace headers.setContentType(MediaType.APPLICATION_JSON) with setting the Accept header (e.g., headers.setAccept(...) or headers.add("Accept", "application/json")) so the client declares the expected response media type; ensure no Content-Type is sent for the bodyless GET.
46-53:⚠️ Potential issue | 🟡 Minor
getBody()may returnnull, causing a silent NPE at callers.
RestTemplate.exchange(...).getBody()returnsnullfor 204/205 responses. Callers ofgetReport()dereference the return value without a null check.🛡️ Proposed fix
- return restTemplate.exchange(this.getReportServerURI() + path, HttpMethod.GET, new HttpEntity<>(headers), new ParameterizedTypeReference<ReportPage>() { }).getBody(); + ReportPage body = restTemplate.exchange(this.getReportServerURI() + path, HttpMethod.GET, new HttpEntity<>(headers), new ParameterizedTypeReference<ReportPage>() { }).getBody(); + if (body == null) { + throw new IllegalStateException("Empty response body for report " + reportId); + } + return body;🤖 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/client/ReportRestClient.java` around lines 46 - 53, The getReport method currently calls restTemplate.exchange(...).getBody() which can be null for 204/205 responses and leads to silent NPEs; update ReportRestClient.getReport to capture the ResponseEntity<ReportPage> from restTemplate.exchange (instead of calling getBody() inline), check response.getStatusCode() and response.getBody() for null, and either return a safe empty ReportPage or throw a clear exception (e.g., IllegalStateException) when body is missing; reference restTemplate.exchange(...) and getReport to locate the change and ensure callers never receive a null ReportPage.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/client/FilterRestClient.java (1)
48-48:⚠️ Potential issue | 🟡 Minor
getBody()may returnnull, causing a downstream NPE.
RestTemplate.exchange(...).getBody()returnsnullfor 204/205 responses or when the converter produces no object. Callers ofgetFilters()would encounter a silentNullPointerException.🛡️ Proposed fix
- return restTemplate.exchange(filterServerBaseUri + path, HttpMethod.GET, null, new ParameterizedTypeReference<List<AbstractFilter>>() { }).getBody(); + List<AbstractFilter> body = restTemplate.exchange(filterServerBaseUri + path, HttpMethod.GET, null, new ParameterizedTypeReference<List<AbstractFilter>>() { }).getBody(); + return body != null ? body : List.of();🤖 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/client/FilterRestClient.java` at line 48, The call in FilterRestClient.getFilters() uses restTemplate.exchange(...).getBody() which may return null for 204/205 or empty responses; change the method to guard against null and return an empty List<AbstractFilter> instead of propagating a null (e.g., wrap the result with Optional.ofNullable(...).orElse(Collections.emptyList()) or an explicit null check), keeping the restTemplate.exchange(...) call and the ParameterizedTypeReference unchanged so callers never receive a null and avoid downstream NPEs.monitor-server/src/test/java/org/gridsuite/monitor/server/services/provider/SecurityAnalysisResultProviderTest.java (1)
37-47:⚠️ Potential issue | 🟡 MinorStale test method names after the
SecurityAnalysisService→SecurityAnalysisRestClientrename.Both
getResultShouldDelegateToSecurityAnalysisServiceanddeleteResultShouldDelegateToSecurityAnalysisServicestill reference "Service" rather than "RestClient".✏️ Proposed rename
- void getResultShouldDelegateToSecurityAnalysisService() { + void getResultShouldDelegateToSecurityAnalysisRestClient() {- void deleteResultShouldDelegateToSecurityAnalysisService() { + void deleteResultShouldDelegateToSecurityAnalysisRestClient() {Also applies to: 51-60
🤖 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/provider/SecurityAnalysisResultProviderTest.java` around lines 37 - 47, Rename the stale test method names that still refer to "SecurityAnalysisService": change getResultShouldDelegateToSecurityAnalysisService to getResultShouldDelegateToSecurityAnalysisRestClient and change deleteResultShouldDelegateToSecurityAnalysisService (the test around lines 51-60) to deleteResultShouldDelegateToSecurityAnalysisRestClient so they reflect the actual collaborator SecurityAnalysisRestClient and keep test names consistent with the class under test (provider.getResult / provider.deleteResult).monitor-server/src/main/java/org/gridsuite/monitor/server/messaging/NotificationService.java (1)
26-31:⚠️ Potential issue | 🟡 MinorIgnored
booleanreturn value frompublisher.send()masks delivery failures.
StreamBridgeexposessend(String bindingName, Object data, MimeType outputContentType)and honors the message content type. The overload used here also returnsbooleanindicating whether the message was dispatched. Silently dropping it means a failed send goes undetected. At minimum, log a warning whensend()returnsfalse.🛡️ Proposed fix
- publisher.send(bindingName, message); + boolean sent = publisher.send(bindingName, message); + if (!sent) { + // use your logger + log.warn("Failed to send ProcessRunMessage for executionId={} to binding={}", executionId, bindingName); + }🤖 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/messaging/NotificationService.java` around lines 26 - 31, sendProcessRunMessage currently ignores the boolean return from publisher.send, so delivery failures are undetected; modify sendProcessRunMessage to capture the boolean result from publisher.send(bindingName, message) and if it is false, call the logger to emit a warning including bindingName, executionId (or caseUuid) and a short description of the failed ProcessRunMessage (use ProcessRunMessage's identifying fields) so failures are visible; optionally use the StreamBridge overload that accepts a MimeType if content-type fidelity is required.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessTest.java (1)
42-92:⚠️ Potential issue | 🟠 MajorDon’t leave core tests commented out.
These execution-path tests are now disabled, which hides regressions in step orchestration. Please re-enable/update them (or remove the file if this coverage is obsolete).
🤖 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/ProcessTest.java` around lines 42 - 92, The tests in ProcessTest are commented out and must be re-enabled to restore coverage: remove the block comment around the two `@Test` methods (executeShouldExecuteAllStepsSuccessfullyWhenNoErrors and executeShouldSkipAllRemainingStepsWhenFirstStepFails) so the test runner executes them, ensure mocks used (processContext, stepExecutionService, process) are initialized in the test class setup, and run/fix any failures by adjusting expectations that reference process.execute(processContext), processContext.createStepContext(...), and stepExecutionService.executeStep/skipStep to match current implementation; if these tests are obsolete, delete the file instead of leaving them commented.
♻️ Duplicate comments (1)
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/NetworkModificationRestClientTest.java (1)
36-39: LGTM – same optional package-alignment note asFilterRestClientTest.The rename and wiring to
NetworkModificationRestClientare correct. As noted forFilterRestClientTest, consider moving this file to aclienttest sub-package to mirror the production layout.🤖 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/NetworkModificationRestClientTest.java` around lines 36 - 39, Test class NetworkModificationRestClientTest is currently placed in a package that doesn't mirror production client layout; move the test to the matching client test sub-package to match FilterRestClientTest's structure. Update the package declaration for NetworkModificationRestClientTest to the client sub-package and relocate the file accordingly so it mirrors the production package for NetworkModificationRestClient; ensure the `@RestClientTest` annotation and the `@Autowired` NetworkModificationRestClient field remain unchanged.
🧹 Nitpick comments (18)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/client/SecurityAnalysisRestClient.java (1)
20-22: Consider replacing@Servicewith@Component.Now that the class is named
SecurityAnalysisRestClientand lives in theclientpackage,@Serviceis a semantic mismatch.@Serviceis conventionally reserved for business/service-layer beans; an HTTP client wrapper is an infrastructure concern better expressed with@Component.♻️ Suggested change
-import org.springframework.stereotype.Service; +import org.springframework.stereotype.Component; -@Service +@Component public class SecurityAnalysisRestClient {🤖 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/client/SecurityAnalysisRestClient.java` around lines 20 - 22, The class SecurityAnalysisRestClient is an infrastructure HTTP client but is annotated with `@Service`; change the annotation to `@Component` to reflect its role as a component bean. Replace the `@Service` annotation on the SecurityAnalysisRestClient class with `@Component` and update imports to use org.springframework.stereotype.Component (removing or replacing the Service import), leaving the existing LOGGER and class body intact.monitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/processexecution/ProcessType.java (1)
7-7:ProcessTypeplacement inprocessexecutionpackage is semantically imprecise.
ProcessTypeidentifies the kind of process (e.g.,SECURITY_ANALYSIS), which is a concept shared byprocessconfig,processexecution, and result types. Nesting it underprocessexecutionimplies it is execution-specific, yetProcessConfigandSecurityAnalysisConfig(inprocessconfig) both import it directly. A neutral peer-level package such asapi/types/processwould better reflect its cross-cutting role.🤖 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/api/types/processexecution/ProcessType.java` at line 7, Move the ProcessType type out of the processexecution package into a neutral peer package (e.g., org.gridsuite.monitor.commons.api.types.process) so it reflects a cross-cutting concept; update the ProcessType file's package declaration accordingly and adjust all imports/usages (references in ProcessConfig and SecurityAnalysisConfig and any other classes that import org.gridsuite.monitor.commons.api.types.processexecution.ProcessType) to the new package, run a build to catch missing imports, and update any module/exports if your build system requires it.monitor-server/src/main/java/org/gridsuite/monitor/server/client/ReportRestClient.java (1)
26-27: Consider replacing@Servicewith@Component.Same semantic concern as
FilterRestClient: this is an HTTP client, not a business-logic service.♻️ Proposed change
-@Service +@Component public class ReportRestClient {🤖 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/client/ReportRestClient.java` around lines 26 - 27, Replace the Spring stereotype on the HTTP client class ReportRestClient: change the `@Service` annotation to `@Component` to reflect that it is a component (client) rather than a business service; update the import to org.springframework.stereotype.Component and remove the org.springframework.stereotype.Service import if present, and run a quick compile to ensure there are no unused-import warnings (apply the same pattern used for FilterRestClient).monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/client/FilterRestClient.java (1)
26-27: Consider replacing@Servicewith@Component.
@Serviceimplies business-layer logic; REST clients are better annotated with@Componentfor semantic accuracy.♻️ Proposed change
-@Service +@Component public class FilterRestClient {🤖 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/client/FilterRestClient.java` around lines 26 - 27, The FilterRestClient class is currently annotated with `@Service` but should use `@Component` for semantic accuracy; replace the `@Service` annotation on the FilterRestClient class with `@Component` and update imports accordingly (use org.springframework.stereotype.Component and remove org.springframework.stereotype.Service), leaving the class name FilterRestClient and its package unchanged.monitor-server/src/test/java/org/gridsuite/monitor/server/messaging/ConsumerServiceTest.java (1)
56-57: Consider adding a test for thedefaultswitch case (unknown message type).
ConsumerService.consumeMonitorUpdate()has adefault ->branch that logs a warning but is never exercised in the test suite. A simple test passing an unrecognisedmessageTypeheader would close that gap and guard against future regressions if the default handling changes.🤖 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/messaging/ConsumerServiceTest.java` around lines 56 - 57, Add a unit test in ConsumerServiceTest that exercises the default branch of ConsumerService.consumeMonitorUpdate by sending a message whose headers contain an unrecognised "messageType" value; construct the Message (or MockMessage) with that header, invoke consumerService.consumeMonitorUpdate(...) and assert the expected outcome (e.g., that a warning was logged or no handlers were invoked) to verify the default -> path is exercised.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/FilterRestClientTest.java (1)
47-50: Consider moving the test file to aclientsub-package to match the source layout.The class under test (
FilterRestClient) lives inorg.gridsuite.monitor.worker.server.client, but this test remains inorg.gridsuite.monitor.worker.server.services. The same pattern appears inNetworkModificationRestClientTest. Aligning test packages with the production package tree avoids confusion during navigation.🤖 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/FilterRestClientTest.java` around lines 47 - 50, Test classes are located in the wrong package; move FilterRestClientTest (and similarly NetworkModificationRestClientTest) into the client test package that mirrors the production class package to avoid navigation confusion: change the package declaration in FilterRestClientTest to the client package that contains FilterRestClient, relocate the file into the corresponding client test directory, and update any imports or annotations (e.g., `@RestClientTest`, Autowired field filterRestClient) as needed so the test class name and referenced symbol FilterRestClient resolve from the new package.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestClientTest.java (1)
1-1: Test class package doesn't match the relocated production class.The test class remains in
org.gridsuite.monitor.worker.server.services, butSecurityAnalysisRestClienthas been moved toorg.gridsuite.monitor.worker.server.client. The monitor-server counterpart (monitor-server/src/test/java/org/gridsuite/monitor/server/client/SecurityAnalysisRestClientTest.java) was correctly moved to theclienttest package. Consider moving this test toorg.gridsuite.monitor.worker.server.clientfor consistency.Also applies to: 30-30
🤖 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/SecurityAnalysisRestClientTest.java` at line 1, The test class SecurityAnalysisRestClientTest is still declared in package org.gridsuite.monitor.worker.server.services but the production class SecurityAnalysisRestClient has been moved to org.gridsuite.monitor.worker.server.client; move the test to the matching test package by changing its package declaration to org.gridsuite.monitor.worker.server.client (and relocate the file if necessary) so the test package mirrors the production package and any package-private access remains valid.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ReportServiceTest.java (1)
7-7: Test class name and package are inconsistent with the renamed production class.The test class is still named
ReportServiceTestin theservicespackage, but it now testsReportRestClientwhich lives in theclientpackage. The monitor-server counterpart was correctly renamed toReportRestClientTestand moved to theclienttest package. Consider renaming this class toReportRestClientTestand moving it toorg.gridsuite.monitor.worker.server.clientfor consistency.Also applies to: 37-37
🤖 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/ReportServiceTest.java` at line 7, Rename the test class ReportServiceTest to ReportRestClientTest and update its package declaration to org.gridsuite.monitor.worker.server.client so it matches the production class ReportRestClient; update the class name, any imports and references inside the file (e.g., any usages of ReportServiceTest) to ReportRestClientTest and ensure imports reference org.gridsuite.monitor.worker.server.client.ReportRestClient (or the correct client package) so the test and production code are consistent.monitor-server/src/main/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionService.java (1)
144-150: External HTTP calls are made within@Transactionalboundaries.
getReportsandgetResultshold a read-only transaction open while making N sequential HTTP calls to external services (reportRestClient.getReport,resultService.getResult). Under load or with many steps, this can exhaust the DB connection pool. Consider fetching the IDs within the transaction, then making the HTTP calls outside the transaction scope.This is a pre-existing pattern (not introduced by this PR), but the rename to
RestClientmakes the external-call nature more visible, so worth noting.Also applies to: 161-167
🤖 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/processexecution/ProcessExecutionService.java` around lines 144 - 150, getReports (and similarly getResults) currently open a `@Transactional` readOnly transaction while performing N external HTTP calls (reportRestClient.getReport / resultService.getResult), which can exhaust DB connections; change the flow so the DB-access portion runs inside a short transaction that only retrieves IDs (e.g., extract getReportIds and getResultIds calls into a `@Transactional`(readOnly=true) helper or keep those private methods transactional) and make getReports and getResults non-transactional (or remove the external calls from the transactional scope) so that you first call the transactional helper to fetch List<UUID> ids, then iterate over ids to call reportRestClient.getReport / resultService.getResult outside any transaction.monitor-server/src/test/java/org/gridsuite/monitor/server/client/ReportRestClientTest.java (1)
44-45: Redundant inline initializer on@Autowiredfield.
ObjectMapper objectMapper = new ObjectMapper()is immediately overwritten by Spring's@Autowiredinjection. The= new ObjectMapper()is dead code and may confuse readers into thinking a non-Spring instance is used.Suggested fix
`@Autowired` - ObjectMapper objectMapper = new ObjectMapper(); + ObjectMapper objectMapper;🤖 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/client/ReportRestClientTest.java` around lines 44 - 45, In ReportRestClientTest remove the redundant inline initializer on the `@Autowired` ObjectMapper field (ObjectMapper objectMapper = new ObjectMapper()) so Spring's injected instance is used; locate the field declaration named objectMapper in class ReportRestClientTest, delete the "= new ObjectMapper()" part and keep only the `@Autowired` ObjectMapper objectMapper declaration.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/messaging/NotificationService.java (1)
50-72: Add@Overrideto methods implementingNotificator.
updateExecutionStatus,updateStepStatus, andupdateStepsStatusesimplement theNotificatorcontract but are missing@Override. Without it, a future signature change inNotificatorwill silently leave these implementations as dead code rather than producing a compile error.♻️ Proposed additions
+ `@Override` public void updateExecutionStatus(UUID executionId, ProcessExecutionStatusUpdate processExecutionStatusUpdate) {+ `@Override` public void updateStepStatus(UUID executionId, ProcessExecutionStep processExecutionStep) {+ `@Override` public void updateStepsStatuses(UUID executionId, List<ProcessExecutionStep> processExecutionSteps) {🤖 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/messaging/NotificationService.java` around lines 50 - 72, The three methods updateExecutionStatus, updateStepStatus, and updateStepsStatuses implement the Notificator interface but lack the `@Override` annotation; add `@Override` immediately above each of these method declarations in NotificationService (updateExecutionStatus, updateStepStatus, updateStepsStatuses) so the compiler will catch any future signature mismatches with Notificator.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessStepExecutionContextTest.java (1)
58-69: Duplicate assertions withinshouldInitializeCorrectly.Lines 60+69 both assert
stepContext.getNetwork()equals the samenetworkmock return value, and lines 61+67 both assertstepContext.getProcessExecutionId()equals the sameexecutionId. Either the mock-call variants (60, 61) or the direct-variable variants (67, 69) can be removed.♻️ Proposed cleanup
assertThat(stepContext.getNetwork()).isEqualTo(processContext.getNetwork()); assertThat(stepContext.getProcessExecutionId()).isEqualTo(processContext.getExecutionId()); assertThat(stepContext.getConfig()).isEqualTo(config); assertThat(stepContext.getProcessStepType()).isEqualTo(stepType); assertThat(stepContext.getStartedAt()).isBeforeOrEqualTo(Instant.now()); assertThat(stepContext.getReportInfos()).isNotNull(); assertThat(stepContext.getReportInfos().reportNode().getMessageKey()).isEqualTo("monitor.worker.server.stepType"); - assertThat(stepContext.getProcessExecutionId()).isEqualTo(executionId); assertThat(stepContext.getCaseUuid()).isEqualTo(caseUuid); - assertThat(stepContext.getNetwork()).isEqualTo(network);🤖 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/ProcessStepExecutionContextTest.java` around lines 58 - 69, Test contains duplicate assertions for network and processExecutionId in shouldInitializeCorrectly: remove the redundant assertions so each property is asserted only once; specifically keep either the assertions that compare stepContext.getNetwork() to processContext.getNetwork() and stepContext.getProcessExecutionId() to processContext.getExecutionId(), or keep the ones that compare to the local variables network and executionId, and delete the other two duplicate assertions to avoid redundancy in ProcessStepExecutionContextTest.shouldInitializeCorrectly.monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.java (2)
41-48:flatMap+Optional.ofingetProcessConfigis equivalent tomap.Using
flatMaprequires the lambda to return anOptional, soOptional.of(...)is added. Replacing withmapeliminates the unnecessary wrapping.♻️ Proposed simplification
- return processConfigRepository.findById(processConfigUuid).flatMap(entity -> switch (entity) { - case SecurityAnalysisConfigEntity sae -> - Optional.of((ProcessConfig) SecurityAnalysisConfigMapper.toDto(sae)); - default -> throw new IllegalArgumentException("Unsupported entity type: " + entity.getType()); - }); + return processConfigRepository.findById(processConfigUuid).map(entity -> switch (entity) { + case SecurityAnalysisConfigEntity sae -> (ProcessConfig) SecurityAnalysisConfigMapper.toDto(sae); + default -> throw new IllegalArgumentException("Unsupported entity type: " + entity.getType()); + });🤖 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/processconfig/ProcessConfigService.java` around lines 41 - 48, Replace the unnecessary use of flatMap + Optional.of in ProcessConfigService.getProcessConfig: change processConfigRepository.findById(...).flatMap(...) to .map(...) and return the mapped DTO directly from the lambda (handle SecurityAnalysisConfigEntity by calling SecurityAnalysisConfigMapper.toDto(sae) cast to ProcessConfig and keep the default branch throwing IllegalArgumentException referencing entity.getType()); this removes the redundant Optional wrapping while preserving behavior.
51-65:updateProcessConfigswitches on the DTO but casts the entity — prefer switching onentityinstead.The switch on
processConfig(Line 57) forces the explicit cast(SecurityAnalysisConfigEntity) entityon Line 59. The type guard on Line 54 prevents a runtimeClassCastExceptiontoday, but this pattern must be kept in sync manually as new subtypes are added. Switching onentityvia pattern matching is more type-safe and consistent with howgetProcessConfighandles dispatch.♻️ Proposed refactor
- switch (processConfig) { - case SecurityAnalysisConfig sac -> - SecurityAnalysisConfigMapper.update((SecurityAnalysisConfigEntity) entity, sac); - default -> throw new IllegalArgumentException("Unsupported process config type: " + processConfig.processType()); - } + switch (entity) { + case SecurityAnalysisConfigEntity sae -> + SecurityAnalysisConfigMapper.update(sae, (SecurityAnalysisConfig) processConfig); + default -> throw new IllegalArgumentException("Unsupported entity type: " + entity.getType()); + }🤖 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/processconfig/ProcessConfigService.java` around lines 51 - 65, In updateProcessConfig in ProcessConfigService, replace the switch on the DTO with a switch on the loaded entity using pattern matching so you don't need to cast the entity; e.g. switch(entity) { case SecurityAnalysisConfigEntity sae -> SecurityAnalysisConfigMapper.update(sae, (SecurityAnalysisConfig) processConfig); default -> throw new IllegalArgumentException(...); } — keep the prior type-mismatch guard using entity.getType(), remove the explicit (SecurityAnalysisConfigEntity) cast, and ensure SecurityAnalysisConfigMapper.update is called with the pattern-bound entity instance and the appropriately cast DTO.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/messaging/ConsumerService.java (1)
22-26:ConsumerServicename is misleading for a@Configurationclass.A class annotated
@Configurationis a bean-factory, not a service.ConsumerConfiguration(orConsumerServiceConfiguration) would be more conventional and avoids confusion with@Service-annotated components.🤖 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/messaging/ConsumerService.java` around lines 22 - 26, The class annotated with `@Configuration` is misnamed ConsumerService; rename it to a configuration-appropriate name (e.g., ConsumerConfiguration or ConsumerServiceConfiguration) to avoid confusion with `@Service` beans: change the class declaration from "public class ConsumerService" to "public class ConsumerConfiguration" (or ConsumerServiceConfiguration), update the file name accordingly, and adjust any references to the type throughout the codebase; keep the existing annotations (`@Configuration`, `@RequiredArgsConstructor`) and the injected field "private final ProcessExecutor executionService" intact so bean wiring and constructor injection remain unchanged.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/process/AbstractProcess.java (2)
53-53: Please track the TODO with a concrete follow-up.If you want, I can help draft a minimal failure policy (e.g., metrics + error propagation).
🤖 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/core/process/AbstractProcess.java` at line 53, Replace the generic "// TODO better error handling" in AbstractProcess (class AbstractProcess) with a concrete follow-up: add a TODO that references a created issue/ticket ID and a short plan (e.g., "TRACK-123: implement FailurePolicy with metrics emission and error propagation"), and add a one-line comment describing the intended failure policy (use a FailurePolicy interface, emit metrics via MetricsReporter.recordFailure(String processId, Throwable), and propagate or wrap exceptions from execute()/runProcess()). Also create the referenced issue that outlines the minimal work (define FailurePolicy, hook MetricsReporter, update AbstractProcess to call policy.onFailure(...)), so the code comment points to an actionable task rather than a vague TODO.
52-54: Log the exception to retain stack traces.Current log only captures the message, which makes postmortems harder.
Suggested update
- LOGGER.error("Execution id: {} - Step failed: {} - {}", context.getExecutionId(), step.getType(), e.getMessage()); + LOGGER.error("Execution id: {} - Step failed: {}", context.getExecutionId(), step.getType(), e);🤖 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/core/process/AbstractProcess.java` around lines 52 - 54, The current AbstractProcess.onStepFailure logs only e.getMessage(), losing the stack trace; update the LOGGER.error call in onStepFailure (class AbstractProcess, method onStepFailure(ProcessExecutionContext<C>, ProcessStep<C>, Exception)) to pass the Exception itself as the final argument to the logger so SLF4J will record the throwable and its stacktrace (i.e., keep the same formatted message with execution id and step type, but supply the Exception as the last parameter so the stack trace is preserved).monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/orchestrator/ProcessExecutionService.java (1)
65-70: Avoid calling defineSteps twice.If defineSteps creates new step instances or IDs, SCHEDULED statuses can drift from executed steps. Reuse the same list for scheduling and execution.
♻️ Suggested refactor
- try { - List<ProcessStep<T>> steps = process.defineSteps(); + try { + List<ProcessStep<T>> steps = process.defineSteps(); notificationService.updateStepsStatuses(context.getExecutionId(), IntStream.range(0, steps.size()) .mapToObj(i -> new ProcessExecutionStep(steps.get(i).getId(), steps.get(i).getType().getName(), i, StepStatus.SCHEDULED, null, null, null, null, null)) .toList()); } catch (Exception e) { updateExecutionStatus(context.getExecutionId(), context.getExecutionEnvName(), ProcessStatus.FAILED); throw e; } @@ - executeSteps(process, context); + executeSteps(process, context, steps); @@ - public <T extends ProcessConfig> void executeSteps(Process<T> process, ProcessExecutionContext<T> context) { - List<ProcessStep<T>> steps = process.defineSteps(); + public <T extends ProcessConfig> void executeSteps(Process<T> process, ProcessExecutionContext<T> context, List<ProcessStep<T>> steps) {Also applies to: 86-88
🤖 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/orchestrator/ProcessExecutionService.java` around lines 65 - 70, The current code calls process.defineSteps() more than once which may create different step instances/IDs; change ProcessExecutionService to call process.defineSteps() a single time, store the returned List<ProcessStep<T>> in a local variable (e.g., steps) and reuse that same list when building the SCHEDULED ProcessExecutionStep objects for notificationService.updateStepsStatuses and later when iterating/executing steps (the second occurrence around the block handling lines ~86-88), ensuring ProcessExecutionStep construction (new ProcessExecutionStep(...)) uses steps.get(i).getId() and steps.get(i).getType().getName() from the same list to avoid drift between scheduled and executed steps.
🤖 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/test/java/org/gridsuite/monitor/server/client/SecurityAnalysisRestClientTest.java`:
- Line 9: Remove the redundant same-package import of SecurityAnalysisRestClient
from the test — the import statement "import
org.gridsuite.monitor.server.client.SecurityAnalysisRestClient;" should be
deleted from SecurityAnalysisRestClientTest; also scan the test for any other
unnecessary same-package or unused imports and remove them so the CI no longer
fails.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/process/ProcessStepType.java`:
- Line 10: The Javadoc for ProcessStepType references ProcessStep using the
short name which causes unresolved-link warnings; update the Javadoc tag to use
the fully-qualified class name {`@link`
org.gridsuite.monitor.worker.server.core.ProcessStep} in the ProcessStepType
interface JavaDoc so the Javadoc tool can resolve the link.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/orchestrator/ProcessExecutionService.java`:
- Around line 99-105: The executeSteps loop currently swallows exceptions (try
around stepExecutor.executeStep) and only sets skipRemaining = true after
calling process.onStepFailure, which allows executeProcess to still mark the
overall ProcessStatus as COMPLETED; modify executeSteps to record that a failure
occurred (e.g., set a boolean failed flag when catching Exception in the catch
block after calling process.onStepFailure) and either rethrow the exception or
return a failure indicator to executeProcess so executeProcess can set
ProcessStatus.FAILED instead of COMPLETED; update references: executeSteps,
executeProcess, stepExecutor.executeStep, process.onStepFailure, skipRemaining
and ensure the failure flag is checked by executeProcess to propagate failure
status.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionServiceTest.java`:
- Around line 90-95: The disabled failure-path test should be re-enabled and
updated to simulate a step execution failure by injecting a mocked StepExecutor
into ProcessExecutionService (instead of using the real StepExecutionService);
configure the mock StepExecutor.execute(...) to throw an exception for the
targeted step (e.g., when executing
loadNetworkStep/applyModificationsStep/runComputationStep) so
ProcessExecutionService transitions to FAILED, then assert the process status is
FAILED and restore/remove the commented-out stubs/verifications; update related
tests at the other ranges similarly (use mock/spy StepExecutor or spy
ProcessExecutionService to force failure and assert FAILED).
---
Outside diff comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/client/ReportRestClient.java`:
- Around line 50-53: The request currently sets Content-Type on a GET which is
incorrect; in ReportRestClient where HttpHeaders headers are created and used in
restTemplate.exchange, replace
headers.setContentType(MediaType.APPLICATION_JSON) with setting the Accept
header (e.g., headers.setAccept(...) or headers.add("Accept",
"application/json")) so the client declares the expected response media type;
ensure no Content-Type is sent for the bodyless GET.
- Around line 46-53: The getReport method currently calls
restTemplate.exchange(...).getBody() which can be null for 204/205 responses and
leads to silent NPEs; update ReportRestClient.getReport to capture the
ResponseEntity<ReportPage> from restTemplate.exchange (instead of calling
getBody() inline), check response.getStatusCode() and response.getBody() for
null, and either return a safe empty ReportPage or throw a clear exception
(e.g., IllegalStateException) when body is missing; reference
restTemplate.exchange(...) and getReport to locate the change and ensure callers
never receive a null ReportPage.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/messaging/NotificationService.java`:
- Around line 26-31: sendProcessRunMessage currently ignores the boolean return
from publisher.send, so delivery failures are undetected; modify
sendProcessRunMessage to capture the boolean result from
publisher.send(bindingName, message) and if it is false, call the logger to emit
a warning including bindingName, executionId (or caseUuid) and a short
description of the failed ProcessRunMessage (use ProcessRunMessage's identifying
fields) so failures are visible; optionally use the StreamBridge overload that
accepts a MimeType if content-type fidelity is required.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/MonitorIntegrationTest.java`:
- Around line 160-168: Replace uses of
executionRepository.findById(...).orElse(null) in MonitorIntegrationTest (the
re-fetch at the second block where variable execution is used) with
executionRepository.findById(...).orElseThrow() so the test fails with a clear
NoSuchElementException instead of NPE; update the second retrieval that assigns
to execution (the one followed by assertions on execution.getSteps()) to use
orElseThrow() to ensure a meaningful failure and avoid null access when
asserting on execution and its steps.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/provider/SecurityAnalysisResultProviderTest.java`:
- Around line 37-47: Rename the stale test method names that still refer to
"SecurityAnalysisService": change
getResultShouldDelegateToSecurityAnalysisService to
getResultShouldDelegateToSecurityAnalysisRestClient and change
deleteResultShouldDelegateToSecurityAnalysisService (the test around lines
51-60) to deleteResultShouldDelegateToSecurityAnalysisRestClient so they reflect
the actual collaborator SecurityAnalysisRestClient and keep test names
consistent with the class under test (provider.getResult /
provider.deleteResult).
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/client/FilterRestClient.java`:
- Line 48: The call in FilterRestClient.getFilters() uses
restTemplate.exchange(...).getBody() which may return null for 204/205 or empty
responses; change the method to guard against null and return an empty
List<AbstractFilter> instead of propagating a null (e.g., wrap the result with
Optional.ofNullable(...).orElse(Collections.emptyList()) or an explicit null
check), keeping the restTemplate.exchange(...) call and the
ParameterizedTypeReference unchanged so callers never receive a null and avoid
downstream NPEs.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessTest.java`:
- Around line 42-92: The tests in ProcessTest are commented out and must be
re-enabled to restore coverage: remove the block comment around the two `@Test`
methods (executeShouldExecuteAllStepsSuccessfullyWhenNoErrors and
executeShouldSkipAllRemainingStepsWhenFirstStepFails) so the test runner
executes them, ensure mocks used (processContext, stepExecutionService, process)
are initialized in the test class setup, and run/fix any failures by adjusting
expectations that reference process.execute(processContext),
processContext.createStepContext(...), and
stepExecutionService.executeStep/skipStep to match current implementation; if
these tests are obsolete, delete the file instead of leaving them commented.
---
Duplicate comments:
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/NetworkModificationRestClientTest.java`:
- Around line 36-39: Test class NetworkModificationRestClientTest is currently
placed in a package that doesn't mirror production client layout; move the test
to the matching client test sub-package to match FilterRestClientTest's
structure. Update the package declaration for NetworkModificationRestClientTest
to the client sub-package and relocate the file accordingly so it mirrors the
production package for NetworkModificationRestClient; ensure the `@RestClientTest`
annotation and the `@Autowired` NetworkModificationRestClient field remain
unchanged.
---
Nitpick comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/processexecution/ProcessType.java`:
- Line 7: Move the ProcessType type out of the processexecution package into a
neutral peer package (e.g., org.gridsuite.monitor.commons.api.types.process) so
it reflects a cross-cutting concept; update the ProcessType file's package
declaration accordingly and adjust all imports/usages (references in
ProcessConfig and SecurityAnalysisConfig and any other classes that import
org.gridsuite.monitor.commons.api.types.processexecution.ProcessType) to the new
package, run a build to catch missing imports, and update any module/exports if
your build system requires it.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/client/ReportRestClient.java`:
- Around line 26-27: Replace the Spring stereotype on the HTTP client class
ReportRestClient: change the `@Service` annotation to `@Component` to reflect that
it is a component (client) rather than a business service; update the import to
org.springframework.stereotype.Component and remove the
org.springframework.stereotype.Service import if present, and run a quick
compile to ensure there are no unused-import warnings (apply the same pattern
used for FilterRestClient).
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.java`:
- Around line 41-48: Replace the unnecessary use of flatMap + Optional.of in
ProcessConfigService.getProcessConfig: change
processConfigRepository.findById(...).flatMap(...) to .map(...) and return the
mapped DTO directly from the lambda (handle SecurityAnalysisConfigEntity by
calling SecurityAnalysisConfigMapper.toDto(sae) cast to ProcessConfig and keep
the default branch throwing IllegalArgumentException referencing
entity.getType()); this removes the redundant Optional wrapping while preserving
behavior.
- Around line 51-65: In updateProcessConfig in ProcessConfigService, replace the
switch on the DTO with a switch on the loaded entity using pattern matching so
you don't need to cast the entity; e.g. switch(entity) { case
SecurityAnalysisConfigEntity sae -> SecurityAnalysisConfigMapper.update(sae,
(SecurityAnalysisConfig) processConfig); default -> throw new
IllegalArgumentException(...); } — keep the prior type-mismatch guard using
entity.getType(), remove the explicit (SecurityAnalysisConfigEntity) cast, and
ensure SecurityAnalysisConfigMapper.update is called with the pattern-bound
entity instance and the appropriately cast DTO.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionService.java`:
- Around line 144-150: getReports (and similarly getResults) currently open a
`@Transactional` readOnly transaction while performing N external HTTP calls
(reportRestClient.getReport / resultService.getResult), which can exhaust DB
connections; change the flow so the DB-access portion runs inside a short
transaction that only retrieves IDs (e.g., extract getReportIds and getResultIds
calls into a `@Transactional`(readOnly=true) helper or keep those private methods
transactional) and make getReports and getResults non-transactional (or remove
the external calls from the transactional scope) so that you first call the
transactional helper to fetch List<UUID> ids, then iterate over ids to call
reportRestClient.getReport / resultService.getResult outside any transaction.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/client/ReportRestClientTest.java`:
- Around line 44-45: In ReportRestClientTest remove the redundant inline
initializer on the `@Autowired` ObjectMapper field (ObjectMapper objectMapper =
new ObjectMapper()) so Spring's injected instance is used; locate the field
declaration named objectMapper in class ReportRestClientTest, delete the "= new
ObjectMapper()" part and keep only the `@Autowired` ObjectMapper objectMapper
declaration.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/messaging/ConsumerServiceTest.java`:
- Around line 56-57: Add a unit test in ConsumerServiceTest that exercises the
default branch of ConsumerService.consumeMonitorUpdate by sending a message
whose headers contain an unrecognised "messageType" value; construct the Message
(or MockMessage) with that header, invoke
consumerService.consumeMonitorUpdate(...) and assert the expected outcome (e.g.,
that a warning was logged or no handlers were invoked) to verify the default ->
path is exercised.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/client/FilterRestClient.java`:
- Around line 26-27: The FilterRestClient class is currently annotated with
`@Service` but should use `@Component` for semantic accuracy; replace the `@Service`
annotation on the FilterRestClient class with `@Component` and update imports
accordingly (use org.springframework.stereotype.Component and remove
org.springframework.stereotype.Service), leaving the class name FilterRestClient
and its package unchanged.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/client/SecurityAnalysisRestClient.java`:
- Around line 20-22: The class SecurityAnalysisRestClient is an infrastructure
HTTP client but is annotated with `@Service`; change the annotation to `@Component`
to reflect its role as a component bean. Replace the `@Service` annotation on the
SecurityAnalysisRestClient class with `@Component` and update imports to use
org.springframework.stereotype.Component (removing or replacing the Service
import), leaving the existing LOGGER and class body intact.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/process/AbstractProcess.java`:
- Line 53: Replace the generic "// TODO better error handling" in
AbstractProcess (class AbstractProcess) with a concrete follow-up: add a TODO
that references a created issue/ticket ID and a short plan (e.g., "TRACK-123:
implement FailurePolicy with metrics emission and error propagation"), and add a
one-line comment describing the intended failure policy (use a FailurePolicy
interface, emit metrics via MetricsReporter.recordFailure(String processId,
Throwable), and propagate or wrap exceptions from execute()/runProcess()). Also
create the referenced issue that outlines the minimal work (define
FailurePolicy, hook MetricsReporter, update AbstractProcess to call
policy.onFailure(...)), so the code comment points to an actionable task rather
than a vague TODO.
- Around line 52-54: The current AbstractProcess.onStepFailure logs only
e.getMessage(), losing the stack trace; update the LOGGER.error call in
onStepFailure (class AbstractProcess, method
onStepFailure(ProcessExecutionContext<C>, ProcessStep<C>, Exception)) to pass
the Exception itself as the final argument to the logger so SLF4J will record
the throwable and its stacktrace (i.e., keep the same formatted message with
execution id and step type, but supply the Exception as the last parameter so
the stack trace is preserved).
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/messaging/ConsumerService.java`:
- Around line 22-26: The class annotated with `@Configuration` is misnamed
ConsumerService; rename it to a configuration-appropriate name (e.g.,
ConsumerConfiguration or ConsumerServiceConfiguration) to avoid confusion with
`@Service` beans: change the class declaration from "public class ConsumerService"
to "public class ConsumerConfiguration" (or ConsumerServiceConfiguration),
update the file name accordingly, and adjust any references to the type
throughout the codebase; keep the existing annotations (`@Configuration`,
`@RequiredArgsConstructor`) and the injected field "private final ProcessExecutor
executionService" intact so bean wiring and constructor injection remain
unchanged.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/messaging/NotificationService.java`:
- Around line 50-72: The three methods updateExecutionStatus, updateStepStatus,
and updateStepsStatuses implement the Notificator interface but lack the
`@Override` annotation; add `@Override` immediately above each of these method
declarations in NotificationService (updateExecutionStatus, updateStepStatus,
updateStepsStatuses) so the compiler will catch any future signature mismatches
with Notificator.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/orchestrator/ProcessExecutionService.java`:
- Around line 65-70: The current code calls process.defineSteps() more than once
which may create different step instances/IDs; change ProcessExecutionService to
call process.defineSteps() a single time, store the returned
List<ProcessStep<T>> in a local variable (e.g., steps) and reuse that same list
when building the SCHEDULED ProcessExecutionStep objects for
notificationService.updateStepsStatuses and later when iterating/executing steps
(the second occurrence around the block handling lines ~86-88), ensuring
ProcessExecutionStep construction (new ProcessExecutionStep(...)) uses
steps.get(i).getId() and steps.get(i).getType().getName() from the same list to
avoid drift between scheduled and executed steps.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessStepExecutionContextTest.java`:
- Around line 58-69: Test contains duplicate assertions for network and
processExecutionId in shouldInitializeCorrectly: remove the redundant assertions
so each property is asserted only once; specifically keep either the assertions
that compare stepContext.getNetwork() to processContext.getNetwork() and
stepContext.getProcessExecutionId() to processContext.getExecutionId(), or keep
the ones that compare to the local variables network and executionId, and delete
the other two duplicate assertions to avoid redundancy in
ProcessStepExecutionContextTest.shouldInitializeCorrectly.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/FilterRestClientTest.java`:
- Around line 47-50: Test classes are located in the wrong package; move
FilterRestClientTest (and similarly NetworkModificationRestClientTest) into the
client test package that mirrors the production class package to avoid
navigation confusion: change the package declaration in FilterRestClientTest to
the client package that contains FilterRestClient, relocate the file into the
corresponding client test directory, and update any imports or annotations
(e.g., `@RestClientTest`, Autowired field filterRestClient) as needed so the test
class name and referenced symbol FilterRestClient resolve from the new package.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ReportServiceTest.java`:
- Line 7: Rename the test class ReportServiceTest to ReportRestClientTest and
update its package declaration to org.gridsuite.monitor.worker.server.client so
it matches the production class ReportRestClient; update the class name, any
imports and references inside the file (e.g., any usages of ReportServiceTest)
to ReportRestClientTest and ensure imports reference
org.gridsuite.monitor.worker.server.client.ReportRestClient (or the correct
client package) so the test and production code are consistent.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestClientTest.java`:
- Line 1: The test class SecurityAnalysisRestClientTest is still declared in
package org.gridsuite.monitor.worker.server.services but the production class
SecurityAnalysisRestClient has been moved to
org.gridsuite.monitor.worker.server.client; move the test to the matching test
package by changing its package declaration to
org.gridsuite.monitor.worker.server.client (and relocate the file if necessary)
so the test package mirrors the production package and any package-private
access remains valid.
...server/src/test/java/org/gridsuite/monitor/server/client/SecurityAnalysisRestClientTest.java
Outdated
Show resolved
Hide resolved
| package org.gridsuite.monitor.worker.server.core.process; | ||
|
|
||
| /** | ||
| * Functional classification of a {@link ProcessStep}. |
There was a problem hiding this comment.
Javadoc {@link ProcessStep} will produce an unresolved-link warning.
ProcessStep lives in org.gridsuite.monitor.worker.server.core, while this interface is in org.gridsuite.monitor.worker.server.core.process. Without an import (not possible in an interface with no body imports driving compilation), the Javadoc toolchain cannot resolve the short name. Use the fully-qualified form:
📝 Proposed fix
-* Functional classification of a {`@link` ProcessStep}.
+* Functional classification of a {`@link` org.gridsuite.monitor.worker.server.core.ProcessStep}.📝 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.
Suggested change
| * Functional classification of a {@link ProcessStep}. | |
| * Functional classification of a {`@link` org.gridsuite.monitor.worker.server.core.ProcessStep}. |
🤖 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/core/process/ProcessStepType.java`
at line 10, The Javadoc for ProcessStepType references ProcessStep using the
short name which causes unresolved-link warnings; update the Javadoc tag to use
the fully-qualified class name {`@link`
org.gridsuite.monitor.worker.server.core.ProcessStep} in the ProcessStepType
interface JavaDoc so the Javadoc tool can resolve the link.
.../src/main/java/org/gridsuite/monitor/worker/server/orchestrator/ProcessExecutionService.java
Show resolved
Hide resolved
.../test/java/org/gridsuite/monitor/worker/server/orchestrator/ProcessExecutionServiceTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Contributor
|
Some naming rules for packages :
|
Contributor
|
common will be only used for shared types ? |
antoinebhs
reviewed
Feb 26, 2026
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
| package org.gridsuite.monitor.commons; | ||
| package org.gridsuite.monitor.commons.api.types.processexecution; |
Contributor
There was a problem hiding this comment.
this is also for messaging?
Contributor
Author
There was a problem hiding this comment.
When a type is placed in common or core, the objective is to make it directly shareable across different components. However, when organizing types into domain-specific subpackages, we should classify them based on the domain they originate from and logically belong to.
antoinebhs
reviewed
Feb 26, 2026
| @@ -6,8 +6,8 @@ | |||
| */ | |||
| package org.gridsuite.monitor.server.mapper; | |||
|
|
|||
| import org.gridsuite.monitor.server.dto.ProcessExecution; | |||
| import org.gridsuite.monitor.server.entities.ProcessExecutionEntity; | |||
| import org.gridsuite.monitor.server.dto.processexecution.ProcessExecution; | |||
Contributor
There was a problem hiding this comment.
same subfolders for mapper?
Contributor
Author
There was a problem hiding this comment.
I will create subfolders ans renaming the parent folder as plurial 'mappers'
antoinebhs
reviewed
Feb 26, 2026
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
| package org.gridsuite.monitor.worker.server.services; | ||
| package org.gridsuite.monitor.worker.server.client; |
Contributor
There was a problem hiding this comment.
you need to be consistent between plural/singular: client or clients//service or services
Contributor
Author
There was a problem hiding this comment.
YES, to rename as 'clients' since every classes inside this package are typed to communicate to outside world
antoinebhs
reviewed
Feb 26, 2026
| @@ -43,10 +44,10 @@ | |||
| /** | |||
| * @author Franck Lecuyer <franck.lecuyer at rte-france.com> | |||
| */ | |||
| @RestClientTest(FilterRestService.class) | |||
| class FilterRestServiceTest { | |||
| @RestClientTest(FilterRestClient.class) | |||
Contributor
There was a problem hiding this comment.
test packages must mirror src organisation
antoinebhs
reviewed
Feb 26, 2026
| public class StepExecutionService<C extends ProcessConfig> { | ||
| public class StepExecutionService implements StepExecutor { | ||
|
|
||
| private final NotificationService notificationService; |
antoinebhs
reviewed
Feb 26, 2026
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ | ||
| package org.gridsuite.monitor.worker.server.services; | ||
| package org.gridsuite.monitor.worker.server.processes.orchestrator; |
Contributor
There was a problem hiding this comment.
would be better to have a top level orchestrator instead of a subpackage of processes?
Contributor
Author
There was a problem hiding this comment.
YES I agree with the idea of separating this top-level service from the process definition components.
antoinebhs
reviewed
Feb 26, 2026
| @@ -0,0 +1,33 @@ | |||
| package org.gridsuite.monitor.worker.server.core.orchestrator; | |||
Contributor
There was a problem hiding this comment.
can you explain what's the use of the interfaces you've added and why it's better than the existing implementations
To merge this quicker I would suggest to do this in a separate PR.
Contributor
Author
There was a problem hiding this comment.
When splitting the system into the messaging and orchestrator subdomains, we introduced ProcessExecutor under core/orchestrator and Notificator under core/messaging.
These subdomains should avoid cyclic dependencies to ensure loose coupling. However, in the messaging domain, the consumer service invokes the execution service, and once execution is completed, it needs to notify the outside world through the notification service. This creates a circular dependency: messaging ↔ orchestrator.
To address this issue, we should extract the tightly coupled parts into small, stable contracts (Java interfaces are a good candidate). These contracts can then be shared between subdomains (similar to COBRA-style components with stub and skeleton). If both subdomains are packaged within the same JAR (e.g., worker-server.jar), the contracts can simply be moved into a shared package, such as a reserved core package. In that case, the dependencies become:
messaging → core
orchestrator → core
To assemble and wire together all components from the subdomains, we need a composition mechanism, such as a dependency injection framework like Spring (or at least a dedicated configuration package at the root of a Spring-based project).
Other interfaces, such as ResultProvider and StepExecutor, were also introduced. Although they were not created for exactly the same reason (i.e. avoid cyclic dependencies), they follow the same principle: enabling component composition through well-defined, explicit, stable contracts to ensure loose coupling, clear boundaries and extensible semantics (plugin-able).
antoinebhs
reviewed
Feb 26, 2026
Contributor
antoinebhs
left a comment
antoinebhs
left a comment
There was a problem hiding this comment.
I would suggest to:
- fix the files in test not at the same spot as files in src
- split interfaces add in the worker in a separate pr to review the documentations etc
- validate this new organization with the team
Contributor
@khouadrired At some point it may contain some part of the worker core. Concerning rest client do you have examples of duplicated ones? |
khouadrired
reviewed
Feb 27, 2026
| @@ -6,8 +6,8 @@ | |||
| */ | |||
Contributor
There was a problem hiding this comment.
we should use singular
Contributor
@antoinebhs not exact duplicate but they serve the same thing , for example the reportRestClient and SecurityAnalysisService , they have same setup just different calls. |
Contributor
|
Add something in the ReadMe to describe the organization choices you made |
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Contributor
|
the best practice is to use singular nouns for package names not plural, could you you do that ? |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/context/ProcessExecutionContext.java (1)
39-41:⚠️ Potential issue | 🟠 MajorChange
ProcessStep<? super C>toProcessStep<C>for consistency with the orchestration pattern.The contravariant bound is inconsistent with all other method signatures in the pattern (
Process.onStepFailure,AbstractProcess.defineSteps,StepExecutor.executeStep, etc.), which use the invariantProcessStep<C>. Since this method only callsstep.getType()andstep.getId()with no type-dependent logic, the contravariant bound provides no practical benefit and should be removed to maintain consistency.🤖 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/core/context/ProcessExecutionContext.java` around lines 39 - 41, Change the method signature of createStepContext to use the invariant type parameter ProcessStep<C> instead of the contravariant ProcessStep<? super C> for consistency with the orchestration pattern; update the declaration in ProcessExecutionContext.createStepContext to accept ProcessStep<C> and leave the body returning new ProcessStepExecutionContext<>(this, step.getType(), step.getId(), stepOrder) unchanged so it matches other APIs like Process.onStepFailure, AbstractProcess.defineSteps and StepExecutor.executeStep.monitor-server/src/main/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionService.java (1)
223-240:⚠️ Potential issue | 🟠 Major
deleteExecutionhas partial-failure risk with remote cleanup calls.This method mixes DB mutation and remote deletions (
resultService/reportRestClient) in one transactional path. If a remote delete fails mid-flow, states can diverge across systems.Prefer after-commit or queued cleanup (with retries/idempotency) to make deletion resilient.
🤖 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/processexecution/ProcessExecutionService.java` around lines 223 - 240, The deleteExecution method currently performs remote deletions (resultService.deleteResult and reportRestClient.deleteReport) inline with the DB deletion (executionRepository.deleteById), risking partial failure and state divergence; change the flow so the DB deletion happens inside the transaction and the remote cleanup is executed asynchronously after commit (e.g., publish a CleanupExecutionEvent or register a TransactionSynchronization callback / use `@TransactionalEventListener`) which enqueues or retries idempotent deletes for resultService.deleteResult and reportRestClient.deleteReport; ensure the cleanup handlers are idempotent and include retry/backoff logic so remote failures do not leave systems inconsistent.
♻️ Duplicate comments (2)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/orchestrator/ProcessExecutionService.java (1)
88-92:⚠️ Potential issue | 🟠 MajorStep failures can still produce
COMPLETEDexecution status.Line 91 always sends
COMPLETED, while exceptions are swallowed in Lines 107-112. This can mark failed runs as successful.🐛 Proposed fix
private <T extends ProcessConfig> void executeSteps(Process<T> process, ProcessExecutionContext<T> context) { updateExecutionStatus(context.getExecutionId(), context.getExecutionEnvName(), ProcessStatus.RUNNING); doExecuteSteps(process, context); updateExecutionStatus(context.getExecutionId(), context.getExecutionEnvName(), ProcessStatus.COMPLETED); } private <T extends ProcessConfig> void doExecuteSteps(Process<T> process, ProcessExecutionContext<T> context) { List<ProcessStep<T>> steps = process.getSteps(); boolean skipRemaining = false; + Exception firstFailure = null; for (int i = 0; i < steps.size(); i++) { ProcessStep<T> step = steps.get(i); ProcessStepExecutionContext<T> stepContext = context.createStepContext(step, i); if (skipRemaining) { stepExecutor.skipStep(stepContext, step); continue; } try { stepExecutor.executeStep(stepContext, step); } catch (Exception e) { process.onStepFailure(context, step, e); skipRemaining = true; + if (firstFailure == null) { + firstFailure = e; + } } } + + if (firstFailure != null) { + throw new RuntimeException("Process step failed", firstFailure); + } }Also applies to: 107-113
🤖 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/orchestrator/ProcessExecutionService.java` around lines 88 - 92, The current executeSteps method sets ProcessStatus.COMPLETED unconditionally after calling doExecuteSteps, which combined with swallowed exceptions later causes failed runs to be marked successful; update executeSteps to wrap doExecuteSteps(process, context) in a try/catch that on success updates to ProcessStatus.COMPLETED but on any Exception updates the execution status to a failure state (e.g., ProcessStatus.FAILED) via updateExecutionStatus(context.getExecutionId(), context.getExecutionEnvName(), ProcessStatus.FAILED) and then rethrow or propagate the exception so callers can handle it; ensure the existing exception handling code around doExecuteSteps (the block referenced at lines 107-112) is removed or adjusted so it no longer swallows exceptions and relies on this explicit status update in executeSteps.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionServiceTest.java (1)
150-173:⚠️ Potential issue | 🟠 MajorRe-enable the failure-path test and assert
FAILEDstatus propagation.Line 150 disables the only explicit failure-path test, so regression on error handling is not covered.
✅ Suggested test update
- `@Disabled` `@Test` void executeProcessShouldSendFailedStatusWhenProcessThrowsException() { UUID executionId = UUID.randomUUID(); UUID caseUuid = UUID.randomUUID(); when(processConfig.processType()).thenReturn(ProcessType.SECURITY_ANALYSIS); - RuntimeException processException = new RuntimeException("Process execution failed"); - //doThrow(processException).when(processExecutionService).executeSteps(eq(process), any(ProcessExecutionContext.class)); + when(process.getSteps()).thenReturn((List) List.of(loadNetworkStep)); + StepExecutor failingStepExecutor = mock(StepExecutor.class); + doThrow(new RuntimeException("Step failed")).when(failingStepExecutor).executeStep(any(), any()); + processExecutionService = new ProcessExecutionService(List.of(process), failingStepExecutor, notificationService, EXECUTION_ENV_NAME); ProcessRunMessage<ProcessConfig> runMessage = new ProcessRunMessage<>(executionId, caseUuid, processConfig, null); assertThrows(RuntimeException.class, () -> processExecutionService.executeProcess(runMessage)); - /*verify(processExecutionService).executeSteps(eq(process), any(ProcessExecutionContext.class));*/ - /*verify(notificationService, times(2)).updateExecutionStatus(eq(executionId), any(ProcessExecutionStatusUpdate.class));*/ - /* InOrder inOrder = inOrder(notificationService); inOrder.verify(notificationService).updateExecutionStatus(eq(executionId), argThat(update -> update.getStatus() == ProcessStatus.RUNNING )); inOrder.verify(notificationService).updateExecutionStatus(eq(executionId), argThat(update -> update.getStatus() == ProcessStatus.FAILED && update.getCompletedAt() != null - ));*/ + )); }🤖 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/ProcessExecutionServiceTest.java` around lines 150 - 173, Remove the `@Disabled` on executeProcessShouldSendFailedStatusWhenProcessThrowsException, mock the internal step execution to throw (use doThrow(processException).when(processExecutionService).executeSteps(eq(process), any(ProcessExecutionContext.class))) so executeProcess(runMessage) throws, keep the assertThrows(RuntimeException.class, () -> processExecutionService.executeProcess(runMessage)); and verify notificationService.updateExecutionStatus was called first with a ProcessStatus.RUNNING update and then with a ProcessStatus.FAILED update whose getCompletedAt() is not null (use InOrder inOrder = inOrder(notificationService) and argThat predicates to assert the two updates and their ordering).
🧹 Nitpick comments (5)
monitor-server/src/main/java/org/gridsuite/monitor/server/entities/processconfig/ProcessConfigEntity.java (1)
55-58: Consider updating the foreign key constraint name for consistency.The constraint name
AbstractProcessConfigEntity_modificationUuids_fk1uses an "Abstract" prefix that doesn't match the current class nameProcessConfigEntity. The class is concrete (not abstract) and other FK constraints in the codebase follow proper naming conventions (e.g.,SecurityAnalysisConfigEntity_contingencies_fk1). While this doesn't affect functionality, updating it would improve naming consistency across the codebase.Since this constraint already exists in the database schema (defined in the migration from 2026-01-30), renaming it would require an additional database migration to drop and recreate the constraint.
♻️ Suggested fix
`@CollectionTable`(name = "process_config_modifications", joinColumns = `@JoinColumn`(name = "process_config_id"), - foreignKey = `@ForeignKey`(name = "AbstractProcessConfigEntity_modificationUuids_fk1")) + foreignKey = `@ForeignKey`(name = "ProcessConfigEntity_modificationUuids_fk"))🤖 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/entities/processconfig/ProcessConfigEntity.java` around lines 55 - 58, The foreign key constraint name on the `@CollectionTable` in ProcessConfigEntity should be updated from "AbstractProcessConfigEntity_modificationUuids_fk1" to a consistent name (e.g., "ProcessConfigEntity_modificationUuids_fk1") in the CollectionTable annotation; change the `@ForeignKey`(name = "...") value in ProcessConfigEntity to the new name and then add a database migration that drops the old FK and recreates it with the new name to keep the schema in sync (ensure the migration references the existing table "process_config_modifications" and its join column "process_config_id").monitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/result/ResultType.java (1)
7-7: Consider package naming convention.The package uses
api.types.resultwheretypesis plural. Based on the PR discussion about package naming rules (lowercase, singular nouns, avoid plurals), consider whetherapi.type.resultwould be more consistent. However,typesas a top-level grouping for API data structures is a common convention.🤖 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/api/types/result/ResultType.java` at line 7, Rename the package from org.gridsuite.monitor.commons.api.types.result to org.gridsuite.monitor.commons.api.type.result for consistency with singular noun convention; update the package declaration at the top of ResultType.java and any other source files referencing this package (e.g., imports referencing org.gridsuite.monitor.commons.api.types.*), and move/rename the folder path from .../api/types/result to .../api/type/result so the package structure and imports remain in sync; run a quick project-wide search/replace for the old package string to update usages and fix any broken imports/build issues.monitor-server/src/main/java/org/gridsuite/monitor/server/mapper/ProcessExecutionStepMapper.java (1)
15-16: Consider adding private constructor instead of suppressing checkstyle.The
@SuppressWarningsannotation suppresses the utility class constructor warning. Adding an explicit private constructor is a cleaner approach that documents intent.♻️ Suggested improvement
-@SuppressWarnings("checkstyle:HideUtilityClassConstructor") -public class ProcessExecutionStepMapper { +public final class ProcessExecutionStepMapper { + private ProcessExecutionStepMapper() { + // Utility class + } + public static ProcessExecutionStep toDto(ProcessExecutionStepEntity entity) {🤖 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/mapper/ProcessExecutionStepMapper.java` around lines 15 - 16, Replace the `@SuppressWarnings`("checkstyle:HideUtilityClassConstructor") on ProcessExecutionStepMapper by adding an explicit private no-arg constructor to document intent and prevent instantiation; add a private ProcessExecutionStepMapper() { /* utility class */ } and remove the suppression annotation so the class remains non-instantiable and checkstyle-compliant.monitor-server/src/test/java/org/gridsuite/monitor/server/services/StateEstimationResultProviderTest.java (1)
52-52: Unnecessary stubbing for void method.
doNothing().when(...)is redundant for void methods since Mockito does nothing by default. This can be removed for cleaner test code.♻️ Suggested simplification
- doNothing().when(stateEstimationService).deleteResult(id); - provider.deleteResult(id);🤖 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/StateEstimationResultProviderTest.java` at line 52, In StateEstimationResultProviderTest remove the unnecessary Mockito stubbing for the void method: delete the line using doNothing().when(stateEstimationService).deleteResult(id); since Mockito does nothing by default for void methods—simply delete that stub and rely on existing verify(...) or other expectations around stateEstimationService.deleteResult(id) if present.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/context/ProcessStepExecutionContext.java (1)
15-17: Avoid DTO-layer coupling insidecoreexecution context.
ProcessStepExecutionContextincorecurrently carriesorg.gridsuite.monitor.worker.server.dto.report.ReportInfos. Consider introducing a neutral core/commons report model and mapping to DTO only at client boundaries.Also applies to: 33-33, 47-51
🤖 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/core/context/ProcessStepExecutionContext.java` around lines 15 - 17, ProcessStepExecutionContext currently depends on the DTO class ReportInfos—remove this DTO coupling by introducing a neutral core report model (e.g., ReportInfoCore / ReportInfosCore) inside the core package and change ProcessStepExecutionContext to use that core model instead of org.gridsuite.monitor.worker.server.dto.report.ReportInfos; then add a mapping layer or mapper method in the adapter/boundary package that converts between ReportInfoCore and the DTO ReportInfos at the client/API boundary (update any constructors/getters/setters in ProcessStepExecutionContext and replace the import of ReportInfos with the new core model).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionService.java`:
- Around line 223-240: The deleteExecution method currently performs remote
deletions (resultService.deleteResult and reportRestClient.deleteReport) inline
with the DB deletion (executionRepository.deleteById), risking partial failure
and state divergence; change the flow so the DB deletion happens inside the
transaction and the remote cleanup is executed asynchronously after commit
(e.g., publish a CleanupExecutionEvent or register a TransactionSynchronization
callback / use `@TransactionalEventListener`) which enqueues or retries idempotent
deletes for resultService.deleteResult and reportRestClient.deleteReport; ensure
the cleanup handlers are idempotent and include retry/backoff logic so remote
failures do not leave systems inconsistent.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/context/ProcessExecutionContext.java`:
- Around line 39-41: Change the method signature of createStepContext to use the
invariant type parameter ProcessStep<C> instead of the contravariant
ProcessStep<? super C> for consistency with the orchestration pattern; update
the declaration in ProcessExecutionContext.createStepContext to accept
ProcessStep<C> and leave the body returning new
ProcessStepExecutionContext<>(this, step.getType(), step.getId(), stepOrder)
unchanged so it matches other APIs like Process.onStepFailure,
AbstractProcess.defineSteps and StepExecutor.executeStep.
---
Duplicate comments:
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/orchestrator/ProcessExecutionService.java`:
- Around line 88-92: The current executeSteps method sets
ProcessStatus.COMPLETED unconditionally after calling doExecuteSteps, which
combined with swallowed exceptions later causes failed runs to be marked
successful; update executeSteps to wrap doExecuteSteps(process, context) in a
try/catch that on success updates to ProcessStatus.COMPLETED but on any
Exception updates the execution status to a failure state (e.g.,
ProcessStatus.FAILED) via updateExecutionStatus(context.getExecutionId(),
context.getExecutionEnvName(), ProcessStatus.FAILED) and then rethrow or
propagate the exception so callers can handle it; ensure the existing exception
handling code around doExecuteSteps (the block referenced at lines 107-112) is
removed or adjusted so it no longer swallows exceptions and relies on this
explicit status update in executeSteps.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionServiceTest.java`:
- Around line 150-173: Remove the `@Disabled` on
executeProcessShouldSendFailedStatusWhenProcessThrowsException, mock the
internal step execution to throw (use
doThrow(processException).when(processExecutionService).executeSteps(eq(process),
any(ProcessExecutionContext.class))) so executeProcess(runMessage) throws, keep
the assertThrows(RuntimeException.class, () ->
processExecutionService.executeProcess(runMessage)); and verify
notificationService.updateExecutionStatus was called first with a
ProcessStatus.RUNNING update and then with a ProcessStatus.FAILED update whose
getCompletedAt() is not null (use InOrder inOrder = inOrder(notificationService)
and argThat predicates to assert the two updates and their ordering).
---
Nitpick comments:
In
`@monitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/result/ResultType.java`:
- Line 7: Rename the package from org.gridsuite.monitor.commons.api.types.result
to org.gridsuite.monitor.commons.api.type.result for consistency with singular
noun convention; update the package declaration at the top of ResultType.java
and any other source files referencing this package (e.g., imports referencing
org.gridsuite.monitor.commons.api.types.*), and move/rename the folder path from
.../api/types/result to .../api/type/result so the package structure and imports
remain in sync; run a quick project-wide search/replace for the old package
string to update usages and fix any broken imports/build issues.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/entities/processconfig/ProcessConfigEntity.java`:
- Around line 55-58: The foreign key constraint name on the `@CollectionTable` in
ProcessConfigEntity should be updated from
"AbstractProcessConfigEntity_modificationUuids_fk1" to a consistent name (e.g.,
"ProcessConfigEntity_modificationUuids_fk1") in the CollectionTable annotation;
change the `@ForeignKey`(name = "...") value in ProcessConfigEntity to the new
name and then add a database migration that drops the old FK and recreates it
with the new name to keep the schema in sync (ensure the migration references
the existing table "process_config_modifications" and its join column
"process_config_id").
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/mapper/ProcessExecutionStepMapper.java`:
- Around line 15-16: Replace the
`@SuppressWarnings`("checkstyle:HideUtilityClassConstructor") on
ProcessExecutionStepMapper by adding an explicit private no-arg constructor to
document intent and prevent instantiation; add a private
ProcessExecutionStepMapper() { /* utility class */ } and remove the suppression
annotation so the class remains non-instantiable and checkstyle-compliant.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/StateEstimationResultProviderTest.java`:
- Line 52: In StateEstimationResultProviderTest remove the unnecessary Mockito
stubbing for the void method: delete the line using
doNothing().when(stateEstimationService).deleteResult(id); since Mockito does
nothing by default for void methods—simply delete that stub and rely on existing
verify(...) or other expectations around stateEstimationService.deleteResult(id)
if present.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/context/ProcessStepExecutionContext.java`:
- Around line 15-17: ProcessStepExecutionContext currently depends on the DTO
class ReportInfos—remove this DTO coupling by introducing a neutral core report
model (e.g., ReportInfoCore / ReportInfosCore) inside the core package and
change ProcessStepExecutionContext to use that core model instead of
org.gridsuite.monitor.worker.server.dto.report.ReportInfos; then add a mapping
layer or mapper method in the adapter/boundary package that converts between
ReportInfoCore and the DTO ReportInfos at the client/API boundary (update any
constructors/getters/setters in ProcessStepExecutionContext and replace the
import of ReportInfos with the new core model).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
monitor-commons/src/main/java/org/gridsuite/monitor/commons/ModifyingProcessConfig.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/PersistedProcessConfig.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/message/ProcessRunMessage.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/processconfig/ProcessConfig.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/processconfig/SecurityAnalysisConfig.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/result/ResultType.javamonitor-server/src/main/java/org/gridsuite/monitor/server/controllers/MonitorController.javamonitor-server/src/main/java/org/gridsuite/monitor/server/controllers/ProcessConfigController.javamonitor-server/src/main/java/org/gridsuite/monitor/server/entities/processconfig/ProcessConfigEntity.javamonitor-server/src/main/java/org/gridsuite/monitor/server/entities/processconfig/SecurityAnalysisConfigEntity.javamonitor-server/src/main/java/org/gridsuite/monitor/server/entities/processexecution/ProcessExecutionEntity.javamonitor-server/src/main/java/org/gridsuite/monitor/server/mapper/ProcessExecutionStepMapper.javamonitor-server/src/main/java/org/gridsuite/monitor/server/mapper/SecurityAnalysisConfigMapper.javamonitor-server/src/main/java/org/gridsuite/monitor/server/messaging/NotificationService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/repositories/ProcessConfigRepository.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/StateEstimationResultProvider.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionService.javamonitor-server/src/test/java/org/gridsuite/monitor/server/MonitorIntegrationTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/controllers/MonitorControllerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/controllers/ProcessConfigControllerTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/messaging/NotificationServiceTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/repositories/ProcessExecutionRepositoryTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/StateEstimationResultProviderTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigServiceTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionServiceTest.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/context/ProcessExecutionContext.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/context/ProcessStepExecutionContext.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/process/AbstractProcess.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/process/Process.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStep.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/orchestrator/ProcessExecutionService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/orchestrator/StepExecutionService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/securityanalysis/SecurityAnalysisProcess.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessExecutionContextTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessStepExecutionContextTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStepTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ConsumerServiceTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (13)
- monitor-server/src/test/java/org/gridsuite/monitor/server/repositories/ProcessExecutionRepositoryTest.java
- monitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/processconfig/ProcessConfig.java
- monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/process/AbstractProcess.java
- monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/ProcessExecutionContextTest.java
- monitor-server/src/main/java/org/gridsuite/monitor/server/messaging/NotificationService.java
- monitor-server/src/main/java/org/gridsuite/monitor/server/repositories/ProcessConfigRepository.java
- monitor-server/src/test/java/org/gridsuite/monitor/server/MonitorIntegrationTest.java
- monitor-server/src/test/java/org/gridsuite/monitor/server/controllers/ProcessConfigControllerTest.java
- monitor-server/src/test/java/org/gridsuite/monitor/server/messaging/NotificationServiceTest.java
- monitor-server/src/main/java/org/gridsuite/monitor/server/entities/processconfig/SecurityAnalysisConfigEntity.java
- monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.java
- monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/commons/steps/ApplyModificationsStep.java
- monitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/message/ProcessRunMessage.java
Contributor
Author
If we use a word to express a functional concept, then YES , using the singular form makes sense. For example, /processes could be renamed to /process. In that case, we are not referring to multiple concrete processes (such as SA process, Modification process, etc.), but rather to the overall process concept. The package then represents everything related to the notion of a process. Another example is package messaging or config, error. This is a common and acceptable practice. However, in some specific cases, especially when grouping elements based on a technical role (e.g., mappers, clients, entities, controllers, repositories), or even functional role, eg. resultproviders , using plural nouns makes more sense, since the package contains multiple components of the same type. Ultimately, this is mostly a team convention or recommendation. In my opinion, it is not a critical issue, as long as the naming remains consistent and clear within the project. |
Contributor
Yes that's why we want some written rules in the README with this PR so that everyone is aligned with the choices. |
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/client/SecurityAnalysisRestClient.java (1)
40-42:⚠️ Potential issue | 🟡 MinorPotential double slashes in URI construction.
The default
securityAnalysisServerBaseUriends with/(line 46), andgetSecurityAnalysisServerBaseUri()unconditionally appendsDELIMITER. Combined with the path insaveResult()starting with/, this produces URLs likehttp://security-analysis-server//v1//results/.... While some servers normalize this, it can cause routing issues.Proposed fix to normalize trailing slashes
private String getSecurityAnalysisServerBaseUri() { - return this.securityAnalysisServerBaseUri + DELIMITER + SA_API_VERSION + DELIMITER; + String baseUri = this.securityAnalysisServerBaseUri.endsWith(DELIMITER) + ? this.securityAnalysisServerBaseUri + : this.securityAnalysisServerBaseUri + DELIMITER; + return baseUri + SA_API_VERSION; }Then update
saveResultto not include the leading slash, or useUriComponentsBuilderconsistently:- var path = UriComponentsBuilder.fromPath("/results/{resultUuid}") + var path = UriComponentsBuilder.fromPath("results/{resultUuid}")🤖 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/client/SecurityAnalysisRestClient.java` around lines 40 - 42, The code currently appends DELIMITER in getSecurityAnalysisServerBaseUri() to a default securityAnalysisServerBaseUri that may already end with '/', causing double slashes when combined with paths used in saveResult(); fix by normalizing the base URI: trim any trailing '/' from securityAnalysisServerBaseUri before appending SA_API_VERSION (or ensure getSecurityAnalysisServerBaseUri() returns a value with exactly one '/' delimiter between parts), and update saveResult() to build the path without a leading '/' (or switch to using UriComponentsBuilder for consistent joining) so final URLs contain single slashes only; reference methods/fields: securityAnalysisServerBaseUri, getSecurityAnalysisServerBaseUri(), DELIMITER, SA_API_VERSION, saveResult().
🧹 Nitpick comments (1)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/client/SecurityAnalysisRestClient.java (1)
51-57: Consider validatingresultUuidfor null.
resultis validated butresultUuidis not. If null,buildAndExpand(resultUuid)will produce the literal string"null"in the path, sending a request to/results/nullinstead of failing fast.Proposed fix
public void saveResult(UUID resultUuid, SecurityAnalysisResult result) { + Objects.requireNonNull(resultUuid); Objects.requireNonNull(result);🤖 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/client/SecurityAnalysisRestClient.java` around lines 51 - 57, In saveResult in SecurityAnalysisRestClient, validate the resultUuid parameter before using it (e.g., call Objects.requireNonNull(resultUuid, "resultUuid must not be null")) so buildAndExpand(resultUuid) cannot produce the literal "null" path; keep the existing Objects.requireNonNull(result) check and add the null-check for resultUuid at the top of the method to fail fast when UUID is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/client/SecurityAnalysisRestClient.java`:
- Around line 40-42: The code currently appends DELIMITER in
getSecurityAnalysisServerBaseUri() to a default securityAnalysisServerBaseUri
that may already end with '/', causing double slashes when combined with paths
used in saveResult(); fix by normalizing the base URI: trim any trailing '/'
from securityAnalysisServerBaseUri before appending SA_API_VERSION (or ensure
getSecurityAnalysisServerBaseUri() returns a value with exactly one '/'
delimiter between parts), and update saveResult() to build the path without a
leading '/' (or switch to using UriComponentsBuilder for consistent joining) so
final URLs contain single slashes only; reference methods/fields:
securityAnalysisServerBaseUri, getSecurityAnalysisServerBaseUri(), DELIMITER,
SA_API_VERSION, saveResult().
---
Nitpick comments:
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/client/SecurityAnalysisRestClient.java`:
- Around line 51-57: In saveResult in SecurityAnalysisRestClient, validate the
resultUuid parameter before using it (e.g., call
Objects.requireNonNull(resultUuid, "resultUuid must not be null")) so
buildAndExpand(resultUuid) cannot produce the literal "null" path; keep the
existing Objects.requireNonNull(result) check and add the null-check for
resultUuid at the top of the method to fail fast when UUID is missing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/client/SecurityAnalysisRestClient.java
Contributor
Author
At the moment, I don’t see any shared functional parts between ReportRestClient and SecurityAnalysisRestClient across the server and worker modules. Sharing these service clients could be a good idea in the future. However, this PR is already large enough, so we can postpone that refactoring until there is actual common functional implementation to justify it. |
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/process/ProcessTest.java (1)
40-90:⚠️ Potential issue | 🟠 MajorCommented-out tests leave no active test coverage for
AbstractProcess.This test class has no active
@Testmethods—bothexecuteShouldExecuteAllStepsSuccessfullyWhenNoErrorsandexecuteShouldSkipAllRemainingStepsWhenFirstStepFailsare commented out. These tests verify critical process execution behavior (step ordering, skip-on-failure logic).Committing commented-out tests is discouraged. If the tests need to be reworked due to the orchestration refactor (moving execution logic to
ProcessExecutor/StepExecutor), consider one of the following:
- Fix and enable the tests as part of this PR to maintain coverage.
- Delete the commented code and create a follow-up issue to track the test gap.
- Move the tests to the appropriate orchestrator component if that's where the logic now resides.
The PR description notes "Remaining work: organize and move tests"—please ensure this is tracked explicitly.
Would you like me to open an issue to track restoring test coverage for process/step execution logic?
🤖 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/process/ProcessTest.java` around lines 40 - 90, The commented-out tests in ProcessTest remove coverage for AbstractProcess execution; restore coverage by moving and adapting these two tests to the new orchestrator tests (e.g., ProcessExecutorTest/StepExecutorTest): update references to call the new execution entrypoint (replace process.execute(processContext) with ProcessExecutor.execute(...) or StepExecutor.execute(...) as appropriate), mock ProcessStepExecutionContext and StepExecutionService as in the original tests, assert executeStep vs skipStep calls and verify step ordering using InOrder on the processContext/createStepContext calls, or if you cannot adapt now delete the commented blocks and open a tracking issue to restore these tests later.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/clients/NetworkModificationRestClient.java (1)
33-46:⚠️ Potential issue | 🟠 MajorDefault URI composition can generate
//v1/...paths.Line 33 includes a trailing
/in the default base URI, and Line 41 prepends another/. This may produce malformed route paths depending on upstream normalization.🔧 Proposed fix
public class NetworkModificationRestClient { @@ public NetworkModificationRestClient(`@Value`("${gridsuite.services.network-modification-server.base-uri:http://network-modification-server/}") String networkModificationServerBaseUri, RestTemplateBuilder restTemplateBuilder) { this.networkModificationServerRest = restTemplateBuilder.build(); - this.networkModificationServerBaseUri = networkModificationServerBaseUri; + this.networkModificationServerBaseUri = networkModificationServerBaseUri.endsWith(DELIMITER) + ? networkModificationServerBaseUri.substring(0, networkModificationServerBaseUri.length() - 1) + : networkModificationServerBaseUri; } @@ - String path = this.networkModificationServerBaseUri + UriComponentsBuilder.fromPath(DELIMITER + NETWORK_MODIFICATION_SERVER_API_VERSION + DELIMITER + - "network-composite-modifications" + DELIMITER + "network-modifications") + String path = UriComponentsBuilder.fromHttpUrl(this.networkModificationServerBaseUri) + .pathSegment(NETWORK_MODIFICATION_SERVER_API_VERSION, "network-composite-modifications", "network-modifications") .queryParam("uuids", modificationsUuids.toArray()) - .queryParam("onlyMetadata", "false") - .buildAndExpand() + .queryParam("onlyMetadata", false) .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/clients/NetworkModificationRestClient.java` around lines 33 - 46, The base URI may contain a trailing slash causing double slashes when you prepend "/" in getModifications; in NetworkModificationRestClient normalize the networkModificationServerBaseUri (set in the constructor) or build the request using UriComponentsBuilder/fromUriString so you don't manually concatenate strings—either trim any trailing '/' from networkModificationServerBaseUri in the constructor or switch getModifications to call UriComponentsBuilder.fromUriString(networkModificationServerBaseUri).path(...).queryParam(...) to safely join the path segments (refer to the constructor, the networkModificationServerBaseUri field, and the getModifications method).
🧹 Nitpick comments (16)
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/context/ProcessStepExecutionContextTest.java (1)
57-69: Consider removing redundant assertions (optional).There are duplicate assertions for the same values:
getNetwork()is asserted twice (lines 59 and 68).getProcessExecutionId()is asserted twice (lines 60 and 66).Since the mocks return the same values, these are effectively testing the same thing. You could consolidate to reduce noise, but this is a minor nit.
🤖 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/context/ProcessStepExecutionContextTest.java` around lines 57 - 69, The test ProcessStepExecutionContextTest contains redundant assertions: remove the duplicate assertions on stepContext.getNetwork() and stepContext.getProcessExecutionId() so each property is asserted only once; keep the first (or the more contextually relevant) assertions for getNetwork() and getProcessExecutionId() and delete the repeated lines that assert the same values to reduce noise while leaving assertions for stepContext.getStepExecutionId(), getStepOrder(), getConfig(), getProcessStepType(), getStartedAt(), getReportInfos(), getCaseUuid(), and getDebugFileLocation() intact.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/process/ProcessTest.java (2)
95-110:TestProcessinner class is unused with no active tests.The
TestProcessmock implementation remains, but since all tests are commented out, it serves no purpose. This should be cleaned up alongside addressing the commented-out tests.🤖 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/process/ProcessTest.java` around lines 95 - 110, The TestProcess inner class (extending AbstractProcess and implementing defineSteps) is dead code because all tests that referenced it are commented out; remove the TestProcess class from ProcessTest.java (or re-enable relevant tests that use TestProcess) and also clean up any unused imports or mocks related to ProcessStep and ProcessConfig to avoid unused symbol warnings; ensure any test that relied on TestProcess instead instantiates a valid process or is deleted so AbstractProcess, TestProcess, defineSteps, and related mocks are no longer unused.
27-38: Unused mock fields and setup method.The
@MockfieldsstepExecutionServiceandprocessContext, along with thesetUp()method, are dead code since all test methods are commented out. Either restore the tests or remove this unused infrastructure.🤖 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/process/ProcessTest.java` around lines 27 - 38, The Test class contains unused mocks (stepExecutionService, processContext) and an unused setup method (setUp) since all test methods are commented out; either remove the dead fields and the setUp() method from ProcessTest, or re-enable/restore the tests that exercise TestProcess so the mocks are used. Specifically, delete the `@Mock` fields stepExecutionService and processContext and the setUp() method from the ProcessTest class if you intend to keep tests commented out, or uncomment/restore the test methods that reference TestProcess and these mocks so they become necessary.monitor-server/src/main/java/org/gridsuite/monitor/server/clients/ReportRestClient.java (1)
46-54: Content-Type header is unnecessary for GET requests.Setting
Content-Type: application/jsonon a GET request has no effect since GET requests typically don't have a request body. Consider removing the header setup or usingAcceptheader if you want to specify the expected response type.♻️ Suggested simplification
public ReportPage getReport(UUID reportId) { var uriBuilder = UriComponentsBuilder.fromPath("{id}/logs"); - var path = uriBuilder.buildAndExpand(reportId).toUriString(); - HttpHeaders headers = new HttpHeaders(); - headers.setContentType(MediaType.APPLICATION_JSON); - return restTemplate.exchange(this.getReportServerURI() + path, HttpMethod.GET, new HttpEntity<>(headers), new ParameterizedTypeReference<ReportPage>() { }).getBody(); + return restTemplate.exchange(this.getReportServerURI() + path, HttpMethod.GET, null, new ParameterizedTypeReference<ReportPage>() { }).getBody(); }🤖 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/clients/ReportRestClient.java` around lines 46 - 54, In getReport(UUID) in ReportRestClient remove the unnecessary Content-Type header on the GET request (headers.setContentType(MediaType.APPLICATION_JSON)) and either omit headers entirely or set an Accept header (headers.setAccept(...)) if you need to declare the expected response type; update the HttpEntity construction used in restTemplate.exchange accordingly so the GET request doesn't send a Content-Type for a body-less call.monitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processconfig/SecurityAnalysisConfigMapper.java (1)
17-18: Same utility class pattern suggestion applies here.Consider adding a private constructor to properly enforce the utility class pattern, matching the recommendation for
ProcessExecutionStepMapper.🤖 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/mappers/processconfig/SecurityAnalysisConfigMapper.java` around lines 17 - 18, SecurityAnalysisConfigMapper is intended as a utility class but lacks a private constructor to prevent instantiation; add a private no-arg constructor (e.g., private SecurityAnalysisConfigMapper()) to the SecurityAnalysisConfigMapper class to enforce the utility class pattern (mirroring the change done for ProcessExecutionStepMapper) and keep or adjust the existing `@SuppressWarnings`("checkstyle:HideUtilityClassConstructor") as needed.monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.java (1)
70-77: Minor optimization opportunity in deleteProcessConfig.The current implementation calls
existsByIdfollowed bydeleteById, resulting in two database queries. Consider usingdeleteByIdwith exception handling or check the return value of a custom repository method if performance becomes a concern.♻️ Alternative approach using findById
`@Transactional` public boolean deleteProcessConfig(UUID processConfigUuid) { - if (processConfigRepository.existsById(processConfigUuid)) { - processConfigRepository.deleteById(processConfigUuid); - return true; - } - return false; + return processConfigRepository.findById(processConfigUuid) + .map(entity -> { + processConfigRepository.delete(entity); + return true; + }) + .orElse(false); }🤖 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/processconfig/ProcessConfigService.java` around lines 70 - 77, The deleteProcessConfig method currently does existsById then deleteById causing two DB calls; replace that with a single-call approach by invoking processConfigRepository.deleteById(processConfigUuid) and handling the potential exception (e.g., catch EmptyResultDataAccessException) to return false on missing entity, or alternatively use processConfigRepository.findById(processConfigUuid).ifPresent(...) to delete and return true/false; update deleteProcessConfig to use one of these patterns and remove the initial existsById check.monitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processexecution/ProcessExecutionStepMapper.java (1)
15-16: Consider adding a private constructor to complete the utility class pattern.The
@SuppressWarnings("checkstyle:HideUtilityClassConstructor")suppresses the warning but doesn't actually prevent instantiation. Adding a private constructor would properly enforce the utility class pattern.♻️ Suggested fix
`@SuppressWarnings`("checkstyle:HideUtilityClassConstructor") public class ProcessExecutionStepMapper { + private ProcessExecutionStepMapper() { + // Utility class + } + public static ProcessExecutionStep toDto(ProcessExecutionStepEntity entity) {🤖 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/mappers/processexecution/ProcessExecutionStepMapper.java` around lines 15 - 16, Add a private no-argument constructor to the ProcessExecutionStepMapper utility class to prevent instantiation and complete the utility class pattern; keep or remove the existing `@SuppressWarnings`("checkstyle:HideUtilityClassConstructor") as desired, but ensure the private constructor is declared within the ProcessExecutionStepMapper class (e.g., a private ProcessExecutionStepMapper() { } ) so the class cannot be instantiated.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/clients/ReportRestClient.java (1)
63-68: Response from PUT request is discarded.The
exchange()call returns aResponseEntity<ReportNode>but the response body is not used. If the response isn't needed, consider usingexchange(..., Void.class)to make intent clearer, or return the result if callers might need confirmation.♻️ Suggested clarification
- restTemplate.exchange(getReportServerURI() + path, HttpMethod.PUT, new HttpEntity<>(str, headers), ReportNode.class); + restTemplate.exchange(getReportServerURI() + path, HttpMethod.PUT, new HttpEntity<>(str, headers), Void.class);🤖 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/clients/ReportRestClient.java` around lines 63 - 68, The PUT response from restTemplate.exchange in ReportRestClient (the call using objectMapper.writeValueAsString(reportInfos.reportNode()) and restTemplate.exchange(getReportServerURI() + path, HttpMethod.PUT, new HttpEntity<>(str, headers), ReportNode.class)) is currently discarded; fix by either changing the exchange generic to Void.class to express no response expected, or capture and return the ResponseEntity<ReportNode> (or its body) to callers so they can inspect confirmation/errors; update the method signature accordingly if you choose to return the response and ensure JsonProcessingException handling remains unchanged.monitor-server/src/main/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionService.java (1)
222-245: Consider transactional consistency for deletion.The
deleteExecutionmethod performs multiple external calls (resultService.deleteResult,reportRestClient.deleteReport) before deleting from the repository. If any external call fails mid-way, the execution record will remain but some results/reports may already be deleted, leading to an inconsistent state.Consider:
- Deleting the database record first (can be rolled back via
@Transactional)- Performing external deletions after commit (e.g., via
@TransactionalEventListener)🤖 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/processexecution/ProcessExecutionService.java` around lines 222 - 245, The deleteExecution method currently calls resultService.deleteResult and reportRestClient.deleteReport before removing the ProcessExecutionEntity, risking partial external deletion if an external call fails; change the flow so the DB delete happens inside the `@Transactional` method first (call executionRepository.deleteById(executionId) while still transactional), publish a domain event (e.g., ExecutionDeletedEvent with executionId and collected ResultInfos/reportIds) and remove the external resources from an async or `@TransactionalEventListener`(phase = AFTER_COMMIT) handler that invokes resultService.deleteResult and reportRestClient.deleteReport; keep deleteExecution, ResultInfos, and the repository call but add the event class and the AFTER_COMMIT listener to perform external deletions post-commit.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/orchestrator/StepExecutionService.java (1)
57-65: Report sending failure will mark step as FAILED.The
reportRestClient.sendReport()call (line 59) is inside the try block, so if report sending fails, the step will be marked asFAILEDeven though the actual computation (step.execute(context)) succeeded.Consider whether this is the intended behavior. If the step computation should be considered successful regardless of report persistence, move the report sending outside the try-catch or handle its exceptions separately.
💡 Alternative: Separate report sending from step status
try { step.execute(context); + updateStepStatus(context, StepStatus.COMPLETED, step); + } catch (Exception e) { + updateStepStatus(context, StepStatus.FAILED, step); + throw e; + } + + try { reportRestClient.sendReport(context.getReportInfos()); - updateStepStatus(context, StepStatus.COMPLETED, step); } catch (Exception e) { - updateStepStatus(context, StepStatus.FAILED, step); + // Log but don't fail the step if report sending fails + LOGGER.warn("Failed to send report for step {}: {}", context.getStepExecutionId(), e.getMessage()); - throw e; }🤖 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/orchestrator/StepExecutionService.java` around lines 57 - 65, The current try/catch in StepExecutionService wraps both step.execute(context) and reportRestClient.sendReport(...), so any failure sending reports will call updateStepStatus(..., StepStatus.FAILED) even when step.execute succeeded; adjust the flow so execution success and reporting failure are handled separately: keep step.execute(context) inside its try and call updateStepStatus(context, StepStatus.COMPLETED, step) immediately after a successful execute, then call reportRestClient.sendReport(context.getReportInfos()) outside that try (or catch/report its exception separately and log/handle it without changing the step status); updateStepStatus should only be invoked with StepStatus.FAILED for real execution exceptions from step.execute.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/process/securityanalysis/steps/SecurityAnalysisRunComputationStep.java (1)
43-47: FIXME items and hardcoded contingency type require attention.Two concerns in this segment:
- The FIXME comments (lines 43, 46) indicate incomplete implementation for parameters and contingency retrieval.
- Line 47 assumes all contingencies are
LineContingency, which may not be correct for generator, bus, or other contingency types.Would you like me to open issues to track these items, or is this being addressed in a follow-up PR?
🤖 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/process/securityanalysis/steps/SecurityAnalysisRunComputationStep.java` around lines 43 - 47, In SecurityAnalysisRunComputationStep replace the FIXME placeholders by fetching the actual parameters UUID and contingency definitions from the computation server/config (use context.getConfig() or the computation client used elsewhere) instead of the hardcoded values, and stop assuming every contingency is a LineContingency: introduce logic (e.g., a ContingencyFactory or a switch in the mapping performed where contingencyList is built) that reads the contingency type/metadata from the config entries returned by context.getConfig().contingencies() (or the server response) and constructs the correct subclass (LineContingency, GeneratorContingency, BusContingency, etc.), and add a default/error path for unknown types; keep references to parametersUuid, contingencies, Contingency, and LineContingency to locate the changes.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/clients/SecurityAnalysisRestClient.java (1)
51-62: Consider adding error handling for REST call failure.The
saveResultmethod performs a POST but does not handle potentialRestClientException. If the external service is unavailable or returns an error, the exception will propagate uncaught. Consider whether this step should have explicit error handling or retry logic for resilience.💡 Example: Add explicit exception handling
public void saveResult(UUID resultUuid, SecurityAnalysisResult result) { Objects.requireNonNull(result); LOGGER.info("Saving result {}", resultUuid); var path = UriComponentsBuilder.fromPath("/results/{resultUuid}") .buildAndExpand(resultUuid) .toUriString(); var headers = new HttpHeaders(); headers.setContentType(MediaType.APPLICATION_JSON); + try { restTemplate.exchange(getSecurityAnalysisServerBaseUri() + path, HttpMethod.POST, new HttpEntity<>(result, headers), Void.class); + } catch (RestClientException e) { + LOGGER.error("Failed to save result {}: {}", resultUuid, e.getMessage()); + throw e; + } }🤖 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/clients/SecurityAnalysisRestClient.java` around lines 51 - 62, The saveResult method currently calls restTemplate.exchange without handling RestClientException; wrap the restTemplate.exchange call in a try-catch that catches RestClientException (and optionally HttpStatusCodeException) to log the failure including resultUuid, the target URI from getSecurityAnalysisServerBaseUri() + path and the SecurityAnalysisResult payload, and then either rethrow a wrapped runtime exception or return/ignore based on desired flow; for resilience also consider adding a retry mechanism (e.g., Spring Retry’s `@Retryable` on saveResult or a RetryTemplate around restTemplate.exchange) so transient errors are retried before final failure.monitor-server/src/test/java/org/gridsuite/monitor/server/clients/ReportRestClientTest.java (1)
44-45: Redundant inline initialization on@Autowiredfield.The
ObjectMapperfield is annotated with@Autowiredbut also has an inline initialization (= new ObjectMapper()). Spring will inject the bean anyway, making the initialization redundant. The worker module'sReportRestClientTestcorrectly uses only@Autowiredwithout inline initialization.♻️ Remove redundant initialization
`@Autowired` - ObjectMapper objectMapper = new ObjectMapper(); + private ObjectMapper objectMapper;🤖 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/clients/ReportRestClientTest.java` around lines 44 - 45, The ObjectMapper field in ReportRestClientTest is annotated with `@Autowired` but also has a redundant inline initialization; remove the "= new ObjectMapper()" from the ObjectMapper field declaration so the `@Autowired` injection is used (leave the field as " `@Autowired` ObjectMapper objectMapper;") to match the worker module pattern and avoid creating an unnecessary instance.monitor-server/src/test/java/org/gridsuite/monitor/server/services/result/providers/SecurityAnalysisResultProviderTest.java (1)
35-47: Test method name references outdated class name.The method
getResultShouldDelegateToSecurityAnalysisService(line 36) anddeleteResultShouldDelegateToSecurityAnalysisService(line 50) now delegate toSecurityAnalysisRestClient, notSecurityAnalysisService. Consider renaming for clarity.♻️ Suggested rename
`@Test` - void getResultShouldDelegateToSecurityAnalysisService() { + void getResultShouldDelegateToSecurityAnalysisRestClient() {`@Test` - void deleteResultShouldDelegateToSecurityAnalysisService() { + void deleteResultShouldDelegateToSecurityAnalysisRestClient() {🤖 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/result/providers/SecurityAnalysisResultProviderTest.java` around lines 35 - 47, The test method names reference the old collaborator name SecurityAnalysisService; rename the two methods getResultShouldDelegateToSecurityAnalysisService and deleteResultShouldDelegateToSecurityAnalysisService to reflect the current collaborator SecurityAnalysisRestClient (e.g., getResultShouldDelegateToSecurityAnalysisRestClient and deleteResultShouldDelegateToSecurityAnalysisRestClient) so the test names match the actual mocked dependency (SecurityAnalysisRestClient) used in SecurityAnalysisResultProviderTest.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/orchestrator/ProcessExecutionServiceTest.java (2)
185-185: Remove commented-out verification.This commented-out line adds no value and should be removed for clarity.
🤖 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/orchestrator/ProcessExecutionServiceTest.java` at line 185, Remove the dead commented-out verification in the test: delete the line "// verify(processExecutionService, never()).executeSteps(process, any());" so the test no longer contains unused commented Mockito verification tied to processExecutionService.executeSteps(...) and any() — leaving only meaningful assertions and verifications.
108-108: Remove or complete commented-out test code.These commented-out stubs and verifications create noise and make the test harder to maintain. Either:
- Remove them if no longer needed with the new architecture, or
- Uncomment and fix if they test valid behavior
Also applies to: 131-137
🤖 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/orchestrator/ProcessExecutionServiceTest.java` at line 108, In ProcessExecutionServiceTest remove or restore the commented-out test stubs around the executeSteps call: either delete the commented line "// doNothing().when(processExecutionService).executeSteps(process, any(ProcessExecutionContext.class));" (and the similar commented blocks at 131-137) if they are obsolete under the new architecture, or uncomment and correct them by using the Mockito style that matches the test setup (e.g., ensure processExecutionService is a mock and use doNothing().when(processExecutionService).executeSteps(eq(process), any(ProcessExecutionContext.class)) or verify(processExecutionService).executeSteps(...) as appropriate) so the test contains only active, valid assertions; update imports/mocks if needed to make the uncommented stubs compile and behave correctly.
🤖 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-worker-server/src/main/java/org/gridsuite/monitor/worker/server/clients/FilterRestClient.java`:
- Around line 36-39: The constructor in FilterRestClient sets
filterServerBaseUri (passed via `@Value`) which may include a trailing slash and
later URL concatenation produces double slashes; fix by normalizing
filterServerBaseUri inside the FilterRestClient constructor (trim any trailing
'/' except keep "http(s)://host/" semantics) so subsequent concatenation (where
you append paths like "/v1/...") always yields a single slash; update the
constructor that takes filterServerBaseUri and restTemplateBuilder to
sanitize/filterServerBaseUri (e.g., remove a trailing slash) before assigning
the field so all callers that build URLs from filterServerBaseUri get normalized
results.
---
Outside diff comments:
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/clients/NetworkModificationRestClient.java`:
- Around line 33-46: The base URI may contain a trailing slash causing double
slashes when you prepend "/" in getModifications; in
NetworkModificationRestClient normalize the networkModificationServerBaseUri
(set in the constructor) or build the request using
UriComponentsBuilder/fromUriString so you don't manually concatenate
strings—either trim any trailing '/' from networkModificationServerBaseUri in
the constructor or switch getModifications to call
UriComponentsBuilder.fromUriString(networkModificationServerBaseUri).path(...).queryParam(...)
to safely join the path segments (refer to the constructor, the
networkModificationServerBaseUri field, and the getModifications method).
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/process/ProcessTest.java`:
- Around line 40-90: The commented-out tests in ProcessTest remove coverage for
AbstractProcess execution; restore coverage by moving and adapting these two
tests to the new orchestrator tests (e.g.,
ProcessExecutorTest/StepExecutorTest): update references to call the new
execution entrypoint (replace process.execute(processContext) with
ProcessExecutor.execute(...) or StepExecutor.execute(...) as appropriate), mock
ProcessStepExecutionContext and StepExecutionService as in the original tests,
assert executeStep vs skipStep calls and verify step ordering using InOrder on
the processContext/createStepContext calls, or if you cannot adapt now delete
the commented blocks and open a tracking issue to restore these tests later.
---
Nitpick comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/clients/ReportRestClient.java`:
- Around line 46-54: In getReport(UUID) in ReportRestClient remove the
unnecessary Content-Type header on the GET request
(headers.setContentType(MediaType.APPLICATION_JSON)) and either omit headers
entirely or set an Accept header (headers.setAccept(...)) if you need to declare
the expected response type; update the HttpEntity construction used in
restTemplate.exchange accordingly so the GET request doesn't send a Content-Type
for a body-less call.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processconfig/SecurityAnalysisConfigMapper.java`:
- Around line 17-18: SecurityAnalysisConfigMapper is intended as a utility class
but lacks a private constructor to prevent instantiation; add a private no-arg
constructor (e.g., private SecurityAnalysisConfigMapper()) to the
SecurityAnalysisConfigMapper class to enforce the utility class pattern
(mirroring the change done for ProcessExecutionStepMapper) and keep or adjust
the existing `@SuppressWarnings`("checkstyle:HideUtilityClassConstructor") as
needed.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processexecution/ProcessExecutionStepMapper.java`:
- Around line 15-16: Add a private no-argument constructor to the
ProcessExecutionStepMapper utility class to prevent instantiation and complete
the utility class pattern; keep or remove the existing
`@SuppressWarnings`("checkstyle:HideUtilityClassConstructor") as desired, but
ensure the private constructor is declared within the ProcessExecutionStepMapper
class (e.g., a private ProcessExecutionStepMapper() { } ) so the class cannot be
instantiated.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/processconfig/ProcessConfigService.java`:
- Around line 70-77: The deleteProcessConfig method currently does existsById
then deleteById causing two DB calls; replace that with a single-call approach
by invoking processConfigRepository.deleteById(processConfigUuid) and handling
the potential exception (e.g., catch EmptyResultDataAccessException) to return
false on missing entity, or alternatively use
processConfigRepository.findById(processConfigUuid).ifPresent(...) to delete and
return true/false; update deleteProcessConfig to use one of these patterns and
remove the initial existsById check.
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionService.java`:
- Around line 222-245: The deleteExecution method currently calls
resultService.deleteResult and reportRestClient.deleteReport before removing the
ProcessExecutionEntity, risking partial external deletion if an external call
fails; change the flow so the DB delete happens inside the `@Transactional` method
first (call executionRepository.deleteById(executionId) while still
transactional), publish a domain event (e.g., ExecutionDeletedEvent with
executionId and collected ResultInfos/reportIds) and remove the external
resources from an async or `@TransactionalEventListener`(phase = AFTER_COMMIT)
handler that invokes resultService.deleteResult and
reportRestClient.deleteReport; keep deleteExecution, ResultInfos, and the
repository call but add the event class and the AFTER_COMMIT listener to perform
external deletions post-commit.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/clients/ReportRestClientTest.java`:
- Around line 44-45: The ObjectMapper field in ReportRestClientTest is annotated
with `@Autowired` but also has a redundant inline initialization; remove the "=
new ObjectMapper()" from the ObjectMapper field declaration so the `@Autowired`
injection is used (leave the field as " `@Autowired` ObjectMapper objectMapper;")
to match the worker module pattern and avoid creating an unnecessary instance.
In
`@monitor-server/src/test/java/org/gridsuite/monitor/server/services/result/providers/SecurityAnalysisResultProviderTest.java`:
- Around line 35-47: The test method names reference the old collaborator name
SecurityAnalysisService; rename the two methods
getResultShouldDelegateToSecurityAnalysisService and
deleteResultShouldDelegateToSecurityAnalysisService to reflect the current
collaborator SecurityAnalysisRestClient (e.g.,
getResultShouldDelegateToSecurityAnalysisRestClient and
deleteResultShouldDelegateToSecurityAnalysisRestClient) so the test names match
the actual mocked dependency (SecurityAnalysisRestClient) used in
SecurityAnalysisResultProviderTest.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/clients/ReportRestClient.java`:
- Around line 63-68: The PUT response from restTemplate.exchange in
ReportRestClient (the call using
objectMapper.writeValueAsString(reportInfos.reportNode()) and
restTemplate.exchange(getReportServerURI() + path, HttpMethod.PUT, new
HttpEntity<>(str, headers), ReportNode.class)) is currently discarded; fix by
either changing the exchange generic to Void.class to express no response
expected, or capture and return the ResponseEntity<ReportNode> (or its body) to
callers so they can inspect confirmation/errors; update the method signature
accordingly if you choose to return the response and ensure
JsonProcessingException handling remains unchanged.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/clients/SecurityAnalysisRestClient.java`:
- Around line 51-62: The saveResult method currently calls restTemplate.exchange
without handling RestClientException; wrap the restTemplate.exchange call in a
try-catch that catches RestClientException (and optionally
HttpStatusCodeException) to log the failure including resultUuid, the target URI
from getSecurityAnalysisServerBaseUri() + path and the SecurityAnalysisResult
payload, and then either rethrow a wrapped runtime exception or return/ignore
based on desired flow; for resilience also consider adding a retry mechanism
(e.g., Spring Retry’s `@Retryable` on saveResult or a RetryTemplate around
restTemplate.exchange) so transient errors are retried before final failure.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/orchestrator/StepExecutionService.java`:
- Around line 57-65: The current try/catch in StepExecutionService wraps both
step.execute(context) and reportRestClient.sendReport(...), so any failure
sending reports will call updateStepStatus(..., StepStatus.FAILED) even when
step.execute succeeded; adjust the flow so execution success and reporting
failure are handled separately: keep step.execute(context) inside its try and
call updateStepStatus(context, StepStatus.COMPLETED, step) immediately after a
successful execute, then call
reportRestClient.sendReport(context.getReportInfos()) outside that try (or
catch/report its exception separately and log/handle it without changing the
step status); updateStepStatus should only be invoked with StepStatus.FAILED for
real execution exceptions from step.execute.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/process/securityanalysis/steps/SecurityAnalysisRunComputationStep.java`:
- Around line 43-47: In SecurityAnalysisRunComputationStep replace the FIXME
placeholders by fetching the actual parameters UUID and contingency definitions
from the computation server/config (use context.getConfig() or the computation
client used elsewhere) instead of the hardcoded values, and stop assuming every
contingency is a LineContingency: introduce logic (e.g., a ContingencyFactory or
a switch in the mapping performed where contingencyList is built) that reads the
contingency type/metadata from the config entries returned by
context.getConfig().contingencies() (or the server response) and constructs the
correct subclass (LineContingency, GeneratorContingency, BusContingency, etc.),
and add a default/error path for unknown types; keep references to
parametersUuid, contingencies, Contingency, and LineContingency to locate the
changes.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/context/ProcessStepExecutionContextTest.java`:
- Around line 57-69: The test ProcessStepExecutionContextTest contains redundant
assertions: remove the duplicate assertions on stepContext.getNetwork() and
stepContext.getProcessExecutionId() so each property is asserted only once; keep
the first (or the more contextually relevant) assertions for getNetwork() and
getProcessExecutionId() and delete the repeated lines that assert the same
values to reduce noise while leaving assertions for
stepContext.getStepExecutionId(), getStepOrder(), getConfig(),
getProcessStepType(), getStartedAt(), getReportInfos(), getCaseUuid(), and
getDebugFileLocation() intact.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/process/ProcessTest.java`:
- Around line 95-110: The TestProcess inner class (extending AbstractProcess and
implementing defineSteps) is dead code because all tests that referenced it are
commented out; remove the TestProcess class from ProcessTest.java (or re-enable
relevant tests that use TestProcess) and also clean up any unused imports or
mocks related to ProcessStep and ProcessConfig to avoid unused symbol warnings;
ensure any test that relied on TestProcess instead instantiates a valid process
or is deleted so AbstractProcess, TestProcess, defineSteps, and related mocks
are no longer unused.
- Around line 27-38: The Test class contains unused mocks (stepExecutionService,
processContext) and an unused setup method (setUp) since all test methods are
commented out; either remove the dead fields and the setUp() method from
ProcessTest, or re-enable/restore the tests that exercise TestProcess so the
mocks are used. Specifically, delete the `@Mock` fields stepExecutionService and
processContext and the setUp() method from the ProcessTest class if you intend
to keep tests commented out, or uncomment/restore the test methods that
reference TestProcess and these mocks so they become necessary.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/orchestrator/ProcessExecutionServiceTest.java`:
- Line 185: Remove the dead commented-out verification in the test: delete the
line "// verify(processExecutionService, never()).executeSteps(process, any());"
so the test no longer contains unused commented Mockito verification tied to
processExecutionService.executeSteps(...) and any() — leaving only meaningful
assertions and verifications.
- Line 108: In ProcessExecutionServiceTest remove or restore the commented-out
test stubs around the executeSteps call: either delete the commented line "//
doNothing().when(processExecutionService).executeSteps(process,
any(ProcessExecutionContext.class));" (and the similar commented blocks at
131-137) if they are obsolete under the new architecture, or uncomment and
correct them by using the Mockito style that matches the test setup (e.g.,
ensure processExecutionService is a mock and use
doNothing().when(processExecutionService).executeSteps(eq(process),
any(ProcessExecutionContext.class)) or
verify(processExecutionService).executeSteps(...) as appropriate) so the test
contains only active, valid assertions; update imports/mocks if needed to make
the uncommented stubs compile and behave correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
monitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/messaging/MessageType.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/messaging/ProcessRunMessage.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/processconfig/ModifyingProcessConfig.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/processconfig/PersistedProcessConfig.javamonitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/processconfig/SecurityAnalysisConfig.javamonitor-server/src/main/java/org/gridsuite/monitor/server/clients/ReportRestClient.javamonitor-server/src/main/java/org/gridsuite/monitor/server/clients/SecurityAnalysisRestClient.javamonitor-server/src/main/java/org/gridsuite/monitor/server/clients/StateEstimationRestClient.javamonitor-server/src/main/java/org/gridsuite/monitor/server/controllers/ProcessConfigController.javamonitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processconfig/SecurityAnalysisConfigMapper.javamonitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processexecution/ProcessExecutionMapper.javamonitor-server/src/main/java/org/gridsuite/monitor/server/mappers/processexecution/ProcessExecutionStepMapper.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/processconfig/ProcessConfigService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/result/providers/SecurityAnalysisResultProvider.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/result/providers/StateEstimationResultProvider.javamonitor-server/src/test/java/org/gridsuite/monitor/server/MonitorIntegrationTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/clients/ReportRestClientTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/clients/SecurityAnalysisRestClientTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/clients/StateEstimationRestClientTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/controllers/ProcessConfigControllerTest.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/processconfig/ProcessConfigServiceTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionServiceTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/result/providers/SecurityAnalysisResultProviderTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/result/providers/StateEstimationResultProviderTest.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/clients/FilterRestClient.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/clients/NetworkModificationRestClient.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/clients/ReportRestClient.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/clients/SecurityAnalysisRestClient.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/core/orchestrator/ProcessExecutor.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/messaging/ConsumerService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/messaging/NotificationService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/orchestrator/ProcessExecutionService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/orchestrator/StepExecutionService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/process/commons/steps/ApplyModificationsStep.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/process/commons/steps/CommonStepType.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/process/commons/steps/LoadNetworkStep.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/process/securityanalysis/SecurityAnalysisProcess.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/process/securityanalysis/SecurityAnalysisStepType.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/process/securityanalysis/steps/SecurityAnalysisRunComputationStep.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterService.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/clients/FilterRestClientTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/clients/NetworkModificationRestClientTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/clients/ReportRestClientTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/clients/SecurityAnalysisRestClientTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/context/ProcessExecutionContextTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/context/ProcessStepExecutionContextTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/core/process/ProcessTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/messaging/ConsumerServiceTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/messaging/NotificationServiceTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/orchestrator/ProcessExecutionServiceTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/orchestrator/StepExecutionServiceTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/process/commons/steps/ApplyModificationsStepTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/process/commons/steps/LoadNetworkStepTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/process/securityanalysis/SecurityAnalysisProcessTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/process/securityanalysis/steps/SecurityAnalysisRunComputationStepTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- monitor-server/src/main/java/org/gridsuite/monitor/server/controllers/ProcessConfigController.java
- monitor-commons/src/main/java/org/gridsuite/monitor/commons/api/types/processconfig/SecurityAnalysisConfig.java
- monitor-server/src/main/java/org/gridsuite/monitor/server/messaging/NotificationService.java
Comment on lines
36
to
39
| public FilterRestClient(@Value("${gridsuite.services.filter-server.base-uri:http://filter-server/}") String filterServerBaseUri, | ||
| RestTemplateBuilder restTemplateBuilder) { | ||
| this.filterServerBaseUri = filterServerBaseUri; | ||
| this.restTemplate = restTemplateBuilder.build(); |
There was a problem hiding this comment.
Normalize base URI/path joining to avoid //v1 URLs.
Line 36 default URI ends with /, while Line 43 path starts with /; concatenation on Line 48 can yield http://filter-server//v1/.... Please normalize once in the constructor (or remove trailing slash in the default).
Suggested fix
- public FilterRestClient(`@Value`("${gridsuite.services.filter-server.base-uri:http://filter-server/}") String filterServerBaseUri,
+ public FilterRestClient(`@Value`("${gridsuite.services.filter-server.base-uri:http://filter-server}") String filterServerBaseUri,
RestTemplateBuilder restTemplateBuilder) {
- this.filterServerBaseUri = filterServerBaseUri;
+ this.filterServerBaseUri = filterServerBaseUri.endsWith("/")
+ ? filterServerBaseUri.substring(0, filterServerBaseUri.length() - 1)
+ : filterServerBaseUri;
this.restTemplate = restTemplateBuilder.build();
}Also applies to: 43-48
🤖 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/clients/FilterRestClient.java`
around lines 36 - 39, The constructor in FilterRestClient sets
filterServerBaseUri (passed via `@Value`) which may include a trailing slash and
later URL concatenation produces double slashes; fix by normalizing
filterServerBaseUri inside the FilterRestClient constructor (trim any trailing
'/' except keep "http(s)://host/" semantics) so subsequent concatenation (where
you append paths like "/v1/...") always yields a single slash; update the
constructor that takes filterServerBaseUri and restTemplateBuilder to
sanitize/filterServerBaseUri (e.g., remove a trailing slash) before assigning
the field so all callers that build URLs from filterServerBaseUri get normalized
results.
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
Contributor
|
@thangqp @khouadrired can you do a last check? I started back from where Thang stopped. It's now up to date with main and tests are OK. I also update the doc to be less detailed on the current implementation. |
Contributor
Author
In the monitor-server, I have not yet introduced interface Notificator in a new core package, i.e. /core/messaging, which helps to avoid cycle dependencies between /messaging and /services. However we can do that later.. |
# Conflicts: # monitor-server/src/main/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionService.java # monitor-server/src/test/java/org/gridsuite/monitor/server/services/processexecution/ProcessExecutionServiceTest.java
antoinebhs
approved these changes
Mar 19, 2026
khouadrired
approved these changes
Mar 19, 2026
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
Remains: organise, move les tests