Unify move endpoints#1000
Open
Meklo wants to merge 22 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 58 minutes and 22 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 (2)
📝 WalkthroughWalkthroughController move endpoint now accepts optional source/target container IDs (with fallback); RebuildNodeService and StudyService propagate container IDs to NetworkModificationService.moveModifications; tests updated to exercise explicit and fallback move scenarios. ChangesContainer-based modification movement
Sequence DiagramsequenceDiagram
participant Client
participant StudyController
participant NetworkModificationTreeService
participant RebuildNodeService
participant StudyService
participant NetworkModificationService
participant ModificationServer
Client->>StudyController: PUT /studies/{studyUuid}/nodes/{nodeUuid}/network-modification/{modificationUuid}?sourceContainerId&targetContainerId&beforeUuid
StudyController->>NetworkModificationTreeService: getModificationGroupUuid(nodeUuid) (when missing)
StudyController->>RebuildNodeService: moveNetworkModification(targetContainerId, sourceContainerId, modificationUuid, beforeUuid, userId)
RebuildNodeService->>StudyService: moveNetworkModifications(studyUuid, targetNodeUuid, targetContainerId, sourceContainerId, [modUuid], beforeUuid, isDifferentTree, userId)
StudyService->>NetworkModificationService: moveModifications(sourceContainerId, targetContainerId, beforeUuid, body, buildTargetNode)
NetworkModificationService->>ModificationServer: PUT /v1/groups/{targetContainerId}?action=MOVE&originGroupUuid={sourceContainerId}&before={before}
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/java/org/gridsuite/study/server/NetworkModificationTest.java`:
- Around line 3188-3250: The three MOVE cases (moveUrlCase1, moveUrlNodeGroup,
moveUrlCase3) currently only assert URL and query params; add assertions that
the forwarded PUT body includes the moved node UUID and the
ModificationApplicationContext payload (the same shape expected by
testReorderModification). Update the three
WireMockUtilsCriteria.verifyPutRequest(...) calls to include a body matcher
(instead of the current null) that verifies the JSON contains the moved UUID
(nodeUuid) in the UUIDs/uuids array and contains the
ModificationApplicationContext field/key expected by the downstream API.
🪄 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: 898fbb3f-cdfa-4a7c-81fd-968c01a87a6d
📒 Files selected for processing (3)
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.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/gridsuite/study/server/controller/StudyController.java
- src/main/java/org/gridsuite/study/server/service/StudyService.java
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.java (1)
785-787:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
secondNodeUuidin the failure-path move call.This branch still passes
firstNodeUuidastargetNodeUuidwhiletargetContainerIdpoints tosecondNode.StudyService.moveNetworkModifications(...)builds application contexts and notifications fromtargetNodeUuid, so this no longer exercises the cross-node failure path the test is describing.Suggested fix
- assertThrows(RuntimeException.class, () -> studyService.moveNetworkModifications(studyUuid, firstNodeUuid, secondNode.getModificationGroupUuid(), firstNode.getModificationGroupUuid(), modificationsToMove, null, false, USER_ID)); + assertThrows(RuntimeException.class, () -> studyService.moveNetworkModifications(studyUuid, secondNodeUuid, secondNode.getModificationGroupUuid(), firstNode.getModificationGroupUuid(), modificationsToMove, null, 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` around lines 785 - 787, The test calls studyService.moveNetworkModifications(...) but mistakenly passes firstNodeUuid as the targetNodeUuid while targetContainerId/target modification group is from secondNode, so change the failure-path invocation to use secondNodeUuid as the targetNodeUuid; update the assertThrows call to pass secondNodeUuid (instead of firstNodeUuid) so the call to networkModificationService.moveModifications(...) exercised by Mockito.doThrow(new RuntimeException()) correctly represents the cross-node failure path (refer to Mockito.doThrow(...).when(networkModificationService).moveModifications(...) and the studyService.moveNetworkModifications(...) invocation and the variables firstNodeUuid, secondNodeUuid, secondNode.getModificationGroupUuid()).src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
2250-2262:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate node-backed source containers against the study.
Line 2250 resolves
sourceContainerIdintooriginNodeUuid, but Line 2262 only checkstargetNodeUuid. The composite-null case is fine, but whenoriginNodeUuidis non-null this still lets a caller pass a modification-group UUID from another study and have it forwarded tonetworkModificationService.moveModifications(...).Suggested fix
try { StudyEntity studyEntity = getStudy(studyUuid); checkStudyContainsNode(studyUuid, targetNodeUuid); + if (originNodeUuid != null) { + checkStudyContainsNode(studyUuid, originNodeUuid); + } List<ModificationApplicationContext> applicationContexts = studyEntity.getRootNetworks().stream() .map(rn -> rootNetworkNodeInfoService.getNetworkModificationApplicationContext(rn.getId(), targetNodeUuid, rn.getNetworkUuid())) .toList();🤖 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 2250 - 2262, The code resolves sourceContainerId into originNodeUuid but never validates that originNodeUuid belongs to the same study; add a check using the existing checkStudyContainsNode(studyUuid, originNodeUuid) for non-null originNodeUuid (i.e., when the source is node-backed) after you obtain studyEntity (or before calling networkModificationService.moveModifications(...)) so a modification-group UUID from another study cannot be forwarded; reference the variables originNodeUuid, sourceContainerId and the method checkStudyContainsNode to locate where to insert this validation.
🤖 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.
Duplicate comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 2250-2262: The code resolves sourceContainerId into originNodeUuid
but never validates that originNodeUuid belongs to the same study; add a check
using the existing checkStudyContainsNode(studyUuid, originNodeUuid) for
non-null originNodeUuid (i.e., when the source is node-backed) after you obtain
studyEntity (or before calling
networkModificationService.moveModifications(...)) so a modification-group UUID
from another study cannot be forwarded; reference the variables originNodeUuid,
sourceContainerId and the method checkStudyContainsNode to locate where to
insert this validation.
In
`@src/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.java`:
- Around line 785-787: The test calls studyService.moveNetworkModifications(...)
but mistakenly passes firstNodeUuid as the targetNodeUuid while
targetContainerId/target modification group is from secondNode, so change the
failure-path invocation to use secondNodeUuid as the targetNodeUuid; update the
assertThrows call to pass secondNodeUuid (instead of firstNodeUuid) so the call
to networkModificationService.moveModifications(...) exercised by
Mockito.doThrow(new RuntimeException()) correctly represents the cross-node
failure path (refer to
Mockito.doThrow(...).when(networkModificationService).moveModifications(...) and
the studyService.moveNetworkModifications(...) invocation and the variables
firstNodeUuid, secondNodeUuid, secondNode.getModificationGroupUuid()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e274c2f2-e31b-40dd-b60f-a100ce2780b9
📒 Files selected for processing (4)
src/main/java/org/gridsuite/study/server/service/RebuildNodeService.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.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/gridsuite/study/server/NetworkModificationTest.java
|
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