Conversation
Signed-off-by: Thang PHAM <phamthang37@gmail.com>
📝 WalkthroughWalkthroughRestructures packages across commons, server, and worker modules; renames service classes to "*RestClient"; replaces direct process execution with a step-based orchestration (ProcessExecutor/StepExecutor/Notificator); removes old core abstractions and adds new process/step interfaces and orchestrator implementations; updates imports, DTO/entity locations, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProcessExecutor
participant ProcessRegistry
participant ExecutionContext
participant StepExecutor
participant Step
participant Notificator
Client->>ProcessExecutor: executeProcess(ProcessRunMessage)
ProcessExecutor->>ProcessRegistry: resolveProcess(ProcessType)
ProcessRegistry-->>ProcessExecutor: Process instance
ProcessExecutor->>ExecutionContext: buildContext(runMessage, process)
ExecutionContext-->>ProcessExecutor: context
ProcessExecutor->>Notificator: updateExecutionStatus(id, RUNNING)
loop each step
ProcessExecutor->>Step: buildStepContext(step)
Step-->>ProcessExecutor: stepContext
ProcessExecutor->>Notificator: updateStepStatus(id, step RUNNING)
ProcessExecutor->>StepExecutor: executeStep(stepContext, step)
alt success
StepExecutor->>Notificator: updateStepStatus(id, step COMPLETED)
else failure
StepExecutor->>Notificator: updateStepStatus(id, step FAILED)
ProcessExecutor->>ProcessExecutor: mark remaining steps skipped
end
end
ProcessExecutor->>Notificator: updateExecutionStatus(id, COMPLETED/FAILED)
ProcessExecutor-->>Client: return / rethrow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.
Comment on lines
+99
to
+105
| try { | ||
| stepExecutor.executeStep(stepContext, step); | ||
| } catch (Exception e) { | ||
| process.onStepFailure(context, step, e); | ||
| skipRemaining = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Process can be marked COMPLETED after a step failure.
executeSteps swallows exceptions, so executeProcess will still set COMPLETED unless onStepFailure throws. Track a failure and propagate it (or return a failure flag) so ProcessStatus becomes FAILED.
🐛 Suggested fix
- boolean skipRemaining = false;
+ boolean skipRemaining = false;
+ Exception firstFailure = null;
@@
- } catch (Exception e) {
+ } 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);
+ }🤖 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 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.
Comment on lines
+90
to
+95
| StepExecutor stepExecutor = new StepExecutionService(notificationService, reportRestClient); | ||
| processExecutionService = new ProcessExecutionService(processList, stepExecutor, notificationService, EXECUTION_ENV_NAME); | ||
|
|
||
| loadNetworkStep = new LoadNetworkStep<>(networkConversionService); | ||
| applyModificationsStep = new ApplyModificationsStep<>(networkModificationService, networkModificationRestService, filterService); | ||
| runComputationStep = new SecurityAnalysisRunComputationStep(securityAnalysisService); | ||
| applyModificationsStep = new ApplyModificationsStep<>(networkModificationService, networkModificationRestClient, filterService); | ||
| runComputationStep = new SecurityAnalysisRunComputationStep(securityAnalysisRestClient); |
There was a problem hiding this comment.
Re-enable the failure-path test instead of disabling it.
@Disabled leaves failure handling unverified, and the commented-out stubs/verifications suggest the test needs an updated mocking strategy. Consider injecting a mocked StepExecutor (or spying ProcessExecutionService) to force a step-execution failure and assert FAILED status, then remove the commented blocks.
Also applies to: 103-132, 147-169, 180-180
🤖 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 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).
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary