Add a modification reference as a new modification type#187
Open
SlimaneAmar wants to merge 1 commit intomainfrom
Open
Add a modification reference as a new modification type#187SlimaneAmar wants to merge 1 commit intomainfrom
SlimaneAmar wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds a ModificationReference feature: new enum value Changes
Sequence DiagramsequenceDiagram
participant Client
participant ModificationReference
participant ReferencedModification
participant Network
participant IFilterService
participant ILoadFlowService
participant ReportNode
Client->>ModificationReference: build & call check(network)
ModificationReference->>Network: check(network)
Client->>ModificationReference: initApplicationContext(filter, loadFlow)
ModificationReference->>IFilterService: store ref
ModificationReference->>ILoadFlowService: store ref
Client->>ModificationReference: apply(network, namingStrategy, reportNode)
ModificationReference->>ReferencedModification: referenceInfos.toModification()
ModificationReference->>ReferencedModification: initApplicationContext(IFilterService, ILoadFlowService)
ModificationReference->>ReferencedModification: check(network)
ModificationReference->>ReferencedModification: apply(network, namingStrategy, subReportNode)
ReferencedModification->>Network: mutate network
ReferencedModification->>ReportNode: append sub-report
ReferencedModification-->>ModificationReference: complete
ModificationReference-->>Client: return
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 5
🤖 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/modification/dto/ModificationReferenceInfos.java`:
- Around line 55-63: ModificationReferenceInfos must validate its required
payloads before delegating: add non-null checks for referenceInfos and
referenceType in the DTO (e.g., at the start of toModification() and
createSubReportNode()) and throw a clear runtime exception
(IllegalStateException or Objects.requireNonNull with message) instead of
silently mapping null referenceType to DIRECTORY; then use the validated
referenceType to select the template (Type.SAMPLE ->
"network.modification.sample.reference.apply", otherwise
"network.modification.directory.reference.apply") and proceed to construct the
ModificationReference and ReportNode as before.
In
`@src/main/java/org/gridsuite/modification/modifications/ModificationReference.java`:
- Around line 24-25: ModificationReference is delegating to other modifications
but never stores the application context services, so filterService and
loadFlowService remain null; update ModificationReference's
initApplicationContext(...) to assign the passed IFilterService and
ILoadFlowService to the protected fields (filterService, loadFlowService) and
ensure any delegation path (the code that forwards to the wrapped/delegated
modification around the current forwarding at ~line 40) calls the delegated
modification's initApplicationContext(...) with the same services so the
delegate receives the captured context.
- Around line 45-46: The getName method in class ModificationReference returns
the misspelled string "ModificationRefence"; update the return value in the
getName() method to the correct "ModificationReference" so the method signature
and returned name match the class and other references.
In `@src/main/java/org/gridsuite/modification/ModificationType.java`:
- Line 14: The TabularBaseInfos formatting falls back to "equipments of unknown
type" for the newly added enum value ModificationType.MODIFICATION_REFERENCE;
update the equipment-type mapping in
src/main/java/org/gridsuite/modification/dto/tabular/TabularBaseInfos (the
method that maps ModificationType to display labels) to add a case for
MODIFICATION_REFERENCE and return the appropriate user-facing label (consistent
with other mappings) instead of the default unknown-type string so the new enum
renders correctly in tabular views.
In
`@src/test/java/org/gridsuite/modification/modifications/ModificationReferenceTest.java`:
- Around line 57-58: The testCreationModificationMessage currently calls
assertNotNull with ModificationType.MODIFICATION_REFERENCE.name() and
modificationInfos.getMessageType(), which doesn’t verify the actual value;
replace that assertion with
assertEquals(ModificationType.MODIFICATION_REFERENCE.name(),
modificationInfos.getMessageType()) in the testCreationModificationMessage
method so the test asserts the message type is exactly the expected value.
🪄 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: 14c0c97c-6d2a-46d6-a9b5-17df814d1948
📒 Files selected for processing (5)
src/main/java/org/gridsuite/modification/ModificationType.javasrc/main/java/org/gridsuite/modification/dto/ModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.javasrc/main/java/org/gridsuite/modification/modifications/ModificationReference.javasrc/test/java/org/gridsuite/modification/modifications/ModificationReferenceTest.java
SlimaneAmar
force-pushed
the
shared-modification
branch
3 times, most recently
from
5b58389 to
244a439
Compare
SlimaneAmar
force-pushed
the
shared-modification
branch
from
244a439 to
c19317b
Compare
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java (1)
62-64:⚠️ Potential issue | 🟠 MajorAvoid silent fallback to DIRECTORY when report template type is invalid/unvalidated.
Line 63 maps any non-
SAMPLEvalue (includingnull) to DIRECTORY. Please validate first (or callcheck()), then use an exhaustiveswitchto prevent accidental misreporting.Proposed fix
`@Override` public ReportNode createSubReportNode(ReportNode reportNode) { - return reportNode.newReportNode().withMessageTemplate(referenceType == Type.SAMPLE ? "network.modification.sample.reference.apply" : "network.modification.directory.reference.apply").add(); + check(); + String template = switch (referenceType) { + case SAMPLE -> "network.modification.sample.reference.apply"; + case DIRECTORY -> "network.modification.directory.reference.apply"; + }; + return reportNode.newReportNode().withMessageTemplate(template).add(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java` around lines 62 - 64, createSubReportNode silently treats any non-SAMPLE referenceType (including null) as DIRECTORY; first validate referenceType (call or inline the existing check() or throw IllegalStateException on null/invalid) and then replace the ternary with an exhaustive switch on referenceType (switch over Type enum with cases SAMPLE and DIRECTORY and a default that throws) to avoid accidental fallback/misreporting; update createSubReportNode to perform validation before constructing the ReportNode and to use the switch so unknown values fail fast.
🧹 Nitpick comments (1)
src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java (1)
67-69: Prefer immutable empty map for no-value payloads.
new HashMap<>()allocates a mutable object each call;Map.of()is cheaper and expresses intent better.Proposed refactor
`@Override` public Map<String, String> getMapMessageValues() { - return new HashMap<>(); + return Map.of(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java` around lines 67 - 69, The getMapMessageValues method in class ModificationReferenceInfos currently returns a newly allocated mutable HashMap on each call; replace that with an immutable empty map (e.g., return Map.of()) to avoid unnecessary allocations and make the intent explicit—update the body of ModificationReferenceInfos.getMapMessageValues() to return an immutable empty map instance instead of new HashMap<>().
🤖 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/modification/dto/ModificationReferenceInfos.java`:
- Around line 62-64: createSubReportNode silently treats any non-SAMPLE
referenceType (including null) as DIRECTORY; first validate referenceType (call
or inline the existing check() or throw IllegalStateException on null/invalid)
and then replace the ternary with an exhaustive switch on referenceType (switch
over Type enum with cases SAMPLE and DIRECTORY and a default that throws) to
avoid accidental fallback/misreporting; update createSubReportNode to perform
validation before constructing the ReportNode and to use the switch so unknown
values fail fast.
---
Nitpick comments:
In
`@src/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.java`:
- Around line 67-69: The getMapMessageValues method in class
ModificationReferenceInfos currently returns a newly allocated mutable HashMap
on each call; replace that with an immutable empty map (e.g., return Map.of())
to avoid unnecessary allocations and make the intent explicit—update the body of
ModificationReferenceInfos.getMapMessageValues() to return an immutable empty
map instance instead of new HashMap<>().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9d1c774-8a13-4bc0-b6a7-2b38aec5cb1d
📒 Files selected for processing (9)
src/main/java/org/gridsuite/modification/ModificationType.javasrc/main/java/org/gridsuite/modification/dto/ModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.javasrc/main/java/org/gridsuite/modification/modifications/ModificationReference.javasrc/test/java/org/gridsuite/modification/modifications/AbstractNetworkModificationTest.javasrc/test/java/org/gridsuite/modification/modifications/CreateVoltageLevelSectionTest.javasrc/test/java/org/gridsuite/modification/modifications/LccCreationInNodeBreakerTest.javasrc/test/java/org/gridsuite/modification/modifications/ModificationReferenceTest.javasrc/test/java/org/gridsuite/modification/modifications/VoltageLevelTopologyModificationTest.java
✅ Files skipped from review due to trivial changes (5)
- src/main/java/org/gridsuite/modification/ModificationType.java
- src/test/java/org/gridsuite/modification/modifications/VoltageLevelTopologyModificationTest.java
- src/test/java/org/gridsuite/modification/modifications/LccCreationInNodeBreakerTest.java
- src/test/java/org/gridsuite/modification/modifications/CreateVoltageLevelSectionTest.java
- src/main/java/org/gridsuite/modification/modifications/ModificationReference.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/gridsuite/modification/modifications/ModificationReferenceTest.java
Contributor
Mathieu-Deharbe
left a comment
Mathieu-Deharbe
left a comment
There was a problem hiding this comment.
I didn't test anythting yet (except unit tests).
So just a few questions and discussions about naming.
Comment on lines
+39
to
+42
| public enum Type { | ||
| SAMPLE, | ||
| DIRECTORY, | ||
| } |
Contributor
There was a problem hiding this comment.
Can you precise here what a DIRECTORY modification reference is ?
Why SAMPLE ? It sounds like it is a reference to an example (sample means échantillon/exemple...). Shouldn't it be STANDARD, MODIFICATION, REGULAR maybe ?
| @Getter | ||
| @Setter | ||
| @ToString(callSuper = true) | ||
| @Schema(description = "Modification reference") |
Contributor
There was a problem hiding this comment.
The naming is fine but just to be sure : this is a modification but also a 'reference to another modification' in the same time right ? So if we decided to go full verbose (don't) we could say this is a ModificationReferenceModification.
Technically it can reference another ModificationReference and even itself ? I didn't see any check about this yet.
|
|
||
| @Override | ||
| public ReportNode createSubReportNode(ReportNode reportNode) { | ||
| return reportNode.newReportNode().withMessageTemplate(referenceType == Type.SAMPLE ? "network.modification.sample.reference.apply" : "network.modification.directory.reference.apply").add(); |
Contributor
There was a problem hiding this comment.
Shouldn't some localization text added for network.modification.sample.reference.apply and network.modification.directory.reference.apply ?
Comment on lines
+27
to
+29
| /** | ||
| * @author Slimane Amar <slimane.amar at rte-france.com> | ||
| */ |
Contributor
There was a problem hiding this comment.
It would be useful to precise somewhere what can be referenced. The jira precises only composites but it looks like anything can be referenced (of course if really any modifictation can and will be referenced, no need to precise anything, but should a modification reference be referenced by another modification reference ?)
|
|
||
| @Override | ||
| public Map<String, String> getMapMessageValues() { | ||
| return new HashMap<>(); |
Contributor
There was a problem hiding this comment.
Why are you overriding an already empty super function ? If I am reading right assignAttributes from ModificationReferenceEntity, this should never be used right ?
Comment on lines
+57
to
+59
| public String getName() { | ||
| return "ModificationReference"; | ||
| } |
Contributor
There was a problem hiding this comment.
Some classes deduce this name from the ModificationType like :
public String getName() {
return ModificationType.CREATE_VOLTAGE_LEVEL_SECTION.name();
}
So it could be :
Suggested change
| public String getName() { | |
| return "ModificationReference"; | |
| } | |
| public String getName() { | |
| return ModificationType.MODIFICATION_REFERENCE.name(); | |
| } |
Or if we want the class name why not : ?
Suggested change
| public String getName() { | |
| return "ModificationReference"; | |
| } | |
| public String getName() { | |
| return getClass().getSimpleName(); | |
| } |
| public void checkModification() { | ||
| Network network = getNetwork(); | ||
| CreateVoltageLevelSectionInfos voltageLevelSectionInfos = (CreateVoltageLevelSectionInfos) buildModification(); | ||
| voltageLevelSectionInfos.setBusbarIndex(1); |
Contributor
There was a problem hiding this comment.
Is there a specific reason to do this in this PR ?
| .referenceType(ModificationReferenceInfos.Type.SAMPLE) | ||
| .referenceId(UUID.randomUUID()) | ||
| .referenceInfos(modificationInfo) | ||
| .stashed(false) |
Contributor
There was a problem hiding this comment.
It is a default value :
Suggested change
| .stashed(false) |
|
|
||
| @Override | ||
| public void apply(Network network, NamingStrategy namingStrategy, ReportNode subReportNode) { | ||
| AbstractModification modification = modificationReferenceInfos.getReferenceInfos().toModification(); |
Contributor
There was a problem hiding this comment.
Shouldn't we check for the stash and activated of the ReferenceInfos here ?
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.
PR Summary