organize monitor-worker-server services #39
Conversation
📝 WalkthroughWalkthroughThis PR restructures the monitor-worker-server service layer by reorganizing classes into dedicated sub-packages (external/client, external/adapter, internal, messaging) and renames REST service classes to REST client classes. All dependent code and tests are updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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.
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/services/external/client/SecurityAnalysisRestClient.java (1)
31-33:⚠️ Potential issue | 🟡 MinorDouble-slash in URL construction.
The default base URI ends with
/(http://security-analysis-server/),DELIMITERis/, and the path fromUriComponentsBuilderstarts with/. This produces URLs likehttp://security-analysis-server//v1//results/{resultUuid}. While most HTTP clients and servers tolerate double slashes, this is fragile and could cause issues with stricter servers or proxies.If you adopt the
rootUripattern suggested above, this is resolved automatically. Otherwise, strip trailing slashes from the base URI or avoid leading slashes in the path.Also applies to: 46-52
🧹 Nitpick comments (6)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/external/client/ReportRestClient.java (1)
64-69:RestTemplate.exchangeerrors (non-JSON) are not caught.
sendReportonly catchesJsonProcessingException. HTTP errors fromrestTemplate.exchange(e.g.,RestClientExceptionon 4xx/5xx) will propagate as unchecked exceptions. This is pre-existing behavior, but worth noting as this class is now explicitly positioned as a REST client — callers should be aware of potentialRestClientExceptions.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/external/client/CaseRestClientTest.java (1)
27-34: Test is quite shallow — consider verifying the UUID is passed through.The
isInstanceOf(CaseDataSourceClient.class)assertion is redundant since the return type is compile-time enforced. The test currently only proves the constructor doesn't throw. Consider asserting that the returnedCaseDataSourceClientis actually wired with the correctcaseUuid(e.g., by inspecting a field or calling a method on it), which would give this test more value.That said, the
@RestClientTestwiring verification itself is useful.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/internal/StepExecutionService.java (1)
60-67: Verify report is not lost on failure path.When
step.execute(context)succeeds butreportRestClient.sendReport(...)on Line 62 throws, the step status is set toFAILED(Line 65) and the exception propagates. This means the step's work is done but the report is lost. This was the same behavior before the refactor (withreportService), so it's not a regression — but worth noting as a pre-existing reliability concern: the report send could be retried or handled separately from the step status.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/external/client/SecurityAnalysisRestClient.java (1)
28-39: Inconsistent REST client pattern vs.CaseRestClient.
CaseRestClientbakes the base URI into theRestTemplateviarestTemplateBuilder.rootUri(baseUri).build(), while this class stores the URI separately and manually prepends it at call time. Using the same pattern across all clients inexternal.clientwould improve consistency and reduce error surface.Also, the
@SetteronsecurityAnalysisServerBaseUrimakes this mutable on a singleton@Service— presumably for tests. Consider using constructor injection only (matchingCaseRestClient's approach) and using@RestClientTestor@TestPropertySourcein tests instead.♻️ Suggested alignment with CaseRestClient pattern
- private final RestTemplate restTemplate; - - `@Setter` - private String securityAnalysisServerBaseUri; - - private String getSecurityAnalysisServerBaseUri() { - return this.securityAnalysisServerBaseUri + DELIMITER + SA_API_VERSION + DELIMITER; - } - - 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(); - } + private final RestTemplate restTemplate; + + public SecurityAnalysisRestClient( + `@Value`("${gridsuite.services.security-analysis-server.base-uri:http://security-analysis-server/}") String securityAnalysisServerBaseUri, + RestTemplateBuilder restTemplateBuilder) { + this.restTemplate = restTemplateBuilder.rootUri(securityAnalysisServerBaseUri).build(); + }monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/external/client/FilterRestClient.java (1)
42-52:getBody()may returnnull.
restTemplate.exchange(...).getBody()can returnnull, which would propagate anulllist to callers. This pre-dates this PR, but since you're touching this class, consider adding a null guard or returning an empty list as a fallback.Proposed defensive null handling
try { - 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(); } catch (HttpStatusCodeException e) {monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/external/client/NetworkModificationRestClientTest.java (1)
77-85: Consider adding a test fornullinput.The production code uses
CollectionUtils.isNotEmpty(modificationsUuids)which also handlesnull. You have a test for an empty list but not fornullinput. A brief test would ensure the guard works for both cases.💡 Suggested test for null input
+ `@Test` + void getModificationsWithNull() { + List<ModificationInfos> resultListModifications = networkModificationRestClient.getModifications(null); + assertThat(resultListModifications).isEmpty(); + }
2 participants
based on #30