Unify move endpoints#1000
Open
Meklo wants to merge 10 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 37 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request refactors the network modification movement flow to use container (modification-group) identifiers instead of node UUIDs and before-ordering semantics. The controller endpoint gains optional fallback resolution logic, StudyService is refactored to derive origin nodes and coordinate notifications, and service layer parameters are renamed for consistency. ChangesContainer-based modification movement
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: 6
🤖 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/controller/StudyController.java`:
- Around line 628-653: Add backward-compatible alias mappings that route the
removed v1 endpoints to the existing moveModification handler: keep the new
method moveModification and add additional `@PutMapping` entries (or new methods
that delegate to moveModification) for the old paths
/network-modification/{modificationUuid} and
/composite-sub-modification/{modificationUuid} so existing clients won’t 404;
ensure the aliases accept the same path variables, request params
(sourceContainerId, targetContainerId, beforeUuid) and HEADER_USER_ID and then
call the same logic (delegate to rebuildNodeService.moveNetworkModification via
moveModification or a shared private helper) so behavior remains identical until
callers migrate.
In
`@src/main/java/org/gridsuite/study/server/service/NetworkModificationTreeService.java`:
- Around line 1298-1302: The method getNodeUuidByModificationGroup currently
returns null when no node is found; change it to fail-fast by throwing a
StudyException with NOT_FOUND instead of returning null: in
getNodeUuidByModificationGroup call
networkModificationNodeInfoRepository.findByModificationGroupUuidIn(List.of(groupUuid))
as before, but if the result is empty throw new
StudyException(StudyException.ErrorCode.NOT_FOUND, "Modification group node not
found: " + groupUuid) (or similar message), otherwise return
node.getFirst().getIdNode(); update imports if needed and keep the method
signature returning UUID (do not return null) so downstream code always receives
a non-null UUID or an exception.
In `@src/main/java/org/gridsuite/study/server/service/RebuildNodeService.java`:
- Around line 91-105: The code calls handleRebuildNode with a lambda that
invokes studyService.moveNetworkModifications passing a hardcoded false for the
cross-tree flag, which can skip origin-tree invalidation/unblock and
notifications; update the single-move flow to compute the cross-tree flag the
same way as the multi-move path (reuse the same logic used there to determine
isTargetInDifferentNodeTree) and pass that computed boolean into
studyService.moveNetworkModifications, ensuring
invalidateNodeTreeWhenMoveModification and unblockNodeTree are still executed in
the same try/finally so origin-tree handling and notifications remain correct;
refer to handleRebuildNode, studyService.moveNetworkModifications,
invalidateNodeTreeWhenMoveModification and unblockNodeTree for where to apply
the change.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 2258-2270: The NetworkModificationsResult returned by
networkModificationService.moveModifications must be null-checked before calling
result.modificationUuids() or result.modificationResults(); update the code
around the call to networkModificationService.moveModifications (which assigns
to variable result) to guard against a null or empty result and only call
rootNetworkNodeInfoService.moveModificationsToExclude(originNodeUuid,
targetNodeUuid, result.modificationUuids()) and
emitNetworkModificationImpactsForAllRootNetworks(result.modificationResults(),
studyEntity, targetNodeUuid) when result is non-null (and contains the expected
collections), otherwise handle the empty response path (e.g., skip those calls
or log/throw as appropriate).
- Around line 2240-2256: The code resolves sourceContainerId to originNodeUuid
via networkModificationTreeService.getNodeUuidByModificationGroup but never
validates that originNodeUuid belongs to the same study, allowing cross-study
moves; after obtaining originNodeUuid (and before using it or emitting
notifications), call the existing checkStudyContainsNode(studyUuid,
originNodeUuid) (or an equivalent containment/authorization check) to enforce
study-boundary validation for sourceContainerId and fail early if it does not
belong to the study.
In
`@src/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.java`:
- Line 787: The test currently calls
studyService.moveNetworkModifications(studyUuid, firstNodeUuid, firstNodeUuid,
...) using firstNodeUuid for both node parameters, which prevents exercising a
cross-node move; change the second occurrence of firstNodeUuid to secondNodeUuid
so the call becomes studyService.moveNetworkModifications(studyUuid,
firstNodeUuid, secondNodeUuid, modificationsToMove, false, USER_ID) to properly
test the failure-path for moving excluded modifications between nodes.
🪄 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: 13e8ef7c-c7f9-489c-a866-f1b641a20b4d
📒 Files selected for processing (8)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationService.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationTreeService.javasrc/main/java/org/gridsuite/study/server/service/RebuildNodeService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/RebuildNodeServiceTest.javasrc/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyControllerRebuildNodeTest.java
💤 Files with no reviewable changes (2)
- src/test/java/org/gridsuite/study/server/studycontroller/StudyControllerRebuildNodeTest.java
- src/test/java/org/gridsuite/study/server/RebuildNodeServiceTest.java
Comment on lines
+628
to
653
| @PutMapping(value = "/studies/{studyUuid}/nodes/{nodeUuid}/modification/{modificationUuid}") | ||
| @Operation(summary = "Move a modification within or between containers (groups or composites)") | ||
| @ApiResponses(value = {@ApiResponse(responseCode = "200", description = "The modification order has been updated")}) | ||
| public ResponseEntity<Void> moveModification( | ||
| @PathVariable("studyUuid") UUID studyUuid, | ||
| @PathVariable("nodeUuid") UUID nodeUuid, | ||
| @PathVariable("modificationUuid") UUID modificationUuid, | ||
| @Nullable @Parameter(description = "Source composite UUID; absent when moving from root level") @RequestParam(value = "sourceCompositeUuid", required = false) UUID sourceCompositeUuid, | ||
| @Nullable @Parameter(description = "Target composite UUID; absent when moving to root level") @RequestParam(value = "targetCompositeUuid", required = false) UUID targetCompositeUuid, | ||
| @Nullable @Parameter(description = "Insert before this UUID; absent means append at end") @RequestParam(value = "beforeUuid", required = false) UUID beforeUuid, | ||
| @Parameter(description = "source container UUID; for GROUP source the node's group is used when absent") @RequestParam(value = "sourceContainerId", required = false) UUID sourceContainerId, | ||
| @Parameter(description = "target container UUID; for GROUP target the node's group is used when absent") @RequestParam(value = "targetContainerId", required = false) UUID targetContainerId, | ||
| @Parameter(description = "insert before this modification (empty = at end)") @RequestParam(value = "beforeUuid", required = false) UUID beforeUuid, | ||
| @RequestHeader(HEADER_USER_ID) String userId) { | ||
| studyService.assertCanUpdateNodeInStudy(studyUuid, nodeUuid); | ||
| studyService.assertNoBlockedNodeInStudy(studyUuid, nodeUuid); | ||
| rebuildNodeService.moveSubModification( | ||
| studyUuid, nodeUuid, | ||
| sourceCompositeUuid, targetCompositeUuid, | ||
| // For GROUP containers without an explicit id, fall back to the node's group. | ||
| UUID resolvedSourceId = sourceContainerId == null | ||
| ? networkModificationTreeService.getModificationGroupUuid(nodeUuid) : sourceContainerId; | ||
| UUID resolvedTargetId = targetContainerId == null | ||
| ? networkModificationTreeService.getModificationGroupUuid(nodeUuid) : targetContainerId; | ||
| rebuildNodeService.moveNetworkModification( | ||
| studyUuid, | ||
| nodeUuid, | ||
| resolvedTargetId, | ||
| resolvedSourceId, | ||
| modificationUuid, beforeUuid, userId); | ||
| return ResponseEntity.ok().build(); | ||
| } |
There was a problem hiding this comment.
Keep backward-compatible aliases for the removed v1 move routes.
This replaces the existing move endpoints with a new path in the same API version. Any client still calling /network-modification/{modificationUuid} or /composite-sub-modification/{modificationUuid} will start getting 404s immediately, even though the underlying operation is the same. Please keep deprecated adapter mappings until callers migrate, or move this contract break behind a new API version.
🤖 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/controller/StudyController.java`
around lines 628 - 653, Add backward-compatible alias mappings that route the
removed v1 endpoints to the existing moveModification handler: keep the new
method moveModification and add additional `@PutMapping` entries (or new methods
that delegate to moveModification) for the old paths
/network-modification/{modificationUuid} and
/composite-sub-modification/{modificationUuid} so existing clients won’t 404;
ensure the aliases accept the same path variables, request params
(sourceContainerId, targetContainerId, beforeUuid) and HEADER_USER_ID and then
call the same logic (delegate to rebuildNodeService.moveNetworkModification via
moveModification or a shared private helper) so behavior remains identical until
callers migrate.
Comment on lines
+1298
to
+1302
| @Transactional(readOnly = true) | ||
| public UUID getNodeUuidByModificationGroup(UUID groupUuid) { | ||
| var node = networkModificationNodeInfoRepository.findByModificationGroupUuidIn(List.of(groupUuid)); | ||
| return node.isEmpty() ? null : node.getFirst().getIdNode(); | ||
| } |
There was a problem hiding this comment.
Avoid nullable return for container→node lookup.
Returning null here makes the move flow fragile (downstream code dereferences the resolved origin node). Prefer throwing StudyException(NOT_FOUND, ...) or returning Optional<UUID> and handling it explicitly.
Suggested fix
`@Transactional`(readOnly = true)
public UUID getNodeUuidByModificationGroup(UUID groupUuid) {
- var node = networkModificationNodeInfoRepository.findByModificationGroupUuidIn(List.of(groupUuid));
- return node.isEmpty() ? null : node.getFirst().getIdNode();
+ return networkModificationNodeInfoRepository.findByModificationGroupUuidIn(List.of(groupUuid)).stream()
+ .findFirst()
+ .map(NetworkModificationNodeInfoEntity::getIdNode)
+ .orElseThrow(() -> new StudyException(NOT_FOUND, "Node not found"));
}🤖 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/NetworkModificationTreeService.java`
around lines 1298 - 1302, The method getNodeUuidByModificationGroup currently
returns null when no node is found; change it to fail-fast by throwing a
StudyException with NOT_FOUND instead of returning null: in
getNodeUuidByModificationGroup call
networkModificationNodeInfoRepository.findByModificationGroupUuidIn(List.of(groupUuid))
as before, but if the result is empty throw new
StudyException(StudyException.ErrorCode.NOT_FOUND, "Modification group node not
found: " + groupUuid) (or similar message), otherwise return
node.getFirst().getIdNode(); update imports if needed and keep the method
signature returning UUID (do not return null) so downstream code always receives
a non-null UUID or an exception.
Comment on lines
+91
to
+105
| handleRebuildNode(studyUuid, nodeUuid, userId, | ||
| () -> { | ||
| studyService.invalidateNodeTreeWhenMoveModification(studyUuid, nodeUuid); | ||
| try { | ||
| studyService.moveNetworkModifications( | ||
| studyUuid, | ||
| nodeUuid, | ||
| targetContainerId, | ||
| sourceContainerId, | ||
| List.of(modificationUuid), beforeUuid, | ||
| false, userId); | ||
| } finally { | ||
| studyService.unblockNodeTree(studyUuid, nodeUuid); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Do not hardcode cross-tree flag in single-move flow.
isTargetInDifferentNodeTree is forced to false here, so cross-container moves can miss origin-tree invalidation/unblock and related notifications. This should be computed the same way as the multi-move path.
Suggested direction
- studyService.moveNetworkModifications(
+ UUID originNodeUuid = networkModificationTreeService.getNodeUuidByModificationGroup(sourceContainerId);
+ boolean isTargetInDifferentNodeTree = studyService.invalidateNodeTreeWhenMoveModifications(studyUuid, nodeUuid, originNodeUuid);
+ studyService.moveNetworkModifications(
studyUuid,
nodeUuid,
targetContainerId,
sourceContainerId,
List.of(modificationUuid), beforeUuid,
- false, userId);
+ isTargetInDifferentNodeTree, userId);🤖 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/RebuildNodeService.java`
around lines 91 - 105, The code calls handleRebuildNode with a lambda that
invokes studyService.moveNetworkModifications passing a hardcoded false for the
cross-tree flag, which can skip origin-tree invalidation/unblock and
notifications; update the single-move flow to compute the cross-tree flag the
same way as the multi-move path (reuse the same logic used there to determine
isTargetInDifferentNodeTree) and pass that computed boolean into
studyService.moveNetworkModifications, ensuring
invalidateNodeTreeWhenMoveModification and unblockNodeTree are still executed in
the same try/finally so origin-tree handling and notifications remain correct;
refer to handleRebuildNode, studyService.moveNetworkModifications,
invalidateNodeTreeWhenMoveModification and unblockNodeTree for where to apply
the change.
Comment on lines
+2240
to
+2256
| UUID originNodeUuid = networkModificationTreeService.getNodeUuidByModificationGroup(sourceContainerId); | ||
|
|
||
| List<UUID> originNodeChildrenUuids = List.of(); | ||
| List<UUID> childrenUuids = networkModificationTreeService.getChildrenUuids(targetNodeUuid); | ||
| List<UUID> originNodeChildrenUuids = new ArrayList<>(); | ||
| notificationService.emitStartModificationEquipmentNotification(studyUuid, targetNodeUuid, childrenUuids, NotificationService.MODIFICATIONS_UPDATING_IN_PROGRESS); | ||
| if (isTargetDifferentNode) { | ||
| if (isTargetInDifferentNodeTree) { | ||
| originNodeChildrenUuids = networkModificationTreeService.getChildrenUuids(originNodeUuid); | ||
| notificationService.emitStartModificationEquipmentNotification(studyUuid, originNodeUuid, originNodeChildrenUuids, NotificationService.MODIFICATIONS_UPDATING_IN_PROGRESS); | ||
| } | ||
| try { | ||
| checkStudyContainsNode(studyUuid, targetNodeUuid); | ||
|
|
||
| try { | ||
| List<ModificationApplicationContext> applicationContexts = List.of(); | ||
| StudyEntity studyEntity = getStudy(studyUuid); | ||
| List<RootNetworkEntity> studyRootNetworkEntities = studyEntity.getRootNetworks(); | ||
| UUID originGroupUuid = networkModificationTreeService.getModificationGroupUuid(originNodeUuid); | ||
| UUID targetGroupUuid = networkModificationTreeService.getModificationGroupUuid(targetNodeUuid); | ||
|
|
||
| List<ModificationApplicationContext> modificationApplicationContexts = studyRootNetworkEntities.stream() | ||
| .map(rootNetworkEntity -> rootNetworkNodeInfoService.getNetworkModificationApplicationContext(rootNetworkEntity.getId(), targetNodeUuid, rootNetworkEntity.getNetworkUuid())) | ||
| .toList(); | ||
|
|
||
| NetworkModificationsResult networkModificationsResult = networkModificationService.moveModifications(originGroupUuid, targetGroupUuid, beforeUuid, Pair.of(modificationUuidList, modificationApplicationContexts), isTargetInDifferentNodeTree); | ||
| rootNetworkNodeInfoService.moveModificationsToExclude(originNodeUuid, targetNodeUuid, networkModificationsResult.modificationUuids()); | ||
|
|
||
| // Target node | ||
| checkStudyContainsNode(studyUuid, targetNodeUuid); | ||
| applicationContexts = studyEntity.getRootNetworks().stream() | ||
| .map(rn -> rootNetworkNodeInfoService.getNetworkModificationApplicationContext(rn.getId(), targetNodeUuid, rn.getNetworkUuid())) | ||
| .toList(); |
There was a problem hiding this comment.
Validate source container belongs to the same study before moving.
Only targetNodeUuid is validated (Line 2253). sourceContainerId is resolved to originNodeUuid and then used without study-boundary validation. This is an authorization/data-integrity gap for cross-study container IDs.
Suggested fix
UUID originNodeUuid = networkModificationTreeService.getNodeUuidByModificationGroup(sourceContainerId);
+if (originNodeUuid == null) {
+ throw new StudyException(NOT_FOUND, "Source container not found");
+}
+checkStudyContainsNode(studyUuid, originNodeUuid);
List<UUID> originNodeChildrenUuids = List.of();
List<UUID> childrenUuids = networkModificationTreeService.getChildrenUuids(targetNodeUuid);🤖 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 2240 - 2256, The code resolves sourceContainerId to originNodeUuid via
networkModificationTreeService.getNodeUuidByModificationGroup but never
validates that originNodeUuid belongs to the same study, allowing cross-study
moves; after obtaining originNodeUuid (and before using it or emitting
notifications), call the existing checkStudyContainsNode(studyUuid,
originNodeUuid) (or an equivalent containment/authorization check) to enforce
study-boundary validation for sourceContainerId and fail early if it does not
belong to the study.
Comment on lines
+2258
to
2270
| NetworkModificationsResult result = networkModificationService.moveModifications( | ||
| sourceContainerId, | ||
| targetContainerId, | ||
| beforeUuid, | ||
| Pair.of(modificationUuidList, applicationContexts), | ||
| isTargetInDifferentNodeTree); | ||
|
|
||
| if (originNodeUuid != null) { | ||
| rootNetworkNodeInfoService.moveModificationsToExclude(originNodeUuid, targetNodeUuid, result.modificationUuids()); | ||
| } | ||
| if (isTargetInDifferentNodeTree) { | ||
| emitNetworkModificationImpactsForAllRootNetworks(networkModificationsResult.modificationResults(), studyEntity, targetNodeUuid); | ||
| emitNetworkModificationImpactsForAllRootNetworks(result.modificationResults(), studyEntity, targetNodeUuid); | ||
| } |
There was a problem hiding this comment.
Guard result before dereferencing.
result.modificationUuids() and result.modificationResults() are used unconditionally. If the remote call returns an empty body, this fails with NPE.
Suggested fix
NetworkModificationsResult result = networkModificationService.moveModifications(
sourceContainerId,
targetContainerId,
beforeUuid,
Pair.of(modificationUuidList, applicationContexts),
isTargetInDifferentNodeTree);
-if (originNodeUuid != null) {
+if (result != null && originNodeUuid != null) {
rootNetworkNodeInfoService.moveModificationsToExclude(originNodeUuid, targetNodeUuid, result.modificationUuids());
}
-if (isTargetInDifferentNodeTree) {
+if (result != null && isTargetInDifferentNodeTree) {
emitNetworkModificationImpactsForAllRootNetworks(result.modificationResults(), studyEntity, targetNodeUuid);
}📝 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
| NetworkModificationsResult result = networkModificationService.moveModifications( | |
| sourceContainerId, | |
| targetContainerId, | |
| beforeUuid, | |
| Pair.of(modificationUuidList, applicationContexts), | |
| isTargetInDifferentNodeTree); | |
| if (originNodeUuid != null) { | |
| rootNetworkNodeInfoService.moveModificationsToExclude(originNodeUuid, targetNodeUuid, result.modificationUuids()); | |
| } | |
| if (isTargetInDifferentNodeTree) { | |
| emitNetworkModificationImpactsForAllRootNetworks(networkModificationsResult.modificationResults(), studyEntity, targetNodeUuid); | |
| emitNetworkModificationImpactsForAllRootNetworks(result.modificationResults(), studyEntity, targetNodeUuid); | |
| } | |
| NetworkModificationsResult result = networkModificationService.moveModifications( | |
| sourceContainerId, | |
| targetContainerId, | |
| beforeUuid, | |
| Pair.of(modificationUuidList, applicationContexts), | |
| isTargetInDifferentNodeTree); | |
| if (result != null && originNodeUuid != null) { | |
| rootNetworkNodeInfoService.moveModificationsToExclude(originNodeUuid, targetNodeUuid, result.modificationUuids()); | |
| } | |
| if (result != null && isTargetInDifferentNodeTree) { | |
| emitNetworkModificationImpactsForAllRootNetworks(result.modificationResults(), studyEntity, targetNodeUuid); | |
| } |
🤖 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 2258 - 2270, The NetworkModificationsResult returned by
networkModificationService.moveModifications must be null-checked before calling
result.modificationUuids() or result.modificationResults(); update the code
around the call to networkModificationService.moveModifications (which assigns
to variable result) to guard against a null or empty result and only call
rootNetworkNodeInfoService.moveModificationsToExclude(originNodeUuid,
targetNodeUuid, result.modificationUuids()) and
emitNetworkModificationImpactsForAllRootNetworks(result.modificationResults(),
studyEntity, targetNodeUuid) when result is non-null (and contains the expected
collections), otherwise handle the empty response path (e.g., skip those calls
or log/throw as appropriate).
| Mockito.doThrow(new RuntimeException()).when(networkModificationService).moveModifications(any(), any(), any(), any(), eq(true)); | ||
| List<UUID> modificationsToMove = List.of(MODIFICATIONS_TO_EXCLUDE_RN_1.stream().findFirst().orElseThrow()); | ||
| assertThrows(RuntimeException.class, () -> studyService.moveNetworkModifications(studyUuid, firstNodeUuid, firstNodeUuid, modificationsToMove, null, false, USER_ID)); | ||
| assertThrows(RuntimeException.class, () -> studyService.moveNetworkModifications(studyUuid, firstNodeUuid, firstNodeUuid, modificationsToMove, false, USER_ID)); |
There was a problem hiding this comment.
Use secondNodeUuid in the failure-path move call.
Line 787 passes firstNodeUuid as both target and source, so this branch no longer exercises the cross-node move that the assertions below describe. That weakens the test coverage for excluded-modification transfer failures.
Suggested fix
- assertThrows(RuntimeException.class, () -> studyService.moveNetworkModifications(studyUuid, firstNodeUuid, firstNodeUuid, modificationsToMove, false, USER_ID));
+ assertThrows(RuntimeException.class, () -> studyService.moveNetworkModifications(studyUuid, secondNodeUuid, firstNodeUuid, modificationsToMove, false, USER_ID));📝 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
| assertThrows(RuntimeException.class, () -> studyService.moveNetworkModifications(studyUuid, firstNodeUuid, firstNodeUuid, modificationsToMove, false, USER_ID)); | |
| assertThrows(RuntimeException.class, () -> studyService.moveNetworkModifications(studyUuid, secondNodeUuid, firstNodeUuid, modificationsToMove, false, USER_ID)); |
🤖 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`
at line 787, The test currently calls
studyService.moveNetworkModifications(studyUuid, firstNodeUuid, firstNodeUuid,
...) using firstNodeUuid for both node parameters, which prevents exercising a
cross-node move; change the second occurrence of firstNodeUuid to secondNodeUuid
so the call becomes studyService.moveNetworkModifications(studyUuid,
firstNodeUuid, secondNodeUuid, modificationsToMove, false, USER_ID) to properly
test the failure-path for moving excluded modifications between nodes.
|
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
1 participant
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