Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 57 minutes and 59 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a DELETE /studies/{studyUuid}/unmount endpoint that triggers supervised unmount: iterates study root networks, unmounts each root network (unbuilds node tree with block nodes, deletes remote infos without deleting cases), marks networks NOT_INDEXED, and emits notifications. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as SupervisionController
participant Supervision as SupervisionService
participant Study as StudyService
participant RootNetwork as RootNetworkService
participant Notification as NotificationService
Client->>Controller: DELETE /v1/supervision/studies/{studyUuid}/unmount
Controller->>Supervision: unmountStudy(studyUuid)
Supervision->>Study: getStudyRootNetworks(studyUuid)
Study-->>Supervision: List<RootNetwork>
loop per root network
Supervision->>Study: unmountStudyRootNetwork(studyUuid, rootNetworkId)
Study->>Study: doUnbuildNodeTree(..., withBlockNodes=true)
Study->>RootNetwork: deleteRootNetworkRemoteInfos(list, deleteCase=false)
RootNetwork-->>Study: deletion tasks scheduled/completed
Study->>Study: set indexation status -> NOT_INDEXED
end
Supervision->>Notification: emitElementUpdated(studyUuid, "Supervision")
Notification-->>Supervision: ack
Supervision-->>Controller: void
Controller-->>Client: 200 OK
Suggested reviewers
🚥 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: 1
🧹 Nitpick comments (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
3706-3708: Avoid exposing managedRootNetworkEntityinstances from this new public API.This now returns the live JPA collection from
StudyEntity, which makes it easy for callers to mutate attached entities or trip lazy-loading outside a transaction.SupervisionServiceonly needs root-network identifiers here, so a DTO/UUID-based method would be safer.🤖 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 3706 - 3708, getStudyRootNetworks currently returns the live JPA collection StudyEntity.getRootNetworks(), exposing managed RootNetworkEntity instances; change it to return immutable identifiers (e.g. List<UUID>) or a lightweight DTO instead so callers (like SupervisionService) don't receive attached entities. Implement getStudyRootNetworkIds(UUID) that calls getStudy(studyUuid) and maps studyEntity.getRootNetworks() to root.getId() (or fetch IDs via a repository query) and return an unmodifiable List<UUID>, update callers to use the new method, and remove direct exposure of RootNetworkEntity from the public API.
🤖 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/service/StudyService.java`:
- Around line 3923-3932: The call to doUnbuildNodeTree(studyUuid,
rootNetworkUuid, true) uses the node-tree root path (expects a node UUID and
applies ALL_WITH_BLOCK_NODES), which targets the wrong identifier and leaves the
tree blocked; replace it with the single-root invalidation path used elsewhere
(the same routine SupervisionService.unmountStudy uses) so the
root-network-specific invalidation is performed (pass the correct root-network
identifier and use the single-root unblock semantics instead of
ALL_WITH_BLOCK_NODES), ensuring the tree is properly unblocked afterward; update
the call in unmountStudyRootNetwork to invoke that single-root invalidation
method and keep the subsequent deleteRootNetworkRemoteInfos,
updateRootNetworkIndexationStatus, and
notificationService.emitRootNetworksUpdated calls unchanged.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3706-3708: getStudyRootNetworks currently returns the live JPA
collection StudyEntity.getRootNetworks(), exposing managed RootNetworkEntity
instances; change it to return immutable identifiers (e.g. List<UUID>) or a
lightweight DTO instead so callers (like SupervisionService) don't receive
attached entities. Implement getStudyRootNetworkIds(UUID) that calls
getStudy(studyUuid) and maps studyEntity.getRootNetworks() to root.getId() (or
fetch IDs via a repository query) and return an unmodifiable List<UUID>, update
callers to use the new method, and remove direct exposure of RootNetworkEntity
from the public 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: 10afa632-97ee-48df-8bd9-5a85bc890155
📒 Files selected for processing (4)
src/main/java/org/gridsuite/study/server/controller/SupervisionController.javasrc/main/java/org/gridsuite/study/server/service/RootNetworkService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/main/java/org/gridsuite/study/server/service/SupervisionService.java
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
3928-3935:⚠️ Potential issue | 🔴 CriticalUse the single-root invalidation path here, not the study-wide helper.
doUnbuildNodeTree(...)invalidates all root networks in the study. Calling it fromunmountStudyRootNetwork(...)means a multi-root unmount re-runs study-wide invalidation once per root, whilefinallyonly unblocks the current root network. That leaves the other root networks blocked and turns the work into an N×N loop.🐛 Proposed fix
- var rootNode = networkModificationTreeService.getStudyRootNodeUuid(studyUuid); + var rootNode = networkModificationTreeService.getStudyRootNodeUuid(studyUuid); try { - doUnbuildNodeTree(studyUuid, rootNode, true); + invalidateNodeTree(studyUuid, rootNode, rootNetworkUuid, ALL_WITH_BLOCK_NODES, true); rootNetworkService.deleteRootNetworkRemoteInfos(List.of(rootNetwork.toDto()), false); updateRootNetworkIndexationStatus(study, rootNetwork, RootNetworkIndexationStatus.NOT_INDEXED); } finally { networkModificationTreeService.unblockNodeTree(rootNetworkUuid, rootNode); }🤖 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 3928 - 3935, The current call to doUnbuildNodeTree(studyUuid, rootNode, true) from unmountStudyRootNetwork(...) invalidates all roots in the study and causes N×N rework; replace it with the single-root invalidation path so only the current root is invalidated. Either call the existing single-root helper (if one exists) instead of doUnbuildNodeTree, or add a focused method (e.g., doUnbuildNodeTreeForRoot or doUnbuildSingleRootNetwork) that takes the rootNetworkUuid and rootNode and only invalidates that root; then keep the finally block calling networkModificationTreeService.unblockNodeTree(rootNetworkUuid, rootNode) and leave the calls to rootNetworkService.deleteRootNetworkRemoteInfos and updateRootNetworkIndexationStatus as-is. Ensure the new/specified single-root method is used here to avoid study-wide invalidation.
🧹 Nitpick comments (2)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
3706-3708: Avoid exposing JPA entities from this new public API.
SupervisionServiceonly needs to iterate root networks, but this now leaksRootNetworkEntityoutsideStudyService. Returning UUIDs or DTOs would keep lazy-loading and persistence coupling contained.🤖 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 3706 - 3708, getStudyRootNetworks currently returns JPA entities (RootNetworkEntity) which leaks persistence objects; change StudyService.getStudyRootNetworks to return a non-entity representation (e.g., List<UUID> or a lightweight DTO) instead of RootNetworkEntity so SupervisionService only iterates IDs/DTOs. Implement by mapping studyEntity.getRootNetworks() inside the method to rootNetwork.getUuid() or new RootNetworkDto(...) and return that collection; keep the mapping inside StudyService (method getStudyRootNetworks) and preserve any `@Transactional` scope if needed to avoid lazy-loading issues.src/test/java/org/gridsuite/study/server/SupervisionControllerTest.java (1)
307-324: Add a multi-root unmount case here.
initStudy()creates a single root network, so this test still passes ifunmountStudyRootNetwork(...)invalidates the whole study once per root. A two-root setup would catch the per-root invalidation/unblock regression immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/study/server/SupervisionControllerTest.java` around lines 307 - 324, Update the test to cover a multi-root unmount scenario by creating two root networks instead of one and then invoking the unmount endpoint; specifically add a new test (or extend testUnmountStudy) that uses a setup method that creates two roots (e.g., modify or add an initStudyTwoRoots helper or call initStudy then create a second root via the same flow), call unmountStudyRootNetwork/unmount endpoint for the study, and assert per-root behavior: verify networkService.deleteVariants was invoked for each root (or called twice for NETWORK_UUIDs), verify rootNetworkService.deleteRootNetworkRemoteInfos was called for each root with deleteCase = false, and assert the study indexation flips appropriately (use assertIndexationStatus as in testUnmountStudy). Reference symbols: testUnmountStudy, initStudy (or new initStudyTwoRoots), unmountStudyRootNetwork, networkService.deleteVariants, rootNetworkService.deleteRootNetworkRemoteInfos, assertIndexationStatus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3928-3935: The current call to doUnbuildNodeTree(studyUuid,
rootNode, true) from unmountStudyRootNetwork(...) invalidates all roots in the
study and causes N×N rework; replace it with the single-root invalidation path
so only the current root is invalidated. Either call the existing single-root
helper (if one exists) instead of doUnbuildNodeTree, or add a focused method
(e.g., doUnbuildNodeTreeForRoot or doUnbuildSingleRootNetwork) that takes the
rootNetworkUuid and rootNode and only invalidates that root; then keep the
finally block calling
networkModificationTreeService.unblockNodeTree(rootNetworkUuid, rootNode) and
leave the calls to rootNetworkService.deleteRootNetworkRemoteInfos and
updateRootNetworkIndexationStatus as-is. Ensure the new/specified single-root
method is used here to avoid study-wide invalidation.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3706-3708: getStudyRootNetworks currently returns JPA entities
(RootNetworkEntity) which leaks persistence objects; change
StudyService.getStudyRootNetworks to return a non-entity representation (e.g.,
List<UUID> or a lightweight DTO) instead of RootNetworkEntity so
SupervisionService only iterates IDs/DTOs. Implement by mapping
studyEntity.getRootNetworks() inside the method to rootNetwork.getUuid() or new
RootNetworkDto(...) and return that collection; keep the mapping inside
StudyService (method getStudyRootNetworks) and preserve any `@Transactional` scope
if needed to avoid lazy-loading issues.
In `@src/test/java/org/gridsuite/study/server/SupervisionControllerTest.java`:
- Around line 307-324: Update the test to cover a multi-root unmount scenario by
creating two root networks instead of one and then invoking the unmount
endpoint; specifically add a new test (or extend testUnmountStudy) that uses a
setup method that creates two roots (e.g., modify or add an initStudyTwoRoots
helper or call initStudy then create a second root via the same flow), call
unmountStudyRootNetwork/unmount endpoint for the study, and assert per-root
behavior: verify networkService.deleteVariants was invoked for each root (or
called twice for NETWORK_UUIDs), verify
rootNetworkService.deleteRootNetworkRemoteInfos was called for each root with
deleteCase = false, and assert the study indexation flips appropriately (use
assertIndexationStatus as in testUnmountStudy). Reference symbols:
testUnmountStudy, initStudy (or new initStudyTwoRoots), unmountStudyRootNetwork,
networkService.deleteVariants, rootNetworkService.deleteRootNetworkRemoteInfos,
assertIndexationStatus.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5e32adb-c662-4fca-abca-aa1224b0521e
📒 Files selected for processing (2)
src/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/SupervisionControllerTest.java
|
| @DeleteMapping(value = "/studies/{studyUuid}/unmount") | ||
| @Operation(summary = "Invalidate nodes builds and delete root node network") | ||
| @ApiResponses(value = {@ApiResponse(responseCode = "200", description = "study has been unmounted")}) | ||
| public ResponseEntity<Void> unmountStudy(@PathVariable("studyUuid") UUID studyUuid) { |
There was a problem hiding this comment.
Not sure if it's accurate, but in the ticket I see that
There was a problem hiding this comment.
I thought it made more sense semantically to have a dedicated term and endpoint since it's not just an invalidation, wdyt @etiennehomer ?
There was a problem hiding this comment.
I agree. So you do more that what's described in the ticket, right ?
There was a problem hiding this comment.
Before partial variants in the network-store, I would easily say that the unbuild of the root node is the same treatment as for the other nodes.
But now that we have partial variants, actually it's partly different. But not so much. The consequence is the delete of a network and its equipments in the network-store.
To me, the main question is: do we want this action to be open to the front (can it be launch by the users) or we want to keep it as a supervision action ?
For now, I would rather say that it's fine to keep it as a supervision action.
| } | ||
|
|
||
| @DeleteMapping(value = "/studies/{studyUuid}/unmount") | ||
| @Operation(summary = "Invalidate nodes builds and delete root node network") |
There was a problem hiding this comment.
| @Operation(summary = "Invalidate nodes builds and delete root node network") | |
| @Operation(summary = "Invalidate built nodes and delete root node network") |
Also, what do you mean by
delete root node network
? When I look quickly to the code it's not clear to me what it does exactly. Looks like it unbuild (invalidate?) nodes and delete all (or some?) root network data.
study-server/src/main/java/org/gridsuite/study/server/service/StudyService.java
Lines 3941 to 3943 in 9e31118
There was a problem hiding this comment.
Yep it invalidates all built nodes the same way as unbuildAllNodes endpoint plus it deletes all networks associated to root nodes as well as elasticssearch data and report nodes.
I think it covers all that can be temporaly deleted for space sake without actual data loss like modifications, parameters, etc..
There was a problem hiding this comment.
You say it deletes all associated networks. So why don't you call
From what I understand you delete the network data but not the network itself.
There was a problem hiding this comment.
We want to keep root networks entities to be able to rebuild them later, here we just want to delete the underlying data in network store server whereas deleteRootNetworks also deletes root network entity from study server if I'm not mistaken
There was a problem hiding this comment.
So actually, as said above, you don't delete the networks themself, rights ?
I just want to point out that, if I understand well, the endpoint description does not match with what it does exactly.
|
|
||
| updateComputationResultUuid(nodeUuid, rootNetworkUuid, result, SENSITIVITY_ANALYSIS); | ||
| notificationService.emitStudyChanged(study.getId(), nodeUuid, rootNetworkUuid, NotificationService.UPDATE_TYPE_SENSITIVITY_ANALYSIS_STATUS); | ||
| notificationService.emitElementUpdated(study.getId(), userId); |
There was a problem hiding this comment.
Validated by POs ? Do we want to change the actions that do emitElementUpdated() ?
There was a problem hiding this comment.
It is part of the ticket description but maybe I should just make the inventory of actions that do update modification date first to crosscheck with PO which ones we do want to add to the pool
There was a problem hiding this comment.
Discussed with PO: we'll enrich this list in another ticket. So to remove for now
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
3933-3949:⚠️ Potential issue | 🔴 CriticalUnmount still fans out across every root network — previous review concern not fully resolved.
doUnbuildNodeTree(studyUuid, rootNode, true)at Line 3941 iteratesgetStudy(studyUuid).getRootNetworks()and invalidates the tree for every root network withALL_WITH_BLOCK_NODES(see Lines 1899-1908). SinceSupervisionService.unmountStudy(...)calls this method once per root network (context snippet 2), the same study-wide invalidation is repeated N times (O(N²) work) for what should be a single-root operation.Additional side effect:
ALL_WITH_BLOCK_NODESblocks the node tree for all root networks, but thefinallyat Line 3945 only unblocks for the currentrootNetworkUuid. On the first iteration, every other root network's tree is left blocked until its own iteration eventually runs.The prior review suggested switching to a single-root invalidation path (
invalidateNodeTree(studyUuid, rootNodeUuid, rootNetworkUuid, ALL_WITH_BLOCK_NODES, true)) for this method. That still looks like the right fix here —doUnbuildNodeTreeis designed for whole-study unbuild and shouldn't be reused in a per-root-network method.🐛 Suggested fix
`@Transactional` public void unmountStudyRootNetwork(UUID studyUuid, UUID rootNetworkUuid) { StudyEntity study = getStudy(studyUuid); RootNetworkEntity rootNetwork = rootNetworkService.getRootNetwork(rootNetworkUuid) .orElseThrow(() -> new StudyException(NOT_FOUND, "Root network not found")); var rootNode = networkModificationTreeService.getStudyRootNodeUuid(studyUuid); try { - doUnbuildNodeTree(studyUuid, rootNode, true); + invalidateNodeTree(studyUuid, rootNode, rootNetworkUuid, ALL_WITH_BLOCK_NODES, true); rootNetworkService.deleteRootNetworkRemoteInfos(List.of(rootNetwork.toDto()), false); updateRootNetworkIndexationStatus(study, rootNetwork, RootNetworkIndexationStatus.NOT_INDEXED); } finally { networkModificationTreeService.unblockNodeTree(rootNetworkUuid, rootNode); } notificationService.emitRootNetworksUpdated(studyUuid); }🤖 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 3933 - 3949, The unmountStudyRootNetwork method currently calls doUnbuildNodeTree(studyUuid, rootNode, true) which performs a study-wide invalidation across all root networks; replace that call with a single-root invalidation call to networkModificationTreeService.invalidateNodeTree(studyUuid, rootNode, rootNetworkUuid, NodeTreeInvalidationMode.ALL_WITH_BLOCK_NODES, true) (or the equivalent method on networkModificationTreeService used for single-root invalidation) so only the targeted root network is invalidated and blocked; keep the finally block calling networkModificationTreeService.unblockNodeTree(rootNetworkUuid, rootNode) and update any imports/enum references (e.g., NodeTreeInvalidationMode/ALL_WITH_BLOCK_NODES) and remove the doUnbuildNodeTree usage from this method to avoid O(N²) work and blocked trees across other root networks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3933-3949: The unmountStudyRootNetwork method currently calls
doUnbuildNodeTree(studyUuid, rootNode, true) which performs a study-wide
invalidation across all root networks; replace that call with a single-root
invalidation call to
networkModificationTreeService.invalidateNodeTree(studyUuid, rootNode,
rootNetworkUuid, NodeTreeInvalidationMode.ALL_WITH_BLOCK_NODES, true) (or the
equivalent method on networkModificationTreeService used for single-root
invalidation) so only the targeted root network is invalidated and blocked; keep
the finally block calling
networkModificationTreeService.unblockNodeTree(rootNetworkUuid, rootNode) and
update any imports/enum references (e.g.,
NodeTreeInvalidationMode/ALL_WITH_BLOCK_NODES) and remove the doUnbuildNodeTree
usage from this method to avoid O(N²) work and blocked trees across other root
networks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c862ea9f-061e-4164-83be-5555f8133e11
📒 Files selected for processing (3)
src/main/java/org/gridsuite/study/server/controller/SupervisionController.javasrc/main/java/org/gridsuite/study/server/service/RootNetworkService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/gridsuite/study/server/controller/SupervisionController.java
- src/main/java/org/gridsuite/study/server/service/RootNetworkService.java
| RootNetworkEntity rootNetwork = rootNetworkService.getRootNetwork(rootNetworkUuid) | ||
| .orElseThrow(() -> new StudyException(NOT_FOUND, "Root network not found")); | ||
|
|
||
| var rootNode = networkModificationTreeService.getStudyRootNodeUuid(studyUuid); |
There was a problem hiding this comment.
| var rootNode = networkModificationTreeService.getStudyRootNodeUuid(studyUuid); | |
| var rootNodeUuid = networkModificationTreeService.getStudyRootNodeUuid(studyUuid); |
| Mockito.verify(rootNetworkService, Mockito.times(1)) | ||
| .deleteRootNetworkRemoteInfos(any(), eq(false)); | ||
|
|
||
| // Indexation flipped to NOT_INDEXED so the auto-detect path will reimport on reopen |
There was a problem hiding this comment.
check ES data is empty:
assertIndexationCount(0, 0);
|
I've added a blocking mode for remote data deletion so that the load gets leaned and when can better mesure the job progress |
3 participants
PR Summary