[GRD-3891] refactor monitor worker server services structure #38
Conversation
📝 WalkthroughWalkthroughThis PR reorganizes services into internal, external.adapter, external.client, and messaging packages, renames many Service classes to RestClient variants, updates constructors/fields and tests to use the new types, and introduces a new CaseRestClient; some internal execution/status update flows were simplified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
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/test/java/org/gridsuite/monitor/worker/server/services/NetworkModificationServiceTest.java (1)
44-45:⚠️ Potential issue | 🟠 Major
@Mockannotation may not initialize withoutMockitoExtension.The
filterServicemock is declared with@Mockbut this test class uses@RestClientTestwithout@ExtendWith(MockitoExtension.class). The@Mockannotation requires Mockito initialization to work. SincefilterServiceis passed toapplyModificationsand used inverifycalls, this mock may benullat runtime causing test failures.Consider adding
@ExtendWith(MockitoExtension.class)or initializing mocks manually in a@BeforeEachmethod.🐛 Proposed fix
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; ... +@ExtendWith(MockitoExtension.class) `@RestClientTest`(NetworkModificationService.class) class NetworkModificationServiceTest {Or alternatively, initialize mocks manually:
+import org.junit.jupiter.api.BeforeEach; +import org.mockito.MockitoAnnotations; ... + `@BeforeEach` + void setUp() { + MockitoAnnotations.openMocks(this); + }
🤖 Fix all issues with AI agents
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/external/client/CaseRestClient.java`:
- Around line 25-27: The property key used in the CaseRestClient constructor is
inconsistent: replace the
'@Value("${powsybl.services.case-server.base-uri:http://case-server/}")' usage
with the gridsuite prefix to match other clients; update the CaseRestClient
constructor parameter annotation to
'@Value("${gridsuite.services.case-server.base-uri:http://case-server/}")'
(keeping the same default) so the RestTemplateBuilder.rootUri(...) receives the
correctly named property (refer to the CaseRestClient constructor and the
caseServerBaseUri parameter).
🧹 Nitpick comments (3)
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/external/client/CaseRestClientTest.java (1)
27-34: Test coverage is minimal and assertion is partially redundant.The test only verifies that
getCaseDataSourcereturns a non-null object. TheisInstanceOf(CaseDataSourceClient.class)check is redundant since the method's return type already guarantees this at compile time. Consider enhancing this test to verify the actual behavior, such as usingMockRestServiceServerto validate that the returnedCaseDataSourceClientmakes correct HTTP calls when used.♻️ Suggested simplified assertion
- assertThat(result).isNotNull().isInstanceOf(CaseDataSourceClient.class); + assertThat(result).isNotNull();monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/external/client/SecurityAnalysisRestClient.java (2)
35-40: Consider removing@SetteronsecurityAnalysisServerBaseUrior documenting its purpose.The
@Setterannotation onsecurityAnalysisServerBaseUriallows external mutation of this field after construction, which is unusual for an immutable service bean. If this is intended for testing purposes, consider using constructor injection with test-specific configuration instead, or add a comment explaining the rationale.
42-47: InconsistentRestTemplateconfiguration compared toCaseRestClient.
CaseRestClientconfigures theRestTemplatewithrootUri()at construction time, while this class builds a plainRestTemplateand manually concatenates the base URI ingetSecurityAnalysisServerBaseUri(). Consider aligning the approach for consistency and to leverage Spring's URI template handling.♻️ Suggested refactor to use rootUri
- public SecurityAnalysisRestClient( - RestTemplateBuilder restTemplateBuilder, - `@Value`("${gridsuite.services.security-analysis-server.base-uri:http://security-analysis-server/}") String securityAnalysisServerBaseUri) { - this.securityAnalysisServerBaseUri = securityAnalysisServerBaseUri; - this.restTemplate = restTemplateBuilder.build(); + public SecurityAnalysisRestClient( + RestTemplateBuilder restTemplateBuilder, + `@Value`("${gridsuite.services.security-analysis-server.base-uri:http://security-analysis-server/}") String securityAnalysisServerBaseUri) { + this.securityAnalysisServerBaseUri = securityAnalysisServerBaseUri; + this.restTemplate = restTemplateBuilder.rootUri(securityAnalysisServerBaseUri).build(); }
| public CaseRestClient(@Value("${powsybl.services.case-server.base-uri:http://case-server/}") String caseServerBaseUri, | ||
| RestTemplateBuilder restTemplateBuilder) { | ||
| this.caseServerRest = restTemplateBuilder.rootUri(caseServerBaseUri).build(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all `@Value` annotations with service base-uri properties
rg -n '@Value.*services.*base-uri' --type java -A 1Repository: gridsuite/monitor-core
Length of output: 3236
Use gridsuite.services.case-server.base-uri instead of powsybl.services.case-server.base-uri.
All other REST clients in the codebase use the gridsuite.services.*.base-uri property prefix (SecurityAnalysisRestClient, ReportRestClient, NetworkModificationRestClient, FilterRestClient). Align this property to the same convention for consistency.
🤖 Prompt for AI Agents
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/external/client/CaseRestClient.java`
around lines 25 - 27, The property key used in the CaseRestClient constructor is
inconsistent: replace the
'@Value("${powsybl.services.case-server.base-uri:http://case-server/}")' usage
with the gridsuite prefix to match other clients; update the CaseRestClient
constructor parameter annotation to
'@Value("${gridsuite.services.case-server.base-uri:http://case-server/}")'
(keeping the same default) so the RestTemplateBuilder.rootUri(...) receives the
correctly named property (refer to the CaseRestClient constructor and the
caseServerBaseUri parameter).
khouadrired
force-pushed
the
GRD-3891-refactor-monitor-worker-server-services-structure
branch
from
d154977 to
90b7085
Compare
khouadrired
force-pushed
the
GRD-3891-refactor-monitor-worker-server-services-structure
branch
from
90b7085 to
9b47571
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/external/adapter/NetworkModificationServiceTest.java (1)
37-46:⚠️ Potential issue | 🟡 MinorPre-existing bug:
@Mockfields are never initialized.
@RestClientTestloads a Spring context but does not activate Mockito annotation processing. The@Mock-annotatedfilterServiceandnetworkfields will remainnullthroughout both tests. The tests happen to pass becausenullis a valid argument for mock interactions, but assertions likeverify(…).check(network)are silently verifyingcheck(null)rather thancheck(<mock>).Consider adding
@ExtendWith(MockitoExtension.class)to actually initialize the mocks, or just use localmock(…)calls (as already done forreportNode) to keep things explicit.Proposed fix
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; + `@RestClientTest`(NetworkModificationService.class) +@ExtendWith(MockitoExtension.class) class NetworkModificationServiceTest {monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/internal/ProcessExecutionService.java (1)
71-79:⚠️ Potential issue | 🔴 CriticalCompilation error:
ProcessExecutionStatusUpdatehas no 3-argument constructor.The pipeline failure confirms this.
ProcessExecutionStatusUpdate(via Lombok@AllArgsConstructor) expects four arguments:(ProcessStatus, String, Instant startedAt, Instant completedAt). The current code passes only three.Additionally, the
RUNNINGstatus should setstartedAtand the terminal statuses (COMPLETED/FAILED) should setcompletedAt.🐛 Proposed fix
private void updateExecutionStatus(UUID executionId, String envName, ProcessStatus status) { ProcessExecutionStatusUpdate processExecutionStatusUpdate = new ProcessExecutionStatusUpdate( status, envName, - status == ProcessStatus.COMPLETED || status == ProcessStatus.FAILED ? Instant.now() : null + status == ProcessStatus.RUNNING ? Instant.now() : null, + status == ProcessStatus.COMPLETED || status == ProcessStatus.FAILED ? Instant.now() : null ); notificationService.updateExecutionStatus(executionId, processExecutionStatusUpdate); }Alternatively, using the builder for clarity:
ProcessExecutionStatusUpdate.builder() .status(status) .executionEnvName(envName) .startedAt(status == ProcessStatus.RUNNING ? Instant.now() : null) .completedAt(status == ProcessStatus.COMPLETED || status == ProcessStatus.FAILED ? Instant.now() : null) .build();monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/external/client/NetworkModificationRestClient.java (1)
40-53:⚠️ Potential issue | 🔴 CriticalTest expectations do not match production code's API request pattern.
The
getModificationsmethod constructs a single bulk request withuuidsas query parameters:/v1/network-composite-modifications/network-modifications?uuids=<uuid1>&uuids=<uuid2>&onlyMetadata=falseHowever, the test in
NetworkModificationRestClientTest.java(lines 65-76) sets up expectations for separate per-UUID requests:/v1/network-composite-modification/<uuid>/network-modifications?onlyMetadata=falseWhen
getModifications(Arrays.asList({uuid1, uuid2}))is called at line 78, the production code makes one bulk request, butMockRestServiceServer.verify()at line 53 expects two separate requests. The test will fail because:
- Endpoint path differs:
network-composite-modifications(plural) vsnetwork-composite-modification(singular)- Request pattern differs: query-param-based bulk vs path-segment-based per-UUID
Update the test expectations to match the production code's bulk request approach, or refactor the production code to iterate per-UUID if that's the intended API contract.
🤖 Fix all issues with AI agents
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/external/client/CaseRestClient.java`:
- Around line 1-6: The file CaseRestClient is missing the SPDX license
identifier in its header; update the file header in CaseRestClient.java to
include the SPDX-License-Identifier: MPL-2.0 comment (matching the other
new/modified files such as NetworkModificationRestClient and FilterRestClient)
so the top-of-file license block includes the SPDX line immediately after the
copyright/license text.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/external/client/NetworkModificationRestClientTest.java`:
- Around line 56-80: The test stubs per-UUID endpoints but production
NetworkModificationRestClient.getModifications issues a single bulk call; update
NetworkModificationRestClientTest.getModifications to expect one GET to the bulk
URL used by the client
("/v1/network-composite-modifications/network-modifications?uuids=<comma-separated-UUIDs>&onlyMetadata=false")
instead of per-UUID MockRestRequestMatchers.requestTo entries, and return the
combined JSON array for that single request so the MockRestServiceServer matches
the actual call.
🧹 Nitpick comments (5)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/securityanalysis/steps/SecurityAnalysisRunComputationStep.java (1)
43-47: Pre-existing FIXME comments on parameter and contingency handling.Two
FIXMEcomments indicate that parameter retrieval (line 43) and contingency fetching (line 46) are placeholder implementations. The current code hardcodesLineContingencyfor every contingency ID, which won't be correct for other contingency types. These are out of scope for this refactor but worth tracking.Would you like me to open an issue to track resolving these FIXMEs?
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/internal/StepExecutionService.java (1)
61-68: Pre-existing:sendReportfailure marks step as FAILED despite successful execution.If
reportRestClient.sendReport()on line 63 throws (e.g., network error to the report server), thecatchblock marks the step asFAILEDeven thoughstep.execute(context)completed successfully. This is pre-existing behavior, but worth considering whethersendReportfailures should be handled separately (e.g., logged/retried) rather than failing the entire step.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/internal/ProcessExecutionServiceTest.java (2)
76-85: Consider verifyingstartedAtis set for theRUNNINGstatus update.Once the constructor bug in
ProcessExecutionServiceis fixed (to passstartedAtforRUNNING), this test should also assert thatstartedAt != nullfor theRUNNINGupdate andstartedAt == nullfor theCOMPLETEDupdate to fully validate the timestamp contract.♻️ Proposed enhancement
inOrder.verify(notificationService).updateExecutionStatus(eq(executionId), argThat(update -> update.getStatus() == ProcessStatus.RUNNING && update.getExecutionEnvName().equals(EXECUTION_ENV_NAME) && - update.getCompletedAt() == null + update.getStartedAt() != null && + update.getCompletedAt() == null )); inOrder.verify(notificationService).updateExecutionStatus(eq(executionId), argThat(update -> update.getStatus() == ProcessStatus.COMPLETED && update.getExecutionEnvName().equals(EXECUTION_ENV_NAME) && + update.getStartedAt() == null && update.getCompletedAt() != null ));
27-28: Minor: mixed assertion styles (assertThrowsvsassertThatThrownBy).Line 97 uses JUnit's
assertThrowswhile Line 116 uses AssertJ'sassertThatThrownBy. Consider picking one style for consistency.assertThatThrownByis generally preferred when already using AssertJ, as it allows fluent chaining of message/cause assertions.Also applies to: 97-97, 116-116
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/external/adapter/NetworkConversionServiceTest.java (1)
91-102: Consider verifyinggetCaseDataSourceis still called in the error path.The happy-path test verifies
caseRestClient.getCaseDataSource(caseUuid)(Line 81), but this verification is missing in the error-path test. Adding it would confirm the full call chain before the exception.Suggested addition
assertThatThrownBy(() -> networkConversionService.createNetwork(caseUuid, reportNode)) .isInstanceOf(PowsyblException.class) .hasMessage("No importer found"); + verify(caseRestClient).getCaseDataSource(caseUuid); }
| /** | ||
| * Copyright (c) 2026, RTE (http://www.rte-france.com) | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | ||
| */ |
There was a problem hiding this comment.
Missing SPDX license identifier.
Other files modified or added in this PR include SPDX-License-Identifier: MPL-2.0 in the license header (e.g., NetworkModificationRestClient.java line 6, FilterRestClient.java line 6). This new file omits it.
Proposed fix
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ * SPDX-License-Identifier: MPL-2.0
*/📝 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.
| /** | |
| * Copyright (c) 2026, RTE (http://www.rte-france.com) | |
| * This Source Code Form is subject to the terms of the Mozilla Public | |
| * License, v. 2.0. If a copy of the MPL was not distributed with this | |
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | |
| */ | |
| /** | |
| * Copyright (c) 2026, RTE (http://www.rte-france.com) | |
| * This Source Code Form is subject to the terms of the Mozilla Public | |
| * License, v. 2.0. If a copy of the MPL was not distributed with this | |
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. | |
| * SPDX-License-Identifier: MPL-2.0 | |
| */ |
🤖 Prompt for AI Agents
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/external/client/CaseRestClient.java`
around lines 1 - 6, The file CaseRestClient is missing the SPDX license
identifier in its header; update the file header in CaseRestClient.java to
include the SPDX-License-Identifier: MPL-2.0 comment (matching the other
new/modified files such as NetworkModificationRestClient and FilterRestClient)
so the top-of-file license block includes the SPDX line immediately after the
copyright/license text.
| @Test | ||
| void getModifications() { | ||
| UUID[] modificationUuids = {MODIFICATION_1_UUID, MODIFICATION_2_UUID}; | ||
|
|
||
| ModificationInfos modificationInfos1 = LoadModificationInfos.builder().equipmentId("load1").q0(new AttributeModification<>(300., OperationType.SET)).build(); | ||
| ModificationInfos modificationInfos2 = LoadModificationInfos.builder().equipmentId("load2").q0(new AttributeModification<>(null, OperationType.UNSET)).build(); | ||
|
|
||
| List<ModificationInfos> modificationInfos = List.of(modificationInfos1, modificationInfos2); | ||
| ModificationInfos[] modificationsArray = modificationInfos.toArray(ModificationInfos[]::new); | ||
| ModificationInfos[] modificationInfos = {modificationInfos1, modificationInfos2}; | ||
|
|
||
| try { | ||
| server.expect(MockRestRequestMatchers.method(HttpMethod.GET)) | ||
| .andExpect(MockRestRequestMatchers.requestTo("http://network-modification-server/v1/network-composite-modifications/network-modifications?uuids=" + MODIFICATION_1_UUID + "&uuids=" + MODIFICATION_2_UUID + "&onlyMetadata=false")) | ||
| .andRespond(MockRestResponseCreators.withSuccess() | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .body(objectMapper.writeValueAsString(modificationsArray))); | ||
| } catch (JsonProcessingException e) { | ||
| throw new RuntimeException(e); | ||
| for (int i = 0; i < modificationUuids.length; i++) { | ||
| ModificationInfos[] modificationsArray = {modificationInfos[i]}; | ||
| try { | ||
| server.expect(MockRestRequestMatchers.method(HttpMethod.GET)) | ||
| .andExpect(MockRestRequestMatchers.requestTo("http://network-modification-server/v1/network-composite-modification/" + modificationUuids[i] + "/network-modifications?onlyMetadata=false")) | ||
| .andRespond(MockRestResponseCreators.withSuccess() | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| .body(objectMapper.writeValueAsString(modificationsArray))); | ||
| } catch (JsonProcessingException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
|
|
||
| List<ModificationInfos> resultListModifications = networkModificationRestService.getModifications(List.of(MODIFICATION_1_UUID, MODIFICATION_2_UUID)); | ||
| assertThat(resultListModifications).usingRecursiveComparison().isEqualTo(modificationInfos); | ||
| List<ModificationInfos> resultListModifications = networkModificationRestClient.getModifications(Arrays.asList(modificationUuids)); | ||
| assertThat(resultListModifications).usingRecursiveComparison().isEqualTo(Arrays.asList(modificationInfos)); | ||
| } |
There was a problem hiding this comment.
Test URL expectations don't match the production getModifications implementation.
The test stubs per-UUID requests at path /v1/network-composite-modification/{uuid}/network-modifications?onlyMetadata=false (line 69), but the production code in NetworkModificationRestClient.getModifications constructs a single bulk request to /v1/network-composite-modifications/network-modifications?uuids=...&onlyMetadata=false.
This test will fail at runtime because MockRestServiceServer will never see requests matching these per-UUID stubs. This is the same root-cause issue flagged in NetworkModificationRestClient.java — the endpoint pattern must be reconciled between the two files.
🤖 Prompt for AI Agents
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/external/client/NetworkModificationRestClientTest.java`
around lines 56 - 80, The test stubs per-UUID endpoints but production
NetworkModificationRestClient.getModifications issues a single bulk call; update
NetworkModificationRestClientTest.getModifications to expect one GET to the bulk
URL used by the client
("/v1/network-composite-modifications/network-modifications?uuids=<comma-separated-UUIDs>&onlyMetadata=false")
instead of per-UUID MockRestRequestMatchers.requestTo entries, and return the
combined JSON array for that single request so the MockRestServiceServer matches
the actual call.
2 participants
based on the PR : #30