Apply network modifications asynchronously#816
Open
ayolab wants to merge 5 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 32 minutes and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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 (4)
📝 WalkthroughWalkthroughModification endpoints now save synchronously and return UUID lists, optional receiver-based application requests are published through Spring Cloud Stream, a worker applies saved modifications and emits results, and tests are rewritten to consume broker messages or helper-based save-and-apply flows. ChangesModification application flow split
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant NetworkModificationService
participant NotificationService
participant ModificationApplicationWorkerService
Client->>Controller: POST/PUT modification request with receiver
Controller->>NetworkModificationService: save...(...)
NetworkModificationService-->>Controller: List<UUID>
Controller->>NetworkModificationService: applicationRequest(groupUuid, uuids, contexts, receiver)
NetworkModificationService->>NotificationService: emitApplicationMessage(message)
NotificationService->>ModificationApplicationWorkerService: publish application message
ModificationApplicationWorkerService->>NetworkModificationService: applyModificationsByUuids(groupUuid, uuids, contexts)
ModificationApplicationWorkerService->>NotificationService: emitApplicationResultMessage(result, receiver)
Suggested reviewers
|
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
ayolab
force-pushed
the
ayolab/apply-network-modifications-asynchronously
branch
from
511f558 to
ce3adf7
Compare
EtienneLt
reviewed
Jun 10, 2026
| */ | ||
| @Getter | ||
| @AllArgsConstructor | ||
| public class ApplicationExecContext { |
Contributor
There was a problem hiding this comment.
Suggested change
| public class ApplicationExecContext { | |
| public class ApplicationExecutionContext { |
| @AllArgsConstructor | ||
| public class ApplicationExecContext { | ||
|
|
||
| static final String GROUP_UUID_HEADER = "groupUuid"; |
| public class ApplicationExecContext { | ||
|
|
||
| static final String GROUP_UUID_HEADER = "groupUuid"; | ||
| static final String MODIFICATION_UUIDS_HEADER = "modificationUuids"; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java (1)
385-392: ⚡ Quick winConsider batch-fetching modifications to avoid N queries.
The current implementation calls
getModificationInfoonce per UUID, resulting in N database round-trips. IfmodificationUuidsis large, consider using a batch-fetch method (e.g.,findAllByIdIn) and then ordering the results to match the input list.🤖 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/modification/server/service/NetworkModificationService.java` around lines 385 - 392, The applyModificationsByUuids method currently fetches each modification individually via getModificationInfo in a stream, causing N database round-trips. Replace the stream-based approach with a batch-fetch method (such as findAllByIdIn on the networkModificationRepository) to fetch all modifications in a single query, then reorder the results to match the original modificationUuids list order before passing them to applyModifications.src/test/java/org/gridsuite/modification/server/ModificationControllerTest.java (1)
229-233: ⚡ Quick winRe-add assertions for broker-side error payloads in the async failure tests.
Line 229 to Line 233 and Line 579 to Line 583 only verify HTTP
200 OK. Since these scenarios explicitly moved error reporting to broker messages, the tests should also assert the emitted async result shape/status to avoid silent regressions.Also applies to: 579-583
🤖 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/modification/server/ModificationControllerTest.java` around lines 229 - 233, The tests at lines 229-233 and 579-583 in ModificationControllerTest.java only verify the HTTP response status (200 OK) for async failure scenarios, but do not assert the broker-side error payloads or async result shape. After the mockMvc.perform calls that check status().isOk(), add assertions to verify the actual error message/result emitted through the broker (such as checking the async result status, error details, and overall message structure) to ensure error reporting works correctly at the broker level and prevent silent regressions.src/test/java/org/gridsuite/modification/server/modifications/byfilter/formula/AbstractByFormulaModificationTest.java (1)
198-203: ⚡ Quick winUse the
saveAndApplyreturn value directly instead of reparsinglastResultJson.Line 200 re-deserializes shared state after
saveAndApply(...), which is redundant and more brittle than asserting directly on the returned object.Suggested change
- saveAndApply(modificationToCreateJson); - - Optional<NetworkModificationsResult> networkModificationsResult = mapper.readValue(lastResultJson, new TypeReference<>() { }); - assertTrue(networkModificationsResult.isPresent()); - assertEquals(1, extractApplicationStatus(networkModificationsResult.get()).size()); - assertEquals(applicationStatus, extractApplicationStatus(networkModificationsResult.get()).getFirst()); + NetworkModificationsResult networkModificationsResult = saveAndApply(modificationToCreateJson); + assertEquals(1, extractApplicationStatus(networkModificationsResult).size()); + assertEquals(applicationStatus, extractApplicationStatus(networkModificationsResult).getFirst());🤖 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/modification/server/modifications/byfilter/formula/AbstractByFormulaModificationTest.java` around lines 198 - 203, The test method is unnecessarily re-deserializing shared state by reading lastResultJson after calling saveAndApply(modificationToCreateJson). Instead, capture and use the return value of the saveAndApply method directly. Modify the code to store the NetworkModificationsResult returned by saveAndApply in a variable, then use that variable for the subsequent assertions on extractApplicationStatus instead of deserializing lastResultJson again.
🤖 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/modification/server/service/ModificationApplicationWorkerService.java`:
- Around line 33-39: The code block that parses the message, applies
modifications, and emits the result lacks error handling. If
applyModificationsByUuids throws an exception, the worker fails without emitting
an ApplicationResultMessage, leaving callers waiting indefinitely. Wrap the
entire block containing ApplicationExecContext.fromMessage,
notificationService.emitApplicationResultMessage, and
networkModificationService.applyModificationsByUuids in a try-catch block that
catches any exceptions and ensures an ApplicationResultMessage with error
information is always emitted to ctx.getReceiver(), so callers receive a failure
response rather than timing out.
In `@src/test/java/org/gridsuite/modification/server/utils/ApiUtils.java`:
- Around line 57-69: The putGroupsDuplications method (and the similar method
also affected on lines 71-84) currently returns a fabricated Optional.empty()
regardless of the actual endpoint response, which masks real failures and
creates false-positive test assertions. Either extract and deserialize the
actual response body from the MockMvc.perform() call and return the real
NetworkModificationResult, or change both method signatures to return void and
update all callers to remove the return value handling. Choose the approach that
best fits how these methods are used in your tests.
---
Nitpick comments:
In
`@src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java`:
- Around line 385-392: The applyModificationsByUuids method currently fetches
each modification individually via getModificationInfo in a stream, causing N
database round-trips. Replace the stream-based approach with a batch-fetch
method (such as findAllByIdIn on the networkModificationRepository) to fetch all
modifications in a single query, then reorder the results to match the original
modificationUuids list order before passing them to applyModifications.
In
`@src/test/java/org/gridsuite/modification/server/ModificationControllerTest.java`:
- Around line 229-233: The tests at lines 229-233 and 579-583 in
ModificationControllerTest.java only verify the HTTP response status (200 OK)
for async failure scenarios, but do not assert the broker-side error payloads or
async result shape. After the mockMvc.perform calls that check status().isOk(),
add assertions to verify the actual error message/result emitted through the
broker (such as checking the async result status, error details, and overall
message structure) to ensure error reporting works correctly at the broker level
and prevent silent regressions.
In
`@src/test/java/org/gridsuite/modification/server/modifications/byfilter/formula/AbstractByFormulaModificationTest.java`:
- Around line 198-203: The test method is unnecessarily re-deserializing shared
state by reading lastResultJson after calling
saveAndApply(modificationToCreateJson). Instead, capture and use the return
value of the saveAndApply method directly. Modify the code to store the
NetworkModificationsResult returned by saveAndApply in a variable, then use that
variable for the subsequent assertions on extractApplicationStatus instead of
deserializing lastResultJson again.
🪄 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: 8feb6c1e-55ff-4041-b532-f18b7a2d651a
📒 Files selected for processing (65)
src/main/java/org/gridsuite/modification/server/CompositeController.javasrc/main/java/org/gridsuite/modification/server/NetworkModificationController.javasrc/main/java/org/gridsuite/modification/server/service/ApplicationExecContext.javasrc/main/java/org/gridsuite/modification/server/service/ModificationApplicationWorkerService.javasrc/main/java/org/gridsuite/modification/server/service/NetworkModificationService.javasrc/main/java/org/gridsuite/modification/server/service/NotificationService.javasrc/main/resources/config/application.yamlsrc/test/java/org/gridsuite/modification/server/CompositeControllerTest.javasrc/test/java/org/gridsuite/modification/server/ModificationControllerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/AbstractInjectionModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/AbstractNetworkModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/BatteryCreationInBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/BatteryCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/BatteryModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/DeleteAttachingLineTest.javasrc/test/java/org/gridsuite/modification/server/modifications/DeleteVoltageLevelOnLineTest.javasrc/test/java/org/gridsuite/modification/server/modifications/EquipmentAttributeModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/EquipmentDeletionTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GenerationDispatchTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GeneratorCreationInBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GeneratorCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GeneratorModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GeneratorScalingTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GroovyScriptTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LccCreationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineAttachToVoltageLevelTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineCreationInBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineSplitWithNewVoltageLevelTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineSplitWithVoltageLevelTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LinesAttachToSplitLinesTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LoadCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LoadModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LoadScalingTest.javasrc/test/java/org/gridsuite/modification/server/modifications/OperatingStatusModificationEnergiseSideOneLineTest.javasrc/test/java/org/gridsuite/modification/server/modifications/OperatingStatusModificationEnergiseSideTwoLineTest.javasrc/test/java/org/gridsuite/modification/server/modifications/OperatingStatusModificationLockoutLineTest.javasrc/test/java/org/gridsuite/modification/server/modifications/ShuntCompensatorCreationInBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/ShuntCompensatorCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/ShuntCompensatorModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/StaticVarCompensatorCreationInBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/StaticVarCompensatorCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/SubstationCreationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/SubstationModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerCreationBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerCreationMixedBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerCreationNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/VoltageInitModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/VoltageLevelCreationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/VoltageLevelModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/VoltageLevelTopologyModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/VscCreationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/byfilter/assignment/AbstractModificationByAssignmentTest.javasrc/test/java/org/gridsuite/modification/server/modifications/byfilter/formula/AbstractByFormulaModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/byfilterdeletion/AbstractByFilterDeletionTest.javasrc/test/java/org/gridsuite/modification/server/modifications/tabularcreations/TabularGeneratorCreationsTest.javasrc/test/java/org/gridsuite/modification/server/modifications/tabularmodifications/TabularGeneratorModificationsTest.javasrc/test/java/org/gridsuite/modification/server/service/BuildTest.javasrc/test/java/org/gridsuite/modification/server/service/EquipmentIndexationTest.javasrc/test/java/org/gridsuite/modification/server/service/ModificationIndexationTest.javasrc/test/java/org/gridsuite/modification/server/utils/ApiUtils.javasrc/test/java/org/gridsuite/modification/server/utils/TestUtils.javasrc/test/resources/application-default.yml
Comment on lines
+33
to
+39
| ApplicationExecContext ctx = ApplicationExecContext.fromMessage(message, objectMapper); | ||
| notificationService.emitApplicationResultMessage( | ||
| new NetworkModificationsResult( | ||
| ctx.getModificationUuids(), | ||
| networkModificationService.applyModificationsByUuids( | ||
| ctx.getGroupUuid(), ctx.getModificationUuids(), ctx.getApplicationContexts())), | ||
| ctx.getReceiver()); |
There was a problem hiding this comment.
Guard the worker path so failures still produce an application result.
Line 33 to Line 39 executes parse/apply/emit without protection. If apply throws, the consumer fails and no publishApplicationResult is emitted, which can leave callers waiting indefinitely and trigger redelivery loops.
Suggested fix
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.util.Optional;
...
public class ModificationApplicationWorkerService {
+ private static final Logger LOGGER = LoggerFactory.getLogger(ModificationApplicationWorkerService.class);
...
public Consumer<Message<String>> consumeApplication() {
return message -> {
ApplicationExecContext ctx = ApplicationExecContext.fromMessage(message, objectMapper);
- notificationService.emitApplicationResultMessage(
- new NetworkModificationsResult(
- ctx.getModificationUuids(),
- networkModificationService.applyModificationsByUuids(
- ctx.getGroupUuid(), ctx.getModificationUuids(), ctx.getApplicationContexts())),
- ctx.getReceiver());
+ try {
+ notificationService.emitApplicationResultMessage(
+ new NetworkModificationsResult(
+ ctx.getModificationUuids(),
+ networkModificationService.applyModificationsByUuids(
+ ctx.getGroupUuid(), ctx.getModificationUuids(), ctx.getApplicationContexts())),
+ ctx.getReceiver());
+ } catch (RuntimeException e) {
+ LOGGER.error("Failed to apply modification batch for group {}", ctx.getGroupUuid(), e);
+ notificationService.emitApplicationResultMessage(
+ new NetworkModificationsResult(
+ ctx.getModificationUuids(),
+ ctx.getModificationUuids().stream()
+ .map(uuid -> Optional.<NetworkModificationResult>empty())
+ .toList()),
+ ctx.getReceiver());
+ }
};
}
}🤖 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/modification/server/service/ModificationApplicationWorkerService.java`
around lines 33 - 39, The code block that parses the message, applies
modifications, and emits the result lacks error handling. If
applyModificationsByUuids throws an exception, the worker fails without emitting
an ApplicationResultMessage, leaving callers waiting indefinitely. Wrap the
entire block containing ApplicationExecContext.fromMessage,
notificationService.emitApplicationResultMessage, and
networkModificationService.applyModificationsByUuids in a try-catch block that
catches any exceptions and ensures an ApplicationResultMessage with error
information is always emitted to ctx.getReceiver(), so callers receive a failure
response rather than timing out.
Comment on lines
57
to
69
| public static Optional<NetworkModificationResult> putGroupsDuplications(MockMvc mockMvc, UUID originGroupUuid, UUID targetGroupUuid, UUID networkUuid) throws Exception { | ||
| ModificationApplicationContext applicationContext = new ModificationApplicationContext(networkUuid, UUID.randomUUID().toString(), UUID.randomUUID(), UUID.randomUUID(), Set.of()); | ||
| String bodyJson = getObjectMapper().writeValueAsString(org.springframework.data.util.Pair.of(List.of(), List.of(applicationContext))); | ||
| ResultActions mockMvcResultActions = mockMvc.perform( | ||
| mockMvc.perform( | ||
| put("/v1/groups/{groupUuid}", targetGroupUuid) | ||
| .param("action", "COPY") | ||
| .param("originGroupUuid", originGroupUuid.toString()) | ||
| .content(bodyJson) | ||
| .contentType(MediaType.APPLICATION_JSON) | ||
| ) | ||
| .andExpect(request().asyncStarted()); | ||
| MvcResult mvcResult = mockMvc.perform(asyncDispatch(mockMvcResultActions.andReturn())) | ||
| .andExpectAll(status().isOk()) | ||
| .andReturn(); | ||
| NetworkModificationsResult result = getObjectMapper().readValue(mvcResult.getResponse().getContentAsString(), new TypeReference<>() { }); | ||
| return result.modificationResults().isEmpty() ? Optional.empty() : result.modificationResults().get(0); | ||
| .andExpectAll(status().isOk()); | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
Do not return fabricated copy/duplication results from helper APIs.
Line 57 to Line 69 and Line 71 to Line 84 now return placeholder empty values independent of actual endpoint behavior. This masks failures and can produce false-positive assertions in downstream tests. Keep the methods behaviorally honest: either deserialize real async results or change the signatures to void and update callers accordingly.
Also applies to: 71-84
🤖 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/modification/server/utils/ApiUtils.java` around
lines 57 - 69, The putGroupsDuplications method (and the similar method also
affected on lines 71-84) currently returns a fabricated Optional.empty()
regardless of the actual endpoint response, which masks real failures and
creates false-positive test assertions. Either extract and deserialize the
actual response body from the MockMvc.perform() call and return the real
NetworkModificationResult, or change both method signatures to return void and
update all callers to remove the return value handling. Choose the approach that
best fits how these methods are used in your tests.
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
ayolab
force-pushed
the
ayolab/apply-network-modifications-asynchronously
branch
from
b2cc89f to
34c3869
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/org/gridsuite/modification/server/service/BuildTest.java (1)
996-1002:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncremental-mode assertions are still validating the previous result object.
After constructing
networkModificationsResult(Lines 996-999), Line 1001 still checksnetworkModificationResultfrom the earlier scenario. This weakens coverage of the new save-then-apply path.Suggested fix
- testEmptyImpactsWithErrors(networkModificationResult); + testEmptyImpactsWithErrors(networkModificationsResult.modificationResults().get(0).orElseThrow());🤖 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/modification/server/service/BuildTest.java` around lines 996 - 1002, The testEmptyImpactsWithErrors method call on line 1001 is validating the wrong result object. After constructing networkModificationsResult (the new save-then-apply result), the test still passes the old networkModificationResult variable from the earlier scenario to testEmptyImpactsWithErrors, which weakens test coverage. Change the argument passed to testEmptyImpactsWithErrors from networkModificationResult to networkModificationsResult to ensure the newly constructed result from the incremental-mode path is properly validated.
🤖 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/modification/server/NetworkModificationController.java`:
- Around line 111-113: The networkModificationService.applicationRequest method
call in the conditional block persists modifications first and then publishes to
the broker, creating a race condition where publish failures after successful
save can cause duplicate modifications on client retries. Implement an outbox
pattern or idempotency strategy to harden this operation: either queue the
publish request in a persistent outbox table before returning success to the
client and have a background worker process these queued publishes, or add a
unique idempotency key/request ID mechanism that persists alongside the
modification data so that duplicate requests can be detected and handled
gracefully without re-persisting already saved modifications. Apply this same
fix to all occurrences of this save-then-publish pattern throughout the
codebase.
- Around line 105-108: The COPY branch in the switch statement is passing
`originGroupUuid` directly to the `saveDuplicateModifications` method, but it
should pass the resolved `sourceGroupUuid` instead to maintain consistency with
the MOVE branch and handle cases where `originGroupUuid` is missing. Replace the
`originGroupUuid` parameter in the `saveDuplicateModifications` call with
`sourceGroupUuid` to use the fallback value that was computed earlier in the
code.
In
`@src/test/java/org/gridsuite/modification/server/ModificationControllerTest.java`:
- Around line 194-199: The assertApplicationStatusOK() method receives a message
from applicationDestination but does not assert the result, which allows tests
to pass even if the application request fails to be emitted. Store the result of
the output.receive(TIMEOUT, applicationDestination) call in a variable and add
an assertion to verify it is not null, similar to how the resultMsg from
applicationResultDestination is validated. This ensures both the application
request message and the application result message are properly checked.
---
Outside diff comments:
In `@src/test/java/org/gridsuite/modification/server/service/BuildTest.java`:
- Around line 996-1002: The testEmptyImpactsWithErrors method call on line 1001
is validating the wrong result object. After constructing
networkModificationsResult (the new save-then-apply result), the test still
passes the old networkModificationResult variable from the earlier scenario to
testEmptyImpactsWithErrors, which weakens test coverage. Change the argument
passed to testEmptyImpactsWithErrors from networkModificationResult to
networkModificationsResult to ensure the newly constructed result from the
incremental-mode path is properly validated.
🪄 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: 9efa8308-9717-41a5-858a-917bd584423a
📒 Files selected for processing (65)
src/main/java/org/gridsuite/modification/server/CompositeController.javasrc/main/java/org/gridsuite/modification/server/NetworkModificationController.javasrc/main/java/org/gridsuite/modification/server/service/ApplicationExecContext.javasrc/main/java/org/gridsuite/modification/server/service/ModificationApplicationWorkerService.javasrc/main/java/org/gridsuite/modification/server/service/NetworkModificationService.javasrc/main/java/org/gridsuite/modification/server/service/NotificationService.javasrc/main/resources/config/application.yamlsrc/test/java/org/gridsuite/modification/server/CompositeControllerTest.javasrc/test/java/org/gridsuite/modification/server/ModificationControllerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/AbstractInjectionModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/AbstractNetworkModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/BatteryCreationInBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/BatteryCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/BatteryModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/DeleteAttachingLineTest.javasrc/test/java/org/gridsuite/modification/server/modifications/DeleteVoltageLevelOnLineTest.javasrc/test/java/org/gridsuite/modification/server/modifications/EquipmentAttributeModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/EquipmentDeletionTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GenerationDispatchTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GeneratorCreationInBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GeneratorCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GeneratorModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GeneratorScalingTest.javasrc/test/java/org/gridsuite/modification/server/modifications/GroovyScriptTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LccCreationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineAttachToVoltageLevelTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineCreationInBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineSplitWithNewVoltageLevelTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LineSplitWithVoltageLevelTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LinesAttachToSplitLinesTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LoadCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LoadModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/LoadScalingTest.javasrc/test/java/org/gridsuite/modification/server/modifications/OperatingStatusModificationEnergiseSideOneLineTest.javasrc/test/java/org/gridsuite/modification/server/modifications/OperatingStatusModificationEnergiseSideTwoLineTest.javasrc/test/java/org/gridsuite/modification/server/modifications/OperatingStatusModificationLockoutLineTest.javasrc/test/java/org/gridsuite/modification/server/modifications/ShuntCompensatorCreationInBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/ShuntCompensatorCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/ShuntCompensatorModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/StaticVarCompensatorCreationInBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/StaticVarCompensatorCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/SubstationCreationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/SubstationModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerCreationBusBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerCreationMixedBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerCreationNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/VoltageInitModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/VoltageLevelCreationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/VoltageLevelModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/VoltageLevelTopologyModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/VscCreationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/byfilter/assignment/AbstractModificationByAssignmentTest.javasrc/test/java/org/gridsuite/modification/server/modifications/byfilter/formula/AbstractByFormulaModificationTest.javasrc/test/java/org/gridsuite/modification/server/modifications/byfilterdeletion/AbstractByFilterDeletionTest.javasrc/test/java/org/gridsuite/modification/server/modifications/tabularcreations/TabularGeneratorCreationsTest.javasrc/test/java/org/gridsuite/modification/server/modifications/tabularmodifications/TabularGeneratorModificationsTest.javasrc/test/java/org/gridsuite/modification/server/service/BuildTest.javasrc/test/java/org/gridsuite/modification/server/service/EquipmentIndexationTest.javasrc/test/java/org/gridsuite/modification/server/service/ModificationIndexationTest.javasrc/test/java/org/gridsuite/modification/server/utils/ApiUtils.javasrc/test/java/org/gridsuite/modification/server/utils/TestUtils.javasrc/test/resources/application-default.yml
✅ Files skipped from review due to trivial changes (1)
- src/test/resources/application-default.yml
🚧 Files skipped from review as they are similar to previous changes (56)
- src/test/java/org/gridsuite/modification/server/modifications/LineCreationInBusBreakerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/LoadModificationTest.java
- src/main/java/org/gridsuite/modification/server/service/NotificationService.java
- src/test/java/org/gridsuite/modification/server/modifications/OperatingStatusModificationEnergiseSideTwoLineTest.java
- src/test/java/org/gridsuite/modification/server/modifications/LineSplitWithNewVoltageLevelTest.java
- src/test/java/org/gridsuite/modification/server/modifications/EquipmentAttributeModificationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/AbstractInjectionModificationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/OperatingStatusModificationLockoutLineTest.java
- src/test/java/org/gridsuite/modification/server/service/ModificationIndexationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/byfilterdeletion/AbstractByFilterDeletionTest.java
- src/test/java/org/gridsuite/modification/server/modifications/VoltageInitModificationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/ShuntCompensatorCreationInNodeBreakerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/VoltageLevelModificationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/EquipmentDeletionTest.java
- src/test/java/org/gridsuite/modification/server/modifications/VoltageLevelTopologyModificationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/BatteryCreationInBusBreakerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/GeneratorCreationInBusBreakerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerCreationMixedBreakerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/GenerationDispatchTest.java
- src/main/java/org/gridsuite/modification/server/service/ModificationApplicationWorkerService.java
- src/test/java/org/gridsuite/modification/server/modifications/byfilter/formula/AbstractByFormulaModificationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerCreationBusBreakerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/StaticVarCompensatorCreationInBusBreakerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/LccCreationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerModificationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/OperatingStatusModificationEnergiseSideOneLineTest.java
- src/test/java/org/gridsuite/modification/server/modifications/LineAttachToVoltageLevelTest.java
- src/test/java/org/gridsuite/modification/server/modifications/LoadScalingTest.java
- src/main/java/org/gridsuite/modification/server/CompositeController.java
- src/test/java/org/gridsuite/modification/server/modifications/GroovyScriptTest.java
- src/test/java/org/gridsuite/modification/server/modifications/LineCreationInNodeBreakerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/GeneratorCreationInNodeBreakerTest.java
- src/main/java/org/gridsuite/modification/server/service/ApplicationExecContext.java
- src/test/java/org/gridsuite/modification/server/service/EquipmentIndexationTest.java
- src/main/resources/config/application.yaml
- src/test/java/org/gridsuite/modification/server/modifications/DeleteVoltageLevelOnLineTest.java
- src/test/java/org/gridsuite/modification/server/modifications/DeleteAttachingLineTest.java
- src/test/java/org/gridsuite/modification/server/modifications/LinesAttachToSplitLinesTest.java
- src/test/java/org/gridsuite/modification/server/modifications/LineSplitWithVoltageLevelTest.java
- src/test/java/org/gridsuite/modification/server/modifications/tabularcreations/TabularGeneratorCreationsTest.java
- src/test/java/org/gridsuite/modification/server/modifications/TwoWindingsTransformerCreationNodeBreakerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/LoadCreationInNodeBreakerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/SubstationModificationTest.java
- src/test/java/org/gridsuite/modification/server/utils/ApiUtils.java
- src/test/java/org/gridsuite/modification/server/modifications/StaticVarCompensatorCreationInNodeBreakerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/GeneratorScalingTest.java
- src/test/java/org/gridsuite/modification/server/modifications/AbstractNetworkModificationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/VoltageLevelCreationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/tabularmodifications/TabularGeneratorModificationsTest.java
- src/test/java/org/gridsuite/modification/server/utils/TestUtils.java
- src/test/java/org/gridsuite/modification/server/modifications/ShuntCompensatorModificationTest.java
- src/test/java/org/gridsuite/modification/server/modifications/SubstationCreationTest.java
- src/main/java/org/gridsuite/modification/server/service/NetworkModificationService.java
- src/test/java/org/gridsuite/modification/server/modifications/BatteryModificationTest.java
- src/test/java/org/gridsuite/modification/server/CompositeControllerTest.java
- src/test/java/org/gridsuite/modification/server/modifications/LineModificationTest.java
Comment on lines
+105
to
+108
| List<UUID> savedUuids = switch (action) { | ||
| case COPY -> networkModificationService.saveDuplicateModifications( | ||
| targetGroupUuid, originGroupUuid, modificationContextInfos.getFirst()); | ||
| case MOVE -> networkModificationService.saveMoveModifications( |
There was a problem hiding this comment.
Use the resolved source group UUID in the COPY branch.
Line 104 computes a fallback sourceGroupUuid for missing originGroupUuid, but Line 107 passes originGroupUuid directly. This makes COPY behavior inconsistent with MOVE and can break same-group copies when originGroupUuid is omitted.
Suggested fix
List<UUID> savedUuids = switch (action) {
case COPY -> networkModificationService.saveDuplicateModifications(
- targetGroupUuid, originGroupUuid, modificationContextInfos.getFirst());
+ targetGroupUuid, sourceGroupUuid, modificationContextInfos.getFirst());
case MOVE -> networkModificationService.saveMoveModifications(
targetGroupUuid, sourceGroupUuid, beforeModificationUuid, modificationContextInfos.getFirst());
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| List<UUID> savedUuids = switch (action) { | |
| case COPY -> networkModificationService.saveDuplicateModifications( | |
| targetGroupUuid, originGroupUuid, modificationContextInfos.getFirst()); | |
| case MOVE -> networkModificationService.saveMoveModifications( | |
| List<UUID> savedUuids = switch (action) { | |
| case COPY -> networkModificationService.saveDuplicateModifications( | |
| targetGroupUuid, sourceGroupUuid, modificationContextInfos.getFirst()); | |
| case MOVE -> networkModificationService.saveMoveModifications( |
🤖 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/modification/server/NetworkModificationController.java`
around lines 105 - 108, The COPY branch in the switch statement is passing
`originGroupUuid` directly to the `saveDuplicateModifications` method, but it
should pass the resolved `sourceGroupUuid` instead to maintain consistency with
the MOVE branch and handle cases where `originGroupUuid` is missing. Replace the
`originGroupUuid` parameter in the `saveDuplicateModifications` call with
`sourceGroupUuid` to use the fallback value that was computed earlier in the
code.
Comment on lines
+111
to
+113
| if (receiver != null) { | ||
| networkModificationService.applicationRequest(targetGroupUuid, savedUuids, modificationContextInfos.getSecond(), receiver); | ||
| } |
There was a problem hiding this comment.
Save and publish are non-atomic; publish failure can cause duplicate persisted modifications on retries.
Both endpoints persist first, then call broker publish. If publish fails after save, the request can fail while data is already committed, and client retries can duplicate modifications. This should be hardened with an outbox/idempotency strategy (or explicit partial-success contract).
Also applies to: 137-139
🤖 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/modification/server/NetworkModificationController.java`
around lines 111 - 113, The networkModificationService.applicationRequest method
call in the conditional block persists modifications first and then publishes to
the broker, creating a race condition where publish failures after successful
save can cause duplicate modifications on client retries. Implement an outbox
pattern or idempotency strategy to harden this operation: either queue the
publish request in a persistent outbox table before returning success to the
client and have a background worker process these queued publishes, or add a
unique idempotency key/request ID mechanism that persists alongside the
modification data so that duplicate requests can be detected and handled
gracefully without re-persisting already saved modifications. Apply this same
fix to all occurrences of this save-then-publish pattern throughout the
codebase.
Comment on lines
+194
to
+199
| private void assertApplicationStatusOK() throws Exception { | ||
| Message<byte[]> resultMsg = output.receive(TIMEOUT, applicationResultDestination); | ||
| assertNotNull(resultMsg, "No message received on " + applicationResultDestination); | ||
| output.receive(TIMEOUT, applicationDestination); | ||
| lastResultJson = new String(resultMsg.getPayload()); | ||
| NetworkModificationsResult networkModificationsResult = mapper.readValue(lastResultJson, new TypeReference<>() { }); |
There was a problem hiding this comment.
Assert the application-message receive result, not just the application-result message.
Line 197 consumes applicationDestination without checking it. Tests can pass even if the application-request emission regresses, as long as a result message appears.
Suggested fix
private void assertApplicationStatusOK() throws Exception {
Message<byte[]> resultMsg = output.receive(TIMEOUT, applicationResultDestination);
assertNotNull(resultMsg, "No message received on " + applicationResultDestination);
- output.receive(TIMEOUT, applicationDestination);
+ Message<byte[]> applicationMsg = output.receive(TIMEOUT, applicationDestination);
+ assertNotNull(applicationMsg, "No message received on " + applicationDestination);
lastResultJson = new String(resultMsg.getPayload());
NetworkModificationsResult networkModificationsResult = mapper.readValue(lastResultJson, new TypeReference<>() { });🤖 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/modification/server/ModificationControllerTest.java`
around lines 194 - 199, The assertApplicationStatusOK() method receives a
message from applicationDestination but does not assert the result, which allows
tests to pass even if the application request fails to be emitted. Store the
result of the output.receive(TIMEOUT, applicationDestination) call in a variable
and add an assertion to verify it is not null, similar to how the resultMsg from
applicationResultDestination is validated. This ensures both the application
request message and the application result message are properly checked.
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
ayolab
force-pushed
the
ayolab/apply-network-modifications-asynchronously
branch
from
b983251 to
3155bd0
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
2 participants
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.