Add pre-checks for temporary limit modifications#186
Open
Conversation
Emit a warning and skip the modification when ADD is requested without a name, or when DELETE/MODIFY is requested without a name or acceptable duration. When the OLG modification type is not MODIFY and the temporary limit modification type is not ADD, force the temporary limit modification type to ADD and log a detail report to make the correction explicit. 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:
📝 WalkthroughWalkthroughAdds Lombok setters to a DTO and changes temporary-limit processing: validates required lookup fields, normalizes some per-row modification types to ADD, adds batch-aware duplicate detection/skipping, tightens deletion/matching/update semantics, adds report messages, and extends tests covering these behaviors. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java (1)
272-280:⚠️ Potential issue | 🟡 MinorStale comment:
nameis now required for MODIFY, update the inline comment.The comment says
// No name modificationbutnameis now being set (it has to, since the newpreModificationCheckrejects MODIFY without a name). Either remove the misleading comment or clarify that the name must be supplied even for a value-only modification.✏️ Proposed fix
- .name(toAttributeModification("name32", OperationType.SET)) // No name modification + .name(toAttributeModification("name32", OperationType.SET)) // Must supply existing name (required by preModificationCheck)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java` around lines 272 - 280, The inline comment next to the name(...) call in the CurrentTemporaryLimitModificationInfos.builder() block is stale because MODIFY now requires a name; update or remove it: locate the CurrentTemporaryLimitModificationInfos.builder() usage where modificationType is TemporaryLimitModificationType.MODIFY and the name is set via name(toAttributeModification("name32", OperationType.SET)), and either delete the comment "// No name modification" or replace it with a note that name must be provided for MODIFY (e.g., "name must be supplied for MODIFY per preModificationCheck"), ensuring consistency with preModificationCheck behavior.src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java (1)
551-563:⚠️ Potential issue | 🟠 MajorValidate MODIFY_OR_ADD before name-based comparisons.
Lines 551 and 561–562 call
limit.getName().getValue()unconditionally.preModificationCheckvalidates ADD/DELETE/MODIFY but leaves MODIFY_OR_ADD unvalidated whenolgModifInfos.getModificationType() == MODIFY. The defensive conversion at lines 464–468 only applies whenolgModifInfos != MODIFY, allowing a null name to reachgetTemporaryLimitToModify, where lines 551 and 561–562 would trigger an NPE.The test case at line 353 (LimitSetModificationsTest) demonstrates this scenario:
olgModifInfos.getModificationType() == MODIFYpaired with a MODIFY_OR_ADD limit. Add explicit validation for MODIFY_OR_ADD with name/duration checks equivalent to DELETE/MODIFY, or ensure the conversion logic covers all MODIFY_OR_ADD paths before callinggetTemporaryLimitToModify.Also note: line 561's duplicate-name check matches by
equalsincluding potential null values. An added limit with null name in the network (rare) could match a proposed null limit name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java` around lines 551 - 563, The code in OperationalLimitsGroupModification calls limit.getName().getValue() in getTemporaryLimitToModify and in the ADD-name-duplication check without handling MODIFY_OR_ADD, which can leave a null name and cause NPEs; update preModificationCheck or the path that converts/normalizes limits so MODIFY_OR_ADD is validated the same as DELETE/MODIFY (i.e., require non-null name/duration) before calling getTemporaryLimitToModify, or add defensive null checks inside getTemporaryLimitToModify and the duplicate-name stream filter (e.g., verify limit.getName()!=null and temporaryLimit.getName()!=null before equals) so TemporaryLimitModificationType.MODIFY_OR_ADD/MODIFY paths cannot dereference a null name.
🧹 Nitpick comments (2)
src/main/resources/org/gridsuite/modification/reports.properties (1)
311-314: Message keys and${modificationType}placeholders align with the new report builders inOperationalLimitsGroupModification.preModificationCheck. LGTM.Minor wording nit (optional): “Missing info (name or duration)” is reported also when both are missing — consider “Missing name and/or duration” for precision. Not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/org/gridsuite/modification/reports.properties` around lines 311 - 314, Update the user-facing message for the key network.modification.temporaryLimitsMissingInfo to be more precise by changing "Missing info (name or duration)" to "Missing name and/or duration"; keep the ${modificationType} placeholder and the rest of the message unchanged so it still matches the usage in OperationalLimitsGroupModification.preModificationCheck and other report builders.src/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java (1)
433-651: Good coverage of the new validation paths. Consider adding a MODIFY_OR_ADD with missing-name test.Tests exercise ADD/DELETE/MODIFY missing-name and missing-duration paths plus the wrong-modification-type normalization, which matches the new
preModificationChecksemantics. Given thatMODIFY_OR_ADDis a valid per-limitmodificationType(seetestModifyOrAddNonExistentLimit), adding a test withmodificationType=MODIFY_OR_ADDandname=nullwould expose the gap flagged in the main source comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java` around lines 433 - 651, Add a test covering the missing-name case when a per-limit modification uses TemporaryLimitModificationType.MODIFY_OR_ADD: duplicate the pattern from testAddTemporaryLimitWithMissingName but set CurrentTemporaryLimitModificationInfos.modificationType to TemporaryLimitModificationType.MODIFY_OR_ADD and name to null, then apply modificationInfos.toModification().apply(...), assert the temporary limit was not added (assertNull on limits.getTemporaryLimit(duration)) and assert the same log via assertLogMessageWithoutRank (use the "Missing name for temporary limit to ADD: ignored" message and "network.modification.temporaryLimitsMissingName" key) to verify MODIFY_OR_ADD follows the ADD missing-name validation path; place the test alongside the other temporary-limit missing-info tests and reference LimitSetsTabularModificationInfos, CurrentTemporaryLimitModificationInfos, TemporaryLimitModificationType.MODIFY_OR_ADD, and assertLogMessageWithoutRank to locate code.
🤖 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/modifications/olg/OperationalLimitsGroupModification.java`:
- Around line 463-473: The current logic in OperationalLimitsGroupModification
silently coerces a per-limit DELETE/MODIFY into an ADD when
olgModifInfos.getModificationType() !=
OperationalLimitsGroupModificationType.MODIFY: locate the block that checks
olgModifInfos.getModificationType() and limit.getModificationType() and either
(preferred) change it to skip the limit and log a warning when
limit.getModificationType() is DELETE or MODIFY (call addToLogsOnSide(...) with
TypedValue.WARN_SEVERITY and do not call limit.setModificationType(ADD)), or
(alternative) elevate the existing ReportNode severity from
TypedValue.DETAIL_SEVERITY to TypedValue.WARN_SEVERITY so the coercion is
visible; reference the symbols olgModifInfos, limit,
TemporaryLimitModificationType, OperationalLimitsGroupModificationType,
addToLogsOnSide, and ReportNode.newRootReportNode() when making the change.
- Around line 440-475: preModificationCheck fails to validate
TemporaryLimitModificationType.MODIFY_OR_ADD which can lead to a NPE later in
getTemporaryLimitToModify; update the validation so MODIFY_OR_ADD is treated
like MODIFY for required fields: include
TemporaryLimitModificationType.MODIFY_OR_ADD in the conditional that checks for
DELETE or MODIFY (the block using hasValue(limit.getName()) ||
hasValue(limit.getAcceptableDuration())) so that name and acceptableDuration are
validated for MODIFY_OR_ADD, and ensure the other branch that coerces non-MODIFY
OLGs to ADD (the block that checks olgModifInfos.getModificationType() !=
OperationalLimitsGroupModificationType.MODIFY && limit.getModificationType() !=
TemporaryLimitModificationType.ADD) still converts MODIFY_OR_ADD to ADD when
appropriate.
---
Outside diff comments:
In
`@src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java`:
- Around line 551-563: The code in OperationalLimitsGroupModification calls
limit.getName().getValue() in getTemporaryLimitToModify and in the
ADD-name-duplication check without handling MODIFY_OR_ADD, which can leave a
null name and cause NPEs; update preModificationCheck or the path that
converts/normalizes limits so MODIFY_OR_ADD is validated the same as
DELETE/MODIFY (i.e., require non-null name/duration) before calling
getTemporaryLimitToModify, or add defensive null checks inside
getTemporaryLimitToModify and the duplicate-name stream filter (e.g., verify
limit.getName()!=null and temporaryLimit.getName()!=null before equals) so
TemporaryLimitModificationType.MODIFY_OR_ADD/MODIFY paths cannot dereference a
null name.
In
`@src/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java`:
- Around line 272-280: The inline comment next to the name(...) call in the
CurrentTemporaryLimitModificationInfos.builder() block is stale because MODIFY
now requires a name; update or remove it: locate the
CurrentTemporaryLimitModificationInfos.builder() usage where modificationType is
TemporaryLimitModificationType.MODIFY and the name is set via
name(toAttributeModification("name32", OperationType.SET)), and either delete
the comment "// No name modification" or replace it with a note that name must
be provided for MODIFY (e.g., "name must be supplied for MODIFY per
preModificationCheck"), ensuring consistency with preModificationCheck behavior.
---
Nitpick comments:
In `@src/main/resources/org/gridsuite/modification/reports.properties`:
- Around line 311-314: Update the user-facing message for the key
network.modification.temporaryLimitsMissingInfo to be more precise by changing
"Missing info (name or duration)" to "Missing name and/or duration"; keep the
${modificationType} placeholder and the rest of the message unchanged so it
still matches the usage in
OperationalLimitsGroupModification.preModificationCheck and other report
builders.
In
`@src/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java`:
- Around line 433-651: Add a test covering the missing-name case when a
per-limit modification uses TemporaryLimitModificationType.MODIFY_OR_ADD:
duplicate the pattern from testAddTemporaryLimitWithMissingName but set
CurrentTemporaryLimitModificationInfos.modificationType to
TemporaryLimitModificationType.MODIFY_OR_ADD and name to null, then apply
modificationInfos.toModification().apply(...), assert the temporary limit was
not added (assertNull on limits.getTemporaryLimit(duration)) and assert the same
log via assertLogMessageWithoutRank (use the "Missing name for temporary limit
to ADD: ignored" message and "network.modification.temporaryLimitsMissingName"
key) to verify MODIFY_OR_ADD follows the ADD missing-name validation path; place
the test alongside the other temporary-limit missing-info tests and reference
LimitSetsTabularModificationInfos, CurrentTemporaryLimitModificationInfos,
TemporaryLimitModificationType.MODIFY_OR_ADD, and assertLogMessageWithoutRank to
locate code.
🪄 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: a45b8919-ecbb-4812-b535-04372f165021
📒 Files selected for processing (4)
src/main/java/org/gridsuite/modification/dto/CurrentTemporaryLimitModificationInfos.javasrc/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.javasrc/main/resources/org/gridsuite/modification/reports.propertiessrc/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java
- Include REPLACE and MODIFY_OR_ADD in name validation for temporary limits. - Update tests to cover new validation scenarios. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java (1)
431-910: Good coverage; suggest one extra scenario.The new tests do a solid job pinning down missing-name/duration behavior for every per-limit op, the wrong-modification-type coercion, and the duplicate name/duration error paths.
Consider also adding a test for OLG
MODIFY_OR_ADDagainst an existing group with a per-limitMODIFY(and a per-limitDELETE), to guard the case flagged inOperationalLimitsGroupModification.javalines 467–477 where the current coercion logic incorrectly rewrites the user’s intent.
🤖 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/modifications/olg/OperationalLimitsGroupModification.java`:
- Around line 467-477: The current coercion in
OperationalLimitsGroupModification unconditionally forces per-limit
modifications to ADD when olgModifInfos.getModificationType() !=
OperationalLimitsGroupModificationType.MODIFY, which incorrectly affects the
MODIFY_OR_ADD case for existing groups; change the condition to only coerce to
TemporaryLimitModificationType.ADD when the OLG mode is one that unconditionally
creates/replace the group (e.g., OperationalLimitsGroupModificationType.ADD or
REPLACE) rather than every non-MODIFY value, so update the if that calls
addToLogsOnSide(...) and
limit.setModificationType(TemporaryLimitModificationType.ADD) to explicitly
check for ADD/REPLACE modes; ensure related methods like
getTemporaryLimitToModify still operate correctly and add a test covering
MODIFY_OR_ADD against an existing group with per-limit MODIFY and DELETE to
prevent regressions.
🪄 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: 0c01f567-ed16-489e-b9d1-20cb3043ccb0
📒 Files selected for processing (2)
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.javasrc/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java
…s and update tests Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
Unify pre-checks to require name and duration for all types, refuse duplicates for ADD/MODIFY/MODIFY_OR_ADD, and make REPLACE take the input as-is instead of preserving previous values. Refactor downstream code now that duration is guaranteed non-null and extend the test suite to cover all per-type cases. Signed-off-by: Florent MILLOT <florent.millot_externe@rte-france.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/modifications/olg/OperationalLimitsGroupModification.java`:
- Around line 543-548: The current early returns after preModificationCheck(...)
and wouldCreateDuplicate(...) can cause modifyTemporaryLimits(...) to run its
group-level REPLACE cleanup even if no temporary-limit rows were accepted; fix
this by tracking and propagating an "acceptedChange" boolean from the per-row
processing (set true when a temporary-limit change is actually applied) and pass
it into modifyTemporaryLimits(...) so that the REPLACE cleanup (the branch that
suppresses unmodifiedTemporaryLimits) only runs when acceptedChange is true;
update the code paths around preModificationCheck(...),
wouldCreateDuplicate(...), and the caller of modifyTemporaryLimits(...) in
OperationalLimitsGroupModification to carry this flag through.
- Around line 604-607: The code currently only nulls limitToModify when the
incoming limit name differs, so a same-batch DELETE + ADD/MODIFY with the same
name still reuses the soon-to-be-deleted limit; change the conditional around
limitToModify (the block using limitToModify, limit.getName().getValue(),
isThisLimitDeleted, and currentLimitsInfos.getTemporaryLimits()) to treat the
limit as missing whenever isThisLimitDeleted(...) returns true (regardless of
name equality) — i.e., remove the name-equality guard and set limitToModify =
null when isThisLimitDeleted(currentLimitsInfos.getTemporaryLimits(),
limitAcceptableDuration) is true so the incoming ADD/MODIFY will not reuse a
limit scheduled for deletion.
🪄 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: ca5be9a7-8d71-48d1-a2f4-3180b696aca6
📒 Files selected for processing (3)
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.javasrc/main/resources/org/gridsuite/modification/reports.propertiessrc/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/org/gridsuite/modification/reports.properties
- Improve name and duration validation logic for temporary limits. - Simplify value assignment logic in REPLACE and MODIFY scenarios. - Update and extend test cases to cover new validation conditions. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java (1)
604-607:⚠️ Potential issue | 🟠 MajorDeleted durations are still reused when the re-added name matches.
After
DELETE(duration=D, name=N), a laterADD/MODIFYfor the samedurationand samenamekeepslimitToModifynon-null here, so the code falls intomodifyTemporaryLimit(...)and can reuse stale fields from the deleted limit instead of treatingDas free. The deleted-in-batch check should null outlimitToModifyfor any non-DELETEoperation, regardless of name equality. Please add a regression next totestAddTemporaryLimitConflictingWithDeletedInSameBatchSucceedsfor the same-name case as well.🩹 Proposed fix
- if (limitToModify != null - && !limitToModify.getName().equals(limit.getName().getValue()) - && isThisLimitDeleted(currentLimitsInfos.getTemporaryLimits(), limitAcceptableDuration)) { + if (limitToModify != null + && limit.getModificationType() != TemporaryLimitModificationType.DELETE + && isThisLimitDeleted(currentLimitsInfos.getTemporaryLimits(), limitAcceptableDuration)) { limitToModify = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java` around lines 604 - 607, The code keeps limitToModify non-null for a later ADD/MODIFY that reuses the same name as a DELETE in the same batch, causing stale fields to be reused; update the logic in OperationalLimitsGroupModification so that whenever the current operation is not a DELETE and isThisLimitDeleted(currentLimitsInfos.getTemporaryLimits(), limitAcceptableDuration) returns true you explicitly set limitToModify = null (so modifyTemporaryLimit(...) is not used and the duration is treated free), and add a regression test next to testAddTemporaryLimitConflictingWithDeletedInSameBatchSucceeds that covers the same-duration same-name re-add case to ensure deleted durations are not reused.
🤖 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/modifications/olg/OperationalLimitsGroupModification.java`:
- Around line 473-525: The duplicate check in wouldCreateDuplicate(...)
currently only inspects the immutable networkCurrentLimits snapshot (and returns
false when networkCurrentLimits is null), so it misses duplicates introduced
earlier in the same request; change the check to use a mutable working set
seeded from networkCurrentLimits (or an empty LoadingLimits if
networkCurrentLimits is null) and then consult that working set instead of
networkCurrentLimits when computing existingByDuration/existingByName; after a
row is accepted elsewhere in the flow, update that working set
(add/modify/remove entries) so subsequent calls to wouldCreateDuplicate see
prior accepted rows; keep existing behavior around isThisLimitDeleted(...) and
preserve log creation via addToLogsOnSide(...) and ReportNode.
---
Duplicate comments:
In
`@src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java`:
- Around line 604-607: The code keeps limitToModify non-null for a later
ADD/MODIFY that reuses the same name as a DELETE in the same batch, causing
stale fields to be reused; update the logic in
OperationalLimitsGroupModification so that whenever the current operation is not
a DELETE and isThisLimitDeleted(currentLimitsInfos.getTemporaryLimits(),
limitAcceptableDuration) returns true you explicitly set limitToModify = null
(so modifyTemporaryLimit(...) is not used and the duration is treated free), and
add a regression test next to
testAddTemporaryLimitConflictingWithDeletedInSameBatchSucceeds that covers the
same-duration same-name re-add case to ensure deleted durations are not reused.
🪄 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: bff42087-3ae4-439a-b2fd-3e99560556bb
📒 Files selected for processing (2)
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.javasrc/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java
- Introduce a working set for temporary limits during batch processing. - Refactor duplicate detection to handle in-batch conflicts. - Update tests to validate in-batch duplicate rejection for ADD operations. Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java (1)
618-621:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame-name delete+add still reuses the deleted snapshot.
This is the same edge case previously flagged: when a later
ADD/MODIFYtargets a duration deleted earlier in the batch with the same name, the name check keepslimitToModifynon-null, so the code goes down the modify path instead of treating the slot as free. That can preserve the old value when the incoming row leavesvalueunset.🩹 Proposed fix
if (limitToModify != null - && !limitToModify.getName().equals(limit.getName().getValue()) + && limit.getModificationType() != TemporaryLimitModificationType.DELETE && isThisLimitDeleted(currentLimitsInfos.getTemporaryLimits(), limitAcceptableDuration)) { limitToModify = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java` around lines 618 - 621, The code in OperationalLimitsGroupModification that decides whether to reuse an existing snapshot (uses limitToModify and the name comparison) misses the case where a slot was deleted earlier in the same batch but the incoming row uses the same name, so it incorrectly follows the modify path; change the logic so that if isThisLimitDeleted(currentLimitsInfos.getTemporaryLimits(), limitAcceptableDuration) is true for the target duration you always treat the slot as missing (set/keep limitToModify null) regardless of the name comparison — i.e., detect the deletion first (or add an explicit check) and null out limitToModify when the temporary limits indicate a prior deletion, referencing limitToModify, isThisLimitDeleted(...), currentLimitsInfos.getTemporaryLimits(), limitAcceptableDuration, and limit.getName().getValue() to locate and fix the branch.
🤖 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/modifications/olg/OperationalLimitsGroupModification.java`:
- Around line 605-609: The method isThisLimitDeleted(...) currently treats any
DELETE row with an acceptableDuration as effective even if that DELETE would be
ignored earlier due to a missing name; update the predicate in
isThisLimitDeleted to only consider DELETE entries that have a valid name (e.g.,
add hasValue(temporaryLimit.getName()) or equivalent) so DELETE rows that would
be skipped by preModificationCheck(...) are ignored here too; locate the stream
in isThisLimitDeleted and add the name presence check alongside the existing
modificationType and acceptableDuration checks.
---
Duplicate comments:
In
`@src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.java`:
- Around line 618-621: The code in OperationalLimitsGroupModification that
decides whether to reuse an existing snapshot (uses limitToModify and the name
comparison) misses the case where a slot was deleted earlier in the same batch
but the incoming row uses the same name, so it incorrectly follows the modify
path; change the logic so that if
isThisLimitDeleted(currentLimitsInfos.getTemporaryLimits(),
limitAcceptableDuration) is true for the target duration you always treat the
slot as missing (set/keep limitToModify null) regardless of the name comparison
— i.e., detect the deletion first (or add an explicit check) and null out
limitToModify when the temporary limits indicate a prior deletion, referencing
limitToModify, isThisLimitDeleted(...), currentLimitsInfos.getTemporaryLimits(),
limitAcceptableDuration, and limit.getName().getValue() to locate and fix the
branch.
🪄 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: 7313de91-cecb-4a43-9002-1c1aaff7368e
📒 Files selected for processing (2)
src/main/java/org/gridsuite/modification/modifications/olg/OperationalLimitsGroupModification.javasrc/test/java/org/gridsuite/modification/modifications/tabularmodifications/LimitSetModificationsTest.java
Signed-off-by: Florent MILLOT <75525996+flomillot@users.noreply.github.com>
|
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.
Summary
ADDis requested without a name, or whenDELETE/MODIFYis requested without a name or acceptable duration (newpreModificationCheckmethod).MODIFYand the temporary limit modification type is notADD, force the temporary limit modification type toADDand log a detail report so the correction is traceable.CurrentTemporaryLimitModificationInfosto allow the modification type to be rewritten.