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>
Meklo
left a comment
Meklo
left a comment
There was a problem hiding this comment.
Adding a test for composite splitting would be interesting or it follows the exact same path as composite insertion in study server ?
| studyService.invalidateNodeTreeWithLF(targetStudyUuid, targetNodeUuid); | ||
| try { | ||
| studyService.duplicateOrInsertNetworkModifications(targetStudyUuid, targetNodeUuid, originStudyUuid, originNodeUuid, modificationsToCopy, userId, action); | ||
| studyService.duplicateNetworkModifications(targetStudyUuid, targetNodeUuid, originStudyUuid, modificationsToCopyUuidList, userId); |
There was a problem hiding this comment.
I may be missing something but in duplicateNetworkModifications, originStudyUuid has been removed as a parameter yet it is still being fed here
There was a problem hiding this comment.
You are right I mixed up originStudyUuid and originNodeUuid ! Thanks a lot. Corrected here : d1ad47b
| copyModificationsToExclude(originNodeUuid, targetNodeUuid, modifications, networkModificationResults); | ||
| copyModificationsToExclude(originNodeUuid, targetNodeUuid, modificationsUuids, networkModificationResults); |
There was a problem hiding this comment.
I noticed this function is called from duplicateNetworkModifications but not insertCompositeNetworkModifications, I don't know much about this exclusion system so is this intentional ?
There was a problem hiding this comment.
This is intentional : this function excludes modification copy according to gridstudy tags (it is not clear though, a comment would help).
For now tags don't appear in gridexplore's composite so this doesn't apply. It probably will change later, but the function will have to be changed to use something else than node's uuids.
Signed-off-by: Mathieu DEHARBE <mathieu.deharbe@rte-france.com>
📝 WalkthroughWalkthroughThis PR refactors network modification handling by introducing a dedicated enum type for composite modification actions, removing the Changes
🚥 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. 📝 Coding Plan
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.41.1)src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/NetworkModificationTest.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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
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
| public ResponseEntity<Void> insertCompositeModifications(@PathVariable("studyUuid") UUID studyUuid, | ||
| @PathVariable("nodeUuid") UUID nodeUuid, | ||
| @RequestParam("action") CompositeModificationsActionType action, | ||
| @RequestBody List<Pair<UUID, String>> modificationsToInsert, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Spring Data Pair JSON deserialization Jackson
💡 Result:
Spring Data's org.springframework.data.util.Pair does not have built-in Jackson JSON serialization/deserialization support. The Pair class is a final immutable tuple without a default constructor or @JsonCreator, so Jackson cannot deserialize it by default. To enable JSON deserialization of Pair (e.g., List<Pair<A, B>>), implement a custom JsonDeserializer and register it with the ObjectMapper via a Jackson SimpleModule. Spring Boot auto-detects @JacksonComponent-annotated serializers/deserializers or Jackson Modules as Spring beans. Example custom deserializer for Pair<MyClassA, MyClassB> (from StackOverflow solution): java import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.DeserializationContext; import com.fasterxml.jackson.databind.JsonDeserializer; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import org.springframework.data.util.Pair; import java.io.IOException; public class MyPairDeserializer extends JsonDeserializer<Pair<MyClassA, MyClassB>> { private static ObjectMapper objectMapper; @Override public Pair<MyClassA, MyClassB> deserialize(JsonParser jsonParser, DeserializationContext deserializationContext) throws IOException, JsonProcessingException { if (objectMapper == null) { objectMapper = new ObjectMapper; objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); } JsonNode treeNode = jsonParser.getCodec.readTree(jsonParser); MyClassA first = objectMapper.treeToValue(treeNode.get("first"), MyClassA.class); MyClassB second = objectMapper.treeToValue(treeNode.get("second"), MyClassB.class); return Pair.of(first, second); } } Register the module (e.g., as a @Bean): java @Configuration public class JacksonConfig { @Bean public Module pairModule { SimpleModule module = new SimpleModule; module.addDeserializer(Pair.class, new MyPairDeserializer); // Note: generics handling may need JavaType return module; } } Expected JSON format: {"first": {...}, "second": {...}} For generic handling across Pair<S, T>, use JsonDeserializer<Pair> with type resolution or parameterize appropriately. A matching serializer may be needed for full round-trip. Spring Data provides Jackson modules for geo types and others, but not Pair (confirmed by absence in official docs). In Spring Data REST, custom modules are auto-registered for domain serialization.
Citations:
- 1: https://stackoverflow.com/questions/53048681/deserialize-org-springframework-data-util-pair-from-json
- 2: https://docs.spring.vmware.com/spring-data-commons/docs/3.0.13/api/org/springframework/data/util/Pair.html
- 3: https://docs.spring.io/spring-boot/reference/features/json.html
- 4: https://docs.spring.io/spring-data/rest/reference/customizing/custom-jackson-deserialization.html
🏁 Script executed:
cat -n src/main/java/org/gridsuite/study/server/controller/StudyController.java | sed -n '685,700p'Repository: gridsuite/study-server
Length of output: 1628
🏁 Script executed:
# Search for custom Pair deserializers or Jackson configurations
rg -i "pairdeserializer|pair.*deserializ|jacksonconfig|JsonDeserializer.*Pair" --type javaRepository: gridsuite/study-server
Length of output: 48
🏁 Script executed:
# Search for Jackson module configuration or SimpleModule registration
rg -i "simplemodule|jacksonmodule|@.*Component.*jackson" --type java -A 3Repository: gridsuite/study-server
Length of output: 48
🏁 Script executed:
# Search for any Jackson ObjectMapper bean configuration
rg -i "objectmapper|jackson.*bean|@bean.*jackson" --type java -B 2 -A 4Repository: gridsuite/study-server
Length of output: 50379
🏁 Script executed:
# Search for Spring `@Configuration` classes
rg "@Configuration" --type java -A 5 | head -50Repository: gridsuite/study-server
Length of output: 3041
🏁 Script executed:
# Check if Pair is imported from Spring Data and if it's used elsewhere
rg "import.*Pair|org\.springframework\.data\.util\.Pair" --type javaRepository: gridsuite/study-server
Length of output: 1243
🏁 Script executed:
# Search for Pair usage in test files to see if the endpoint is tested
rg "insertCompositeModifications" --type java -B 2 -A 5Repository: gridsuite/study-server
Length of output: 3721
🏁 Script executed:
# Check pom.xml or gradle files for Jackson dependencies
find . -name "pom.xml" -o -name "build.gradle" | head -5Repository: gridsuite/study-server
Length of output: 75
🏁 Script executed:
cat pom.xml | grep -A 5 -B 5 "spring-boot\|jackson\|spring-data" | head -60Repository: gridsuite/study-server
Length of output: 2181
🏁 Script executed:
# Search for any custom Jackson deserializers or Jackson bean definitions in the codebase
rg "JsonDeserializer|@JsonDeserialize|ObjectMapper.*bean|addDeserializer" --type javaRepository: gridsuite/study-server
Length of output: 48
🏁 Script executed:
# Search for test classes that test the insertCompositeModifications endpoint
rg "mockMvc.*insertCompositeModifications|post.*composite-modifications" --type java -B 2 -A 5Repository: gridsuite/study-server
Length of output: 48
🏁 Script executed:
# Check if there are any integration tests for the StudyController
rg "class.*StudyControllerTest|class.*StudyTest.*Test" --type java | head -10Repository: gridsuite/study-server
Length of output: 177
🏁 Script executed:
# Search for actual calls to insertCompositeModifications in tests
rg "insertCompositeModifications" --type java -C 3Repository: gridsuite/study-server
Length of output: 3562
The endpoint cannot be invoked via HTTP as currently written — use a dedicated DTO for the request body.
org.springframework.data.util.Pair lacks native Jackson deserialization support and requires a custom JsonDeserializer to deserialize from JSON. Without one, any HTTP call to this endpoint will fail at the deserialization stage. A custom record or class like CompositeModificationInfo(UUID uuid, String name) is not just clearer for API contracts and OpenAPI documentation — it's required for the endpoint to function.
♻️ Example DTO approach
// Create a new record/class
public record CompositeModificationInfo(UUID uuid, String name) {}
// Update the endpoint signature
`@RequestBody` List<CompositeModificationInfo> modificationsToInsert,🤖 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/controller/StudyController.java`
around lines 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.
There was a problem hiding this comment.
Interesting : it wants me to go back to the dto that I just removed.
And it is wrong : this Pair system works fine (it doesn't mean that this is the best way of course, but it does work).
There was a problem hiding this comment.
@Mathieu-Deharbe, you're right — I apologize for the incorrect analysis. Spring Boot auto-configures Jackson support for org.springframework.data.util.Pair via spring-data-commons (through SpringDataWebAutoConfiguration), so deserialization works without any custom deserializer. My comment was wrong on both counts: the Pair serialization works fine, and the suggested fix would have essentially re-introduced the ModificationsToCopyInfos DTO you deliberately removed.
✏️ Learnings added
Learnt from: Mathieu-Deharbe
Repo: gridsuite/study-server PR: 965
File: src/main/java/org/gridsuite/study/server/controller/StudyController.java:689-692
Timestamp: 2026-03-20T11:42:11.589Z
Learning: In the gridsuite/study-server project (Java/Spring Boot), `org.springframework.data.util.Pair` can be used as a `RequestBody` type (e.g., `List<Pair<UUID, String>>`) in Spring MVC controllers without a custom Jackson deserializer. Spring Boot auto-configures Jackson support for `Pair` via `spring-data-commons` (`SpringDataWebAutoConfiguration`), so no custom `JsonDeserializer` is needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| 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.
| public ResponseEntity<Void> insertCompositeModifications(@PathVariable("studyUuid") UUID studyUuid, | ||
| @PathVariable("nodeUuid") UUID nodeUuid, | ||
| @RequestParam("action") CompositeModificationsActionType action, | ||
| @RequestBody List<Pair<UUID, String>> modificationsToInsert, | ||
| @RequestHeader(HEADER_USER_ID) String userId) { |
There was a problem hiding this comment.
Using List<Pair<UUID, String>> as a request body couples the public API schema to org.springframework.data.util.Pair (with first/second fields) and makes the contract unclear for API consumers. Consider introducing a dedicated DTO (e.g., { uuid, name }) for composite modifications instead of Pair to produce a stable, self-describing OpenAPI schema and JSON representation.
| public void insertCompositeNetworkModifications( | ||
| UUID targetStudyUuid, | ||
| UUID targetNodeUuid, | ||
| List<Pair<UUID, String>> modifications, | ||
| String userId, | ||
| StudyConstants.CompositeModificationsActionType action) { |
There was a problem hiding this comment.
insertCompositeNetworkModifications takes an action that can be SPLIT as well as INSERT, so the method name is misleading (it may not actually insert composites). Renaming it (and corresponding controller helper) to something action-neutral (e.g., applyCompositeNetworkModifications) would make call sites clearer and reduce the risk of misuse.
| @Operation(summary = "For a list of composite network modifications passed in body, insert them into the target node") | ||
| @ApiResponses(value = {@ApiResponse(responseCode = "200", description = "The composite modification list has been inserted.")}) |
There was a problem hiding this comment.
The new /composite-modifications endpoint accepts an action that can be SPLIT or INSERT, but the OpenAPI summary/response description only describes insertion. This is misleading for SPLIT; please update the summary/ApiResponse text (or split into two endpoints) so behavior matches the documented contract.
| @Operation(summary = "For a list of composite network modifications passed in body, insert them into the target node") | |
| @ApiResponses(value = {@ApiResponse(responseCode = "200", description = "The composite modification list has been inserted.")}) | |
| @Operation(summary = "For a list of composite network modifications passed in body, apply the requested action (insert or split) into the target node") | |
| @ApiResponses(value = {@ApiResponse(responseCode = "200", description = "The composite modification list has been updated according to the requested action.")}) |
Separates composite handling from regular network modification handling