Homogenise rest services and replace restTemplate with restClient#67
Merged
antoinebhs merged 6 commits intomainfrom Mar 16, 2026
Merged
Homogenise rest services and replace restTemplate with restClient#67antoinebhs merged 6 commits intomainfrom
antoinebhs merged 6 commits intomainfrom
Conversation
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughA systematic migration from Spring's RestTemplate to RestClient across multiple service classes in the monitor codebase. Updates constructors to accept RestClient.Builder instead of RestTemplateBuilder, refactors HTTP calls to use RestClient's fluent API, and introduces a new ReportRestService while updating dependent components. Exception handling is simplified by relying on RestClient's built-in error propagation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
🧹 Nitpick comments (7)
monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/FilterRestServiceTest.java (1)
90-99: Test name does not match error scenario.Consistent with other test files in this PR,
getFiltersNotFoundsimulates HTTP 500 viawithServerError(), not HTTP 404. Consider renaming togetFiltersServerErrorfor consistency and clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/FilterRestServiceTest.java` around lines 90 - 99, Rename the misleading test method getFiltersNotFound to getFiltersServerError in the FilterRestServiceTest class to reflect the simulated HTTP 500 scenario; update the test method name and any references to it (keeping the existing assertions that call filterRestService.getFilters and use FILTER_ERROR_UUID) so the test name matches the withServerError() behavior and remains consistent with other tests.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestService.java (1)
29-35: Same potential double slash issue in base URL construction.Consistent with other services, normalize the base URI to prevent double slashes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestService.java` around lines 29 - 35, The constructor SecurityAnalysisRestService currently concatenates securityAnalysisServerBaseUri + DELIMITER + SA_API_VERSION which can produce a double slash; update the constructor to normalize the incoming securityAnalysisServerBaseUri (trim any trailing '/') before passing into restClientBuilder.baseUrl(...) or use a safe join/URI resolve approach so DELIMITER and SA_API_VERSION are appended without duplicating slashes; ensure you adjust the code around restClientBuilder, baseUrl, DELIMITER and SA_API_VERSION references so the final base URL is always well-formed.monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterRestService.java (1)
30-34: Potential double slash in base URL construction.If
filterServerBaseUriends with a trailing slash (which is common, e.g.,http://filter-server/), thenfilterServerBaseUri + DELIMITER + FILTER_SERVER_API_VERSIONproduceshttp://filter-server//v1. Consider normalizing the URI or usingUriComponentsBuilderfor safer composition.♻️ Proposed fix to handle trailing slash
public FilterRestService(`@Value`("${gridsuite.services.filter-server.base-uri:http://filter-server/}") String filterServerBaseUri, RestClient.Builder restClientBuilder) { + String normalizedUri = filterServerBaseUri.endsWith(DELIMITER) + ? filterServerBaseUri.substring(0, filterServerBaseUri.length() - 1) + : filterServerBaseUri; this.restClient = restClientBuilder - .baseUrl(filterServerBaseUri + DELIMITER + FILTER_SERVER_API_VERSION) + .baseUrl(normalizedUri + DELIMITER + FILTER_SERVER_API_VERSION) .build(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterRestService.java` around lines 30 - 34, The constructor in FilterRestService currently concatenates filterServerBaseUri + DELIMITER + FILTER_SERVER_API_VERSION which can produce a double slash if filterServerBaseUri ends with '/' (e.g., http://filter-server/ + / + v1 -> //v1); fix by normalizing the base URI before building the RestClient: trim any trailing '/' from filterServerBaseUri or use a URI builder (e.g., UriComponentsBuilder) to join paths safely, then pass the normalized base URL to restClientBuilder.baseUrl(...) so restClient is built with a single correct slash between host and FILTER_SERVER_API_VERSION.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestServiceTest.java (1)
96-106: Test name does not match error scenario.Similar to
LoadFlowRestServiceTest, this test is namedgetParametersNotFoundbut simulates an HTTP 500 error withwithServerError(). Consider renaming togetParametersServerErrorfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestServiceTest.java` around lines 96 - 106, The test method name getParametersNotFound in SecurityAnalysisRestServiceTest does not match the simulated HTTP 500 server error; rename the test to reflect the actual scenario (e.g., getParametersServerError) and update any references or annotations accordingly so it clearly corresponds to the server error path exercised by the call to securityAnalysisRestService.getParameters(PARAMETERS_ERROR_UUID) which uses MockRestResponseCreators.withServerError().monitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisService.java (1)
27-33: Same potential double slash issue in base URL construction.Similar to
FilterRestService, ifsecurityAnalysisServerBaseUriends with/, this will produce a double slash. Consider normalizing the URI consistently across all services in this PR.🤖 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/SecurityAnalysisService.java` around lines 27 - 33, Normalize the base URI before building the RestClient to avoid double slashes: in the SecurityAnalysisService constructor, sanitize securityAnalysisServerBaseUri (used with restClientBuilder, DELIMITER and SA_API_VERSION) by trimming any trailing '/' (or ensuring exactly one '/' between base and DELIMITER) and then call .baseUrl(...) with the normalized string so concatenation of securityAnalysisServerBaseUri + DELIMITER + SA_API_VERSION cannot produce a double slash.monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestServiceTest.java (1)
68-77: Test name does not match the simulated error scenario.The test is named
getParametersNotFound, which implies a 404 Not Found scenario. However,MockRestResponseCreators.withServerError()returns HTTP 500 Internal Server Error. Consider renaming togetParametersServerErroror usingwithResourceNotFound()if you intend to test a 404 scenario.🤖 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/LoadFlowRestServiceTest.java` around lines 68 - 77, The test method getParametersNotFound is misnamed versus the simulated response: either rename the test to getParametersServerError to reflect that MockRestResponseCreators.withServerError() returns a 500, or change the mock to MockRestResponseCreators.withResourceNotFound() to simulate a 404 and keep the name; update the assertion if you switch to 404 to expect HttpClientErrorException.NotFound when calling loadFlowRestService.getParameters(PARAMETERS_ERROR_UUID).monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkModificationRestService.java (1)
31-36: Same potential double slash issue in base URL construction.Consistent with other services in this PR, consider normalizing the base URI to avoid potential double slashes when the property value includes a trailing slash.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkModificationRestService.java` around lines 31 - 36, The constructor in NetworkModificationRestService currently concatenates networkModificationServerBaseUri + DELIMITER + NETWORK_MODIFICATION_SERVER_API_VERSION which can produce a double slash if the injected property ends with a slash; update the constructor to normalize networkModificationServerBaseUri (trim any trailing '/') before concatenation or use a safe join approach when calling restClientBuilder.baseUrl so networkModificationServerRest is built with a single slash between the base URI and NETWORK_MODIFICATION_SERVER_API_VERSION (refer to the constructor, networkModificationServerRest, DELIMITER and NETWORK_MODIFICATION_SERVER_API_VERSION).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestService.java`:
- Around line 47-54: The getParameters method in SecurityAnalysisRestService
currently adds a hardcoded header "userId" with value "user1"; remove this
hardcoded header (i.e., delete the .header("userId", "user1") call) to match
other RestService classes, or if a userId is actually required change the method
signature of getParameters(UUID securityAnalysisParametersUuid, String userId)
and use that parameter in the .header(...) call, or alternatively obtain the
user id from the security context (e.g., SecurityContextHolder) before calling
restClient; update any callers of getParameters accordingly if you choose the
parameter or security-context approach.
---
Nitpick comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisService.java`:
- Around line 27-33: Normalize the base URI before building the RestClient to
avoid double slashes: in the SecurityAnalysisService constructor, sanitize
securityAnalysisServerBaseUri (used with restClientBuilder, DELIMITER and
SA_API_VERSION) by trimming any trailing '/' (or ensuring exactly one '/'
between base and DELIMITER) and then call .baseUrl(...) with the normalized
string so concatenation of securityAnalysisServerBaseUri + DELIMITER +
SA_API_VERSION cannot produce a double slash.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterRestService.java`:
- Around line 30-34: The constructor in FilterRestService currently concatenates
filterServerBaseUri + DELIMITER + FILTER_SERVER_API_VERSION which can produce a
double slash if filterServerBaseUri ends with '/' (e.g., http://filter-server/ +
/ + v1 -> //v1); fix by normalizing the base URI before building the RestClient:
trim any trailing '/' from filterServerBaseUri or use a URI builder (e.g.,
UriComponentsBuilder) to join paths safely, then pass the normalized base URL to
restClientBuilder.baseUrl(...) so restClient is built with a single correct
slash between host and FILTER_SERVER_API_VERSION.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkModificationRestService.java`:
- Around line 31-36: The constructor in NetworkModificationRestService currently
concatenates networkModificationServerBaseUri + DELIMITER +
NETWORK_MODIFICATION_SERVER_API_VERSION which can produce a double slash if the
injected property ends with a slash; update the constructor to normalize
networkModificationServerBaseUri (trim any trailing '/') before concatenation or
use a safe join approach when calling restClientBuilder.baseUrl so
networkModificationServerRest is built with a single slash between the base URI
and NETWORK_MODIFICATION_SERVER_API_VERSION (refer to the constructor,
networkModificationServerRest, DELIMITER and
NETWORK_MODIFICATION_SERVER_API_VERSION).
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestService.java`:
- Around line 29-35: The constructor SecurityAnalysisRestService currently
concatenates securityAnalysisServerBaseUri + DELIMITER + SA_API_VERSION which
can produce a double slash; update the constructor to normalize the incoming
securityAnalysisServerBaseUri (trim any trailing '/') before passing into
restClientBuilder.baseUrl(...) or use a safe join/URI resolve approach so
DELIMITER and SA_API_VERSION are appended without duplicating slashes; ensure
you adjust the code around restClientBuilder, baseUrl, DELIMITER and
SA_API_VERSION references so the final base URL is always well-formed.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/FilterRestServiceTest.java`:
- Around line 90-99: Rename the misleading test method getFiltersNotFound to
getFiltersServerError in the FilterRestServiceTest class to reflect the
simulated HTTP 500 scenario; update the test method name and any references to
it (keeping the existing assertions that call filterRestService.getFilters and
use FILTER_ERROR_UUID) so the test name matches the withServerError() behavior
and remains consistent with other tests.
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestServiceTest.java`:
- Around line 68-77: The test method getParametersNotFound is misnamed versus
the simulated response: either rename the test to getParametersServerError to
reflect that MockRestResponseCreators.withServerError() returns a 500, or change
the mock to MockRestResponseCreators.withResourceNotFound() to simulate a 404
and keep the name; update the assertion if you switch to 404 to expect
HttpClientErrorException.NotFound when calling
loadFlowRestService.getParameters(PARAMETERS_ERROR_UUID).
In
`@monitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestServiceTest.java`:
- Around line 96-106: The test method name getParametersNotFound in
SecurityAnalysisRestServiceTest does not match the simulated HTTP 500 server
error; rename the test to reflect the actual scenario (e.g.,
getParametersServerError) and update any references or annotations accordingly
so it clearly corresponds to the server error path exercised by the call to
securityAnalysisRestService.getParameters(PARAMETERS_ERROR_UUID) which uses
MockRestResponseCreators.withServerError().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b7b59df-7e42-4271-a1ea-5afec21fb7cf
📒 Files selected for processing (13)
monitor-server/src/main/java/org/gridsuite/monitor/server/services/ReportService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ActionsRestService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterRestService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkConversionService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkModificationRestService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ReportService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestService.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ActionsRestServiceTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/FilterRestServiceTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestServiceTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestServiceTest.java
Comment on lines
47
to
54
| public SecurityAnalysisParametersValues getParameters(UUID securityAnalysisParametersUuid) { | ||
| LOGGER.info("Get security analysis parameters {}", securityAnalysisParametersUuid); | ||
|
|
||
| var path = securityAnalysisServerBaseUri + UriComponentsBuilder.fromPath(DELIMITER + SA_API_VERSION + DELIMITER + "parameters/{uuid}") | ||
| .buildAndExpand(securityAnalysisParametersUuid) | ||
| .toUriString(); | ||
|
|
||
| try { | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.set("userId", "user1"); // TODO : to remove after the fix that will remove the userId header when getting parameters | ||
| HttpEntity<?> requestEntity = new HttpEntity<>(headers); | ||
|
|
||
| return restTemplate.exchange(path, HttpMethod.GET, requestEntity, SecurityAnalysisParametersValues.class).getBody(); | ||
| } catch (RestClientException e) { | ||
| throw new PowsyblException("Error retrieving security analysis parameters for UUID: " + securityAnalysisParametersUuid + " - " + e.getMessage(), e); | ||
| } | ||
| return restClient.get() | ||
| .uri("/parameters/{securityAnalysisParametersUuid}", | ||
| securityAnalysisParametersUuid) | ||
| .header("userId", "user1") | ||
| .retrieve() | ||
| .body(SecurityAnalysisParametersValues.class); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how getParameters is called to understand where userId should come from
ast-grep --pattern 'getParameters($$$)'Repository: gridsuite/monitor-core
Length of output: 48
🏁 Script executed:
rg -t java 'getParameters' --context 3Repository: gridsuite/monitor-core
Length of output: 15211
🏁 Script executed:
rg -t java 'userId' monitor-worker-server/src --context 2Repository: gridsuite/monitor-core
Length of output: 2686
🏁 Script executed:
cat -n monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestService.javaRepository: gridsuite/monitor-core
Length of output: 2546
🏁 Script executed:
find monitor-worker-server/src/main/java -name "*RestService.java" -type fRepository: gridsuite/monitor-core
Length of output: 711
🏁 Script executed:
rg -t java 'header.*userId' monitor-worker-server/src/main/javaRepository: gridsuite/monitor-core
Length of output: 218
🏁 Script executed:
head -60 monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/LoadFlowRestService.javaRepository: gridsuite/monitor-core
Length of output: 1670
🏁 Script executed:
head -50 monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ActionsRestService.javaRepository: gridsuite/monitor-core
Length of output: 1934
🏁 Script executed:
head -50 monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkModificationRestService.javaRepository: gridsuite/monitor-core
Length of output: 2349
Hardcoded userId header value should not be in production code.
The getParameters method hardcodes "user1" as the userId header value. This appears to be a placeholder value and is inconsistent with other RestService classes in the codebase (LoadFlowRestService, ActionsRestService, NetworkModificationRestService) which do not use userId headers.
The actual user ID should either be:
- Removed if not required (following the pattern of LoadFlowRestService)
- Passed as a method parameter if needed
- Retrieved from the security context
Proposed fix to accept userId as parameter
-public SecurityAnalysisParametersValues getParameters(UUID securityAnalysisParametersUuid) {
+public SecurityAnalysisParametersValues getParameters(UUID securityAnalysisParametersUuid, String userId) {
return restClient.get()
.uri("/parameters/{securityAnalysisParametersUuid}",
securityAnalysisParametersUuid)
- .header("userId", "user1")
+ .header("userId", userId)
.retrieve()
.body(SecurityAnalysisParametersValues.class);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/SecurityAnalysisRestService.java`
around lines 47 - 54, The getParameters method in SecurityAnalysisRestService
currently adds a hardcoded header "userId" with value "user1"; remove this
hardcoded header (i.e., delete the .header("userId", "user1") call) to match
other RestService classes, or if a userId is actually required change the method
signature of getParameters(UUID securityAnalysisParametersUuid, String userId)
and use that parameter in the .header(...) call, or alternatively obtain the
user id from the security context (e.g., SecurityContextHolder) before calling
restClient; update any callers of getParameters accordingly if you choose the
parameter or security-context approach.
antoinebhs
reviewed
Mar 13, 2026
...-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/ReportService.java
Outdated
Show resolved
Hide resolved
...ver/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkConversionService.java
Show resolved
Hide resolved
monitor-server/src/main/java/org/gridsuite/monitor/server/services/ReportRestService.java
Show resolved
Hide resolved
...er-server/src/main/java/org/gridsuite/monitor/worker/server/services/ActionsRestService.java
Show resolved
Hide resolved
...ker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterRestService.java
Outdated
Show resolved
Hide resolved
monitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisService.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/gridsuite/monitor/worker/server/services/NetworkModificationRestService.java
Outdated
Show resolved
Hide resolved
antoinebhs
reviewed
Mar 13, 2026
...ver/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkConversionService.java
Show resolved
Hide resolved
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monitor-server/src/main/java/org/gridsuite/monitor/server/services/ReportRestService.java (1)
34-39: Consider using.body(ReportPage.class)for non-generic types.
ParameterizedTypeReferenceis typically needed for generic types (e.g.,List<ReportPage>). SinceReportPageis not a generic type, you can simplify this to:return restClient.get() .uri("/{reportId}/logs", reportId) .retrieve() .body(ReportPage.class);This is functionally equivalent but more concise.
🤖 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/ReportRestService.java` around lines 34 - 39, The call in ReportRestService.getReport uses a ParameterizedTypeReference for a non-generic return type; change the final retrieval to use .body(ReportPage.class) instead of .body(new ParameterizedTypeReference<>() { }) so getReport(UUID reportId) returns a ReportPage more concisely — update the fluent chain on restClient.get().uri("/{reportId}/logs", reportId).retrieve() to call .body(ReportPage.class).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@monitor-server/src/main/java/org/gridsuite/monitor/server/services/ReportRestService.java`:
- Around line 34-39: The call in ReportRestService.getReport uses a
ParameterizedTypeReference for a non-generic return type; change the final
retrieval to use .body(ReportPage.class) instead of .body(new
ParameterizedTypeReference<>() { }) so getReport(UUID reportId) returns a
ReportPage more concisely — update the fluent chain on
restClient.get().uri("/{reportId}/logs", reportId).retrieve() to call
.body(ReportPage.class).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c36b5d9-e686-4eed-b6c0-fde9bf4cac81
📒 Files selected for processing (10)
monitor-server/src/main/java/org/gridsuite/monitor/server/services/MonitorService.javamonitor-server/src/main/java/org/gridsuite/monitor/server/services/ReportRestService.javamonitor-server/src/test/java/org/gridsuite/monitor/server/MonitorIntegrationTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/MonitorServiceTest.javamonitor-server/src/test/java/org/gridsuite/monitor/server/services/ReportRestServiceTest.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/processes/commons/steps/LoadNetworkStep.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/CaseRestService.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/processes/commons/steps/LoadNetworkStepTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/CaseRestServiceTest.javamonitor-worker-server/src/test/java/org/gridsuite/monitor/worker/server/services/ProcessExecutionServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterRestService.java (1)
37-43: Return an empty list when response body is absent.
retrieve().body(...)may returnnull; returningList.of()here prevents downstream null handling issues forgetFilters.Proposed fix
public List<AbstractFilter> getFilters(List<UUID> filtersUuids) { - return restClient.get() + List<AbstractFilter> filters = restClient.get() .uri(uriBuilder -> uriBuilder .path("/filters/metadata") .queryParam("ids", filtersUuids.toArray()) .build()) .retrieve() .body(new ParameterizedTypeReference<>() { }); + return filters != null ? filters : 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/services/FilterRestService.java` around lines 37 - 43, The call returning the filters list from the restClient chain can yield null; after invoking the retrieve().body(...) call in FilterRestService (the restClient.get()... .retrieve().body(...) expression), capture the result into a variable and return Optional.ofNullable(result).orElse(List.of()) (or explicitly check for null and return List.of()) so the method never returns null and downstream getFilters callers receive an empty list instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterRestService.java`:
- Around line 32-33: The baseUrl construction in FilterRestService appends
DELIMITER to filterServerBaseUri then FILTER_SERVER_API_VERSION, which produces
a double slash when filterServerBaseUri already ends with '/' (resulting in
"//v1"); update the code that builds the WebClient baseUrl to normalize/truncate
any trailing '/' from filterServerBaseUri before concatenating DELIMITER and
FILTER_SERVER_API_VERSION (or use a join that ensures a single delimiter) —
adjust the logic around the baseUrl(...) call that references
filterServerBaseUri, DELIMITER, and FILTER_SERVER_API_VERSION so the final base
URL contains exactly one '/' between the host and the version.
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkModificationRestService.java`:
- Around line 31-34: The base URI concatenation in the
NetworkModificationRestService constructor can produce a double slash; normalize
networkModificationServerBaseUri before appending DELIMITER and
NETWORK_MODIFICATION_SERVER_API_VERSION by trimming any trailing '/' (or
otherwise ensuring exactly one separator) so the call to
restClientBuilder.baseUrl(...) always receives a single-slash-separated URL;
update the constructor logic that builds networkModificationServerRest (using
networkModificationServerBaseUri, DELIMITER,
NETWORK_MODIFICATION_SERVER_API_VERSION, restClientBuilder) to remove trailing
slashes from the input base URI before concatenation.
---
Nitpick comments:
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterRestService.java`:
- Around line 37-43: The call returning the filters list from the restClient
chain can yield null; after invoking the retrieve().body(...) call in
FilterRestService (the restClient.get()... .retrieve().body(...) expression),
capture the result into a variable and return
Optional.ofNullable(result).orElse(List.of()) (or explicitly check for null and
return List.of()) so the method never returns null and downstream getFilters
callers receive an empty list instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1dcf1145-f57a-4a76-ab56-676a291cc757
📒 Files selected for processing (3)
monitor-server/src/main/java/org/gridsuite/monitor/server/services/SecurityAnalysisService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterRestService.javamonitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkModificationRestService.java
...ker-server/src/main/java/org/gridsuite/monitor/worker/server/services/FilterRestService.java
Show resolved
Hide resolved
...c/main/java/org/gridsuite/monitor/worker/server/services/NetworkModificationRestService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkConversionService.java (1)
33-34: ValidatecaseServerBaseUriat startup (fail fast).At Line 33, the URI is accepted as-is. A null/blank/malformed value will surface only when requests are made. Validate once in the constructor to fail early with a clear config error.
Proposed patch
+import java.net.URI; +import java.util.Objects; import java.util.Properties; import java.util.UUID; @@ public NetworkConversionService(`@Value`("${powsybl.services.case-server.base-uri:http://case-server/}") String caseServerBaseUri) { - this.caseServerBaseUri = caseServerBaseUri; + Objects.requireNonNull(caseServerBaseUri, "powsybl.services.case-server.base-uri must not be null"); + if (caseServerBaseUri.isBlank()) { + throw new IllegalArgumentException("powsybl.services.case-server.base-uri must not be blank"); + } + URI.create(caseServerBaseUri); // validate syntax + this.caseServerBaseUri = caseServerBaseUri; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkConversionService.java` around lines 33 - 34, In the NetworkConversionService constructor, validate the injected caseServerBaseUri: trim and check it's not null or blank, attempt to parse it with java.net.URI (or new URL) to ensure it's well-formed, and if validation fails throw an IllegalArgumentException with a clear message so the application fails fast; assign the validated value to the caseServerBaseUri field (optionally normalizing it, e.g., ensuring a trailing slash) so NetworkConversionService uses a guaranteed-valid URI at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkConversionService.java`:
- Around line 33-34: In the NetworkConversionService constructor, validate the
injected caseServerBaseUri: trim and check it's not null or blank, attempt to
parse it with java.net.URI (or new URL) to ensure it's well-formed, and if
validation fails throw an IllegalArgumentException with a clear message so the
application fails fast; assign the validated value to the caseServerBaseUri
field (optionally normalizing it, e.g., ensuring a trailing slash) so
NetworkConversionService uses a guaranteed-valid URI at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0444beb-1bb5-4e7e-a1c2-955322b818ad
📒 Files selected for processing (1)
monitor-worker-server/src/main/java/org/gridsuite/monitor/worker/server/services/NetworkConversionService.java
|
antoinebhs
approved these changes
Mar 16, 2026
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