Open
Conversation
Contributor
Mathieu-Deharbe
commented
Mar 11, 2026
Mathieu-Deharbe
commented
- Creates a specific controller for the composite modifications
-
- new composite controler TU class
- Split some endpoints, enum etc to separate both
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
reviewed
Mar 19, 2026
src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java
Show resolved
Hide resolved
| public CompletableFuture<ResponseEntity<NetworkModificationsResult>> insertCompositeModifications( | ||
| @Parameter(description = "updated group UUID, where modifications are inserted") @PathVariable("groupUuid") UUID targetGroupUuid, | ||
| @Parameter(description = "Insertion method", required = true) @RequestParam(value = "action") CompositeModificationAction action, | ||
| @RequestBody Pair<List<Pair<UUID, String>>, List<ModificationApplicationContext>> modificationContextInfos) { |
Contributor
There was a problem hiding this comment.
Why the need of a Pair inbetween List<Pair<UUID, String>> and List I'm not sure I get the logic here, there can be multiple instances of List<Pair<UUID, String> as input ?
Contributor
Author
There was a problem hiding this comment.
I am not sure that I understand your question.
Pair<List<Pair<UUID, String>>, List<ModificationApplicationContext>>
mans :
- one
List<Pair<UUID, String>: yes there might be several composite modifications inserted simultaneously. - one
List<ModificationApplicationContext>: I didn't look into it much but there are severalModificationApplicationContext, one for each root network in the targetted study.
But no there can't be multiple instances of List<Pair<UUID, String> as input.
I don't really know why this data is sent as a pair though. I just kept the previous system from handleNetworkModifications. It could be separated, but they are in the body so this is probably the reason.
Contributor
There was a problem hiding this comment.
Yeah i just checked it follows what was done in handleNetworkModifications, that said it is getting really confusing the way those data structure get imbricated
Contributor
Author
There was a problem hiding this comment.
Indeed. Maybe in the end we will have to use a dto and copy it to study-server...
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIntroduces a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CompositeController as CompositeController
participant Service as NetworkModificationService
participant Repo as NetworkModificationRepository
participant Store as NetworkStoreService
Client->>CompositeController: HTTP PUT /v{v}/network-composite-modifications/{target}?action=INSERT (body: Pair<List<Pair<UUID,String>>, List<Context>>)
CompositeController->>Service: insertCompositeModificationsIntoGroup(targetGroupUuid, modificationContextInfos)
Service->>Repo: insertCompositeModificationsIntoGroup(targetGroupUuid, modificationContextInfos.getFirst())
Repo->>Store: load ModificationInfos for UUIDs
Store-->>Repo: ModificationInfos list
Repo-->>Service: persisted ModificationInfos (with applied names)
Service-->>CompositeController: CompletableFuture<NetworkModificationsResult>
CompositeController-->>Client: HTTP 200 + result
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/test/java/org/gridsuite/modification/server/CompositeControllerTest.java (2)
298-299: Prefer non-exact exception assertion for resilience.Asserting the full message string is brittle. It’s safer to assert exception type and key fragment(s) (e.g., error code and UUID).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/modification/server/CompositeControllerTest.java` around lines 298 - 299, Update the test in CompositeControllerTest to avoid asserting the full exception message; instead call assertThrows(NetworkModificationException.class, () -> modificationRepository.getModificationInfo(returnedNewId)) to capture the exception, then assert that the exception message contains the expected fragments (e.g., "MODIFICATION_NOT_FOUND" and the returnedNewId) using contains/containsAll-style assertions; reference the existing modificationRepository.getModificationInfo(returnedNewId), returnedNewId and NetworkModificationException to locate and modify the assertion accordingly.
326-332: Strengthen update verification beyond list size.Line 331 checks only count. The test can still pass if wrong modifications are returned with the same size. Consider asserting UUID equality with
newModificationUuids.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/modification/server/CompositeControllerTest.java` around lines 326 - 332, The test currently only checks list size; strengthen it by verifying returned modification UUIDs match expected newModificationUuids: after deserializing into updatedCompositeContent (List<ModificationInfos>), extract the UUIDs from each ModificationInfos (use the UUID/getId accessor on ModificationInfos) and compare to newModificationUuids — ideally compare as unordered sets to avoid order sensitivity; use the same request/response variables (mvcResult, mapper, URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT, compositeModificationUuid) and replace or augment the assertEquals(newModificationsNumber, updatedCompositeContent.size()) with an assertion that the sets of UUIDs are equal.
🤖 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/modification/server/NetworkModificationController.java`:
- Around line 97-105: The new handleNetworkModifications signature narrows
supported actions and changes the request body shape, breaking existing clients;
restore backward compatibility by accepting the previous payload shape and
legacy action values: update handleNetworkModifications to detect both the new
Pair<List<UUID>,List<ModificationApplicationContext>> body and the old payload
(e.g., a wrapper object or plain List<UUID>) and branch accordingly, and allow
legacy GroupModificationAction values (or map them to the new enum) when parsing
the action request param; mark the old-path handling as deprecated (log a
deprecation warning) so you can safely remove it in the next release.
In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`:
- Around line 790-804: The loop in the method that processes compositesUuidName
can throw ClassCastException and reuses the same DTO for duplicate UUIDs; fix by
first building a Map<UUID, Deque<CompositeModificationInfos>> from the
List<ModificationInfos> returned by getModificationsInfosNonTransactional(...)
by filtering only instances of CompositeModificationInfos and grouping them into
Deque queues per UUID, then iterate compositesUuidName and for each Pair poll()
one CompositeModificationInfos from the map entry for
compositeUuidName.getFirst(), setName(compositeUuidName.getSecond()) on that
polled instance and add it to newCompositeModifications; if the map has no entry
or the deque is empty, log via LOGGER.error that the composite UUID/name could
not be applied.
In
`@src/test/java/org/gridsuite/modification/server/CompositeControllerTest.java`:
- Around line 66-67: Tests construct endpoints with duplicated slashes because
URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT ends with a '/' and callers concatenate
values that also start with '/', causing fragile URLs; fix by normalizing
constants and usages: remove the trailing slash from
URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT (and any other constants that end with
'/') or remove leading slashes from the concatenated segments so concatenation
like URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT + URI_NETWORK_MODIF_BASE produces a
single slash; update all affected constants/usages referenced
(URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT, URI_NETWORK_MODIF_BASE and the other
similar constants around the commented ranges) to consistently store paths
without duplicate slashes.
---
Nitpick comments:
In
`@src/test/java/org/gridsuite/modification/server/CompositeControllerTest.java`:
- Around line 298-299: Update the test in CompositeControllerTest to avoid
asserting the full exception message; instead call
assertThrows(NetworkModificationException.class, () ->
modificationRepository.getModificationInfo(returnedNewId)) to capture the
exception, then assert that the exception message contains the expected
fragments (e.g., "MODIFICATION_NOT_FOUND" and the returnedNewId) using
contains/containsAll-style assertions; reference the existing
modificationRepository.getModificationInfo(returnedNewId), returnedNewId and
NetworkModificationException to locate and modify the assertion accordingly.
- Around line 326-332: The test currently only checks list size; strengthen it
by verifying returned modification UUIDs match expected newModificationUuids:
after deserializing into updatedCompositeContent (List<ModificationInfos>),
extract the UUIDs from each ModificationInfos (use the UUID/getId accessor on
ModificationInfos) and compare to newModificationUuids — ideally compare as
unordered sets to avoid order sensitivity; use the same request/response
variables (mvcResult, mapper, URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT,
compositeModificationUuid) and replace or augment the
assertEquals(newModificationsNumber, updatedCompositeContent.size()) with an
assertion that the sets of UUIDs are equal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a05d48d2-8788-4b08-8db3-af9e3f7d32aa
📒 Files selected for processing (10)
src/main/java/org/gridsuite/modification/server/CompositeController.javasrc/main/java/org/gridsuite/modification/server/NetworkModificationController.javasrc/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.javasrc/main/java/org/gridsuite/modification/server/service/NetworkModificationService.javasrc/test/java/org/gridsuite/modification/server/CompositeControllerTest.javasrc/test/java/org/gridsuite/modification/server/ModificationControllerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/AbstractNetworkModificationTest.javasrc/test/java/org/gridsuite/modification/server/service/ModificationIndexationTest.javasrc/test/java/org/gridsuite/modification/server/utils/ApiUtils.javasrc/test/java/org/gridsuite/modification/server/utils/TestUtils.java
Comment on lines
+97
to
+105
| @Operation(summary = "For a list of network modifications passed in body, Move them before another one or at the end of the list, or Duplicate them at the end of the list") | ||
| @ApiResponse(responseCode = "200", description = "The modification list of the group has been updated.") | ||
| public CompletableFuture<ResponseEntity<NetworkModificationsResult>> handleNetworkModifications( | ||
| @Parameter(description = "updated group UUID, where modifications are pasted") @PathVariable("groupUuid") UUID targetGroupUuid, | ||
| @Parameter(description = "kind of modification", required = true) @RequestParam(value = "action") GroupModificationAction action, | ||
| @Parameter(description = "the modification Uuid to move before (MOVE option, empty means moving at the end)") @RequestParam(value = "before", required = false) UUID beforeModificationUuid, | ||
| @Parameter(description = "origin group UUID, where modifications are copied or cut") @RequestParam(value = "originGroupUuid", required = false) UUID originGroupUuid, | ||
| @Parameter(description = "modifications can be applied (default is true)") @RequestParam(value = "build", required = false, defaultValue = "true") Boolean canApply, | ||
| @RequestBody Pair<List<ModificationsToCopyInfos>, List<ModificationApplicationContext>> modificationContextInfos) { | ||
| List<UUID> modificationsUuids = modificationContextInfos.getFirst().stream().map(ModificationsToCopyInfos::getUuid).toList(); | ||
| @RequestBody Pair<List<UUID>, List<ModificationApplicationContext>> modificationContextInfos) { |
There was a problem hiding this comment.
This narrows an existing v1 endpoint in a breaking way.
The /v1/groups/{groupUuid} contract now only binds COPY|MOVE, and the request body is now a bare UUID list instead of the previous object payload shape. Existing clients will start getting 400s until they are redeployed against the new composite route and payload. Please keep a deprecated compatibility layer for one release, or version this API change explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/modification/server/NetworkModificationController.java`
around lines 97 - 105, The new handleNetworkModifications signature narrows
supported actions and changes the request body shape, breaking existing clients;
restore backward compatibility by accepting the previous payload shape and
legacy action values: update handleNetworkModifications to detect both the new
Pair<List<UUID>,List<ModificationApplicationContext>> body and the old payload
(e.g., a wrapper object or plain List<UUID>) and branch accordingly, and allow
legacy GroupModificationAction values (or map them to the new enum) when parsing
the action request param; mark the old-path handling as deprecated (log a
deprecation warning) so you can safely remove it in the next release.
Comment on lines
+790
to
+804
| @NonNull List<Pair<UUID, String>> compositesUuidName) { | ||
| List<UUID> compositeUuids = compositesUuidName.stream().map(Pair::getFirst).toList(); | ||
| List<ModificationInfos> newCompositeModifications = new ArrayList<>(); | ||
| for (ModificationsToCopyInfos compositeModification : compositeModifications) { | ||
| CompositeModificationInfos newCompositeModification = cloneCompositeModification(compositeModification); | ||
| newCompositeModifications.add(newCompositeModification); | ||
| List<ModificationInfos> modificationInfos = getModificationsInfosNonTransactional(compositeUuids); | ||
| // apply the new composite name to the corresponding composite modifications | ||
| for (Pair<UUID, String> compositeUuidName : compositesUuidName) { | ||
| CompositeModificationInfos newCompositeModification = (CompositeModificationInfos) modificationInfos.stream() | ||
| .filter(modif -> modif.getUuid().equals(compositeUuidName.getFirst())) | ||
| .findFirst().orElse(null); | ||
| if (newCompositeModification != null) { | ||
| newCompositeModification.setName(compositeUuidName.getSecond()); | ||
| newCompositeModifications.add(newCompositeModification); | ||
| } else { | ||
| LOGGER.error("Could not find composite modification with uuid {} to apply its name {}", compositeUuidName.getFirst(), compositeUuidName.getSecond()); | ||
| } |
There was a problem hiding this comment.
Validate the fetched DTO type and consume one match per requested insert.
Line 796 can throw a ClassCastException when a non-composite UUID slips into this endpoint, and the findFirst() lookup on Lines 796-798 reuses the same DTO when the same composite UUID is inserted twice. For a request like [(A, "name-1"), (A, "name-2")], both saved copies end up carrying the last assigned name. Build a validated Map<UUID, Deque<CompositeModificationInfos>> (or fetch composites only) and poll() one DTO per input pair.
💡 Sketch of a safe lookup
- List<ModificationInfos> modificationInfos = getModificationsInfosNonTransactional(compositeUuids);
- // apply the new composite name to the corresponding composite modifications
- for (Pair<UUID, String> compositeUuidName : compositesUuidName) {
- CompositeModificationInfos newCompositeModification = (CompositeModificationInfos) modificationInfos.stream()
- .filter(modif -> modif.getUuid().equals(compositeUuidName.getFirst()))
- .findFirst().orElse(null);
+ Map<UUID, Deque<CompositeModificationInfos>> compositesById = getModificationsInfosNonTransactional(compositeUuids).stream()
+ .map(modif -> {
+ if (!(modif instanceof CompositeModificationInfos composite)) {
+ throw new NetworkModificationException(MODIFICATION_ERROR,
+ String.format("Modification (%s) is not a composite modification", modif.getUuid()));
+ }
+ return composite;
+ })
+ .collect(Collectors.groupingBy(
+ CompositeModificationInfos::getUuid,
+ LinkedHashMap::new,
+ Collectors.toCollection(ArrayDeque::new)
+ ));
+ for (Pair<UUID, String> compositeUuidName : compositesUuidName) {
+ CompositeModificationInfos newCompositeModification = Optional.ofNullable(compositesById.get(compositeUuidName.getFirst()))
+ .map(Deque::pollFirst)
+ .orElse(null);
if (newCompositeModification != null) {
newCompositeModification.setName(compositeUuidName.getSecond());
newCompositeModifications.add(newCompositeModification);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/modification/server/repositories/NetworkModificationRepository.java`
around lines 790 - 804, The loop in the method that processes compositesUuidName
can throw ClassCastException and reuses the same DTO for duplicate UUIDs; fix by
first building a Map<UUID, Deque<CompositeModificationInfos>> from the
List<ModificationInfos> returned by getModificationsInfosNonTransactional(...)
by filtering only instances of CompositeModificationInfos and grouping them into
Deque queues per UUID, then iterate compositesUuidName and for each Pair poll()
one CompositeModificationInfos from the map entry for
compositeUuidName.getFirst(), setName(compositeUuidName.getSecond()) on that
polled instance and add it to newCompositeModifications; if the map has no entry
or the deque is empty, log via LOGGER.error that the composite UUID/name could
not be applied.
Comment on lines
+66
to
+67
| private static final String URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT = "/v1/network-composite-modifications/"; | ||
| private static final String URI_NETWORK_MODIF_BASE = "/v1/network-modifications"; |
There was a problem hiding this comment.
Avoid double-slash endpoint construction in test URLs.
Line 66 already ends with /, and later concatenations also prepend /, producing URLs like ...//network-modifications. This is fragile across path matchers.
Proposed fix
- private static final String URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT = "/v1/network-composite-modifications/";
+ private static final String URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT = "/v1/network-composite-modifications";Also applies to: 125-127, 150-151, 156-157, 327-328, 362-363
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/gridsuite/modification/server/CompositeControllerTest.java`
around lines 66 - 67, Tests construct endpoints with duplicated slashes because
URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT ends with a '/' and callers concatenate
values that also start with '/', causing fragile URLs; fix by normalizing
constants and usages: remove the trailing slash from
URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT (and any other constants that end with
'/') or remove leading slashes from the concatenated segments so concatenation
like URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT + URI_NETWORK_MODIF_BASE produces a
single slash; update all affected constants/usages referenced
(URI_GET_COMPOSITE_NETWORK_MODIF_CONTENT, URI_NETWORK_MODIF_BASE and the other
similar constants around the commented ranges) to consistently store paths
without duplicate slashes.
Meklo
approved these changes
Mar 26, 2026
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.