Use the modification applicability per root network tag - #1053
Conversation
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change replaces study-side excluded-modification storage with root-network-tag applicability. It updates duplication, build, application-context, authorization, and voltage-init flows, and adds integration coverage for applicability behavior. ChangesRoot-network applicability migration
Sequence Diagram(s)sequenceDiagram
participant StudyController
participant StudyService
participant DirectoryService
participant NetworkModificationService
participant NetworkModificationServer
StudyController->>StudyService: request applicability update
StudyService->>DirectoryService: check WRITE permission
StudyService->>NetworkModificationService: update modification UUIDs and rootNetworkTag
NetworkModificationService->>NetworkModificationServer: PUT applicability state
Suggested reviewers: Mergeability Score: ⚪ Minimal · up to The PR changes how modification applicability is stored and exposed, with no actionable merge-blocking risk currently identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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.
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/study/server/VoltageInitTest.java (1)
422-423: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTighten the applicability request assertion.
At Lines [422]-[423], the dispatcher accepts any query string and any request body for the applicability endpoint.
At Lines [894]-[900],
allMatchonly checks that the five requests belong to an allowlist. It does not require an applicability request.At Lines [908]-[914], Mockito verifies the client method arguments before serialization, not the wire payload. Capture the
RecordedRequestand assert exactly onePUTcontains the second root-network tag,VOLTAGE_INIT_MODIFICATION_UUID, andfalse.Also applies to: 894-900, 908-914
🤖 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/VoltageInitTest.java` around lines 422 - 423, In VoltageInitTest, tighten the mock dispatcher branch for /v1/network-modifications/root-network-applicability so it validates the PUT request body instead of accepting any query or payload. Update the request assertions around the allMatch verification to require exactly one applicability PUT, capture its RecordedRequest, and assert the serialized payload contains the second root-network tag, VOLTAGE_INIT_MODIFICATION_UUID, and false; retain the existing checks for the other requests.
🤖 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.
Nitpick comments:
In `@src/test/java/org/gridsuite/study/server/VoltageInitTest.java`:
- Around line 422-423: In VoltageInitTest, tighten the mock dispatcher branch
for /v1/network-modifications/root-network-applicability so it validates the PUT
request body instead of accepting any query or payload. Update the request
assertions around the allMatch verification to require exactly one applicability
PUT, capture its RecordedRequest, and assert the serialized payload contains the
second root-network tag, VOLTAGE_INIT_MODIFICATION_UUID, and false; retain the
existing checks for the other requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aeb0b844-b27d-4001-8c96-1807a95ddf5d
📒 Files selected for processing (15)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/dto/BuildInfos.javasrc/main/java/org/gridsuite/study/server/dto/modification/ModificationApplicationContext.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/dto/ExcludedNetworkModifications.javasrc/main/java/org/gridsuite/study/server/networkmodificationtree/entities/RootNetworkNodeInfoEntity.javasrc/main/java/org/gridsuite/study/server/repository/rootnetwork/RootNetworkNodeInfoRepository.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationService.javasrc/main/java/org/gridsuite/study/server/service/NetworkModificationTreeService.javasrc/main/java/org/gridsuite/study/server/service/RootNetworkNodeInfoService.javasrc/main/java/org/gridsuite/study/server/service/RootNetworkService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/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/rootnetworks/RootNetworkApplicabilityTest.java
💤 Files with no reviewable changes (3)
- src/main/java/org/gridsuite/study/server/networkmodificationtree/dto/ExcludedNetworkModifications.java
- src/main/java/org/gridsuite/study/server/networkmodificationtree/entities/RootNetworkNodeInfoEntity.java
- src/test/java/org/gridsuite/study/server/rootnetworks/ModificationToExcludeTest.java
…fication-applicability
The network modification server names it after the applicability it carries, not after the modification wide activation flag. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
The endpoint sets the applicability of modifications on one root network, not their global activation flag, so activated was ambiguous. The value now keeps the same name from the front-end call down to the network modification server. Also name the two root network tags of RootNetworkApplicabilityTest after their rank rather than their position. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
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 (2)
src/main/java/org/gridsuite/study/server/service/StudyService.java (2)
1869-1881: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate root-network ownership before resolving the tag.
The controller validates
nodeUuid, but the shown path does not validate thatrootNetworkUuidbelongs tostudyUuid. This service only checks node ownership at Line 1874.rootNetworkService.getRootNetworkTag(rootNetworkUuid)can therefore resolve a root network from another study beforeupdateRootNetworkApplicabilitychanges its applicability. CallrootNetworkService.assertIsRootNetworkInStudy(studyUuid, rootNetworkUuid)orassertIsRootNetworkAndNodeInStudy(...)before this update.🤖 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 1869 - 1881, In updateNetworkModificationsApplicabilityInRootNetwork, validate that rootNetworkUuid belongs to studyUuid before resolving its tag or updating applicability. Add rootNetworkService.assertIsRootNetworkInStudy(studyUuid, rootNetworkUuid), or the combined assertIsRootNetworkAndNodeInStudy equivalent, before getRootNetworkTag and updateRootNetworkApplicability.
2617-2619: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake non-target applicability updates atomic or recoverable.
updateRootNetworkApplicabilitysends one remote request per root-network tag. If a later request fails, earlier changes remain, the local transaction cannot roll them back, andresetModificationsGroupUuidis not called. Use a batch endpoint or a safe compensation/retry workflow. Add a mid-loop failure test.🤖 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 2617 - 2619, Make the root-network applicability updates in the flow containing createdModificationUuids and rootNetworkTagsToDeactivate atomic or recoverable: replace the per-tag updateRootNetworkApplicability calls with a batch operation, or add compensation/retry handling that restores earlier successful updates and ensures resetModificationsGroupUuid is invoked on failure. Add a test that simulates a failure during the loop and verifies no partial state remains.
🤖 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/StudyService.java`:
- Around line 1869-1881: In
updateNetworkModificationsApplicabilityInRootNetwork, validate that
rootNetworkUuid belongs to studyUuid before resolving its tag or updating
applicability. Add rootNetworkService.assertIsRootNetworkInStudy(studyUuid,
rootNetworkUuid), or the combined assertIsRootNetworkAndNodeInStudy equivalent,
before getRootNetworkTag and updateRootNetworkApplicability.
- Around line 2617-2619: Make the root-network applicability updates in the flow
containing createdModificationUuids and rootNetworkTagsToDeactivate atomic or
recoverable: replace the per-tag updateRootNetworkApplicability calls with a
batch operation, or add compensation/retry handling that restores earlier
successful updates and ensures resetModificationsGroupUuid is invoked on
failure. Add a test that simulates a failure during the loop and verifies no
partial state remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0845892e-6f4c-443d-875b-10264d25a9d4
📒 Files selected for processing (6)
src/main/java/org/gridsuite/study/server/controller/StudyController.javasrc/main/java/org/gridsuite/study/server/service/RebuildNodeService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.javasrc/test/java/org/gridsuite/study/server/NetworkModificationTest.javasrc/test/java/org/gridsuite/study/server/rootnetworks/RootNetworkApplicabilityTest.javasrc/test/java/org/gridsuite/study/server/studycontroller/StudyControllerRebuildNodeTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/org/gridsuite/study/server/NetworkModificationTest.java
- src/test/java/org/gridsuite/study/server/rootnetworks/RootNetworkApplicabilityTest.java
- src/main/java/org/gridsuite/study/server/controller/StudyController.java
…fication-applicability
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 (2)
src/main/java/org/gridsuite/study/server/service/StudyService.java (2)
1877-1890: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate
rootNetworkUuidagainststudyUuidbefore updating applicability.The controller checks only the node. The service reads the root-network tag without checking ownership. Call
assertIsRootNetworkAndNodeInStudy(studyUuid, rootNetworkUuid, nodeUuid)at the start of this service method, before side effects.🤖 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 1877 - 1890, At the start of updateNetworkModificationsApplicabilityInRootNetwork, call assertIsRootNetworkAndNodeInStudy(studyUuid, rootNetworkUuid, nodeUuid) before retrieving children, verifying modifications, or emitting notifications. Preserve the existing applicability update flow after this validation.
945-946: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the battery DC power-factor parameter.
Line [945] initializes the battery map with
QUERY_PARAM_DC_POWERFACTOR. Line [964] replaces that map with a new map containing onlyQUERY_PARAM_LOAD_REGULATING_TERMINALS. Battery map-data requests therefore lose the DC power factor.Add the regulating-terminal parameter to the existing mutable battery map, or define both parameters in one map.
Proposed fix
- optionalParameters.put(String.valueOf(ElementType.BATTERY), - Map.of( - InfoTypeParameters.QUERY_PARAM_LOAD_REGULATING_TERMINALS, - String.valueOf(studyEntity.getSpreadsheetParameters().isSpreadsheetLoadBatteryRegulatingTerminal()))); + optionalParameters.get(String.valueOf(ElementType.BATTERY)).put( + InfoTypeParameters.QUERY_PARAM_LOAD_REGULATING_TERMINALS, + String.valueOf(studyEntity.getSpreadsheetParameters().isSpreadsheetLoadBatteryRegulatingTerminal()));Also applies to: 964-967
🤖 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 945 - 946, Update the battery parameter map initialization and subsequent handling so the existing QUERY_PARAM_DC_POWERFACTOR entry is preserved while adding QUERY_PARAM_LOAD_REGULATING_TERMINALS to the same map. Do not replace the battery map with a new map containing only the regulating-terminal parameter; ensure battery map-data requests include both parameters.
🤖 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/StudyService.java`:
- Around line 1877-1890: At the start of
updateNetworkModificationsApplicabilityInRootNetwork, call
assertIsRootNetworkAndNodeInStudy(studyUuid, rootNetworkUuid, nodeUuid) before
retrieving children, verifying modifications, or emitting notifications.
Preserve the existing applicability update flow after this validation.
- Around line 945-946: Update the battery parameter map initialization and
subsequent handling so the existing QUERY_PARAM_DC_POWERFACTOR entry is
preserved while adding QUERY_PARAM_LOAD_REGULATING_TERMINALS to the same map. Do
not replace the battery map with a new map containing only the
regulating-terminal parameter; ensure battery map-data requests include both
parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c1575af4-32e0-4336-90be-a13a4fd8fc70
📒 Files selected for processing (2)
src/main/java/org/gridsuite/study/server/service/NetworkModificationService.javasrc/main/java/org/gridsuite/study/server/service/StudyService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/gridsuite/study/server/service/NetworkModificationService.java
Static import the Mockito methods and keep a single throwing invocation in the assertThrows lambda. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
|
1 participant
Depends on gridsuite/network-modification-server#867.
The applicabilities per root network are now held by the modifications themselves, keyed by root network tag, so study-server stops storing them:
RootNetworkNodeInfoEntity.modificationsUuidsToExcludeand everything that maintained it are removed: the copy on paste, the transfer on cut, the tag matching when a node or a study is duplicated, and the purge when modifications are deleted. A modification now keeps its applicability wherever it goes.BuildInfoscarry the tag of the root network instead of the excluded modification uuids.GET /studies/{studyUuid}/nodes/{nodeUuid}/excluded-network-modificationsis replaced byGET /studies/{studyUuid}/nodes/{nodeUuid}/network-modifications/applicability, which relays the applicability per tag of every modification of the node.PUT /studies/{studyUuid}/root-networks/{rootNetworkUuid}/nodes/{nodeUuid}/network-modificationstakes anapplicablequery parameter instead ofactivated: it sets the applicability on one root network, not the global activation flag. The parameter also discriminates that mapping, so Handle the modification applicability per root network tag commons-ui#1281 must go with it.The applicabilities stored so far are not migrated: every modification goes back to applicable on every root network. Only production database will be migrated.