Open
Conversation
Signed-off-by: Etienne LESOT <etienne.lesot@rte-france.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:
📝 WalkthroughWalkthroughCaptures operational-limits context from both source lines before revert/merge operations, applies the revert modification, then computes and applies merged OperationalLimitsGroup instances and selected-group decisions to the replacing line, emitting detailed merge/delete messages into ReportNode. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client as DeleteAttachingLine / DeleteVoltageLevelOnLine
participant Network as Network
participant ModUtils as ModificationLimitsUtils
participant Reverter as RevertCreate/ConnectOnLine
participant Report as ReportNode
Client->>Network: read source lines (line1, line2) and their operational limits
Client->>ModUtils: call applyRevertModificationWithMergingOfLimits(line1Id,line2Id,replacingId,algo,subReport)
ModUtils->>Network: snapshot selectedOperationalLimitsGroupIds and groups for both sides
ModUtils->>Reverter: invoke provided revert algorithm (algo.apply)
Reverter->>Network: perform revert/merge and produce replacingLine
Reverter-->>ModUtils: return control
ModUtils->>Network: clear existing groups on replacingLine
ModUtils->>ModUtils: createMergedOperationalLimitsGroups(side1) — intersect IDs, merge CurrentLimits, reconcile properties, log deletions/mismatches
ModUtils->>ModUtils: createMergedOperationalLimitsGroups(side2)
ModUtils->>Network: attach merged groups and set selected group IDs on replacingLine
ModUtils->>Report: emit merge summary and detailed deletion/property-difference entries
Client->>Report: finalize modification report
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (2)
src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java (2)
40-41: PotentialConcurrentModificationExceptionwhen removing while iterating.The
forEachcreates an iterator over the collection whileremoveOperationalLimitsGroupmodifies the underlying collection. This may cause aConcurrentModificationExceptiondepending on the collection implementation.♻️ Safer approach: collect IDs first, then remove
// remove all limitsGroups as the modification does not do as wanted - mergedLine.getOperationalLimitsGroups1().forEach(group -> mergedLine.removeOperationalLimitsGroup1(group.getId())); - mergedLine.getOperationalLimitsGroups2().forEach(group -> mergedLine.removeOperationalLimitsGroup2(group.getId())); + List<String> groupIds1 = mergedLine.getOperationalLimitsGroups1().stream() + .map(OperationalLimitsGroup::getId).toList(); + groupIds1.forEach(mergedLine::removeOperationalLimitsGroup1); + List<String> groupIds2 = mergedLine.getOperationalLimitsGroups2().stream() + .map(OperationalLimitsGroup::getId).toList(); + groupIds2.forEach(mergedLine::removeOperationalLimitsGroup2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java` around lines 40 - 41, The current removal loops iterate mergedLine.getOperationalLimitsGroups1() and getOperationalLimitsGroups2() while calling mergedLine.removeOperationalLimitsGroup1(...) / removeOperationalLimitsGroup2(...), which can cause ConcurrentModificationException; fix by first collecting the group IDs into a separate List (e.g., via stream().map(Group::getId).collect(toList())) for each of getOperationalLimitsGroups1() and getOperationalLimitsGroups2(), then iterate those ID lists and call removeOperationalLimitsGroup1(id) and removeOperationalLimitsGroup2(id) respectively so removals do not mutate the collection being iterated.
52-57: Selected group not set when both lines have the same group but only one has it selected.Per PR description, limits should be merged when present on both lines. However, if
selectedGroupLine1Side1isnullbutselectedGroupLine2Side1is not (or vice versa), the selected group won't be set even though both lines have the group. Consider setting the selected group if the group exists and at least one line has it selected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java` around lines 52 - 57, The current checks only set mergedLine.setSelectedOperationalLimitsGroup1/Group2 when selectedGroupLine1Side1/Side2 is non-null and equals the other line's selected value, so if one line has the group selected and the other doesn't the merged selection is skipped; change the logic to pick the non-null selected group when either selectedGroupLine1Side1 or selectedGroupLine2Side1 is non-null (and likewise for Side2) but only after verifying the group exists on both source lines — e.g., compute a selected = selectedGroupLine1Side1 != null ? selectedGroupLine1Side1 : selectedGroupLine2Side1 and set mergedLine.setSelectedOperationalLimitsGroup1(selected) when selected != null and the same group is present on the other line (use the existing group-presence flags/variables for the two lines); apply the same change for setSelectedOperationalLimitsGroup2 using selectedGroupLine1Side2/selectedGroupLine2Side2.
🤖 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/utils/ModificationLimitsUtils.java`:
- Around line 174-185: The loop over temporaryLimitsOnLine2.keySet() uses the
wrong map in the existence check causing items only in line2 to never be treated
as deleted; change the condition to check
!temporaryLimitsOnLine1.containsKey(temporaryLimitName) so that when a key from
temporaryLimitsOnLine2 is absent in temporaryLimitsOnLine1 you call
logDeletedTemporaryLimits with the removedTemporaryLimit
(temporaryLimitsOnLine2.get(temporaryLimitName)); keep the existing
duration-comparison branch that compares temporaryLimitLine1 and
temporaryLimitLine2 using temporaryLimitsOnLine1.get(...) and
temporaryLimitsOnLine2.get(...).
In `@src/main/resources/org/gridsuite/modification/reports.properties`:
- Line 239: The message value for the key network.modification.propertyChanged
contains an accidental character sequence "-}" between ${from} and ${to}; update
the string to restore the intended arrow "->" so it reads "... ${from} -> ${to}"
and remove the stray brace. Also search for the same typo in the other affected
key mentioned (around the duplicate at line 309) and make the identical
correction in that property entry.
In `@src/test/java/org/gridsuite/modification/utils/TestUtils.java`:
- Around line 175-188: In TestUtils.testReportNode, the InputStream refStream
obtained via TestUtils.class.getResourceAsStream(reportsFile) is not closed;
wrap the resource use in a try-with-resources (e.g., try (InputStream refStream
= TestUtils.class.getResourceAsStream(reportsFile)) { ... }) so the stream is
automatically closed after reading, keep the assertNotNull check inside the try
(or handle a null by failing early), and compute expected using
ByteStreams.toByteArray(refStream) within that try block before comparing
normalized strings.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java`:
- Around line 40-41: The current removal loops iterate
mergedLine.getOperationalLimitsGroups1() and getOperationalLimitsGroups2() while
calling mergedLine.removeOperationalLimitsGroup1(...) /
removeOperationalLimitsGroup2(...), which can cause
ConcurrentModificationException; fix by first collecting the group IDs into a
separate List (e.g., via stream().map(Group::getId).collect(toList())) for each
of getOperationalLimitsGroups1() and getOperationalLimitsGroups2(), then iterate
those ID lists and call removeOperationalLimitsGroup1(id) and
removeOperationalLimitsGroup2(id) respectively so removals do not mutate the
collection being iterated.
- Around line 52-57: The current checks only set
mergedLine.setSelectedOperationalLimitsGroup1/Group2 when
selectedGroupLine1Side1/Side2 is non-null and equals the other line's selected
value, so if one line has the group selected and the other doesn't the merged
selection is skipped; change the logic to pick the non-null selected group when
either selectedGroupLine1Side1 or selectedGroupLine2Side1 is non-null (and
likewise for Side2) but only after verifying the group exists on both source
lines — e.g., compute a selected = selectedGroupLine1Side1 != null ?
selectedGroupLine1Side1 : selectedGroupLine2Side1 and set
mergedLine.setSelectedOperationalLimitsGroup1(selected) when selected != null
and the same group is present on the other line (use the existing group-presence
flags/variables for the two lines); apply the same change for
setSelectedOperationalLimitsGroup2 using
selectedGroupLine1Side2/selectedGroupLine2Side2.
🪄 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: 567defb6-36dc-4b54-9dd9-440119e206aa
📒 Files selected for processing (10)
src/main/java/org/gridsuite/modification/modifications/DeleteAttachingLine.javasrc/main/java/org/gridsuite/modification/modifications/DeleteVoltageLevelOnLine.javasrc/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.javasrc/main/resources/org/gridsuite/modification/reports.propertiessrc/test/java/org/gridsuite/modification/modifications/DeleteAttachingLineTest.javasrc/test/java/org/gridsuite/modification/modifications/DeleteVoltageLevelOnLineTest.javasrc/test/java/org/gridsuite/modification/utils/MergingLimitsTest.javasrc/test/java/org/gridsuite/modification/utils/TestUtils.javasrc/test/resources/reportNode/delete-attaching-line-report.txtsrc/test/resources/reportNode/delete-voltagelevel-on-line-report.txt
src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java
Show resolved
Hide resolved
src/main/resources/org/gridsuite/modification/reports.properties
Outdated
Show resolved
Hide resolved
Signed-off-by: Etienne LESOT <etienne.lesot@rte-france.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/org/gridsuite/modification/utils/MergingLimitsTest.java (1)
84-89: Document precondition:property1must exist on both source lines' groups.The test asserts
property1exists with value"value1"(lines 86-87), but this property is not set anywhere in this method. The calling test must ensure the network hasproperty1already configured on bothl1andl2'sgroup1with identical values, otherwise this assertion will fail unexpectedly.Consider adding a brief comment or setup assertion to make this precondition explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/modification/utils/MergingLimitsTest.java` around lines 84 - 89, The test MergingLimitsTest currently asserts group1.get().getProperty("property1") equals "value1" but never ensures that property1 was set on the source groups (the groups for l1 and l2), which can make the assertion flaky; update the test setup to explicitly set or assert the precondition that property1 exists with the value "value1" on both sources before proceeding (e.g., set property1 on the groups used by l1 and l2 or add an explicit assert that group1 for each source contains property1 == "value1"), referencing the existing group1/getProperty calls to locate where to add the setup/precondition check.pom.xml (1)
58-61: Consider usinguser.countryinstead ofuser.region.The system property
user.regionis deprecated. For setting the locale, use-Duser.country=USinstead, which is the standard property for locale configuration.Suggested fix
- <argLine>@{argLine} -Duser.language=en -Duser.region=US -XX:+EnableDynamicAgentLoading</argLine> + <argLine>@{argLine} -Duser.language=en -Duser.country=US -XX:+EnableDynamicAgentLoading</argLine>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 58 - 61, Update the Maven surefire/failsafe argLine configuration to use the standard locale system property: replace "-Duser.region=US" with "-Duser.country=US" inside the <configuration> argLine entry so the default locale flags (the argLine that currently includes "-Duser.language=en -Duser.region=US -XX:+EnableDynamicAgentLoading") use the non-deprecated property.
🤖 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/test/java/org/gridsuite/modification/utils/MergingLimitsTest.java`:
- Around line 78-82: In ModificationLimitsUtils.mergedCurrentLimits there is a
logic bug: the loop that intends to log temporary limits present on line2 but
not on line1 checks temporaryLimitsOnLine2.containsKey(temporaryLimitName)
(using the same map being iterated) making the branch unreachable; change the
condition to check temporaryLimitsOnLine1 (i.e., if
(!temporaryLimitsOnLine1.containsKey(temporaryLimitName))) so the method
correctly logs limits that exist on line2 but were deleted from or missing on
line1, and run/adjust unit tests that exercise the deleted-on-line2 scenario.
---
Nitpick comments:
In `@pom.xml`:
- Around line 58-61: Update the Maven surefire/failsafe argLine configuration to
use the standard locale system property: replace "-Duser.region=US" with
"-Duser.country=US" inside the <configuration> argLine entry so the default
locale flags (the argLine that currently includes "-Duser.language=en
-Duser.region=US -XX:+EnableDynamicAgentLoading") use the non-deprecated
property.
In `@src/test/java/org/gridsuite/modification/utils/MergingLimitsTest.java`:
- Around line 84-89: The test MergingLimitsTest currently asserts
group1.get().getProperty("property1") equals "value1" but never ensures that
property1 was set on the source groups (the groups for l1 and l2), which can
make the assertion flaky; update the test setup to explicitly set or assert the
precondition that property1 exists with the value "value1" on both sources
before proceeding (e.g., set property1 on the groups used by l1 and l2 or add an
explicit assert that group1 for each source contains property1 == "value1"),
referencing the existing group1/getProperty calls to locate where to add the
setup/precondition check.
🪄 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: 960f00ae-4d33-4f5a-89a7-8ccce47132a3
📒 Files selected for processing (4)
pom.xmlsrc/test/java/org/gridsuite/modification/utils/MergingLimitsTest.javasrc/test/resources/reportNode/delete-attaching-line-report.txtsrc/test/resources/reportNode/delete-voltagelevel-on-line-report.txt
✅ Files skipped from review due to trivial changes (2)
- src/test/resources/reportNode/delete-voltagelevel-on-line-report.txt
- src/test/resources/reportNode/delete-attaching-line-report.txt
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java (1)
179-191:⚠️ Potential issue | 🔴 CriticalCritical bug: Wrong map checked causes temporary limits from line2 to never be logged as deleted, plus potential NPE.
This issue was previously flagged and remains unfixed. Line 181 checks
!temporaryLimitsOnLine2.containsKey(temporaryLimitName)while iterating overtemporaryLimitsOnLine2.keySet()— this condition is alwaysfalse.Additionally, because the
elsebranch at line 184 is always taken,temporaryLimitsOnLine1.get(temporaryLimitName)on line 185 returnsnullwhen the key exists only inline2, causing aNullPointerExceptionon line 187.🐛 Proposed fix
// logs for limit set deleted on line2 for (String temporaryLimitName : temporaryLimitsOnLine2.keySet()) { - if (!temporaryLimitsOnLine2.containsKey(temporaryLimitName)) { + if (!temporaryLimitsOnLine1.containsKey(temporaryLimitName)) { LoadingLimits.TemporaryLimit removedTemporaryLimit = temporaryLimitsOnLine2.get(temporaryLimitName); logDeletedTemporaryLimits(reportNode, removedTemporaryLimit, operationalLimitsGroupId, line2Id, line1Id, newLineId); - } else { - LoadingLimits.TemporaryLimit temporaryLimitLine1 = temporaryLimitsOnLine1.get(temporaryLimitName); - LoadingLimits.TemporaryLimit temporaryLimitLine2 = temporaryLimitsOnLine2.get(temporaryLimitName); - if (temporaryLimitLine1.getAcceptableDuration() != temporaryLimitLine2.getAcceptableDuration()) { - logDeletedTemporaryLimits(reportNode, temporaryLimitLine2, operationalLimitsGroupId, line2Id, line1Id, newLineId); - } } + // Note: Duration mismatch logging is already handled in the first loop (lines 167-168) } adder.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/utils/ModificationLimitsUtils.java` around lines 179 - 191, The loop over temporaryLimitsOnLine2 incorrectly checks temporaryLimitsOnLine2.containsKey(...) causing the deleted-from-line2 case to never fire and a potential NPE when temporaryLimitsOnLine1.get(...) returns null; change the existence check to if (!temporaryLimitsOnLine1.containsKey(temporaryLimitName)) so you log the removed limit from temporaryLimitsOnLine2 (use LoadingLimits.TemporaryLimit removedTemporaryLimit = temporaryLimitsOnLine2.get(temporaryLimitName) and call logDeletedTemporaryLimits(...)), and keep the else branch to retrieve both temporaryLimitLine1 and temporaryLimitLine2 and compare getAcceptableDuration(); this ensures you only call temporaryLimitsOnLine1.get(...) when the key exists and avoids the NPE.
🧹 Nitpick comments (2)
src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java (2)
195-206: Missing severity specification in report node.Unlike other logging methods in this class (lines 40, 127, 143), this method does not call
withSeverity()beforeadd(). This inconsistency may result in a different default severity for deleted temporary limits messages.🔧 Proposed fix: add severity for consistency
reportNode.newReportNode() .withMessageTemplate("network.modification.temporaryLimitsDeletedAfterMerge") .withUntypedValue("limit_name", temporaryLimit.getName()) .withUntypedValue("tempo", temporaryLimit.getAcceptableDuration()) .withUntypedValue("limitset_name", operationalLimitsGroupName) .withUntypedValue("line_with_limit", lineWithTemporaryLimit) .withUntypedValue("line_without_limit", lineWithoutTemporaryLimit) .withUntypedValue("replacing_line", newLineId) + .withSeverity(TypedValue.INFO_SEVERITY) .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/utils/ModificationLimitsUtils.java` around lines 195 - 206, The logDeletedTemporaryLimits method is missing a severity on the ReportNode chain, causing inconsistency with other methods; update the report chain in logDeletedTemporaryLimits to call withSeverity(...) (use the same severity used by the other methods in this class, e.g., the severity passed in lines like the methods around withSeverity(...) calls) before calling add() so the deleted temporary limits message uses the same severity level as the other report entries.
43-45: Consider collecting IDs before removing to improve code clarity.While the current iteration-removal pattern works correctly, collecting group IDs before removal is safer and more explicit:
Suggested refactoring
// remove all limitsGroups as the modification does not do as wanted - mergedLine.getOperationalLimitsGroups1().forEach(group -> mergedLine.removeOperationalLimitsGroup1(group.getId())); - mergedLine.getOperationalLimitsGroups2().forEach(group -> mergedLine.removeOperationalLimitsGroup2(group.getId())); + List<String> groupIds1 = mergedLine.getOperationalLimitsGroups1().stream() + .map(OperationalLimitsGroup::getId).toList(); + groupIds1.forEach(mergedLine::removeOperationalLimitsGroup1); + List<String> groupIds2 = mergedLine.getOperationalLimitsGroups2().stream() + .map(OperationalLimitsGroup::getId).toList(); + groupIds2.forEach(mergedLine::removeOperationalLimitsGroup2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java` around lines 43 - 45, The current code iterates over mergedLine.getOperationalLimitsGroups1() and getOperationalLimitsGroups2() while removing from mergedLine, which can be clearer and safer by first collecting the group IDs and then removing by ID; change the blocks using mergedLine.getOperationalLimitsGroups1().forEach(group -> mergedLine.removeOperationalLimitsGroup1(group.getId())) and mergedLine.getOperationalLimitsGroups2().forEach(group -> mergedLine.removeOperationalLimitsGroup2(group.getId())) to first build lists of IDs (e.g., List<String> ids1 = mergedLine.getOperationalLimitsGroups1().stream().map(Group::getId).collect(...)) and then iterate over those ID lists to call mergedLine.removeOperationalLimitsGroup1(id) and mergedLine.removeOperationalLimitsGroup2(id).
🤖 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/utils/ModificationLimitsUtils.java`:
- Around line 111-130: The current merge loop only iterates
groupOnLine1.getPropertyNames(), so properties that exist only on groupOnLine2
(or only on groupOnLine1) are silently dropped; change the logic to iterate over
the union of property names from groupOnLine1 and groupOnLine2 (e.g., collect
both property name sets), and for each propertyName: fetch valueLine1 via
groupOnLine1.getProperty(propertyName) and valueLine2 via
groupOnLine2.getProperty(propertyName), set newGroup.setProperty when both
non-null and equal, and otherwise create a report via reportNode.newReportNode()
using the same "network.modification.propertyOfLimitsGroupDeletedAfterMerge"
template but correctly populate property_value_A/B and line_A/B for cases where
one side is null (unique-to-one-line) as well as differing values; use the same
TypedValue.INFO_SEVERITY and replacing_line/newGroup.getId() fields to keep
behavior consistent.
---
Duplicate comments:
In `@src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java`:
- Around line 179-191: The loop over temporaryLimitsOnLine2 incorrectly checks
temporaryLimitsOnLine2.containsKey(...) causing the deleted-from-line2 case to
never fire and a potential NPE when temporaryLimitsOnLine1.get(...) returns
null; change the existence check to if
(!temporaryLimitsOnLine1.containsKey(temporaryLimitName)) so you log the removed
limit from temporaryLimitsOnLine2 (use LoadingLimits.TemporaryLimit
removedTemporaryLimit = temporaryLimitsOnLine2.get(temporaryLimitName) and call
logDeletedTemporaryLimits(...)), and keep the else branch to retrieve both
temporaryLimitLine1 and temporaryLimitLine2 and compare getAcceptableDuration();
this ensures you only call temporaryLimitsOnLine1.get(...) when the key exists
and avoids the NPE.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java`:
- Around line 195-206: The logDeletedTemporaryLimits method is missing a
severity on the ReportNode chain, causing inconsistency with other methods;
update the report chain in logDeletedTemporaryLimits to call withSeverity(...)
(use the same severity used by the other methods in this class, e.g., the
severity passed in lines like the methods around withSeverity(...) calls) before
calling add() so the deleted temporary limits message uses the same severity
level as the other report entries.
- Around line 43-45: The current code iterates over
mergedLine.getOperationalLimitsGroups1() and getOperationalLimitsGroups2() while
removing from mergedLine, which can be clearer and safer by first collecting the
group IDs and then removing by ID; change the blocks using
mergedLine.getOperationalLimitsGroups1().forEach(group ->
mergedLine.removeOperationalLimitsGroup1(group.getId())) and
mergedLine.getOperationalLimitsGroups2().forEach(group ->
mergedLine.removeOperationalLimitsGroup2(group.getId())) to first build lists of
IDs (e.g., List<String> ids1 =
mergedLine.getOperationalLimitsGroups1().stream().map(Group::getId).collect(...))
and then iterate over those ID lists to call
mergedLine.removeOperationalLimitsGroup1(id) and
mergedLine.removeOperationalLimitsGroup2(id).
🪄 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: c31af441-bef2-43dd-bdcf-5062c9299ad9
📒 Files selected for processing (3)
pom.xmlsrc/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.javasrc/test/java/org/gridsuite/modification/utils/MergingLimitsTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/gridsuite/modification/utils/MergingLimitsTest.java
src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java
Show resolved
Hide resolved
Signed-off-by: Etienne LESOT <etienne.lesot@rte-france.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java (1)
81-83: Consider defensive copying when removing elements during iteration for safer code.While the current
forEachpattern may work becausegetOperationalLimitsGroups1()likely returns an unmodifiable collection view, the proposed fix provides better defensive programming. The safer approach would collect group IDs first before removal to eliminate any ambiguity about concurrent modification behavior:Proposed improvement for defensive coding
// remove all limitsGroups as the modification does not do as wanted - mergedLine.getOperationalLimitsGroups1().forEach(group -> mergedLine.removeOperationalLimitsGroup1(group.getId())); - mergedLine.getOperationalLimitsGroups2().forEach(group -> mergedLine.removeOperationalLimitsGroup2(group.getId())); + mergedLine.getOperationalLimitsGroups1().stream() + .map(OperationalLimitsGroup::getId) + .toList() + .forEach(mergedLine::removeOperationalLimitsGroup1); + + mergedLine.getOperationalLimitsGroups2().stream() + .map(OperationalLimitsGroup::getId) + .toList() + .forEach(mergedLine::removeOperationalLimitsGroup2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java` around lines 81 - 83, Collect the IDs of the groups before mutating the collections to avoid potential concurrent-modification issues: iterate mergedLine.getOperationalLimitsGroups1() and mergedLine.getOperationalLimitsGroups2() to build lists of group IDs, then call mergedLine.removeOperationalLimitsGroup1(id) and mergedLine.removeOperationalLimitsGroup2(id) using those ID lists instead of removing inside the forEach over the original collections.
🤖 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/utils/ModificationLimitsUtils.java`:
- Around line 120-125: When only one of the two Optional<CurrentLimits>
(currentLimitsLine1 or currentLimitsLine2) is present the code currently drops
the limits; update the logic so that: if both are present call
mergedCurrentLimits(currentLimitsLine1.get(), currentLimitsLine2.get(),
newGroup.newCurrentLimits(), operationalLimitGroupIdLine1, line1Id, line2Id,
newLineId, reportNode) as now, but if only one is present copy that
CurrentLimits into newGroup.newCurrentLimits() (preserve the same fields/ids
that mergedCurrentLimits would set) and emit a report/log entry using the same
reporting pattern used for deleted groups (use reportNode or the existing
logger) including operationalLimitGroupIdLine1, line1Id/line2Id and newLineId
for traceability.
- Around line 233-244: The report node created in logDeletedTemporaryLimits
(ModificationLimitsUtils.logDeletedTemporaryLimits) is missing a severity level;
update the ReportNode builder chain used in logDeletedTemporaryLimits to call
.withSeverity(TypedValue.INFO_SEVERITY) (matching other methods like
logDeletedGroups) just before the final .add() so the created report entry has
the correct INFO severity.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java`:
- Around line 81-83: Collect the IDs of the groups before mutating the
collections to avoid potential concurrent-modification issues: iterate
mergedLine.getOperationalLimitsGroups1() and
mergedLine.getOperationalLimitsGroups2() to build lists of group IDs, then call
mergedLine.removeOperationalLimitsGroup1(id) and
mergedLine.removeOperationalLimitsGroup2(id) using those ID lists instead of
removing inside the forEach over the original collections.
🪄 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: 110b5c15-da13-4cdc-8aa7-992f55d071aa
📒 Files selected for processing (3)
src/main/java/org/gridsuite/modification/modifications/DeleteAttachingLine.javasrc/main/java/org/gridsuite/modification/modifications/DeleteVoltageLevelOnLine.javasrc/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/gridsuite/modification/modifications/DeleteAttachingLine.java
src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java
Show resolved
Hide resolved
src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java
Show resolved
Hide resolved
Signed-off-by: Etienne LESOT <etienne.lesot@rte-france.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java (2)
227-238:⚠️ Potential issue | 🔴 CriticalCritical bug: condition always evaluates to
falsemaking the branch unreachable.The loop iterates over
temporaryLimitsOnLine2.keySet()but checks!temporaryLimitsOnLine2.containsKey(temporaryLimitName). This condition will always befalse, so temporary limits existing only on line2 will never be logged as deleted.🐛 Proposed fix
// logs for limit set deleted on line2 for (String temporaryLimitName : temporaryLimitsOnLine2.keySet()) { - if (!temporaryLimitsOnLine2.containsKey(temporaryLimitName)) { + if (!temporaryLimitsOnLine1.containsKey(temporaryLimitName)) { LoadingLimits.TemporaryLimit removedTemporaryLimit = temporaryLimitsOnLine2.get(temporaryLimitName); logDeletedTemporaryLimits(reportNode, removedTemporaryLimit, operationalLimitsGroupId, line2Id, line1Id, newLineId); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java` around lines 227 - 238, In the loop in ModificationLimitsUtils (iterating temporaryLimitsOnLine2.keySet()), the if condition mistakenly checks temporaryLimitsOnLine2.containsKey(temporaryLimitName) which is always true; change it to check temporaryLimitsOnLine1.containsKey(temporaryLimitName) so limits present in line2 but missing in line1 get logged as deleted by logDeletedTemporaryLimits; update any null-safety if temporaryLimitsOnLine1 can be null before calling containsKey and keep using temporaryLimitsOnLine2.get(temporaryLimitName) to fetch the removedTemporaryLimit.
244-252:⚠️ Potential issue | 🟡 MinorMissing severity on report node creates inconsistency with other logging methods.
Unlike
logDeletedGroups(line 189) and property merge logging (line 164), this method does not set severity, which may affect report filtering.🔧 Proposed fix
reportNode.newReportNode() .withMessageTemplate("network.modification.temporaryLimitsDeletedAfterMerge") .withUntypedValue("limit_name", temporaryLimit.getName()) .withUntypedValue("tempo", temporaryLimit.getAcceptableDuration()) .withUntypedValue("limitset_name", operationalLimitsGroupName) .withUntypedValue("line_with_limit", lineWithTemporaryLimit) .withUntypedValue("line_without_limit", lineWithoutTemporaryLimit) .withUntypedValue("replacing_line", newLineId) + .withSeverity(TypedValue.INFO_SEVERITY) .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/utils/ModificationLimitsUtils.java` around lines 244 - 252, The report node created by reportNode.newReportNode().withMessageTemplate(...) is missing a severity setting, causing inconsistency with other logs; update this chain to call .withSeverity(...) with the same severity used by logDeletedGroups and the property-merge logging (e.g., the same enum value those methods use) so the temporaryLimitsDeletedAfterMerge entry uses the same severity level and add any needed import for the severity enum.
🤖 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/utils/ModificationLimitsUtils.java`:
- Around line 82-83: The loop removes items from the same collection view
returned by
mergedLine.getOperationalLimitsGroups1()/getOperationalLimitsGroups2(), risking
ConcurrentModificationException; fix by first collecting the group IDs into an
independent list (e.g., List<String> ids1 =
mergedLine.getOperationalLimitsGroups1().stream().map(g ->
g.getId()).collect(...)) and then iterating that list to call
mergedLine.removeOperationalLimitsGroup1(id) (do the same for
getOperationalLimitsGroups2()/removeOperationalLimitsGroup2); update the code
around mergedLine usage to avoid mutating the collection while iterating.
---
Duplicate comments:
In `@src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java`:
- Around line 227-238: In the loop in ModificationLimitsUtils (iterating
temporaryLimitsOnLine2.keySet()), the if condition mistakenly checks
temporaryLimitsOnLine2.containsKey(temporaryLimitName) which is always true;
change it to check temporaryLimitsOnLine1.containsKey(temporaryLimitName) so
limits present in line2 but missing in line1 get logged as deleted by
logDeletedTemporaryLimits; update any null-safety if temporaryLimitsOnLine1 can
be null before calling containsKey and keep using
temporaryLimitsOnLine2.get(temporaryLimitName) to fetch the
removedTemporaryLimit.
- Around line 244-252: The report node created by
reportNode.newReportNode().withMessageTemplate(...) is missing a severity
setting, causing inconsistency with other logs; update this chain to call
.withSeverity(...) with the same severity used by logDeletedGroups and the
property-merge logging (e.g., the same enum value those methods use) so the
temporaryLimitsDeletedAfterMerge entry uses the same severity level and add any
needed import for the severity enum.
🪄 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: 114e6391-85fc-428e-9d68-95b9ab3aa8bb
📒 Files selected for processing (2)
src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.javasrc/test/java/org/gridsuite/modification/utils/MergingLimitsTest.java
src/main/java/org/gridsuite/modification/utils/ModificationLimitsUtils.java
Outdated
Show resolved
Hide resolved
|
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.
PR Summary
for DeleteVoltageLevelOnLine and DeleteAttachingLine