Add invalidate study endpoint#978
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds a study invalidation feature accessible via a new REST endpoint. The workflow refactors StudyService to expose root network retrieval and add per-root-network invalidation logic, introduces a new RootNetworkService method to handle async remote resource deletions, wires everything through SupervisionService, exposes the operation through a DELETE controller endpoint, and validates the flow with an integration test. ChangesStudy Invalidation Workflow
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: 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
3915-3925:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid using the study-wide unbuild helper inside this per-root unmount path.
In the supervision flow this method is invoked once per root network, but
doUnbuildNodeTree(...)still invalidates all study roots with block nodes on every call. Thefinallyblock only unblocks the currentrootNetworkUuid, so a failure mid-loop can leave the other roots blocked.🤖 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 3915 - 3925, doUnbuildNodeTree(...) currently operates on the whole study and will block/unblock all roots per call, so calling it inside this per-root unmount loop can leave other roots blocked if an error occurs; change the call to only unbuild the subtree for the current root network (e.g., introduce/use a per-root helper like doUnbuildNodeTreeForRoot(studyUuid, rootNodeUuid, true) or add a root-specific parameter to doUnbuildNodeTree to limit its scope), and ensure the finally block unblocks exactly the same scope (or, alternatively, call networkModificationTreeService.unblockAllStudyNodeTrees(studyUuid) if you intentionally must clear every block) so other roots are not left blocked; update calls to networkModificationTreeService.unblockNodeTree/unblockAll... consistently with the new per-root behavior.
🤖 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/elasticsearch/EquipmentInfosService.java`:
- Around line 131-135: The deleteVariants method was turned into a no-op and
must be restored to remove stale documents: re-enable logic in
deleteVariants(UUID networkUuid, List<String> variantIds) to iterate over each
variantId and call
equipmentInfosRepository.deleteAllByNetworkUuidAndVariantId(networkUuid,
variantId) and
tombstonedEquipmentInfosRepository.deleteAllByNetworkUuidAndVariantId(networkUuid,
variantId); also guard against null/empty variantIds and consider annotating or
wrapping the method in a transaction if deletions should be atomic.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 1894-1902: doUnbuildNodeTree currently only waits for the wrapper
tasks submitted via studyServerExecutionService.runAsync but not for the inner
non-blocking invalidateNodeTree calls, so deleteInvalidationInfos may still be
running; fix by ensuring you collect and wait on the actual CompletableFuture
returned by invalidateNodeTree(UUID,UUID,Long,InvalidateNodeTreeParameters)
instead of only the runAsync wrapper—update the mapping to submit a task that
returns the inner future and then flatten (e.g., use runAsync that returns
CompletableFuture and thenCompose/flatMap or have the lambda call
invalidateNodeTree(...).join() inside the runAsync so the wrapper completes
after invalidateNodeTree completes), then call CompletableFuture.allOf(...) on
those inner futures and join to guarantee cleanup (references:
doUnbuildNodeTree, invalidateNodeTree, deleteInvalidationInfos,
studyServerExecutionService.runAsync).
---
Duplicate comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3915-3925: doUnbuildNodeTree(...) currently operates on the whole
study and will block/unblock all roots per call, so calling it inside this
per-root unmount loop can leave other roots blocked if an error occurs; change
the call to only unbuild the subtree for the current root network (e.g.,
introduce/use a per-root helper like doUnbuildNodeTreeForRoot(studyUuid,
rootNodeUuid, true) or add a root-specific parameter to doUnbuildNodeTree to
limit its scope), and ensure the finally block unblocks exactly the same scope
(or, alternatively, call
networkModificationTreeService.unblockAllStudyNodeTrees(studyUuid) if you
intentionally must clear every block) so other roots are not left blocked;
update calls to networkModificationTreeService.unblockNodeTree/unblockAll...
consistently with the new per-root behavior.
🪄 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: 77c45fd2-1dfd-4623-b5f2-8abf6055e295
📒 Files selected for processing (5)
src/main/java/org/gridsuite/study/server/elasticsearch/EquipmentInfosService.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.javasrc/test/java/org/gridsuite/study/server/SupervisionControllerTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/gridsuite/study/server/service/SupervisionService.java
- src/test/java/org/gridsuite/study/server/SupervisionControllerTest.java
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/org/gridsuite/study/server/controller/SupervisionController.java (1)
183-184:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the endpoint description with actual behavior.
The summary/response text says “delete root node network”, but this flow invalidates builds and purges remote root-network data while keeping root-network entities. Update wording to avoid API misuse.
🤖 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/SupervisionController.java` around lines 183 - 184, Update the endpoint annotations on SupervisionController so the `@Operation`(summary=...) and `@ApiResponses`(...) accurately reflect behavior: change the summary from "Invalidate built nodes and delete root node network" to something like "Invalidate built nodes and purge remote root-network data (root-network entities preserved)" and update the ApiResponse description from "study has been invalidated" / "delete root node network" to explicitly state "study has been invalidated and remote root-network data purged; root-network entities are retained" so the annotations match the actual flow that invalidates builds and purges remote data without deleting local root-network entities.src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
3947-3954:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse single-root invalidation here to avoid re-invalidating all roots and leaving trees blocked on failures.
Line 3948 calls
doUnbuildNodeTree(...), which invalidates all root networks even though this method handles one root network. In the supervision loop, this repeats study-wide invalidation and can leave non-current roots blocked if one iteration fails (only the current root is unblocked on Line 3953).🐛 Proposed fix
var rootNodeUuid = networkModificationTreeService.getStudyRootNodeUuid(studyUuid); try { - // First we unbuild all nodes - doUnbuildNodeTree(studyUuid, rootNodeUuid, true, true); + // First we unbuild nodes for this root network only + invalidateNodeTree(studyUuid, rootNodeUuid, rootNetworkUuid, InvalidateNodeTreeParameters.ALL_WITH_BLOCK_NODES, true); // Then we erase data linked to root node on all root networks rootNetworkService.invalidateRootNetworkRemoteInfos(List.of(rootNetwork.toDto()), true); updateRootNetworkIndexationStatus(study, rootNetwork, RootNetworkIndexationStatus.NOT_INDEXED); } finally { networkModificationTreeService.unblockNodeTree(rootNetworkUuid, rootNodeUuid); }🤖 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 3947 - 3954, The call sequence currently causes study-wide invalidation because doUnbuildNodeTree(...) and rootNetworkService.invalidateRootNetworkRemoteInfos(List.of(rootNetwork.toDto()), true) operate on all roots; change this to a single-root invalidation to avoid re-invalidating other roots and leaving trees blocked. Replace the call to rootNetworkService.invalidateRootNetworkRemoteInfos(List.of(rootNetwork.toDto()), true) with a single-root variant (e.g., invalidateRootNetworkRemoteInfo(rootNetwork.toDto(), true) or an overload that accepts a single RootNetwork DTO) or modify doUnbuildNodeTree to accept a flag to limit invalidation to the provided rootNodeUuid/rootNetworkUuid; keep updateRootNetworkIndexationStatus(study, rootNetwork, RootNetworkIndexationStatus.NOT_INDEXED) and ensure networkModificationTreeService.unblockNodeTree(rootNetworkUuid, rootNodeUuid) in the finally block still executes for the current root.
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/study/server/SupervisionControllerTest.java (1)
319-320: 💤 Low valueConsider verifying the StudyService orchestration call.
Since
studyServiceis a spy bean, you could add explicit verification thatstudyService.invalidateStudyRootNetworkwas invoked to document the complete call chain more thoroughly. While the downstream effects are already verified, this would match the pattern intestReindexStudy(lines 280–282) and provide clearer traceability of the orchestration flow.Optional verification to add
// Remote root-network data was deleted Mockito.verify(rootNetworkService, Mockito.times(1)) .invalidateRootNetworkRemoteInfos(any(), eq(true)); + +// Verify orchestration layer was invoked +UUID rootNetworkUuid = studyTestUtils.getOneRootNetworkUuid(STUDY_UUID); +Mockito.verify(studyService, Mockito.times(1)) + .invalidateStudyRootNetwork(eq(STUDY_UUID), eq(rootNetworkUuid));🤖 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/SupervisionControllerTest.java` around lines 319 - 320, Add an explicit verification that the spy bean studyService was invoked: after the existing Mockito.verify on rootNetworkService, add a Mockito.verify(studyService, Mockito.times(1)).invalidateStudyRootNetwork(...) call using the same argument matchers as the controller invocation (e.g., Mockito.any() for the study id argument and Mockito.eq(true) for the boolean) so the test mirrors the pattern used in testReindexStudy and documents the orchestration invocation.
🤖 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 3941-3943: The code fetches a RootNetworkEntity by UUID without
verifying it belongs to the StudyEntity returned by getStudy(studyUuid),
allowing cross-study mutations; update the logic in StudyService (around
getStudy and rootNetworkService.getRootNetwork) to check that the retrieved
RootNetworkEntity.getStudy() (or equivalent study reference/field) matches the
StudyEntity (or its id) and throw a StudyException(NOT_FOUND, "Root network not
found") if it does not; ensure this membership guard runs before any
mutation/indexation logic so only root networks belonging to the provided study
are modified.
---
Duplicate comments:
In
`@src/main/java/org/gridsuite/study/server/controller/SupervisionController.java`:
- Around line 183-184: Update the endpoint annotations on SupervisionController
so the `@Operation`(summary=...) and `@ApiResponses`(...) accurately reflect
behavior: change the summary from "Invalidate built nodes and delete root node
network" to something like "Invalidate built nodes and purge remote root-network
data (root-network entities preserved)" and update the ApiResponse description
from "study has been invalidated" / "delete root node network" to explicitly
state "study has been invalidated and remote root-network data purged;
root-network entities are retained" so the annotations match the actual flow
that invalidates builds and purges remote data without deleting local
root-network entities.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 3947-3954: The call sequence currently causes study-wide
invalidation because doUnbuildNodeTree(...) and
rootNetworkService.invalidateRootNetworkRemoteInfos(List.of(rootNetwork.toDto()),
true) operate on all roots; change this to a single-root invalidation to avoid
re-invalidating other roots and leaving trees blocked. Replace the call to
rootNetworkService.invalidateRootNetworkRemoteInfos(List.of(rootNetwork.toDto()),
true) with a single-root variant (e.g.,
invalidateRootNetworkRemoteInfo(rootNetwork.toDto(), true) or an overload that
accepts a single RootNetwork DTO) or modify doUnbuildNodeTree to accept a flag
to limit invalidation to the provided rootNodeUuid/rootNetworkUuid; keep
updateRootNetworkIndexationStatus(study, rootNetwork,
RootNetworkIndexationStatus.NOT_INDEXED) and ensure
networkModificationTreeService.unblockNodeTree(rootNetworkUuid, rootNodeUuid) in
the finally block still executes for the current root.
---
Nitpick comments:
In `@src/test/java/org/gridsuite/study/server/SupervisionControllerTest.java`:
- Around line 319-320: Add an explicit verification that the spy bean
studyService was invoked: after the existing Mockito.verify on
rootNetworkService, add a Mockito.verify(studyService,
Mockito.times(1)).invalidateStudyRootNetwork(...) call using the same argument
matchers as the controller invocation (e.g., Mockito.any() for the study id
argument and Mockito.eq(true) for the boolean) so the test mirrors the pattern
used in testReindexStudy and documents the orchestration invocation.
🪄 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: c2b39fef-fab5-4b34-a74e-95a5d70796f4
📒 Files selected for processing (5)
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.javasrc/test/java/org/gridsuite/study/server/SupervisionControllerTest.java
|
3 participants
PR Summary