Handle async network modification application results#994
Open
ayolab wants to merge 1 commit into
Open
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (14)
📝 WalkthroughWalkthroughThis PR refactors network modification workflows from synchronous result handling to asynchronous message-driven completion. The changes introduce ChangesAsynchronous modification result messaging
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
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/study/server/NetworkModificationTest.java (1)
695-703: 🏗️ Heavy liftExtract the repeated async-modification assertion flow into a helper.
The same sequence is duplicated throughout this file (request → in-progress assertions →
sendApplicationResult(...)→ finished assertions). Centralizing it would reduce brittleness when message ordering/contracts evolve.♻️ Helper sketch
+private void assertAsyncModificationFlow( + UUID studyUuid, + UUID nodeUuid, + Runnable triggerRequest, + Runnable preResultAssertions, + Runnable postResultAssertions) { + triggerRequest.run(); + checkUpdateStatusMessagesReceived(studyUuid, nodeUuid, output); + checkEquipmentCreatingMessagesReceived(studyUuid, nodeUuid); + preResultAssertions.run(); + studyTestUtils.sendApplicationResult(studyUuid, nodeUuid); + checkEquipmentUpdatingFinishedMessagesReceived(studyUuid, nodeUuid); + postResultAssertions.run(); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/study/server/NetworkModificationTest.java` around lines 695 - 703, Extract the repeated async-modification flow into a single helper method (e.g., assertAsyncModificationFlow or performAsyncModificationAndAssert) that encapsulates the POST call and the four assertions: mockMvc.perform(post(URI_NETWORK_MODIF, studyNameUserIdUuid, modificationNode1Uuid).content(bodyJson).contentType(MediaType.APPLICATION_JSON).header(USER_ID_HEADER, userId)).andExpect(status().isOk()), checkUpdateStatusMessagesReceived(studyNameUserIdUuid, modificationNode1Uuid, output), checkEquipmentCreatingMessagesReceived(studyNameUserIdUuid, modificationNode1Uuid), studyTestUtils.sendApplicationResult(studyNameUserIdUuid, modificationNode1Uuid), and checkEquipmentUpdatingFinishedMessagesReceived(studyNameUserIdUuid, modificationNode1Uuid); make the helper accept the needed parameters (studyNameUserIdUuid, modificationNode1Uuid, bodyJson, userId, output) and replace all duplicated sequences in NetworkModificationTest with calls to this new helper.
🤖 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/ConsumerService.java`:
- Around line 223-229: The catch blocks in consumeApplicationResult and
consumeApplicationFailed currently swallow all exceptions by logging only
e.toString(), which prevents retries/DLQ; update those methods (and other
consumer handlers named consumeQueuedCommand, consumeSessionStart,
consumeSessionEnd, consumeApplicationResult, consumeApplicationFailed, etc.) to
rethrow the exception after logging (or wrap and throw a RuntimeException)
instead of returning normally—locate the try/catch around objectMapper.readValue
and studyService.handleApplicationResult/handleApplicationFailed and replace the
silent LOGGER.error(e.toString()) with LOGGER.error(..., e) followed by throw e;
or if checked, throw new RuntimeException("Error processing consumer message",
e) so the framework can trigger retries/DLQ.
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 2433-2441: The loop in handleApplicationResult assumes
modificationResults() and receiver.rootNetworkUuids() are the same size and may
skip emitApplicationEnd if an index error or emitNetworkModificationImpacts
throws; validate sizes first (compute minSize =
Math.min(modificationResults.size(), rootNetworkUuids.size())) and iterate only
to minSize, and wrap the per-item call to emitNetworkModificationImpacts in a
try/catch so one failure doesn’t abort the loop; move the
emitApplicationEnd(receiver) call into a finally block so it always runs
regardless of exceptions, referencing the existing methods modificationResults,
rootNetworkUuids, emitNetworkModificationImpacts, and emitApplicationEnd.
- Around line 3385-3387: The finally block currently calls
networkModificationTreeService.unblockNodeTree(rootNetworkUuid, nodeUuid)
immediately, but duplicateModificationsFromGroup(...) is now asynchronous so
unblocking there can race with the remote apply; remove the unblock from the
finally and instead invoke
networkModificationTreeService.unblockNodeTree(rootNetworkUuid, nodeUuid) inside
the async duplicateModificationsFromGroup(...) completion handlers (both success
and failure paths) so the tree is only unblocked after the async apply finishes;
locate the callsite invoking duplicateModificationsFromGroup(...) and move the
unblock logic into its async callback/then/exception handling blocks.
- Around line 2501-2515: In copyModificationsToExclude, before doing positional
mapping into mappingModificationsUuids, validate that savedUuids.size() ==
modificationsUuids.size() and copyChildren.size() == originalChildren.size(); if
either size differs, fail fast (throw an IllegalStateException or a
domain-specific exception) with a clear message identifying the mismatched
lists; then proceed to build the maps using the verified lists (references:
method copyModificationsToExclude, variables mappingModificationsUuids,
modificationsUuids, savedUuids, originalChildren, copyChildren, and calls to
networkModificationService.findAllChildrenUuids).
In `@src/test/java/org/gridsuite/study/server/NetworkModificationTest.java`:
- Around line 2228-2229: The test sends two modification UUIDs via copyUuids2
but only supplies one result entry, so update the call to
studyTestUtils.sendApplicationResult to provide a result element for each UUID
(e.g., pass a List with two Optional<NetworkModificationResult> entries) so
NetworkModificationsResult aligns with copyUuids2; adjust the List.of(...)
argument currently wrapping
Optional.of(NetworkModificationResult.builder().build()) to include a second
Optional (or an explicit Optional.empty() if intended) so
checkEquipmentMessagesReceived receives payloads for both duplicated
modification UUIDs.
In `@src/test/java/org/gridsuite/study/server/utils/TestUtils.java`:
- Around line 114-117: The test helper currently injects InputDestination
optionally which leads to a late NPE in sendApplicationResult(...) when the test
binder is missing; make the dependency required so startup fails fast (change
the setter injection for InputDestination in setInput(InputDestination) to
`@Autowired` with required=true or drop required=false) or alternatively add a
`@PostConstruct` check that throws an IllegalStateException with a clear message
referencing sendApplicationResult when input is null; apply the same pattern to
the analogous setter/field referenced around lines 139-145 so both input/output
test-binder dependencies fail fast with a clear setup error.
---
Nitpick comments:
In `@src/test/java/org/gridsuite/study/server/NetworkModificationTest.java`:
- Around line 695-703: Extract the repeated async-modification flow into a
single helper method (e.g., assertAsyncModificationFlow or
performAsyncModificationAndAssert) that encapsulates the POST call and the four
assertions: mockMvc.perform(post(URI_NETWORK_MODIF, studyNameUserIdUuid,
modificationNode1Uuid).content(bodyJson).contentType(MediaType.APPLICATION_JSON).header(USER_ID_HEADER,
userId)).andExpect(status().isOk()),
checkUpdateStatusMessagesReceived(studyNameUserIdUuid, modificationNode1Uuid,
output), checkEquipmentCreatingMessagesReceived(studyNameUserIdUuid,
modificationNode1Uuid),
studyTestUtils.sendApplicationResult(studyNameUserIdUuid,
modificationNode1Uuid), and
checkEquipmentUpdatingFinishedMessagesReceived(studyNameUserIdUuid,
modificationNode1Uuid); make the helper accept the needed parameters
(studyNameUserIdUuid, modificationNode1Uuid, bodyJson, userId, output) and
replace all duplicated sequences in NetworkModificationTest with calls to this
new helper.
🪄 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: 077a9acf-f37f-45ca-8939-fa8e20c0e760
📒 Files selected for processing (11)
src/main/java/org/gridsuite/study/server/dto/modification/ModificationReceiver.javasrc/main/java/org/gridsuite/study/server/service/ConsumerService.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/main/resources/config/application.yamlsrc/test/java/org/gridsuite/study/server/NetworkModificationTest.javasrc/test/java/org/gridsuite/study/server/VoltageInitTest.javasrc/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/NodeControllerTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyTest.javasrc/test/java/org/gridsuite/study/server/utils/TestUtils.java
ayolab
force-pushed
the
ayolab/handle-async-midification-application-results
branch
from
5ddd50d to
2f2e3d8
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/gridsuite/study/server/service/RebuildNodeService.java (1)
30-32:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDefer rebuilds until async application completion.
These entry points still run through
handleRebuildNode(...), which rebuilds previously built root networks immediately afterstudyService.createNetworkModification(...)/studyService.moveNetworkModifications(...)returns. After this refactor those calls only submit async application work, so the rebuild can now run before the modification result callback has applied the change and unblocked the tree. Please trigger the rebuild from the async completion path instead of keeping these operations under the synchronoushandleRebuildNode(...)flow.Also applies to: 37-42, 84-86, 123-131
🤖 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 30 - 32, The current createNetworkModification (and the other similar entry points that call handleRebuildNode) invokes handleRebuildNode synchronously around the call to handleCreateNetworkModification, which causes rebuilds to run before the async application of the modification completes; instead remove the synchronous handleRebuildNode wrapper and invoke handleRebuildNode (or the rebuild logic) from the async completion/callback/future returned by the studyService call (the path that actually applies the modification result), so change createNetworkModification, moveNetworkModifications and the other listed methods to only submit the async work (call handleCreateNetworkModification / handleMoveNetworkModifications) and attach a continuation that calls handleRebuildNode(studyUuid, nodeUuid, userId) after the studyService.createNetworkModification / studyService.moveNetworkModifications completion promise/callback indicates the change is applied.
🧹 Nitpick comments (1)
src/main/java/org/gridsuite/study/server/service/StudyService.java (1)
2445-2457: 💤 Low valuePast review concern partially addressed:
finallyblock now ensures cleanup.The
emitApplicationEnd(receiver)is now in afinallyblock, so the node will always be unblocked even ifemitNetworkModificationImpactsthrows. However, ifmodificationResults.size() > rootNetworkUuids.size(), anIndexOutOfBoundsExceptionwould occur at line 2452. While cleanup will still happen, adding a size check would provide clearer error handling.🛡️ Optional defensive size validation
public void handleApplicationResult(ModificationReceiver receiver, NetworkModificationsResult result) { try { List<Optional<NetworkModificationResult>> modificationResults = result.modificationResults(); List<UUID> rootNetworkUuids = receiver.rootNetworkUuids(); + if (modificationResults.size() != rootNetworkUuids.size()) { + LOGGER.warn("Mismatch between modification results ({}) and root networks ({})", + modificationResults.size(), rootNetworkUuids.size()); + } + int limit = Math.min(modificationResults.size(), rootNetworkUuids.size()); - for (int i = 0; i < modificationResults.size(); i++) { + for (int i = 0; i < limit; i++) { final int index = i;🤖 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 2445 - 2457, In handleApplicationResult, guard against a possible IndexOutOfBoundsException when modificationResults.size() > rootNetworkUuids.size() by validating sizes before accessing rootNetworkUuids: compute the smaller of result.modificationResults().size() and receiver.rootNetworkUuids().size() (or explicitly check and log/throw a descriptive IllegalStateException) and iterate up to that bound when calling emitNetworkModificationImpacts(receiver.studyUuid(), receiver.nodeUuid(), rootNetworkUuids.get(index), r); keep the existing finally block with emitApplicationEnd(receiver) unchanged so cleanup still runs.
🤖 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.
Outside diff comments:
In `@src/main/java/org/gridsuite/study/server/service/RebuildNodeService.java`:
- Around line 30-32: The current createNetworkModification (and the other
similar entry points that call handleRebuildNode) invokes handleRebuildNode
synchronously around the call to handleCreateNetworkModification, which causes
rebuilds to run before the async application of the modification completes;
instead remove the synchronous handleRebuildNode wrapper and invoke
handleRebuildNode (or the rebuild logic) from the async
completion/callback/future returned by the studyService call (the path that
actually applies the modification result), so change createNetworkModification,
moveNetworkModifications and the other listed methods to only submit the async
work (call handleCreateNetworkModification / handleMoveNetworkModifications) and
attach a continuation that calls handleRebuildNode(studyUuid, nodeUuid, userId)
after the studyService.createNetworkModification /
studyService.moveNetworkModifications completion promise/callback indicates the
change is applied.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/study/server/service/StudyService.java`:
- Around line 2445-2457: In handleApplicationResult, guard against a possible
IndexOutOfBoundsException when modificationResults.size() >
rootNetworkUuids.size() by validating sizes before accessing rootNetworkUuids:
compute the smaller of result.modificationResults().size() and
receiver.rootNetworkUuids().size() (or explicitly check and log/throw a
descriptive IllegalStateException) and iterate up to that bound when calling
emitNetworkModificationImpacts(receiver.studyUuid(), receiver.nodeUuid(),
rootNetworkUuids.get(index), r); keep the existing finally block with
emitApplicationEnd(receiver) unchanged so cleanup still runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 405080c3-8cfe-4e3c-9a8b-e4a0403e7d98
📒 Files selected for processing (13)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/dto/modification/ModificationReceiver.javasrc/main/java/org/gridsuite/study/server/service/ConsumerService.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationService.javasrc/main/java/org/gridsuite/study/server/service/RebuildNodeService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/main/resources/config/application.yamlsrc/test/java/org/gridsuite/study/server/NetworkModificationTest.javasrc/test/java/org/gridsuite/study/server/VoltageInitTest.javasrc/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/NodeControllerTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyTest.javasrc/test/java/org/gridsuite/study/server/utils/TestUtils.java
🚧 Files skipped from review as they are similar to previous changes (10)
- src/main/java/org/gridsuite/study/server/dto/modification/ModificationReceiver.java
- src/main/resources/config/application.yaml
- src/test/java/org/gridsuite/study/server/utils/TestUtils.java
- src/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.java
- src/test/java/org/gridsuite/study/server/studycontroller/StudyTest.java
- src/test/java/org/gridsuite/study/server/studycontroller/NodeControllerTest.java
- src/test/java/org/gridsuite/study/server/NetworkModificationTest.java
- src/main/java/org/gridsuite/study/server/service/ConsumerService.java
- src/main/java/org/gridsuite/study/server/service/NetworkModificationService.java
- src/test/java/org/gridsuite/study/server/VoltageInitTest.java
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
ayolab
force-pushed
the
ayolab/handle-async-midification-application-results
branch
from
2f2e3d8 to
61d2ea7
Compare
|
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.
No description provided.