Get computation statuses for branch#986
Open
sBouzols wants to merge 15 commits into
Open
Conversation
Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThis PR refactors computation services to use batch status retrieval APIs returning typed status enums instead of strings. Services now fetch statuses for multiple result UUIDs via POST to ChangesBatch Status Retrieval Pattern Refactoring
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javaThanks 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 |
SlimaneAmar
reviewed
May 5, 2026
…computation Co-authored-by: Copilot <copilot@github.com> Signed-off-by: sBouzols <sylvain.bouzols@gmail.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: 18
♻️ Duplicate comments (1)
src/main/java/org/gridsuite/study/server/service/LoadFlowService.java (1)
155-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden batch status response against null body
Line 155 returns
getBody()directly; if the load-flow service responds with an empty body, downstream calls (e.g.,assertNoLoadFlowRunning) can throw NPE. Return an empty map fallback here to keep callers safe.Proposed fix
- return restTemplate.exchange(loadFlowServerBaseUri + path, HttpMethod.POST, httpEntity, new ParameterizedTypeReference<Map<UUID, LoadFlowStatus>>() { - }).getBody(); + Map<UUID, LoadFlowStatus> statuses = restTemplate.exchange( + loadFlowServerBaseUri + path, + HttpMethod.POST, + httpEntity, + new ParameterizedTypeReference<Map<UUID, LoadFlowStatus>>() { + } + ).getBody(); + return statuses != null ? statuses : 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/LoadFlowService.java` around lines 155 - 156, The call in LoadFlowService that returns restTemplate.exchange(...).getBody() can return null; update the code in the method that calls restTemplate.exchange (the POST to loadFlowServerBaseUri + path in LoadFlowService) to guard against a null body by returning an empty Map<UUID, LoadFlowStatus> when getBody() is null (e.g., assign the result to a local variable and return result != null ? result : Collections.emptyMap()) so downstream callers like assertNoLoadFlowRunning never see a null.
🧹 Nitpick comments (5)
src/main/java/org/gridsuite/study/server/service/dynamicsecurityanalysis/DynamicSecurityAnalysisService.java (1)
137-143: 💤 Low valueConsider avoiding unnecessary HashSet allocation.
Creating a
HashSetfrom the map values is unnecessary. You can check for the presence ofRUNNINGstatus directly on theCollection<DynamicSecurityAnalysisStatus>returned by.values().♻️ Proposed refactor
public void assertNoDynamicSecurityAnalysisRunning(List<UUID> resultUuids) { Map<UUID, DynamicSecurityAnalysisStatus> dynamicSecurityAnalysisStatuses = getDynamicSecurityAnalysisStatuses(resultUuids); - Set<DynamicSecurityAnalysisStatus> values = new HashSet<>(dynamicSecurityAnalysisStatuses.values()); - if (values.contains(DynamicSecurityAnalysisStatus.RUNNING)) { + if (dynamicSecurityAnalysisStatuses.values().contains(DynamicSecurityAnalysisStatus.RUNNING)) { throw new StudyException(COMPUTATION_RUNNING); } }🤖 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/dynamicsecurityanalysis/DynamicSecurityAnalysisService.java` around lines 137 - 143, The method assertNoDynamicSecurityAnalysisRunning creates an unnecessary HashSet from dynamicSecurityAnalysisStatuses.values(); instead check for RUNNING directly on the Collection returned by getDynamicSecurityAnalysisStatuses(resultUuids) to avoid allocation—e.g. replace the new HashSet<>(dynamicSecurityAnalysisStatuses.values()) usage with a direct contains/stream anyMatch on dynamicSecurityAnalysisStatuses.values() and keep throwing StudyException(COMPUTATION_RUNNING) when DynamicSecurityAnalysisStatus.RUNNING is found.src/main/java/org/gridsuite/study/server/service/shortcircuit/ShortCircuitService.java (1)
202-215: 💤 Low valueInconsistent null-safety in empty list check.
Line 203 uses
resultUuids.isEmpty()which will throw NPE ifresultUuidsis null, while other similar services (PccMinService, StateEstimationService, VoltageInitService, SensitivityAnalysisService) useCollectionUtils.isEmpty(resultUuids)for null-safe checking. Consider usingCollectionUtils.isEmpty()for consistency and defensive coding.🤖 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/shortcircuit/ShortCircuitService.java` around lines 202 - 215, In ShortCircuitService.getShortCircuitAnalysisStatuses, replace the null-unsafe resultUuids.isEmpty() check with a null-safe CollectionUtils.isEmpty(resultUuids) to match other services (PccMinService, StateEstimationService, VoltageInitService, SensitivityAnalysisService); ensure you import org.springframework.util.CollectionUtils if not already present and keep the early return Map.of() behavior when the collection is empty or null.src/main/java/org/gridsuite/study/server/service/VoltageInitService.java (1)
13-13: 💤 Low valueInconsistent CollectionUtils import.
This file imports
org.apache.commons.collections4.CollectionUtilswhile other services in this PR (PccMinService, StateEstimationService, SensitivityAnalysisService) useorg.springframework.util.CollectionUtils. Consider using Spring's CollectionUtils for consistency across the codebase.🤖 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/VoltageInitService.java` at line 13, VoltageInitService imports org.apache.commons.collections4.CollectionUtils which is inconsistent with other services; change the import to org.springframework.util.CollectionUtils and update any calls if needed to match Spring's API (e.g., CollectionUtils.isEmpty/hasLength semantics) so usages in the VoltageInitService class (look for references inside the VoltageInitService methods/constructor) compile and follow the same convention as PccMinService, StateEstimationService, and SensitivityAnalysisService.src/main/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClient.java (1)
208-208: ⚡ Quick winUse
.build()instead of.buildAndExpand().Since no path variables are being expanded,
.build()is more appropriate than.buildAndExpand().♻️ Suggested simplification
- var uriComponents = UriComponentsBuilder.fromUriString(endPointUrl + "/statuses").buildAndExpand(); + var uriComponents = UriComponentsBuilder.fromUriString(endPointUrl + "/statuses").build();🤖 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/client/dynamicmargincalculation/DynamicMarginCalculationClient.java` at line 208, Replace the unnecessary call to buildAndExpand() with build() when constructing the URI for statuses: in DynamicMarginCalculationClient where uriComponents is assigned (var uriComponents = UriComponentsBuilder.fromUriString(endPointUrl + "/statuses")...), use .build() instead of .buildAndExpand() because there are no path variables to expand.src/main/java/org/gridsuite/study/server/service/dynamicmargincalculation/DynamicMarginCalculationService.java (1)
135-135: ⚡ Quick winUnnecessary
HashSetcreation.Creating a
HashSetfromvalues()adds overhead. Thecontainscheck can be performed directly onCollection<DynamicMarginCalculationStatus>returned by.values().♻️ Simplified implementation
public void assertNoDynamicMarginCalculationRunning(List<UUID> resultUuids) { Map<UUID, DynamicMarginCalculationStatus> dynamicMarginCalculationStatuses = getDynamicMarginCalculationStatuses(resultUuids); - Set<DynamicMarginCalculationStatus> values = new HashSet<>(dynamicMarginCalculationStatuses.values()); - if (values.contains(DynamicMarginCalculationStatus.RUNNING)) { + if (dynamicMarginCalculationStatuses.values().contains(DynamicMarginCalculationStatus.RUNNING)) { throw new StudyException(COMPUTATION_RUNNING); } }🤖 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/dynamicmargincalculation/DynamicMarginCalculationService.java` at line 135, Replace the unnecessary HashSet allocation in DynamicMarginCalculationService: instead of creating Set<DynamicMarginCalculationStatus> values = new HashSet<>(dynamicMarginCalculationStatuses.values()), use the collection returned by dynamicMarginCalculationStatuses.values() directly (e.g. assign to Collection<DynamicMarginCalculationStatus> or call .values().contains(...) inline) to perform the contains check without the extra copy/overhead.
🤖 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/client/dynamicmargincalculation/DynamicMarginCalculationClient.java`:
- Around line 197-199: getStatus currently may return null when
getStatuses(List.of(resultUuid)) doesn't contain resultUuid; change getStatus to
return Optional<DynamicMarginCalculationStatus> and wrap the map lookup with
Optional.ofNullable(getStatuses(List.of(resultUuid)).get(resultUuid)) so callers
must handle missing status explicitly; update any callers of
DynamicMarginCalculationClient.getStatus to handle Optional (or alternatively
throw a clear checked/unchecked exception with resultUuid in the message if you
prefer fail-fast semantics).
- Around line 215-216: The REST call currently returns
getRestTemplate().exchange(...).getBody() which can be null and cause NPEs; in
DynamicMarginCalculationClient replace the direct getBody() usage with a
defensive check: capture the response body into a local Map (from the exchange
call using the same ParameterizedTypeReference), then if the body is null return
Collections.emptyMap() (or throw a clear IllegalStateException if you prefer
failing fast) so callers always receive a non-null Map; ensure to import or
reference java.util.Collections for the empty map fallback.
- Around line 201-217: The remote /statuses POST may return a null body or omit
entries, so update DynamicMarginCalculationClient.getStatuses(List<UUID>) to
defensively handle null responses (e.g., if exchange().getBody() is null return
an empty Map or a map with requested UUIDs mapped to a default/UNKNOWN
DynamicMarginCalculationStatus) and ensure callers (notably getStatus(UUID))
check the returned map and handle missing keys instead of assuming non-null
values; adjust getStatus(UUID) to null-check the map and return an explicit
Optional/throw a clear exception or a default status when the requested UUID is
not present.
In
`@src/main/java/org/gridsuite/study/server/service/client/dynamicsecurityanalysis/DynamicSecurityAnalysisClient.java`:
- Around line 184-186: The getStatus method delegates to
getStatuses(List.of(resultUuid)).get(resultUuid) which can return null; update
DynamicSecurityAnalysisClient.getStatus to handle a missing entry by checking
the map result from getStatuses(List.of(resultUuid)) and if the value for
resultUuid is null, throw a clear runtime exception (e.g., IllegalStateException
or NoSuchElementException) with a message containing the resultUuid, otherwise
return the non-null DynamicSecurityAnalysisStatus; reference the getStatus(...)
method and the getStatuses(...) call when making this change.
- Around line 188-205: Annotate the getStatuses(List<UUID> resultUuids)
parameter with the same `@NonNull` used elsewhere in
DynamicSecurityAnalysisClient, and add a null-safe handling of the REST
response: capture the ResponseEntity<Map<UUID, DynamicSecurityAnalysisStatus>>
from getRestTemplate().exchange(...) into a variable, check its getBody() for
null and return Map.of() (or Objects.requireNonNullElse(body, Map.of())) when
null instead of returning getBody() directly; keep existing empty-input check
using CollectionUtils.isEmpty and reuse getRestTemplate() and the existing
ParameterizedTypeReference to locate the exchange call.
In
`@src/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/impl/DynamicSimulationClientImpl.java`:
- Around line 208-210: The getStatus method delegates to
getStatuses(List.of(resultUuid)).get(resultUuid) which can NPE if getStatuses
returns null or the map lacks the key; update getStatus(UUID resultUuid) to call
getStatuses(List.of(resultUuid)) into a local Map variable, check for null and
for containsKey(resultUuid) and handle both cases safely (e.g., return null or
Optional<DynamicSimulationStatus> per project convention) instead of calling
.get() directly; reference methods getStatus, getStatuses and the type
DynamicSimulationStatus/resultUuid when making the checks and returning the safe
value.
- Around line 213-229: The getStatuses method in DynamicSimulationClientImpl may
return null because RestTemplate.exchange(...).getBody() can be null; update
getStatuses to defensively handle a null response by capturing the result of
getRestTemplate().exchange(...).getBody(), and if it is null return an empty map
(e.g. Map.of() or Collections.emptyMap()) so callers (like getStatus() which
calls .get() on the map) never get a null; apply the same null-safe pattern to
the corresponding methods in DynamicMarginCalculationClient and
DynamicSecurityAnalysisClient.
In
`@src/main/java/org/gridsuite/study/server/service/dynamicmargincalculation/DynamicMarginCalculationService.java`:
- Around line 88-90: getStatus(UUID resultUuid) calls
getDynamicMarginCalculationStatuses(List.of(resultUuid)).get(resultUuid) which
can return null if the map lacks the UUID; add a defensive null check in
getStatus to avoid returning null (e.g., check the map result for resultUuid and
either throw a clear exception such as
NotFoundException/IllegalArgumentException or change the method to return
Optional<DynamicMarginCalculationStatus>) so callers never receive a raw null;
update getStatus to locate the map entry from
getDynamicMarginCalculationStatuses and handle the null case accordingly.
- Around line 133-139: Add defensive checks in
assertNoDynamicMarginCalculationRunning: validate the input resultUuids (throw
IllegalArgumentException if null or empty) and null-safe the call to
getDynamicMarginCalculationStatuses (treat a null map as empty). When building
values from dynamicMarginCalculationStatuses.values(), guard against a null
collection to avoid NPEs; only check for DynamicMarginCalculationStatus.RUNNING
if the map/values are non-null. Use the existing
StudyException(COMPUTATION_RUNNING) only for the running-state detection and use
a clear IllegalArgumentException for bad input.
In
`@src/main/java/org/gridsuite/study/server/service/dynamicsecurityanalysis/DynamicSecurityAnalysisService.java`:
- Around line 89-91: The call in getStatus delegates to
getDynamicSecurityAnalysisStatuses(List.of(resultUuid)) and then directly
.get(resultUuid), which can return null and cause NPEs; update getStatus to
first capture the Map returned by getDynamicSecurityAnalysisStatuses, check for
the presence of resultUuid (e.g. containsKey or get != null) and return a safe
value (null or an Optional) or throw a clear exception instead of dereferencing
a potentially missing entry; refer to the methods getStatus and
getDynamicSecurityAnalysisStatuses and the type DynamicSecurityAnalysisStatus to
locate where to add the null-safety check.
In
`@src/main/java/org/gridsuite/study/server/service/dynamicsimulation/impl/DynamicSimulationServiceImpl.java`:
- Around line 245-251: The method assertNoDynamicSimulationRunning currently
assumes getDynamicSimulationStatuses(...) returns a non-null Map and will NPE
when it's null; update assertNoDynamicSimulationRunning to defensively handle a
null (or empty) result from getDynamicSimulationStatuses by treating a null map
as empty before creating the HashSet or checking values, then proceed to check
for DynamicSimulationStatus.RUNNING and throw
StudyException(COMPUTATION_RUNNING) if found; reference the
assertNoDynamicSimulationRunning method, getDynamicSimulationStatuses, and
DynamicSimulationStatus.RUNNING when locating and applying the fix.
- Around line 206-208: The getStatus method delegates to
getDynamicSimulationStatuses but can NPE if that method returns null or the map
lacks the key; update DynamicSimulationServiceImpl.getStatus to first call
Map<UUID, DynamicSimulationStatus> statuses =
getDynamicSimulationStatuses(List.of(resultUuid)), check if statuses is null and
return null (or an Optional if your API prefers), then return
statuses.get(resultUuid) only after confirming the key exists (or use
statuses.getOrDefault(resultUuid, null)) to avoid a NullPointerException when
the map is missing the entry.
- Line 246: The local variable name loadFlowStatuses in
DynamicSimulationServiceImpl is misleading; rename it to
dynamicSimulationStatuses (or simulationStatuses) where it's assigned from
getDynamicSimulationStatuses(computationResultUuids) and update all subsequent
references in the method to use the new name so the variable accurately reflects
it holds DynamicSimulationStatus values.
In `@src/main/java/org/gridsuite/study/server/service/PccMinService.java`:
- Around line 134-144: The RestTemplate call is using a relative path only, so
prepend the configured base URI (pccMinServerBaseUri) to the built path before
calling restTemplate.exchange; update the code around where PCC_MIN_URI +
DELIMITER + "results/statuses" is composed (in PccMinService) to build a full
URL (e.g., concatenate pccMinServerBaseUri and the path) and then call
restTemplate.exchange(fullUrl, HttpMethod.POST, httpEntity, new
ParameterizedTypeReference<Map<UUID, PccMinStatus>>() { }). Ensure you keep the
existing headers and HttpEntity<List<UUID>> httpEntity and return the response
body as before.
In
`@src/main/java/org/gridsuite/study/server/service/SecurityAnalysisService.java`:
- Around line 189-191: getSecurityAnalysisStatus currently calls
getSecurityAnalysisStatuses(List.of(resultUuid)) which will throw NPE when
resultUuid is null; add a null guard in getSecurityAnalysisStatus (check
resultUuid == null) and handle that case explicitly (e.g., return null or an
appropriate default SecurityAnalysisStatus) instead of calling List.of, so
callers like RootNetworkNodeInfoService which use getComputationResultUuid()
that may be null won't break the call chain; reference the methods
getSecurityAnalysisStatus and getSecurityAnalysisStatuses and the caller
RootNetworkNodeInfoService#getComputationResultUuid when making the change.
In
`@src/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.java`:
- Around line 179-193: The call in getSensitivityAnalysisStatuses builds a
relative path but calls restTemplate.exchange(path, ...) without prepending
sensitivityAnalysisServerBaseUri, causing failed requests; fix by resolving the
full URL before the exchange (e.g., prefixing the path with
sensitivityAnalysisServerBaseUri or using
UriComponentsBuilder.fromHttpUrl(sensitivityAnalysisServerBaseUri).path(path) to
build the absoluteUri) and then call restTemplate.exchange(absoluteUri,
HttpMethod.POST, httpEntity, new ParameterizedTypeReference<Map<UUID,
SensitivityAnalysisStatus>>() { }).getBody() so the restTemplate uses the full
base URI.
In
`@src/main/java/org/gridsuite/study/server/service/StateEstimationService.java`:
- Around line 134-147: getStateEstimationStatuses builds a relative path and
calls restTemplate.exchange(path, ...) without prepending the configured base
URI (stateEstimationServerServerBaseUri), causing requests to fail; fix by
concatenating or resolving stateEstimationServerServerBaseUri with the local
path before calling restTemplate.exchange (use the existing path variable and
the stateEstimationServerServerBaseUri field), then call
restTemplate.exchange(fullUri, HttpMethod.POST, httpEntity, new
ParameterizedTypeReference<Map<UUID, StateEstimationStatus>>() { }).getBody() so
the request uses the full service URI.
In `@src/main/java/org/gridsuite/study/server/service/VoltageInitService.java`:
- Around line 134-146: getVoltageInitStatuses builds only a relative path and
calls restTemplate.exchange(path, ...) so requests fail; update the method to
prepend the configured base URI (voltageInitServerBaseUri) when building the
request URI (e.g. combine voltageInitServerBaseUri with the current path or use
UriComponentsBuilder starting from voltageInitServerBaseUri) and then call
restTemplate.exchange with that full URI while keeping the existing HttpEntity,
headers, and ParameterizedTypeReference to return the Map<UUID,
VoltageInitStatus>.
---
Duplicate comments:
In `@src/main/java/org/gridsuite/study/server/service/LoadFlowService.java`:
- Around line 155-156: The call in LoadFlowService that returns
restTemplate.exchange(...).getBody() can return null; update the code in the
method that calls restTemplate.exchange (the POST to loadFlowServerBaseUri +
path in LoadFlowService) to guard against a null body by returning an empty
Map<UUID, LoadFlowStatus> when getBody() is null (e.g., assign the result to a
local variable and return result != null ? result : Collections.emptyMap()) so
downstream callers like assertNoLoadFlowRunning never see a null.
---
Nitpick comments:
In
`@src/main/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClient.java`:
- Line 208: Replace the unnecessary call to buildAndExpand() with build() when
constructing the URI for statuses: in DynamicMarginCalculationClient where
uriComponents is assigned (var uriComponents =
UriComponentsBuilder.fromUriString(endPointUrl + "/statuses")...), use .build()
instead of .buildAndExpand() because there are no path variables to expand.
In
`@src/main/java/org/gridsuite/study/server/service/dynamicmargincalculation/DynamicMarginCalculationService.java`:
- Line 135: Replace the unnecessary HashSet allocation in
DynamicMarginCalculationService: instead of creating
Set<DynamicMarginCalculationStatus> values = new
HashSet<>(dynamicMarginCalculationStatuses.values()), use the collection
returned by dynamicMarginCalculationStatuses.values() directly (e.g. assign to
Collection<DynamicMarginCalculationStatus> or call .values().contains(...)
inline) to perform the contains check without the extra copy/overhead.
In
`@src/main/java/org/gridsuite/study/server/service/dynamicsecurityanalysis/DynamicSecurityAnalysisService.java`:
- Around line 137-143: The method assertNoDynamicSecurityAnalysisRunning creates
an unnecessary HashSet from dynamicSecurityAnalysisStatuses.values(); instead
check for RUNNING directly on the Collection returned by
getDynamicSecurityAnalysisStatuses(resultUuids) to avoid allocation—e.g. replace
the new HashSet<>(dynamicSecurityAnalysisStatuses.values()) usage with a direct
contains/stream anyMatch on dynamicSecurityAnalysisStatuses.values() and keep
throwing StudyException(COMPUTATION_RUNNING) when
DynamicSecurityAnalysisStatus.RUNNING is found.
In
`@src/main/java/org/gridsuite/study/server/service/shortcircuit/ShortCircuitService.java`:
- Around line 202-215: In ShortCircuitService.getShortCircuitAnalysisStatuses,
replace the null-unsafe resultUuids.isEmpty() check with a null-safe
CollectionUtils.isEmpty(resultUuids) to match other services (PccMinService,
StateEstimationService, VoltageInitService, SensitivityAnalysisService); ensure
you import org.springframework.util.CollectionUtils if not already present and
keep the early return Map.of() behavior when the collection is empty or null.
In `@src/main/java/org/gridsuite/study/server/service/VoltageInitService.java`:
- Line 13: VoltageInitService imports
org.apache.commons.collections4.CollectionUtils which is inconsistent with other
services; change the import to org.springframework.util.CollectionUtils and
update any calls if needed to match Spring's API (e.g.,
CollectionUtils.isEmpty/hasLength semantics) so usages in the VoltageInitService
class (look for references inside the VoltageInitService methods/constructor)
compile and follow the same convention as PccMinService, StateEstimationService,
and SensitivityAnalysisService.
🪄 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: 2918cf6f-5027-4cfd-a02f-8d2fd133efae
📒 Files selected for processing (22)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/dto/ShortCircuitAnalysisStatus.javasrc/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkNodeInfoRepository.javasrc/main/java/org/gridsuite/study/server/service/LoadFlowService.javasrc/main/java/org/gridsuite/study/server/service/PccMinService.javasrc/main/java/org/gridsuite/study/server/service/RootNetworkNodeInfoService.javasrc/main/java/org/gridsuite/study/server/service/SecurityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/StateEstimationService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/main/java/org/gridsuite/study/server/service/VoltageInitService.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicmargincalculation/DynamicMarginCalculationClient.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicsecurityanalysis/DynamicSecurityAnalysisClient.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/DynamicSimulationClient.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/impl/DynamicSimulationClientImpl.javasrc/main/java/org/gridsuite/study/server/service/dynamicmargincalculation/DynamicMarginCalculationService.javasrc/main/java/org/gridsuite/study/server/service/dynamicsecurityanalysis/DynamicSecurityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/dynamicsimulation/DynamicSimulationService.javasrc/main/java/org/gridsuite/study/server/service/dynamicsimulation/impl/DynamicSimulationServiceImpl.javasrc/main/java/org/gridsuite/study/server/service/shortcircuit/ShortCircuitService.javasrc/test/java/org/gridsuite/study/server/NetworkModificationTreeTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/NodeControllerTest.java
Comment on lines
197
to
+199
| public DynamicMarginCalculationStatus getStatus(@NonNull UUID resultUuid) { | ||
| Objects.requireNonNull(resultUuid); | ||
| return getStatuses(List.of(resultUuid)).get(resultUuid); | ||
| } |
There was a problem hiding this comment.
Potential null return when status not found in map.
The .get(resultUuid) call will return null if the map from getStatuses() doesn't contain the requested UUID. This can happen if the remote service returns an incomplete response. Callers expecting a non-null DynamicMarginCalculationStatus may encounter NPEs.
🛡️ Suggested fix to handle missing status
public DynamicMarginCalculationStatus getStatus(`@NonNull` UUID resultUuid) {
- return getStatuses(List.of(resultUuid)).get(resultUuid);
+ Map<UUID, DynamicMarginCalculationStatus> statuses = getStatuses(List.of(resultUuid));
+ DynamicMarginCalculationStatus status = statuses.get(resultUuid);
+ if (status == null) {
+ throw new IllegalStateException("No status returned for result UUID: " + resultUuid);
+ }
+ return status;
}📝 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
| public DynamicMarginCalculationStatus getStatus(@NonNull UUID resultUuid) { | |
| Objects.requireNonNull(resultUuid); | |
| return getStatuses(List.of(resultUuid)).get(resultUuid); | |
| } | |
| public DynamicMarginCalculationStatus getStatus(`@NonNull` UUID resultUuid) { | |
| Map<UUID, DynamicMarginCalculationStatus> statuses = getStatuses(List.of(resultUuid)); | |
| DynamicMarginCalculationStatus status = statuses.get(resultUuid); | |
| if (status == null) { | |
| throw new IllegalStateException("No status returned for result UUID: " + resultUuid); | |
| } | |
| return status; | |
| } |
🤖 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/client/dynamicmargincalculation/DynamicMarginCalculationClient.java`
around lines 197 - 199, getStatus currently may return null when
getStatuses(List.of(resultUuid)) doesn't contain resultUuid; change getStatus to
return Optional<DynamicMarginCalculationStatus> and wrap the map lookup with
Optional.ofNullable(getStatuses(List.of(resultUuid)).get(resultUuid)) so callers
must handle missing status explicitly; update any callers of
DynamicMarginCalculationClient.getStatus to handle Optional (or alternatively
throw a clear checked/unchecked exception with resultUuid in the message if you
prefer fail-fast semantics).
Comment on lines
184
to
+186
| public DynamicSecurityAnalysisStatus getStatus(@NonNull UUID resultUuid) { | ||
| Objects.requireNonNull(resultUuid); | ||
| return getStatuses(List.of(resultUuid)).get(resultUuid); | ||
| } |
There was a problem hiding this comment.
Add null safety check for the delegated batch call.
The delegation to getStatuses(List.of(resultUuid)).get(resultUuid) can return null if the server's response map does not contain the requested UUID. This would break the expected contract and cause NPEs in calling code that expects a non-null DynamicSecurityAnalysisStatus.
🛡️ Proposed fix to add null safety
public DynamicSecurityAnalysisStatus getStatus(`@NonNull` UUID resultUuid) {
- return getStatuses(List.of(resultUuid)).get(resultUuid);
+ DynamicSecurityAnalysisStatus status = getStatuses(List.of(resultUuid)).get(resultUuid);
+ if (status == null) {
+ throw new IllegalStateException("Status not found for result UUID: " + resultUuid);
+ }
+ return status;
}📝 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
| public DynamicSecurityAnalysisStatus getStatus(@NonNull UUID resultUuid) { | |
| Objects.requireNonNull(resultUuid); | |
| return getStatuses(List.of(resultUuid)).get(resultUuid); | |
| } | |
| public DynamicSecurityAnalysisStatus getStatus(`@NonNull` UUID resultUuid) { | |
| DynamicSecurityAnalysisStatus status = getStatuses(List.of(resultUuid)).get(resultUuid); | |
| if (status == null) { | |
| throw new IllegalStateException("Status not found for result UUID: " + resultUuid); | |
| } | |
| return status; | |
| } |
🤖 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/client/dynamicsecurityanalysis/DynamicSecurityAnalysisClient.java`
around lines 184 - 186, The getStatus method delegates to
getStatuses(List.of(resultUuid)).get(resultUuid) which can return null; update
DynamicSecurityAnalysisClient.getStatus to handle a missing entry by checking
the map result from getStatuses(List.of(resultUuid)) and if the value for
resultUuid is null, throw a clear runtime exception (e.g., IllegalStateException
or NoSuchElementException) with a message containing the resultUuid, otherwise
return the non-null DynamicSecurityAnalysisStatus; reference the getStatus(...)
method and the getStatuses(...) call when making this change.
Comment on lines
134
to
144
| String path = UriComponentsBuilder | ||
| .fromPath(PCC_MIN_URI + DELIMITER + "results/{resultUuid}/status") | ||
| .buildAndExpand(resultUuid).toUriString(); | ||
| return restTemplate.getForObject(pccMinServerBaseUri + path, String.class); | ||
| .fromPath(PCC_MIN_URI + DELIMITER + "results/statuses") | ||
| .toUriString(); | ||
|
|
||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers.setContentType(MediaType.APPLICATION_JSON); | ||
|
|
||
| HttpEntity<List<UUID>> httpEntity = new HttpEntity<>(resultUuids, headers); | ||
| return restTemplate.exchange(path, HttpMethod.POST, httpEntity, new ParameterizedTypeReference<Map<UUID, PccMinStatus>>() { | ||
| }).getBody(); | ||
| } |
There was a problem hiding this comment.
Missing base URI in RestTemplate call causes request to fail.
Line 142 invokes restTemplate.exchange(path, ...) with only the relative path, but the pccMinServerBaseUri is not prepended. This will cause the HTTP request to fail because RestTemplate doesn't know the target server.
🐛 Proposed fix
- return restTemplate.exchange(path, HttpMethod.POST, httpEntity, new ParameterizedTypeReference<Map<UUID, PccMinStatus>>() {
+ return restTemplate.exchange(pccMinServerBaseUri + path, HttpMethod.POST, httpEntity, new ParameterizedTypeReference<Map<UUID, PccMinStatus>>() {
}).getBody();📝 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
| String path = UriComponentsBuilder | |
| .fromPath(PCC_MIN_URI + DELIMITER + "results/{resultUuid}/status") | |
| .buildAndExpand(resultUuid).toUriString(); | |
| return restTemplate.getForObject(pccMinServerBaseUri + path, String.class); | |
| .fromPath(PCC_MIN_URI + DELIMITER + "results/statuses") | |
| .toUriString(); | |
| HttpHeaders headers = new HttpHeaders(); | |
| headers.setContentType(MediaType.APPLICATION_JSON); | |
| HttpEntity<List<UUID>> httpEntity = new HttpEntity<>(resultUuids, headers); | |
| return restTemplate.exchange(path, HttpMethod.POST, httpEntity, new ParameterizedTypeReference<Map<UUID, PccMinStatus>>() { | |
| }).getBody(); | |
| } | |
| String path = UriComponentsBuilder | |
| .fromPath(PCC_MIN_URI + DELIMITER + "results/statuses") | |
| .toUriString(); | |
| HttpHeaders headers = new HttpHeaders(); | |
| headers.setContentType(MediaType.APPLICATION_JSON); | |
| HttpEntity<List<UUID>> httpEntity = new HttpEntity<>(resultUuids, headers); | |
| return restTemplate.exchange(pccMinServerBaseUri + path, HttpMethod.POST, httpEntity, new ParameterizedTypeReference<Map<UUID, PccMinStatus>>() { | |
| }).getBody(); | |
| } |
🤖 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/PccMinService.java` around
lines 134 - 144, The RestTemplate call is using a relative path only, so prepend
the configured base URI (pccMinServerBaseUri) to the built path before calling
restTemplate.exchange; update the code around where PCC_MIN_URI + DELIMITER +
"results/statuses" is composed (in PccMinService) to build a full URL (e.g.,
concatenate pccMinServerBaseUri and the path) and then call
restTemplate.exchange(fullUrl, HttpMethod.POST, httpEntity, new
ParameterizedTypeReference<Map<UUID, PccMinStatus>>() { }). Ensure you keep the
existing headers and HttpEntity<List<UUID>> httpEntity and return the response
body as before.
sBouzols
changed the title
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 (2)
src/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.java (1)
175-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a null guard before delegating in
SensitivityAnalysisService#getSensitivityAnalysisStatus(UUID)(src/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.java:175-177)
RootNetworkNodeInfoServicecan pass a nullresultUuidwhen the sensitivity computation UUID is missing, but the current implementation doesList.of(resultUuid), which throws before the controller can reach the expected204/noContentpath.🐛 Proposed fix
public SensitivityAnalysisStatus getSensitivityAnalysisStatus(UUID resultUuid) { + if (resultUuid == null) { + return null; + } return getSensitivityAnalysisStatuses(List.of(resultUuid)).get(resultUuid); }🤖 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/SensitivityAnalysisService.java` around lines 175 - 177, Add a null guard in SensitivityAnalysisService#getSensitivityAnalysisStatus(UUID): if the incoming resultUuid is null return null (or the appropriate "no content" sentinel) immediately instead of calling List.of(resultUuid); then delegate to getSensitivityAnalysisStatuses(List<UUID>) only when resultUuid is non-null. This prevents List.of from throwing on a null UUID and allows the controller to handle the 204/noContent path.src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
2507-2521:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd bounds checks before index-based access.
The method assumes
copyUuidsandcopyChildrenhave at least as many elements as their corresponding source lists, but performs no validation:
- Line 2511:
copyUuids.get(i)will throwIndexOutOfBoundsExceptionifcopyUuids.size() < modificationsUuids.size()- Line 2517:
copyChildren.get(i)will throwIndexOutOfBoundsExceptionifcopyChildren.size() < originalChildren.size()Additionally, the code assumes
findAllChildrenUuidsreturns children in a consistent, deterministic order for both original and copy operations. If the order differs or the duplication is partial, the UUID mappings will be incorrect.🛡️ Proposed fix with size validation
private void copyModificationsToExclude(UUID originNodeUuid, UUID targetNodeUuid, List<UUID> modificationsUuids, NetworkModificationsResult networkModificationResults) { Map<UUID, UUID> mappingModificationsUuids = new HashMap<>(); List<UUID> copyUuids = networkModificationResults.modificationUuids(); + if (copyUuids.size() != modificationsUuids.size()) { + throw new IllegalStateException(String.format( + "Modification duplication size mismatch: expected %d, got %d", + modificationsUuids.size(), copyUuids.size())); + } + // Map root-level modifications for (int i = 0; i < modificationsUuids.size(); i++) { mappingModificationsUuids.put(modificationsUuids.get(i), copyUuids.get(i)); } List<UUID> originalChildren = networkModificationService.findAllChildrenUuids(modificationsUuids); List<UUID> copyChildren = networkModificationService.findAllChildrenUuids(copyUuids); + if (copyChildren.size() != originalChildren.size()) { + throw new IllegalStateException(String.format( + "Children duplication size mismatch: expected %d, got %d", + originalChildren.size(), copyChildren.size())); + } + for (int i = 0; i < originalChildren.size(); i++) { mappingModificationsUuids.put(originalChildren.get(i), copyChildren.get(i)); } rootNetworkNodeInfoService.copyModificationsToExcludeFromTags(originNodeUuid, targetNodeUuid, mappingModificationsUuids); }🤖 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/StudyService.java` around lines 2507 - 2521, The loops in StudyService that map modifications (using modificationsUuids -> copyUuids and originalChildren -> copyChildren) lack bounds checks and assume both lists are same size/order; update the code in StudyService to iterate only up to Math.min(source.size(), dest.size()) (e.g., use min of modificationsUuids and copyUuids, and min of originalChildren and copyChildren) and handle size mismatches by logging an error or throwing a clear exception before calling rootNetworkNodeInfoService.copyModificationsToExcludeFromTags; also consider validating deterministic ordering (or documenting/ensuring the contract of networkModificationService.findAllChildrenUuids) so mappingModificationsUuids is correct.
🤖 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.
Outside diff comments:
In
`@src/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.java`:
- Around line 175-177: Add a null guard in
SensitivityAnalysisService#getSensitivityAnalysisStatus(UUID): if the incoming
resultUuid is null return null (or the appropriate "no content" sentinel)
immediately instead of calling List.of(resultUuid); then delegate to
getSensitivityAnalysisStatuses(List<UUID>) only when resultUuid is non-null.
This prevents List.of from throwing on a null UUID and allows the controller to
handle the 204/noContent path.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 2507-2521: The loops in StudyService that map modifications (using
modificationsUuids -> copyUuids and originalChildren -> copyChildren) lack
bounds checks and assume both lists are same size/order; update the code in
StudyService to iterate only up to Math.min(source.size(), dest.size()) (e.g.,
use min of modificationsUuids and copyUuids, and min of originalChildren and
copyChildren) and handle size mismatches by logging an error or throwing a clear
exception before calling
rootNetworkNodeInfoService.copyModificationsToExcludeFromTags; also consider
validating deterministic ordering (or documenting/ensuring the contract of
networkModificationService.findAllChildrenUuids) so mappingModificationsUuids is
correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54a48ae2-677c-42ca-b2f4-99f864e67efc
📒 Files selected for processing (4)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/service/SecurityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.java
Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>
…com/gridsuite/study-server into get_computation_statuses_for_branch
Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>
…framework.util.CollectionUtils for consistency across the codebase Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>
Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>
Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>
Contributor
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
…ses_for_branch Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>
Signed-off-by: sBouzols <sylvain.bouzols@gmail.com>
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 366-384: The catch block for RestClientException in the
getElementIds method is currently masking remote failures by returning an empty
list, making it impossible to distinguish between legitimate empty responses and
actual backend failures. Instead of silently catching and returning List.of()
when RestClientException occurs, propagate the exception to the caller or
re-throw it as a domain/runtime exception after logging the error with clear
context. This ensures downstream code can properly handle actual failures rather
than treating them as empty element IDs.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3702-3709: The invalidateStudyRootNetwork method receives a
specific rootNetworkUuid parameter but does not pass it to the doUnbuildNodeTree
call on line 3705, causing doUnbuildNodeTree to iterate and unbuild nodes for
all root networks in the study instead of just the targeted one. Modify the
doUnbuildNodeTree method call to accept and use the rootNetworkUuid parameter so
that only the specified root network's nodes are unbuilt, rather than affecting
all root networks in the study.
🪄 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: 80b7bff7-4a80-4086-9d6c-c7e3b857c911
📒 Files selected for processing (16)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkNodeInfoRepository.javasrc/main/java/org/gridsuite/study/server/service/LoadFlowService.javasrc/main/java/org/gridsuite/study/server/service/PccMinService.javasrc/main/java/org/gridsuite/study/server/service/SecurityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/StateEstimationService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/main/java/org/gridsuite/study/server/service/VoltageInitService.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/impl/DynamicSimulationClientImpl.javasrc/main/java/org/gridsuite/study/server/service/dynamicmargincalculation/DynamicMarginCalculationService.javasrc/main/java/org/gridsuite/study/server/service/dynamicsecurityanalysis/DynamicSecurityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/dynamicsimulation/impl/DynamicSimulationServiceImpl.javasrc/main/java/org/gridsuite/study/server/service/shortcircuit/ShortCircuitService.javasrc/test/java/org/gridsuite/study/server/NetworkModificationTreeTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/NodeControllerTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/gridsuite/study/server/NetworkModificationTreeTest.java
🚧 Files skipped from review as they are similar to previous changes (10)
- src/test/java/org/gridsuite/study/server/studycontroller/NodeControllerTest.java
- src/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkNodeInfoRepository.java
- src/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/impl/DynamicSimulationClientImpl.java
- src/main/java/org/gridsuite/study/server/service/VoltageInitService.java
- src/main/java/org/gridsuite/study/server/service/dynamicmargincalculation/DynamicMarginCalculationService.java
- src/main/java/org/gridsuite/study/server/service/dynamicsecurityanalysis/DynamicSecurityAnalysisService.java
- src/main/java/org/gridsuite/study/server/service/LoadFlowService.java
- src/main/java/org/gridsuite/study/server/service/SecurityAnalysisService.java
- src/main/java/org/gridsuite/study/server/service/PccMinService.java
- src/main/java/org/gridsuite/study/server/service/shortcircuit/ShortCircuitService.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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 366-384: The catch block for RestClientException in the
getElementIds method is currently masking remote failures by returning an empty
list, making it impossible to distinguish between legitimate empty responses and
actual backend failures. Instead of silently catching and returning List.of()
when RestClientException occurs, propagate the exception to the caller or
re-throw it as a domain/runtime exception after logging the error with clear
context. This ensures downstream code can properly handle actual failures rather
than treating them as empty element IDs.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3702-3709: The invalidateStudyRootNetwork method receives a
specific rootNetworkUuid parameter but does not pass it to the doUnbuildNodeTree
call on line 3705, causing doUnbuildNodeTree to iterate and unbuild nodes for
all root networks in the study instead of just the targeted one. Modify the
doUnbuildNodeTree method call to accept and use the rootNetworkUuid parameter so
that only the specified root network's nodes are unbuilt, rather than affecting
all root networks in the study.
🪄 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: 80b7bff7-4a80-4086-9d6c-c7e3b857c911
📒 Files selected for processing (16)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkNodeInfoRepository.javasrc/main/java/org/gridsuite/study/server/service/LoadFlowService.javasrc/main/java/org/gridsuite/study/server/service/PccMinService.javasrc/main/java/org/gridsuite/study/server/service/SecurityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/StateEstimationService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/main/java/org/gridsuite/study/server/service/VoltageInitService.javasrc/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/impl/DynamicSimulationClientImpl.javasrc/main/java/org/gridsuite/study/server/service/dynamicmargincalculation/DynamicMarginCalculationService.javasrc/main/java/org/gridsuite/study/server/service/dynamicsecurityanalysis/DynamicSecurityAnalysisService.javasrc/main/java/org/gridsuite/study/server/service/dynamicsimulation/impl/DynamicSimulationServiceImpl.javasrc/main/java/org/gridsuite/study/server/service/shortcircuit/ShortCircuitService.javasrc/test/java/org/gridsuite/study/server/NetworkModificationTreeTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/NodeControllerTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/gridsuite/study/server/NetworkModificationTreeTest.java
🚧 Files skipped from review as they are similar to previous changes (10)
- src/test/java/org/gridsuite/study/server/studycontroller/NodeControllerTest.java
- src/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkNodeInfoRepository.java
- src/main/java/org/gridsuite/study/server/service/client/dynamicsimulation/impl/DynamicSimulationClientImpl.java
- src/main/java/org/gridsuite/study/server/service/VoltageInitService.java
- src/main/java/org/gridsuite/study/server/service/dynamicmargincalculation/DynamicMarginCalculationService.java
- src/main/java/org/gridsuite/study/server/service/dynamicsecurityanalysis/DynamicSecurityAnalysisService.java
- src/main/java/org/gridsuite/study/server/service/LoadFlowService.java
- src/main/java/org/gridsuite/study/server/service/SecurityAnalysisService.java
- src/main/java/org/gridsuite/study/server/service/PccMinService.java
- src/main/java/org/gridsuite/study/server/service/shortcircuit/ShortCircuitService.java
🛑 Comments failed to post (2)
src/main/java/org/gridsuite/study/server/service/SensitivityAnalysisService.java (1)
366-384:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not mask remote failures as “empty element IDs”.
Line 382 catches
RestClientExceptionand returnsList.of(), which makes backend failures indistinguishable from a legitimate empty response and can drive incorrect downstream behavior. Prefer propagating a domain/runtime exception (or at least logging with clear error context before handling).Suggested change
} catch (RestClientException e) { - return List.of(); + throw new IllegalStateException("Failed to fetch sensitivity element IDs for parametersUuid=" + parametersUuid, e); }📝 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.public List<UUID> getElementIds(UUID parametersUuid) { String path = UriComponentsBuilder .fromPath(DELIMITER + SENSITIVITY_ANALYSIS_API_VERSION + PARAMETERS_URI + "/contingency-lists-and-filters") .buildAndExpand(parametersUuid) .toUriString(); try { List<UUID> elementIds = restTemplate.exchange( sensitivityAnalysisServerBaseUri + path, HttpMethod.GET, null, new ParameterizedTypeReference<List<UUID>>() { } ).getBody(); return elementIds != null ? elementIds : List.of(); } catch (RestClientException e) { throw new IllegalStateException("Failed to fetch sensitivity element IDs for parametersUuid=" + parametersUuid, e); } }🤖 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/SensitivityAnalysisService.java` around lines 366 - 384, The catch block for RestClientException in the getElementIds method is currently masking remote failures by returning an empty list, making it impossible to distinguish between legitimate empty responses and actual backend failures. Instead of silently catching and returning List.of() when RestClientException occurs, propagate the exception to the caller or re-throw it as a domain/runtime exception after logging the error with clear context. This ensures downstream code can properly handle actual failures rather than treating them as empty element IDs.src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
3702-3709:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
invalidateStudyRootNetworkcurrently invalidates every root network, not only the targeted one.Line 3706 calls
doUnbuildNodeTree(...), and that helper iterates all study root networks (Lines 1774–1778). This broadens the side effects of a single-root operation and can clear computation/build state for unrelated root networks.Suggested fix
public void invalidateStudyRootNetwork(UUID studyUuid, UUID rootNetworkUuid, String userId) { rootNetworkService.assertIsRootNetworkInStudy(studyUuid, rootNetworkUuid); var rootNodeUuid = networkModificationTreeService.getStudyRootNodeUuid(studyUuid); - // First we unbuild all nodes - doUnbuildNodeTree(studyUuid, rootNodeUuid, true, true, userId); + // First unbuild nodes only for the targeted root network + invalidateNodeTree( + studyUuid, + rootNodeUuid, + rootNetworkUuid, + InvalidateNodeTreeParameters.ALL_WITH_BLOCK_NODES, + true + ); + notificationService.emitElementUpdated(studyUuid, userId); // Then we erase data linked to root node on all root networks rootNetworkService.invalidateRootNetworkRemoteInfos(List.of(rootNetworkService.getRootNetworkInfos(rootNetworkUuid)), true, false); rootNetworkService.updateRootNetworkIndexationStatus(studyUuid, rootNetworkUuid, RootNetworkIndexationStatus.NOT_INDEXED); notificationService.emitRootNetworksUpdated(studyUuid); }🤖 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/StudyService.java` around lines 3702 - 3709, The invalidateStudyRootNetwork method receives a specific rootNetworkUuid parameter but does not pass it to the doUnbuildNodeTree call on line 3705, causing doUnbuildNodeTree to iterate and unbuild nodes for all root networks in the study instead of just the targeted one. Modify the doUnbuildNodeTree method call to accept and use the rootNetworkUuid parameter so that only the specified root network's nodes are unbuilt, rather than affecting all root networks in the study.
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
Allow to get multiple computation result statuses at once for each computation server.
This is useful for example when checking computation result statuses on multiple nodes (branch) in the network modification tree.
choices :
findAllById: Returns all instances of the type T with the given IDs.If some or all ids are not found, no entities are returned for these IDs.
Then if we ask to a computation service statuses for 3
resultUuidsfor example, we could obtain 3 or less statuses.Then the size of
resultUuidsand of the returned map could not be the same. The map is less or equally sized.Observation:
Why not managing NO_CONTENT returns like other kind of StudyException and manage it in the
StudyExceptionHandler?