keep applicability when move composites#1006
Open
souissimai wants to merge 6 commits into
Open
Conversation
Signed-off-by: SOUISSI Maissa (Externe) <[email protected]>
|
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 (1)
📝 WalkthroughWalkthroughmoveNetworkModifications now expands moved modification UUIDs to leaf-level UUIDs before updating node exclusion mappings. Tests add WireMock stubs/verifications for the children-UUIDs endpoint and mock expandToLeafUuids; test setup now removes root network trees before deleting studies. ChangesLeaf UUID Expansion for Moved Modifications
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.java (1)
805-808: ⚡ Quick winMake the test prove expansion is actually part of the behavior, not just stubbed.
Right now the stub returns the same set as input, so this path still passes without validating that expansion materially impacts exclusion remapping. Add an interaction assertion (
verify(expandToLeafUuids(...))) and preferably return a superset (includes a leaf UUID) to make the assertion behavior-sensitive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.java` around lines 805 - 808, The test currently stubs networkModificationService.expandToLeafUuids(modificationsToMove) to return the same set so expansion isn't exercised; change the stub to return a superset that includes an additional leaf UUID (e.g., create a new HashSet<>(modificationsToMove) and add a specific leafUuid) and update expected remapped exclusions to account for that leaf so the behavior is sensitive to expansion, then add a Mockito.verify(networkModificationService).expandToLeafUuids(modificationsToMove) interaction assertion to ensure the method was actually invoked during the test; keep references to expandToLeafUuids, networkModificationService, modificationsToMove, and the new leaf UUID in your changes.
🤖 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/StudyService.java`:
- Around line 2271-2272: The code calls
rootNetworkNodeInfoService.moveModificationsToExclude immediately after
performing the remote move but dereferences the expansion result without
checking for failure, which can skip local exclusion remapping if
expandToLeafUuids fails; first call
networkModificationService.expandToLeafUuids(networkModificationsResult.modificationUuids())
and validate its result (handle null or exceptions) before—or as part of—a
single transactional flow so the remote move and local remapping stay
consistent; if expansion fails, fall back to an empty list or retry/log and
still attempt to update local exclusion mappings (or roll back the remote move),
and ensure expandToLeafUuids and moveModificationsToExclude calls are wrapped
with try/catch that logs errors and preserves consistency between remote
modifications and local exclusion state using the originNodeUuid and
targetNodeUuid identifiers.
In `@src/test/java/org/gridsuite/study/server/NetworkModificationTest.java`:
- Around line 2432-2438: The WireMock stub in NetworkModificationTest currently
only verifies that the "uuids" query param contains modification1, which is
insufficient for MOVE payloads that include both modification1 and
modification2; update the withQueryParam matcher on the stubFor call (the
WireMock.get(...).withQueryParam("uuids", ...)) to assert that both
modification1.toString() and modification2.toString() are present (for example
by using a matching/regex or a combined containing matcher that checks for both
UUID substrings) so the test fails if either UUID is omitted.
---
Nitpick comments:
In
`@src/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.java`:
- Around line 805-808: The test currently stubs
networkModificationService.expandToLeafUuids(modificationsToMove) to return the
same set so expansion isn't exercised; change the stub to return a superset that
includes an additional leaf UUID (e.g., create a new
HashSet<>(modificationsToMove) and add a specific leafUuid) and update expected
remapped exclusions to account for that leaf so the behavior is sensitive to
expansion, then add a
Mockito.verify(networkModificationService).expandToLeafUuids(modificationsToMove)
interaction assertion to ensure the method was actually invoked during the test;
keep references to expandToLeafUuids, networkModificationService,
modificationsToMove, and the new leaf UUID in your changes.
🪄 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: 57db6a4d-69ee-43b0-ad36-5e3c0b2bfc8f
📒 Files selected for processing (3)
src/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.java
Comment on lines
+2271
to
+2272
| Set<UUID> allMovedUuids = networkModificationService.expandToLeafUuids(networkModificationsResult.modificationUuids()); | ||
| rootNetworkNodeInfoService.moveModificationsToExclude(originNodeUuid, targetNodeUuid, new ArrayList<>(allMovedUuids)); |
There was a problem hiding this comment.
Prevent post-move failure from desynchronizing remote modifications and local exclusion mappings.
Line 2270 executes the remote move first; then Line 2271 performs another remote call and Line 2272 dereferences it directly. If expansion fails (or returns null), this throws after the move already happened remotely, so local exclusion remapping is skipped.
Suggested hardening
NetworkModificationsResult networkModificationsResult = networkModificationService.moveModifications(originGroupUuid, targetGroupUuid, beforeUuid, Pair.of(modificationUuidList, modificationApplicationContexts), isTargetInDifferentNodeTree);
- Set<UUID> allMovedUuids = networkModificationService.expandToLeafUuids(networkModificationsResult.modificationUuids());
- rootNetworkNodeInfoService.moveModificationsToExclude(originNodeUuid, targetNodeUuid, new ArrayList<>(allMovedUuids));
+ List<UUID> movedUuids = networkModificationsResult.modificationUuids();
+ Set<UUID> allMovedUuids;
+ try {
+ allMovedUuids = Optional.ofNullable(networkModificationService.expandToLeafUuids(movedUuids))
+ .orElseGet(() -> new HashSet<>(movedUuids));
+ } catch (Exception e) {
+ LOGGER.warn("Leaf expansion failed after move; falling back to moved UUIDs", e);
+ allMovedUuids = new HashSet<>(movedUuids);
+ }
+ rootNetworkNodeInfoService.moveModificationsToExclude(originNodeUuid, targetNodeUuid, new ArrayList<>(allMovedUuids));🤖 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 2271 - 2272, The code calls
rootNetworkNodeInfoService.moveModificationsToExclude immediately after
performing the remote move but dereferences the expansion result without
checking for failure, which can skip local exclusion remapping if
expandToLeafUuids fails; first call
networkModificationService.expandToLeafUuids(networkModificationsResult.modificationUuids())
and validate its result (handle null or exceptions) before—or as part of—a
single transactional flow so the remote move and local remapping stay
consistent; if expansion fails, fall back to an empty list or retry/log and
still attempt to update local exclusion mappings (or roll back the remote move),
and ensure expandToLeafUuids and moveModificationsToExclude calls are wrapped
with try/catch that logs errors and preserves consistency between remote
modifications and local exclusion state using the originNodeUuid and
targetNodeUuid identifiers.
Comment on lines
+2432
to
+2438
| UUID childrenStubId = wireMockServer.stubFor( | ||
| WireMock.get(WireMock.urlPathEqualTo("/v1/network-composite-modifications/children-uuids")) | ||
| .withQueryParam("uuids", WireMock.containing(modification1.toString())) | ||
| .willReturn(WireMock.ok() | ||
| .withBody(mapper.writeValueAsString(List.of(modification1, modification2))) | ||
| .withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE)) | ||
| ).getId(); |
There was a problem hiding this comment.
Strengthen children-UUIDs assertions for multi-UUID MOVE payloads.
At Line 2434 and Line 2493, the matcher only checks modification1. In this test, MOVE sends [modification1, modification2] (Line 2430), so the current checks can still pass if modification2 is accidentally omitted from the request.
Suggested tightening
- UUID childrenStubId = wireMockServer.stubFor(
+ UUID childrenStubId = wireMockServer.stubFor(
WireMock.get(WireMock.urlPathEqualTo("/v1/network-composite-modifications/children-uuids"))
.withQueryParam("uuids", WireMock.containing(modification1.toString()))
+ .withQueryParam("uuids", WireMock.containing(modification2.toString()))
.willReturn(WireMock.ok()
.withBody(mapper.writeValueAsString(List.of(modification1, modification2)))
.withHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON_VALUE))
).getId();
@@
WireMockUtils.verifyGetRequest(wireMockServer, childrenStubId,
"/v1/network-composite-modifications/children-uuids",
Map.of("uuids", WireMock.containing(modification1.toString())),
2);
+ WireMockUtils.verifyGetRequest(wireMockServer, childrenStubId,
+ "/v1/network-composite-modifications/children-uuids",
+ Map.of("uuids", WireMock.containing(modification2.toString())),
+ 2);Also applies to: 2491-2494
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/java/org/gridsuite/study/server/NetworkModificationTest.java` around
lines 2432 - 2438, The WireMock stub in NetworkModificationTest currently only
verifies that the "uuids" query param contains modification1, which is
insufficient for MOVE payloads that include both modification1 and
modification2; update the withQueryParam matcher on the stubFor call (the
WireMock.get(...).withQueryParam("uuids", ...)) to assert that both
modification1.toString() and modification2.toString() are present (for example
by using a matching/regex or a combined containing matcher that checks for both
UUID substrings) so the test fails if either UUID is omitted.
Meklo
force-pushed
the
composite-modifications-applicability
branch
from
333c618 to
441c0ce
Compare
Meklo
approved these changes
Jun 12, 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.
PR Summary