Open
Conversation
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
📝 WalkthroughWalkthroughController and service APIs were changed to use UUID lists for modification operations, a new enum for composite actions was introduced, a DTO was removed, a new composite-insert endpoint added, and corresponding service and tests updated to the new payload shapes and flows. Changes
Sequence Diagram(s)mermaid mermaid 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Contributor
Author
They follow exactly the same path so I don't think a specific test would bring anything. Actually |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/gridsuite/study/server/service/NetworkModificationService.java (1)
260-277:⚠️ Potential issue | 🔴 CriticalFail fast when the remote mutation response is empty.
These methods all return
exchange(...).getBody()directly, but the new callers immediately consume duplicated UUIDs and impact results from that body. A successfulPUTwith no payload will now surface later as a local NPE/IOOBE after the remote write already happened.Suggested guard at the client boundary
- return restTemplate.exchange( + NetworkModificationsResult result = restTemplate.exchange( getNetworkModificationServerURI(false) + path.buildAndExpand(targetGroupUuid).toUriString(), HttpMethod.PUT, httpEntity, - NetworkModificationsResult.class).getBody(); + NetworkModificationsResult.class).getBody(); + return Objects.requireNonNull(result, "Expected NetworkModificationsResult body");Apply the same pattern to the other changed methods that currently return
getBody()directly.Also applies to: 280-300, 303-321, 344-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/study/server/service/NetworkModificationService.java` around lines 260 - 277, The REST client call in moveModifications currently returns restTemplate.exchange(...).getBody() directly which can be null and cause NPE/IOOBE downstream; update moveModifications (and the other similar methods that call restTemplate.exchange(...).getBody()) to capture the ResponseEntity<NetworkModificationsResult>, check responseEntity.getBody() for null, and if null throw a clear, fast-fail exception (e.g., IllegalStateException or a custom exception) with a descriptive message indicating the remote PUT returned an empty body for the MOVE action and include context (originGroupUuid, targetGroupUuid); ensure you reference the restTemplate.exchange call and NetworkModificationsResult so the guard is added at the client boundary for all analogous methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/gridsuite/study/server/controller/StudyController.java`:
- Around line 689-692: The endpoint StudyController.insertCompositeModifications
currently accepts a RequestBody List of org.springframework.data.util.Pair which
Jackson cannot deserialize; create a dedicated DTO (e.g.,
CompositeModificationInfo with fields uuid and name) and replace the parameter
type modificationsToInsert from List<Pair<UUID,String>> to
List<CompositeModificationInfo>, update any imports/usages and OpenAPI
annotations accordingly, and ensure the new DTO is a public record or class with
JVM-accessible properties so Jackson can serialize/deserialize it without custom
deserializers.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 2424-2426: The call to
networkModificationService.duplicateModifications(...) assumes the response has
one duplicated UUID per requested source and then blindly calls
copyModificationsToExclude; update the code around duplicateModifications and
copyModificationsToExclude to first validate that NetworkModificationsResult (or
its duplicates list/map) contains exactly the same number of entries as
modificationsUuids (and that each source UUID maps to a duplicate UUID), and if
the sizes/mapping do not match, fail fast (throw an exception or return an
error) or log and skip to avoid misapplying exclusion tags; apply the same
validation to the other block referenced (around the 2476-2485 area) so you
always map source->duplicate deterministically rather than assuming order.
---
Outside diff comments:
In
`@src/main/java/org/gridsuite/study/server/service/NetworkModificationService.java`:
- Around line 260-277: The REST client call in moveModifications currently
returns restTemplate.exchange(...).getBody() directly which can be null and
cause NPE/IOOBE downstream; update moveModifications (and the other similar
methods that call restTemplate.exchange(...).getBody()) to capture the
ResponseEntity<NetworkModificationsResult>, check responseEntity.getBody() for
null, and if null throw a clear, fast-fail exception (e.g.,
IllegalStateException or a custom exception) with a descriptive message
indicating the remote PUT returned an empty body for the MOVE action and include
context (originGroupUuid, targetGroupUuid); ensure you reference the
restTemplate.exchange call and NetworkModificationsResult so the guard is added
at the client boundary for all analogous methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 996d639b-b58b-4954-a7a4-c2d5a565cee2
📒 Files selected for processing (8)
src/main/java/org/gridsuite/study/server/StudyConstants.javasrc/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/dto/ModificationsToCopyInfos.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/NetworkModificationTest.javasrc/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyControllerRebuildNodeTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/gridsuite/study/server/dto/ModificationsToCopyInfos.java
src/main/java/org/gridsuite/study/server/controller/StudyController.java
Show resolved
Hide resolved
Comment on lines
+2424
to
+2426
| NetworkModificationsResult networkModificationResults = networkModificationService.duplicateModifications(groupUuid, Pair.of(modificationsUuids, modificationApplicationContexts)); | ||
|
|
||
| copyModificationsToExclude(originNodeUuid, targetNodeUuid, modifications, networkModificationResults); | ||
| copyModificationsToExclude(originNodeUuid, targetNodeUuid, modificationsUuids, networkModificationResults); |
There was a problem hiding this comment.
Validate the duplicate response size before copying exclusion tags.
Lines 2426 and 2481-2482 assume the server returns exactly one duplicated UUID per requested source UUID. If one item is missing, this throws; if the contract ever drifts, exclusion tags can be copied onto the wrong duplicate.
Suggested guard
private void copyModificationsToExclude(UUID originNodeUuid,
UUID targetNodeUuid,
List<UUID> modificationsUuids,
NetworkModificationsResult networkModificationResults) {
+ List<UUID> duplicatedModificationUuids = networkModificationResults.modificationUuids();
+ if (duplicatedModificationUuids.size() != modificationsUuids.size()) {
+ throw new IllegalStateException("Duplicate response size mismatch");
+ }
Map<UUID, UUID> mappingModificationsUuids = new HashMap<>();
for (int i = 0; i < modificationsUuids.size(); i++) {
- mappingModificationsUuids.put(modificationsUuids.get(i), networkModificationResults.modificationUuids().get(i));
+ mappingModificationsUuids.put(modificationsUuids.get(i), duplicatedModificationUuids.get(i));
}
rootNetworkNodeInfoService.copyModificationsToExcludeFromTags(originNodeUuid, targetNodeUuid, mappingModificationsUuids);
}Also applies to: 2476-2485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java` around
lines 2424 - 2426, The call to
networkModificationService.duplicateModifications(...) assumes the response has
one duplicated UUID per requested source and then blindly calls
copyModificationsToExclude; update the code around duplicateModifications and
copyModificationsToExclude to first validate that NetworkModificationsResult (or
its duplicates list/map) contains exactly the same number of entries as
modificationsUuids (and that each source UUID maps to a duplicate UUID), and if
the sizes/mapping do not match, fail fast (throw an exception or return an
error) or log and skip to avoid misapplying exclusion tags; apply the same
validation to the other block referenced (around the 2476-2485 area) so you
always map source->duplicate deterministically rather than assuming order.
There was a problem hiding this comment.
Pull request overview
This PR separates composite modification handling from regular network modification operations by introducing a dedicated composite endpoint and simplifying regular copy/move payloads to UUID lists.
Changes:
- Replace
ModificationsToCopyInfospayloads withList<UUID>for regular move/copy/duplicate flows and update impacted services/tests. - Introduce a dedicated
/composite-modificationsendpoint and service path to handle composite-specific insert/split actions. - Remove the
ModificationsToCopyInfosDTO and split action enums intoModificationsActionTypevsCompositeModificationsActionType.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/org/gridsuite/study/server/studycontroller/StudyControllerRebuildNodeTest.java | Updates controller test to pass UUID lists instead of DTOs. |
| src/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.java | Updates duplication tests to new APIs (duplicateNetworkModifications, duplicateModifications). |
| src/test/java/org/gridsuite/study/server/NetworkModificationTest.java | Adapts move/duplicate tests to UUID list payloads and adds composite insertion test coverage. |
| src/main/java/org/gridsuite/study/server/service/StudyService.java | Refactors duplication flow, adds composite insert/split flow, and centralizes impact notification emission. |
| src/main/java/org/gridsuite/study/server/service/NetworkModificationService.java | Updates REST payload types to UUID lists and adds composite modifications REST call. |
| src/main/java/org/gridsuite/study/server/dto/ModificationsToCopyInfos.java | Removes obsolete DTO. |
| src/main/java/org/gridsuite/study/server/controller/StudyController.java | Updates move/copy endpoint payload type and adds composite-modifications endpoint. |
| src/main/java/org/gridsuite/study/server/StudyConstants.java | Splits action enums for regular vs composite modifications. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/org/gridsuite/study/server/controller/StudyController.java
Show resolved
Hide resolved
src/main/java/org/gridsuite/study/server/service/StudyService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/gridsuite/study/server/controller/StudyController.java
Show resolved
Hide resolved
Meklo
approved these changes
Mar 26, 2026
|
SlimaneAmar
reviewed
Mar 27, 2026
| } | ||
|
|
||
| @Transactional | ||
| public void insertCompositeNetworkModifications( |
Contributor
Author
There was a problem hiding this comment.
Agreed : I applied your patch to remove the duplicate : 909f6ff
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
2474-2480:⚠️ Potential issue | 🟠 MajorValidate the duplicated UUID list before copying exclusions.
This still assumes the response contains one duplicated UUID per source UUID in the same order. A short or partial response will either throw here or copy exclusion tags onto the wrong duplicate.
🛡️ Suggested guard
private void copyModificationsToExclude(UUID originNodeUuid, UUID targetNodeUuid, List<UUID> modificationsUuids, NetworkModificationsResult networkModificationResults) { + List<UUID> duplicatedModificationUuids = Optional.ofNullable(networkModificationResults) + .map(NetworkModificationsResult::modificationUuids) + .orElseThrow(() -> new IllegalStateException("Missing duplicated modification UUIDs")); + if (duplicatedModificationUuids.size() != modificationsUuids.size()) { + throw new IllegalStateException("Duplicate response size mismatch"); + } Map<UUID, UUID> mappingModificationsUuids = new HashMap<>(); for (int i = 0; i < modificationsUuids.size(); i++) { - mappingModificationsUuids.put(modificationsUuids.get(i), networkModificationResults.modificationUuids().get(i)); + mappingModificationsUuids.put(modificationsUuids.get(i), duplicatedModificationUuids.get(i)); } rootNetworkNodeInfoService.copyModificationsToExcludeFromTags(originNodeUuid, targetNodeUuid, mappingModificationsUuids); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/study/server/service/StudyService.java` around lines 2474 - 2480, The loop in copyModificationsToExclude assumes networkModificationResults.modificationUuids() has the same length and one-to-one ordering as modificationsUuids, which can throw or mis-map when the response is partial; validate the duplicated UUID list first by checking that networkModificationResults.modificationUuids() is non-null and its size equals modificationsUuids.size(), and if not either throw a clear IllegalStateException or log and abort the copy; alternatively build a lookup (e.g., Map<UUID,List<UUID>>) from networkModificationResults.modificationUuids() and explicitly match each source UUID to a single target UUID (or fail) before populating mappingModificationsUuids to ensure correct one-to-one mapping rather than relying on positional indexing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 2461-2466: The method sendImpactNotifications currently calls
networkModificationResults.modificationResults() without guarding for null,
which can NPE; before iterating in sendImpactNotifications, check that
networkModificationResults != null and
networkModificationResults.modificationResults() != null (and optionally not
empty) and return early if null/empty; update the for-loop that iterates over
modificationResultOpt to only run when modificationResults() is non-null,
keeping the existing handling of Optional<NetworkModificationResult> and
studyRootNetworkEntities.get(index) unchanged.
---
Duplicate comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 2474-2480: The loop in copyModificationsToExclude assumes
networkModificationResults.modificationUuids() has the same length and
one-to-one ordering as modificationsUuids, which can throw or mis-map when the
response is partial; validate the duplicated UUID list first by checking that
networkModificationResults.modificationUuids() is non-null and its size equals
modificationsUuids.size(), and if not either throw a clear IllegalStateException
or log and abort the copy; alternatively build a lookup (e.g.,
Map<UUID,List<UUID>>) from networkModificationResults.modificationUuids() and
explicitly match each source UUID to a single target UUID (or fail) before
populating mappingModificationsUuids to ensure correct one-to-one mapping rather
than relying on positional indexing.
🪄 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: a00136ba-229c-4e83-971f-5c48da489a82
📒 Files selected for processing (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java
Comment on lines
+2461
to
+2466
| private void sendImpactNotifications(UUID targetStudyUuid, UUID targetNodeUuid, NetworkModificationsResult networkModificationResults, List<RootNetworkEntity> studyRootNetworkEntities) { | ||
| if (networkModificationResults != null) { | ||
| int index = 0; | ||
| // for each NetworkModificationResult, send an impact notification - studyRootNetworkEntities are ordered in the same way as networkModificationResults | ||
| for (Optional<NetworkModificationResult> modificationResultOpt : networkModificationResults.modificationResults()) { | ||
| if (modificationResultOpt.isPresent() && studyRootNetworkEntities.get(index) != null) { |
There was a problem hiding this comment.
Guard modificationResults() before iterating.
This helper only checks the wrapper object. The same DTO is treated as nullable elsewhere in this class, so a backend response without per-root results will NPE after the remote duplicate/insert has already succeeded.
🛡️ Suggested guard
private void sendImpactNotifications(UUID targetStudyUuid, UUID targetNodeUuid, NetworkModificationsResult networkModificationResults, List<RootNetworkEntity> studyRootNetworkEntities) {
- if (networkModificationResults != null) {
+ if (networkModificationResults != null && networkModificationResults.modificationResults() != null) {
int index = 0;
// for each NetworkModificationResult, send an impact notification - studyRootNetworkEntities are ordered in the same way as networkModificationResults
for (Optional<NetworkModificationResult> modificationResultOpt : networkModificationResults.modificationResults()) {
if (modificationResultOpt.isPresent() && studyRootNetworkEntities.get(index) != null) {
emitNetworkModificationImpacts(targetStudyUuid, targetNodeUuid, studyRootNetworkEntities.get(index).getId(), modificationResultOpt.get());📝 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
| private void sendImpactNotifications(UUID targetStudyUuid, UUID targetNodeUuid, NetworkModificationsResult networkModificationResults, List<RootNetworkEntity> studyRootNetworkEntities) { | |
| if (networkModificationResults != null) { | |
| int index = 0; | |
| // for each NetworkModificationResult, send an impact notification - studyRootNetworkEntities are ordered in the same way as networkModificationResults | |
| for (Optional<NetworkModificationResult> modificationResultOpt : networkModificationResults.modificationResults()) { | |
| if (modificationResultOpt.isPresent() && studyRootNetworkEntities.get(index) != null) { | |
| private void sendImpactNotifications(UUID targetStudyUuid, UUID targetNodeUuid, NetworkModificationsResult networkModificationResults, List<RootNetworkEntity> studyRootNetworkEntities) { | |
| if (networkModificationResults != null && networkModificationResults.modificationResults() != null) { | |
| int index = 0; | |
| // for each NetworkModificationResult, send an impact notification - studyRootNetworkEntities are ordered in the same way as networkModificationResults | |
| for (Optional<NetworkModificationResult> modificationResultOpt : networkModificationResults.modificationResults()) { | |
| if (modificationResultOpt.isPresent() && studyRootNetworkEntities.get(index) != null) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java` around
lines 2461 - 2466, The method sendImpactNotifications currently calls
networkModificationResults.modificationResults() without guarding for null,
which can NPE; before iterating in sendImpactNotifications, check that
networkModificationResults != null and
networkModificationResults.modificationResults() != null (and optionally not
empty) and return early if null/empty; update the for-loop that iterates over
modificationResultOpt to only run when modificationResults() is non-null,
keeping the existing handling of Optional<NetworkModificationResult> and
studyRootNetworkEntities.get(index) unchanged.
SlimaneAmar
approved these changes
Mar 27, 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
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.
Separates composite handling from regular network modification handling