Get the names of filters contained in sensi parameters and give it to…#997
Merged
Conversation
… sensi server before execution Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds element name resolution to sensitivity analysis: DirectoryService fetches names, SensitivityAnalysisService retrieves IDs and accepts a UUID→name map, StudyService orchestrates lookups and passes the map into the analysis run, and tests/stubs are updated to assert these interactions. ChangesElement name resolution for sensitivity analysis
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
![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
🧹 Nitpick comments (1)
src/main/java/org/gridsuite/study/server/service/DirectoryService.java (1)
86-87: ⚡ Quick winLog directory lookup failures before fallback.
At Line 86, swallowing the exception without a log makes missing element-name payloads hard to diagnose in production.
Proposed patch
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; @@ public class DirectoryService { + private static final Logger LOGGER = LoggerFactory.getLogger(DirectoryService.class); @@ } catch (RestClientException e) { + LOGGER.warn("Failed to resolve element names for {} IDs", elementUuids.size(), e); return Map.of(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/study/server/service/DirectoryService.java` around lines 86 - 87, The catch block in DirectoryService that currently swallows RestClientException and returns Map.of() should log the failure before returning so missing element-name payloads can be diagnosed; inside the method (the try/catch around the REST lookup—refer to the method in DirectoryService that catches RestClientException) replace the silent swallow with a processLogger/LOGGER.error or warn call that includes a descriptive message and the exception (e.g., "Directory lookup failed for [context/identifier]" plus the exception), then continue to return Map.of() as the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.java`:
- Around line 349-355: getElementIds currently returns response.getBody() from
restTemplate.exchange which may be null; change the method to capture the
ResponseEntity from restTemplate.exchange and return a non-null List by using
Optional.ofNullable(response.getBody()).orElseGet(Collections::emptyList) (or
Collections.emptyList()) so callers building a HashSet won't get a
NullPointerException; update the code around the restTemplate.exchange call in
getElementIds to assign to a ResponseEntity<List<UUID>> variable and return a
guaranteed non-null list.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 2702-2707: sensiParamsUuid is being read from
study.getSensitivityAnalysisParametersUuid() and used to prefetch element IDs
before the sensitivity parameters are guaranteed to be initialized, which can
cause a null-related failure; modify the flow so the sensitivity parameters UUID
is initialized/created on the Study before calling
sensitivityAnalysisService.getElementIds — for example call or inline the
existing initialization routine (or create a new
ensureSensitivityAnalysisParametersInitialized(study) step) so that
sensiParamsUuid is non-null, then assign UUID sensiParamsUuid =
study.getSensitivityAnalysisParametersUuid(); and only then call
sensitivityAnalysisService.getElementIds(sensiParamsUuid) and
directoryService.getElementNames(...).
In `@src/test/java/org/gridsuite/study/server/SensitivityAnalysisTest.java`:
- Line 204: Fix the Checkstyle WhitespaceAfter issue by adding a space after the
comma in the SensitivityAnalysisServerStubs constructor call: change the
invocation of SensitivityAnalysisServerStubs(wireMockServer,objectMapper) to
include a space after the comma so it reads
SensitivityAnalysisServerStubs(wireMockServer, objectMapper);; update the call
site where SensitivityAnalysisServerStubs is instantiated (the line constructing
sensitivityAnalysisStubs) to follow this spacing.
In
`@src/test/java/org/gridsuite/study/server/utils/wiremock/DirectoryServerStubs.java`:
- Around line 114-119: The current verifyGetElementNames builds a full URL
string which leads to brittle ordering-dependent matching; change
verifyGetElementNames to verify the request by path-only matching and explicit
query-parameter matchers: call WireMockUtilsCriteria.verifyGetRequest with the
path "/v1/elements/names" (so it uses urlPathEqualTo internally) and supply a
query map containing PARAM_IDS mapped to the givenElementUuids and "strictMode"
mapped to "false" so the verification checks query parameters irrespective of
their order.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/study/server/service/DirectoryService.java`:
- Around line 86-87: The catch block in DirectoryService that currently swallows
RestClientException and returns Map.of() should log the failure before returning
so missing element-name payloads can be diagnosed; inside the method (the
try/catch around the REST lookup—refer to the method in DirectoryService that
catches RestClientException) replace the silent swallow with a
processLogger/LOGGER.error or warn call that includes a descriptive message and
the exception (e.g., "Directory lookup failed for [context/identifier]" plus the
exception), then continue to return Map.of() as the fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8a88af5-2e70-469e-a10f-e8f467e505ec
📒 Files selected for processing (9)
src/main/java/org/gridsuite/study/server/service/DirectoryService.javasrc/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/SensitivityAnalysisTest.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/ComputationServerStubs.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/DirectoryServerStubs.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/SensitivityAnalysisServerStubs.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/WireMockStubs.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/WireMockUtilsCriteria.java
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/test/java/org/gridsuite/study/server/utils/wiremock/DirectoryServerStubs.java (1)
91-100:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winStub won't match actual requests – missing
idsquery parameter and using exact URL matching.The stub builds a URL with only
strictMode=false(line 93) and usesurlEqualTofor exact matching (line 95). However, the actual service calls will include theidsquery parameter (as shown inverifyGetElementNamesat line 104), so this stub will never match and requests will fail with 404.Use path-only matching like
stubGetElementIdsdoes inSensitivityAnalysisServerStubs.🔧 Proposed fix
public void stubGetElementNames(String responseBody) { - UriComponentsBuilder pathBuilder = UriComponentsBuilder.fromPath("/v1/elements/names"); - pathBuilder.queryParam("strictMode", "false"); - - wireMock.stubFor(WireMock.get(WireMock.urlEqualTo(pathBuilder.buildAndExpand().toUriString())) + wireMock.stubFor(WireMock.get(WireMock.urlPathEqualTo("/v1/elements/names")) .willReturn(WireMock.aResponse() .withStatus(HttpStatus.OK.value()) .withHeader("Content-Type", "application/json") .withBody(responseBody))); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/study/server/utils/wiremock/DirectoryServerStubs.java` around lines 91 - 100, The stubGetElementNames currently uses urlEqualTo with only strictMode in the built URL so it won't match requests that include the ids query parameter; update stubGetElementNames to use path-based matching (e.g., WireMock.urlPathEqualTo("/v1/elements/names")) and add a query param matcher for strictMode (WireMock.withQueryParam("strictMode", WireMock.equalTo("false"))) so requests including ids will match (follow the approach used by stubGetElementIds in SensitivityAnalysisServerStubs); verifyGetElementNames should then correctly correspond to the stub.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@src/test/java/org/gridsuite/study/server/utils/wiremock/DirectoryServerStubs.java`:
- Around line 91-100: The stubGetElementNames currently uses urlEqualTo with
only strictMode in the built URL so it won't match requests that include the ids
query parameter; update stubGetElementNames to use path-based matching (e.g.,
WireMock.urlPathEqualTo("/v1/elements/names")) and add a query param matcher for
strictMode (WireMock.withQueryParam("strictMode", WireMock.equalTo("false"))) so
requests including ids will match (follow the approach used by stubGetElementIds
in SensitivityAnalysisServerStubs); verifyGetElementNames should then correctly
correspond to the stub.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a37ed161-30ff-4415-8e81-eb931839994f
📒 Files selected for processing (3)
src/test/java/org/gridsuite/study/server/SensitivityAnalysisTest.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/DirectoryServerStubs.javasrc/test/java/org/gridsuite/study/server/utils/wiremock/SensitivityAnalysisServerStubs.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/gridsuite/study/server/SensitivityAnalysisTest.java
…-before-run Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com> # Conflicts: # src/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.java
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
Signed-off-by: Caroline Jeandat <caroline.jeandat@rte-france.com>
thangqp
approved these changes
Jun 2, 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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… sensi server before execution
PR Summary