refactor: make AbstractModification objects buildable independently from ModifcationInfos objects#203
Merged
TheMaskedTurtle merged 16 commits intoJun 24, 2026
Conversation
Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
…nd harmonize lombok annotations Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
…stead of a DTO Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
…m dto because they inherit from ModificationInfos Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
…special cases Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
|
Warning Review limit reached
More reviews will be available in 44 minutes and 2 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR replaces DTO-backed construction with builder-based field models across creation, modification, utility, test, and documentation code paths. Shared abstractions were added, and execution classes now read direct fields instead of nested infos objects. ChangesBuilder-backed modification refactor
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
TheMaskedTurtle
changed the title
Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com> Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/main/java/org/gridsuite/modification/modifications/OperatingStatusModification.java (1)
60-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
actionbefore switching on it.A direct-built modification with a missing
actionpassescheck()and then hitsswitch (action), which throws a raw null-pointer failure instead of aNetworkModificationException.Suggested fix
public void check(Network network) throws NetworkModificationException { Identifiable<?> equipment = network.getIdentifiable(equipmentId); if (equipment == null) { throw new NetworkModificationException(EQUIPMENT_NOT_FOUND, equipmentId); } + checkAction(); } @@ String equipmentType = String.valueOf(equipment.getType()); + checkAction(); switch (action) { @@ } } + + private void checkAction() { + if (action == null) { + throw new NetworkModificationException(OPERATING_STATUS_MODIFICATION_ERROR, "Missing operating status action"); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/OperatingStatusModification.java` around lines 60 - 75, The check() method in OperatingStatusModification validates that the equipment exists but does not validate that the action field is not null. This causes a NullPointerException when the apply() method attempts to switch on a null action instead of throwing a proper NetworkModificationException. Add a null validation check for the action field in the check(Network network) method immediately after validating the equipment, throwing a NetworkModificationException with an appropriate error message if action is null, similar to the existing EQUIPMENT_NOT_FOUND validation pattern.src/main/java/org/gridsuite/modification/modifications/VoltageLevelTopologyModification.java (1)
47-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle a missing modification list before calling
isEmpty().With the builder,
equipmentAttributeModificationListcan benull; Line 47 then throws an NPE instead of the intendedNetworkModificationException.Proposed fix
- if (!equipmentAttributeModificationList.isEmpty()) { - for (EquipmentAttributeModification equipmentAttributeModification : equipmentAttributeModificationList) { - equipmentAttributeModification.check(network); - } - } else { + if (equipmentAttributeModificationList == null || equipmentAttributeModificationList.isEmpty()) { throw new NetworkModificationException(MODIFY_VOLTAGE_LEVEL_TOPOLOGY_ERROR, "Missing required switches to modify the voltage level topology"); } + for (EquipmentAttributeModification equipmentAttributeModification : equipmentAttributeModificationList) { + equipmentAttributeModification.check(network); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/VoltageLevelTopologyModification.java` around lines 47 - 53, The code in the VoltageLevelTopologyModification class checks if equipmentAttributeModificationList is empty by calling isEmpty() on line 47, but equipmentAttributeModificationList can be null when set via the builder, which will throw a NullPointerException instead of the intended NetworkModificationException. Add a null check before calling isEmpty() to handle the case where equipmentAttributeModificationList is null, and ensure that both null and empty cases throw the same NetworkModificationException with the message about missing required switches.src/main/java/org/gridsuite/modification/modifications/LineAttachToVoltageLevel.java (1)
118-142:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate attachment-point override values before mutating the voltage level.
attachmentPointDetailInformationbypassesVoltageLevelCreation.check, so negative voltage limits or IP values can be applied directly inupdateAttachmentVoltageLevel.Proposed fix
// override attachment point if (attachmentPointDetailInformation != null) { + String errorMessage = "Voltage level '" + attachmentPointId + "' : "; + ModificationUtils.checkIsNotNegativeValue(errorMessage, attachmentPointDetailInformation.getLowVoltageLimit(), + CREATE_VOLTAGE_LEVEL_ERROR, "Low voltage limit"); + ModificationUtils.checkIsNotNegativeValue(errorMessage, attachmentPointDetailInformation.getHighVoltageLimit(), + CREATE_VOLTAGE_LEVEL_ERROR, "High voltage limit"); // override voltage level updateAttachmentVoltageLevel(network, attachmentPointDetailInformation); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/LineAttachToVoltageLevel.java` around lines 118 - 142, Add validation in the updateAttachmentVoltageLevel method to ensure that attachment-point override values are valid before mutating the voltage level. Since attachmentPointDetailInformation bypasses VoltageLevelCreation.check, you need to manually validate the retrieved values (from getHighVoltageLimit, getLowVoltageLimit, getIpMax, and getIpMin) to ensure they meet the required constraints (such as non-negative values) before applying them to the voltage level through setHighVoltageLimit, setLowVoltageLimit, withIpMax, and withIpMin calls. Throw an appropriate validation exception if any invalid values are detected.src/main/java/org/gridsuite/modification/modifications/LccCreation.java (1)
76-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
lossFactor, notpowerFactor, for the loss-factor rule.The error says “Loss factor”, but the condition checks
getPowerFactor(). This also unboxes a possibly absent power factor before the optional guards used later in creation.Suggested fix
- if (converterStation.getPowerFactor() > 1) { + if (converterStation.getLossFactor() != null && converterStation.getLossFactor() > 1) { throw new NetworkModificationException(CREATE_LCC_ERROR, "Loss factor should be less or equal to 1"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/LccCreation.java` around lines 76 - 82, In the checkLccConverterStation method, the validation is checking the wrong property. Change the condition from checking converterStation.getPowerFactor() to checking converterStation.getLossFactor() instead, so that the actual validation matches the error message which correctly states "Loss factor should be less or equal to 1". This also avoids prematurely unboxing an optional value.src/main/java/org/gridsuite/modification/modifications/BalancesAdjustmentModification.java (2)
88-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply balance iteration settings before the no-load-flow return.
When
withLoadFlowis false, the early return skipsmaxNumberIterations,thresholdNetPosition, and mismatch mode, so user-provided balance settings are silently ignored.Suggested fix
parameters.getScalingParameters().setPriority(ScalingParameters.Priority.RESPECT_OF_VOLUME_ASKED); parameters.setWithLoadFlow(withLoadFlow); + parameters.setMaxNumberIterations(maxNumberIterations); + parameters.setThresholdNetPosition(thresholdNetPosition); + parameters.setMismatchMode(BalanceComputationParameters.MismatchMode.MAX); if (!withLoadFlow) { return parameters; } @@ private void overrideBalanceComputationParameters(BalanceComputationParameters parameters) { - parameters.setMaxNumberIterations(maxNumberIterations); - parameters.setThresholdNetPosition(thresholdNetPosition); - parameters.setMismatchMode(BalanceComputationParameters.MismatchMode.MAX); parameters.setSubtractLoadFlowBalancing(subtractLoadFlowBalancing);Also applies to: 145-149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/BalancesAdjustmentModification.java` around lines 88 - 107, The overrideBalanceComputationParameters method call in createBalanceComputationParameters is positioned after the early return when withLoadFlow is false, causing user-provided balance settings to be skipped. Move the overrideBalanceComputationParameters(parameters) call to execute before the early return statement so that balance iteration settings like maxNumberIterations and thresholdNetPosition are always applied regardless of the withLoadFlow flag state.
99-107:⚠️ Potential issue | 🟠 MajorGuard the
OpenLoadFlowParametersextension before dereferencing it.
overrideBalanceComputationParameters()is called unconditionally at line 106, but theOpenLoadFlowParametersextension is only added ifmapLoadFlowPrameters()is called (lines 99-104), which happens only when the provider is "OpenLoadFlow" and specific parameters exist. If either condition is false, or if the specific parameters map is empty, the extension is never created, causing a null pointer exception at lines 156-157 when dereferencinggetExtension(OpenLoadFlowParameters.class).Suggested fix
- parameters.getLoadFlowParameters().getExtension(OpenLoadFlowParameters.class) - .setSlackDistributionFailureBehavior(OpenLoadFlowParameters.SlackDistributionFailureBehavior.FAIL); + OpenLoadFlowParameters openLoadFlowParameters = parameters.getLoadFlowParameters() + .getExtension(OpenLoadFlowParameters.class); + if (openLoadFlowParameters != null) { + openLoadFlowParameters.setSlackDistributionFailureBehavior(OpenLoadFlowParameters.SlackDistributionFailureBehavior.FAIL); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/BalancesAdjustmentModification.java` around lines 99 - 107, The method overrideBalanceComputationParameters() is called unconditionally at line 106 but it attempts to dereference getExtension(OpenLoadFlowParameters.class) which is only created conditionally when the provider is "OpenLoadFlow" and mapLoadFlowPrameters() is invoked. To fix this, move the call to overrideBalanceComputationParameters() inside the conditional block (after line 104 where the extension is guaranteed to exist) so it only executes when the OpenLoadFlowParameters extension has been properly created.src/main/java/org/gridsuite/modification/modifications/TwoWindingsTransformerCreation.java (1)
203-207:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck tap changer steps before dereferencing them.
Both paths call
getSteps().size()before the null guard. If a builder-created tap changer omits steps, transformer creation fails with an NPE.Proposed fix
- tapsReports.add(getInstance().buildCreationReport(phaseTapChangerInfos.getLowTapPosition(), "Low tap position")); - tapsReports.add(getInstance().buildCreationReport(phaseTapChangerInfos.getTapPosition(), "Tap position")); - tapsReports.add(getInstance().buildCreationReport(phaseTapChangerInfos.getSteps().size() - 1, "High tap position")); - - if (phaseTapChangerInfos.getSteps() != null) { + if (CollectionUtils.isNotEmpty(phaseTapChangerInfos.getSteps())) { + tapsReports.add(getInstance().buildCreationReport(phaseTapChangerInfos.getLowTapPosition(), "Low tap position")); + tapsReports.add(getInstance().buildCreationReport(phaseTapChangerInfos.getTapPosition(), "Tap position")); + tapsReports.add(getInstance().buildCreationReport(phaseTapChangerInfos.getSteps().size() - 1, "High tap position")); for (TapChangerStepCreationInfos step : phaseTapChangerInfos.getSteps()) { phaseTapChangerAdder.beginStep().setR(step.getR()).setX(step.getX()).setG(step.getG()).setB(step.getB()).setRho(step.getRho()).setAlpha(step.getAlpha()).endStep(); }- tapsReports.add(getInstance().buildCreationReport(ratioTapChangerInfos.getLowTapPosition(), "Low tap position")); - tapsReports.add(getInstance().buildCreationReport(ratioTapChangerInfos.getTapPosition(), "Tap position")); - tapsReports.add(getInstance().buildCreationReport(ratioTapChangerInfos.getSteps().size() - 1, "High tap position")); - - if (ratioTapChangerInfos.getSteps() != null) { + if (CollectionUtils.isNotEmpty(ratioTapChangerInfos.getSteps())) { + tapsReports.add(getInstance().buildCreationReport(ratioTapChangerInfos.getLowTapPosition(), "Low tap position")); + tapsReports.add(getInstance().buildCreationReport(ratioTapChangerInfos.getTapPosition(), "Tap position")); + tapsReports.add(getInstance().buildCreationReport(ratioTapChangerInfos.getSteps().size() - 1, "High tap position")); for (TapChangerStepCreationInfos step : ratioTapChangerInfos.getSteps()) { ratioTapChangerAdder.beginStep().setR(step.getR()).setX(step.getX()).setG(step.getG()).setB(step.getB()).setRho(step.getRho()).endStep(); }Also applies to: 257-262
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/TwoWindingsTransformerCreation.java` around lines 203 - 207, The code calls getSteps().size() on phaseTapChangerInfos before checking if getSteps() returns null, which will cause a NullPointerException if steps is not initialized. Move the null check for phaseTapChangerInfos.getSteps() to occur before the line that builds the "High tap position" report using getSteps().size() - 1. Only call getInstance().buildCreationReport() for the "High tap position" when getSteps() is not null. Apply this same fix to the similar code block mentioned at lines 257-262 where phaseTapChangerInfos is used.
🟡 Minor comments (1)
src/main/java/org/gridsuite/modification/modifications/VscCreation.java-317-319 (1)
317-319:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReport the voltage setpoint value, not reactive power.
Line 318 is inside the
getVoltageSetpoint()branch but builds the "Voltage" report fromgetReactivePowerSetpoint().Proposed fix
- setPointsVoltageReports.add(ModificationUtils.getInstance().buildCreationReport(vscConverterStationCreation.getReactivePowerSetpoint(), "Voltage")); + setPointsVoltageReports.add(ModificationUtils.getInstance().buildCreationReport(vscConverterStationCreation.getVoltageSetpoint(), "Voltage"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/VscCreation.java` around lines 317 - 319, In the VscCreation class within the if block checking vscConverterStationCreation.getVoltageSetpoint() != null, the buildCreationReport call is incorrectly using getReactivePowerSetpoint() instead of getVoltageSetpoint(). Change the argument passed to ModificationUtils.getInstance().buildCreationReport() from vscConverterStationCreation.getReactivePowerSetpoint() to vscConverterStationCreation.getVoltageSetpoint() so that the voltage setpoint value is reported in the "Voltage" report instead of the reactive power setpoint.
🧹 Nitpick comments (9)
src/main/java/org/gridsuite/modification/modifications/AbstractEquipmentModification.java (1)
27-27: ⚡ Quick winNarrow constructor visibility on abstract modifier base.
Line 27 should be
protected;publicexposes a constructor intended only for subclasses.Suggested change
- public AbstractEquipmentModification(String equipmentId, List<FreePropertyInfos> properties, AttributeModification<String> equipmentName) { + protected AbstractEquipmentModification(String equipmentId, List<FreePropertyInfos> properties, AttributeModification<String> equipmentName) { super(equipmentId, properties); this.equipmentName = equipmentName; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/AbstractEquipmentModification.java` at line 27, The constructor AbstractEquipmentModification(String equipmentId, List<FreePropertyInfos> properties, AttributeModification<String> equipmentName) currently uses the public access modifier, but since this is an abstract class, the constructor should only be accessible to subclasses. Change the access modifier from public to protected to prevent direct instantiation from outside the class hierarchy and properly enforce that this constructor is intended only for subclass use.Source: Linters/SAST tools
src/main/java/org/gridsuite/modification/modifications/AbstractEquipmentBase.java (1)
27-27: ⚡ Quick winRestrict abstract-base constructor visibility.
Line 27 can be
protectedinstead ofpublicto keep the inheritance API narrow.Suggested change
- public AbstractEquipmentBase(String equipmentId, List<FreePropertyInfos> properties) { + protected AbstractEquipmentBase(String equipmentId, List<FreePropertyInfos> properties) { this.equipmentId = equipmentId; this.properties = properties; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/AbstractEquipmentBase.java` at line 27, Change the visibility modifier of the AbstractEquipmentBase constructor that takes String equipmentId and List<FreePropertyInfos> properties parameters from public to protected. This will restrict the constructor access to only subclasses and classes within the same package, which is more appropriate for an abstract base class and keeps the inheritance API narrow.Source: Linters/SAST tools
src/main/java/org/gridsuite/modification/modifications/data/LccConverterStationCreation.java (1)
6-17: 🏗️ Heavy liftAvoid re-coupling modification models to DTO types.
Line 6 and Line 17 bind
modifications.data.LccConverterStationCreationtodto.LccShuntCompensatorInfos. That partially reintroduces the DTO dependency this refactor is trying to remove. Prefer a modification-side type for shunt compensators and map from DTO in conversion code.Possible direction
-import org.gridsuite.modification.dto.LccShuntCompensatorInfos; @@ - private List<LccShuntCompensatorInfos> shuntCompensatorsOnSide; + private List<LccShuntCompensatorCreation> shuntCompensatorsOnSide;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/data/LccConverterStationCreation.java` around lines 6 - 17, The LccConverterStationCreation class currently imports and uses LccShuntCompensatorInfos from the DTO package, which reintroduces DTO coupling that should be removed. Create a new modification-side model class for shunt compensators (similar to other modification models in this package) instead of using the DTO type directly. Replace the import of org.gridsuite.modification.dto.LccShuntCompensatorInfos and update the shuntCompensatorsOnSide field in LccConverterStationCreation to use the new modification-side type. Then add mapping logic in the conversion code to convert from the DTO LccShuntCompensatorInfos to the new modification-side type.src/main/java/org/gridsuite/modification/modifications/MoveVoltageLevelFeederBays.java (1)
130-134: ⚡ Quick winMake the adapter override explicit.
Sonar flags this empty override; throwing makes accidental use fail loudly while keeping the connectivity-only adapter intent clear.
Suggested fix
`@Override` protected void modifyCharacteristics(Branch<?> branch, ReportNode subReportNode) { - + throw new UnsupportedOperationException("MoveVoltageLevelFeederBays only uses branch connectivity attributes"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/MoveVoltageLevelFeederBays.java` around lines 130 - 134, The empty override of the modifyCharacteristics method in the AbstractBranchModification anonymous class inside MoveVoltageLevelFeederBays is flagged by Sonar. Make the adapter's intent explicit by replacing the empty method body with a throw statement (throw new UnsupportedOperationException) to indicate that this connectivity-only adapter should not have its characteristics modified, ensuring accidental use fails loudly.Source: Linters/SAST tools
src/main/java/org/gridsuite/modification/modifications/AbstractBranchModification.java (1)
75-101: ⚡ Quick winKeep the abstract base constructor protected.
This constructor is only for subclasses; making it public exposes a 30-parameter API surface on an abstract base and is already flagged by the quality gate.
Suggested fix
- public AbstractBranchModification(String equipmentId, List<FreePropertyInfos> properties, + protected AbstractBranchModification(String equipmentId, List<FreePropertyInfos> properties,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/AbstractBranchModification.java` around lines 75 - 101, The AbstractBranchModification constructor has public visibility, but since this is an abstract base class meant only for subclasses, it should be protected instead. Change the access modifier of the AbstractBranchModification constructor from public to protected to reduce the exposed API surface and align with the quality gate requirements.Source: Linters/SAST tools
src/main/java/org/gridsuite/modification/modifications/AbstractInjectionModification.java (1)
45-56: ⚡ Quick winUse
protectedvisibility for the abstract-base constructor.Line 45 does not need to be
publicon an abstract class;protectedis sufficient for subclasses and reduces exposed API.Suggested change
- public AbstractInjectionModification(String equipmentId, List<FreePropertyInfos> properties, + protected AbstractInjectionModification(String equipmentId, List<FreePropertyInfos> properties,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/AbstractInjectionModification.java` around lines 45 - 56, The AbstractInjectionModification constructor is declared as public, but since this is an abstract class it only needs to be accessible to subclasses. Change the visibility modifier from public to protected on the AbstractInjectionModification constructor to reduce the exposed API surface while still allowing subclasses to access it.Source: Linters/SAST tools
src/main/java/org/gridsuite/modification/modifications/AbstractBranchCreation.java (1)
42-49: ⚡ Quick winRestrict abstract-base constructor visibility to
protected.Line 42 exposes a
publicconstructor on an abstract class;protectedkeeps subclassing behavior and reduces unnecessary API surface.Suggested change
- public AbstractBranchCreation(String equipmentId, List<FreePropertyInfos> properties, String equipmentName, + protected AbstractBranchCreation(String equipmentId, List<FreePropertyInfos> properties, String equipmentName,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/AbstractBranchCreation.java` around lines 42 - 49, The constructor for the AbstractBranchCreation class is currently declared as public, but since it is an abstract class, the constructor visibility should be restricted to protected to prevent unnecessary direct instantiation and reduce the API surface. Change the access modifier of the AbstractBranchCreation constructor (the one taking parameters such as equipmentId, properties, equipmentName, busOrBusbarSectionId2, and other connection-related parameters) from public to protected.Source: Linters/SAST tools
src/main/java/org/gridsuite/modification/modifications/LineCreation.java (1)
101-169: ⚡ Quick winSplit
addLimitsto satisfy the quality gate.Sonar reports this method at cognitive complexity 22 vs. the allowed 15. Extract the side-selection/reporting branches into small helpers so the PR does not keep the gate failing.
Refactor direction
- if (selectedOperationalLimitsGroupId1 != null) { - if (!ModificationUtils.hasLimitSet(opLimitsGroupSide1, selectedOperationalLimitsGroupId1)) { - limitSetsOnSideReportNodes.add(ReportNode.newRootReportNode() - .withMessageTemplate("network.modification.limitSetAbsentOnSide1") - .withUntypedValue("selectedOperationalLimitsGroup", selectedOperationalLimitsGroupId1) - .withSeverity(TypedValue.WARN_SEVERITY) - .build()); - } else { - line.setSelectedOperationalLimitsGroup1(selectedOperationalLimitsGroupId1); - limitSetsOnSideReportNodes.add(ReportNode.newRootReportNode() - .withMessageTemplate("network.modification.limitSetSelectedOnSide1") - .withUntypedValue("selectedOperationalLimitsGroup1", selectedOperationalLimitsGroupId1) - .withSeverity(TypedValue.INFO_SEVERITY) - .build()); - } - } + addSelectedLimitSetReport( + selectedOperationalLimitsGroupId1, + opLimitsGroupSide1, + () -> line.setSelectedOperationalLimitsGroup1(selectedOperationalLimitsGroupId1), + "network.modification.limitSetAbsentOnSide1", + "network.modification.limitSetSelectedOnSide1", + "selectedOperationalLimitsGroup1", + limitSetsOnSideReportNodes);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/LineCreation.java` around lines 101 - 169, The addLimits method exceeds the cognitive complexity threshold of 15 with a complexity of 22, primarily due to repeated conditional logic for handling selectedOperationalLimitsGroupId1 and selectedOperationalLimitsGroupId2. Extract the two nearly identical side-selection branches (one handling selectedOperationalLimitsGroupId1 with opLimitsGroupSide1 and the other handling selectedOperationalLimitsGroupId2 with opLimitsGroupSide2) into separate private helper methods. Each helper should encapsulate the hasLimitSet check, the setSelectedOperationalLimitsGroup call, and the report node creation for that specific side, then call these helpers from addLimits to reduce its cognitive complexity below the allowed threshold.Source: Linters/SAST tools
src/main/java/org/gridsuite/modification/modifications/GeneratorModification.java (1)
475-479: ⚡ Quick winRename the local report variable to avoid shadowing the field.
SonarCloud flags
voltageRegulationOnhere because it hides the class field of the same name; renaming it keeps this method easier to read and avoids a quality-gate warning.Proposed fix
- ReportNode voltageRegulationOn = ModificationUtils.getInstance().applyElementaryModificationsAndReturnReport(generator::setVoltageRegulatorOn, generator::isVoltageRegulatorOn, + ReportNode voltageRegulationOnReport = ModificationUtils.getInstance().applyElementaryModificationsAndReturnReport(generator::setVoltageRegulatorOn, generator::isVoltageRegulatorOn, this.voltageRegulationOn, "VoltageRegulationOn"); - if (voltageRegulationOn != null) { - voltageRegulationReports.add(voltageRegulationOn); + if (voltageRegulationOnReport != null) { + voltageRegulationReports.add(voltageRegulationOnReport); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/GeneratorModification.java` around lines 475 - 479, The local variable `voltageRegulationOn` shadows the class field with the same name, which causes a code quality warning. Rename this local variable in the applyElementaryModificationsAndReturnReport call to a more specific name that reflects its purpose as a report (such as `voltageRegulationReport` or `voltageRegulationModificationReport`), then update the subsequent if condition and voltageRegulationReports.add() call to use the new variable name.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56fabdcd-1e33-4c6d-bea2-8e2a6ebf2a60
📒 Files selected for processing (115)
src/main/java/org/gridsuite/modification/dto/BalancesAdjustmentModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/BatteryCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/BatteryModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/ByFilterDeletionInfos.javasrc/main/java/org/gridsuite/modification/dto/ByFormulaModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/CreateCouplingDeviceInfos.javasrc/main/java/org/gridsuite/modification/dto/CreateVoltageLevelSectionInfos.javasrc/main/java/org/gridsuite/modification/dto/CreateVoltageLevelTopologyInfos.javasrc/main/java/org/gridsuite/modification/dto/DeleteAttachingLineInfos.javasrc/main/java/org/gridsuite/modification/dto/DeleteVoltageLevelOnLineInfos.javasrc/main/java/org/gridsuite/modification/dto/EquipmentAttributeModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/EquipmentDeletionInfos.javasrc/main/java/org/gridsuite/modification/dto/GenerationDispatchInfos.javasrc/main/java/org/gridsuite/modification/dto/GeneratorCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/GeneratorModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/GeneratorScalingInfos.javasrc/main/java/org/gridsuite/modification/dto/GroovyScriptInfos.javasrc/main/java/org/gridsuite/modification/dto/LccCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/LccModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/LineAttachToVoltageLevelInfos.javasrc/main/java/org/gridsuite/modification/dto/LineCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/LineModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/LineSplitWithVoltageLevelInfos.javasrc/main/java/org/gridsuite/modification/dto/LinesAttachToSplitLinesInfos.javasrc/main/java/org/gridsuite/modification/dto/LoadCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/LoadModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/LoadScalingInfos.javasrc/main/java/org/gridsuite/modification/dto/ModificationByAssignmentInfos.javasrc/main/java/org/gridsuite/modification/dto/ModificationReferenceInfos.javasrc/main/java/org/gridsuite/modification/dto/MoveVoltageLevelFeederBaysInfos.javasrc/main/java/org/gridsuite/modification/dto/OperatingStatusModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/ShuntCompensatorCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/ShuntCompensatorModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/StaticVarCompensatorCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/SubstationCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/SubstationModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/TwoWindingsTransformerCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/TwoWindingsTransformerModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/VoltageInitModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/VoltageLevelCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/VoltageLevelModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/VoltageLevelTopologyModificationInfos.javasrc/main/java/org/gridsuite/modification/dto/VscCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/VscModificationInfos.javasrc/main/java/org/gridsuite/modification/modifications/AbstractBranchCreation.javasrc/main/java/org/gridsuite/modification/modifications/AbstractBranchModification.javasrc/main/java/org/gridsuite/modification/modifications/AbstractEquipmentBase.javasrc/main/java/org/gridsuite/modification/modifications/AbstractEquipmentCreation.javasrc/main/java/org/gridsuite/modification/modifications/AbstractEquipmentModification.javasrc/main/java/org/gridsuite/modification/modifications/AbstractInjectionCreation.javasrc/main/java/org/gridsuite/modification/modifications/AbstractInjectionModification.javasrc/main/java/org/gridsuite/modification/modifications/AbstractModification.javasrc/main/java/org/gridsuite/modification/modifications/AbstractScaling.javasrc/main/java/org/gridsuite/modification/modifications/BalancesAdjustmentModification.javasrc/main/java/org/gridsuite/modification/modifications/BatteryCreation.javasrc/main/java/org/gridsuite/modification/modifications/BatteryModification.javasrc/main/java/org/gridsuite/modification/modifications/ByFilterDeletion.javasrc/main/java/org/gridsuite/modification/modifications/CreateCouplingDevice.javasrc/main/java/org/gridsuite/modification/modifications/CreateVoltageLevelSection.javasrc/main/java/org/gridsuite/modification/modifications/CreateVoltageLevelTopology.javasrc/main/java/org/gridsuite/modification/modifications/DeleteAttachingLine.javasrc/main/java/org/gridsuite/modification/modifications/DeleteVoltageLevelOnLine.javasrc/main/java/org/gridsuite/modification/modifications/EquipmentAttributeModification.javasrc/main/java/org/gridsuite/modification/modifications/EquipmentDeletion.javasrc/main/java/org/gridsuite/modification/modifications/GenerationDispatch.javasrc/main/java/org/gridsuite/modification/modifications/GeneratorCreation.javasrc/main/java/org/gridsuite/modification/modifications/GeneratorModification.javasrc/main/java/org/gridsuite/modification/modifications/GeneratorScaling.javasrc/main/java/org/gridsuite/modification/modifications/GroovyScript.javasrc/main/java/org/gridsuite/modification/modifications/InjectionCreation.javasrc/main/java/org/gridsuite/modification/modifications/InjectionModification.javasrc/main/java/org/gridsuite/modification/modifications/LccCreation.javasrc/main/java/org/gridsuite/modification/modifications/LccModification.javasrc/main/java/org/gridsuite/modification/modifications/LineAttachToVoltageLevel.javasrc/main/java/org/gridsuite/modification/modifications/LineCreation.javasrc/main/java/org/gridsuite/modification/modifications/LineModification.javasrc/main/java/org/gridsuite/modification/modifications/LineSplitWithVoltageLevel.javasrc/main/java/org/gridsuite/modification/modifications/LinesAttachToSplitLines.javasrc/main/java/org/gridsuite/modification/modifications/LoadCreation.javasrc/main/java/org/gridsuite/modification/modifications/LoadModification.javasrc/main/java/org/gridsuite/modification/modifications/LoadScaling.javasrc/main/java/org/gridsuite/modification/modifications/ModificationReference.javasrc/main/java/org/gridsuite/modification/modifications/MoveVoltageLevelFeederBays.javasrc/main/java/org/gridsuite/modification/modifications/OperatingStatusModification.javasrc/main/java/org/gridsuite/modification/modifications/ShuntCompensatorCreation.javasrc/main/java/org/gridsuite/modification/modifications/ShuntCompensatorModification.javasrc/main/java/org/gridsuite/modification/modifications/StaticVarCompensatorCreation.javasrc/main/java/org/gridsuite/modification/modifications/SubstationCreation.javasrc/main/java/org/gridsuite/modification/modifications/SubstationModification.javasrc/main/java/org/gridsuite/modification/modifications/TwoWindingsTransformerCreation.javasrc/main/java/org/gridsuite/modification/modifications/TwoWindingsTransformerModification.javasrc/main/java/org/gridsuite/modification/modifications/VoltageInitModification.javasrc/main/java/org/gridsuite/modification/modifications/VoltageLevelCreation.javasrc/main/java/org/gridsuite/modification/modifications/VoltageLevelModification.javasrc/main/java/org/gridsuite/modification/modifications/VoltageLevelTopologyModification.javasrc/main/java/org/gridsuite/modification/modifications/VscCreation.javasrc/main/java/org/gridsuite/modification/modifications/VscModification.javasrc/main/java/org/gridsuite/modification/modifications/byfilter/AbstractModificationByAssignment.javasrc/main/java/org/gridsuite/modification/modifications/byfilter/ByFormulaModification.javasrc/main/java/org/gridsuite/modification/modifications/byfilter/ModificationByAssignment.javasrc/main/java/org/gridsuite/modification/modifications/data/AbstractInjectionCreation.javasrc/main/java/org/gridsuite/modification/modifications/data/AbstractInjectionModification.javasrc/main/java/org/gridsuite/modification/modifications/data/LccConverterStationCreation.javasrc/main/java/org/gridsuite/modification/modifications/data/LccConverterStationModification.javasrc/main/java/org/gridsuite/modification/modifications/data/VscConverterStationCreation.javasrc/main/java/org/gridsuite/modification/modifications/data/VscConverterStationModification.javasrc/main/java/org/gridsuite/modification/utils/ModificationUtils.javasrc/test/java/org/gridsuite/modification/modifications/CreateVoltageLevelSectionTest.javasrc/test/java/org/gridsuite/modification/modifications/CreateVoltageLevelTopologyTest.javasrc/test/java/org/gridsuite/modification/modifications/GenerationDispatchTest.javasrc/test/java/org/gridsuite/modification/modifications/LccModificationTest.javasrc/test/java/org/gridsuite/modification/modifications/MoveVoltageLevelFeederBaysTest.javasrc/test/java/org/gridsuite/modification/modifications/TwoWindingsTransformerModificationTest.javasrc/test/java/org/gridsuite/modification/modifications/VoltageLevelTopologyModificationTest.javasrc/test/java/org/gridsuite/modification/modifications/VscModificationTest.java
Comment on lines
+72
to
+77
| this.lossCoefficient = lossCoefficient; | ||
| this.defaultOutageRate = defaultOutageRate; | ||
| this.generatorsWithoutOutage = generatorsWithoutOutage; | ||
| this.generatorsWithFixedSupply = generatorsWithFixedSupply; | ||
| this.generatorsFrequencyReserve = generatorsFrequencyReserve; | ||
| this.substationsGeneratorsOrdering = substationsGeneratorsOrdering; |
There was a problem hiding this comment.
Handle omitted builder values before check/apply.
The new direct fields are nullable: omitted lossCoefficient/defaultOutageRate are unboxed in check(), and omitted filter lists are iterated in collectGeneratorsWithFrequencyReserve() and checkMissingFilters().
Proposed fix
- this.generatorsWithoutOutage = generatorsWithoutOutage;
- this.generatorsWithFixedSupply = generatorsWithFixedSupply;
- this.generatorsFrequencyReserve = generatorsFrequencyReserve;
- this.substationsGeneratorsOrdering = substationsGeneratorsOrdering;
+ this.generatorsWithoutOutage = generatorsWithoutOutage == null ? List.of() : generatorsWithoutOutage;
+ this.generatorsWithFixedSupply = generatorsWithFixedSupply == null ? List.of() : generatorsWithFixedSupply;
+ this.generatorsFrequencyReserve = generatorsFrequencyReserve == null ? List.of() : generatorsFrequencyReserve;
+ this.substationsGeneratorsOrdering = substationsGeneratorsOrdering == null ? List.of() : substationsGeneratorsOrdering;
@@
- if (lossCoefficient < 0. || lossCoefficient > 100.) {
+ if (lossCoefficient == null || lossCoefficient < 0. || lossCoefficient > 100.) {
throw new NetworkModificationException(GENERATION_DISPATCH_ERROR, "The loss coefficient must be between 0 and 100");
}
- if (defaultOutageRate < 0. || defaultOutageRate > 100.) {
+ if (defaultOutageRate == null || defaultOutageRate < 0. || defaultOutageRate > 100.) {
throw new NetworkModificationException(GENERATION_DISPATCH_ERROR, "The default outage rate must be between 0 and 100");
}Also applies to: 367-370, 420-423, 474-478
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/org/gridsuite/modification/modifications/GenerationDispatch.java`
around lines 72 - 77, The nullable builder fields assigned in the constructor
(lossCoefficient, defaultOutageRate, generatorsWithoutOutage,
generatorsWithFixedSupply, generatorsFrequencyReserve,
substationsGeneratorsOrdering) are not being properly handled before use. These
fields are being unboxed in the check() method and iterated in the
collectGeneratorsWithFrequencyReserve() and checkMissingFilters() methods
without null checks. Add null handling in the constructor by assigning default
values when the builder values are omitted (null), such as using empty
collections for the filter lists and appropriate default numeric values for
lossCoefficient and defaultOutageRate.
Comment on lines
+61
to
+68
| if (maximumSectionCount < 1) { | ||
| throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, "Maximum section count should be greater or equal to 1"); | ||
| } | ||
|
|
||
| if (modificationInfos.getSectionCount() < 0 || modificationInfos.getSectionCount() > modificationInfos.getMaximumSectionCount()) { | ||
| if (sectionCount < 0 || sectionCount > maximumSectionCount) { | ||
| throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, String.format("Section count should be between 0 and Maximum section count (%d), actual : %d", | ||
| modificationInfos.getMaximumSectionCount(), | ||
| modificationInfos.getSectionCount())); | ||
| maximumSectionCount, | ||
| sectionCount)); |
There was a problem hiding this comment.
Validate required shunt fields before unboxing them.
With the new builder, maximumSectionCount, sectionCount, and maxSusceptance can be absent. Lines 61 and 65 can throw before returning a domain error, and maxSusceptance / maximumSectionCount later has the same risk when susceptance cannot be computed.
Proposed fix
+ if (maximumSectionCount == null) {
+ throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, "Maximum section count is required");
+ }
if (maximumSectionCount < 1) {
throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, "Maximum section count should be greater or equal to 1");
}
+ if (sectionCount == null) {
+ throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, "Section count is required");
+ }
if (sectionCount < 0 || sectionCount > maximumSectionCount) {
throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, String.format("Section count should be between 0 and Maximum section count (%d), actual : %d",
maximumSectionCount,
sectionCount));
}
+ if (maxSusceptance == null && (maxQAtNominalV == null || shuntCompensatorType == null)) {
+ throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, "Max susceptance is required when max Q and shunt compensator type are not both provided");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (maximumSectionCount < 1) { | |
| throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, "Maximum section count should be greater or equal to 1"); | |
| } | |
| if (modificationInfos.getSectionCount() < 0 || modificationInfos.getSectionCount() > modificationInfos.getMaximumSectionCount()) { | |
| if (sectionCount < 0 || sectionCount > maximumSectionCount) { | |
| throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, String.format("Section count should be between 0 and Maximum section count (%d), actual : %d", | |
| modificationInfos.getMaximumSectionCount(), | |
| modificationInfos.getSectionCount())); | |
| maximumSectionCount, | |
| sectionCount)); | |
| if (maximumSectionCount == null) { | |
| throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, "Maximum section count is required"); | |
| } | |
| if (maximumSectionCount < 1) { | |
| throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, "Maximum section count should be greater or equal to 1"); | |
| } | |
| if (sectionCount == null) { | |
| throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, "Section count is required"); | |
| } | |
| if (sectionCount < 0 || sectionCount > maximumSectionCount) { | |
| throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, String.format("Section count should be between 0 and Maximum section count (%d), actual : %d", | |
| maximumSectionCount, | |
| sectionCount)); | |
| } | |
| if (maxSusceptance == null && (maxQAtNominalV == null || shuntCompensatorType == null)) { | |
| throw new NetworkModificationException(CREATE_SHUNT_COMPENSATOR_ERROR, "Max susceptance is required when max Q and shunt compensator type are not both provided"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/org/gridsuite/modification/modifications/ShuntCompensatorCreation.java`
around lines 61 - 68, The ShuntCompensatorCreation class needs to validate that
required fields are present before attempting to unbox or use them. Before
checking if maximumSectionCount is greater than or equal to 1 (line 61) and
before validating sectionCount against maximumSectionCount (line 65), add null
or Optional presence checks for these fields and throw a
NetworkModificationException with CREATE_SHUNT_COMPENSATOR_ERROR if they are
absent. Similarly, check that maxSusceptance is present before any division or
arithmetic operations involving it and maximumSectionCount to prevent null
pointer exceptions when unboxing these potentially absent values.
Comment on lines
276
to
284
| VscConverterStation vscConverterStation = voltageLevel.newVscConverterStation() | ||
| .setId(converterStationCreationInfos.getEquipmentId()) | ||
| .setName(converterStationCreationInfos.getEquipmentName()) | ||
| .setVoltageRegulatorOn(converterStationCreationInfos.getVoltageRegulationOn()) | ||
| .setReactivePowerSetpoint(converterStationCreationInfos.getReactivePowerSetpoint()) | ||
| .setId(vscConverterStationCreation.getEquipmentId()) | ||
| .setName(vscConverterStationCreation.getEquipmentName()) | ||
| .setVoltageRegulatorOn(vscConverterStationCreation.getVoltageRegulationOn()) | ||
| .setReactivePowerSetpoint(vscConverterStationCreation.getReactivePowerSetpoint()) | ||
| .setBus(bus.getId()) | ||
| .setLossFactor(converterStationCreationInfos.getLossFactor()) | ||
| .setVoltageSetpoint(converterStationCreationInfos.getVoltageSetpoint()) | ||
| .setLossFactor(vscConverterStationCreation.getLossFactor()) | ||
| .setVoltageSetpoint(vscConverterStationCreation.getVoltageSetpoint()) | ||
| .add(); |
There was a problem hiding this comment.
Mirror the node-breaker null guards in bus-breaker creation.
The node-breaker path skips nullable reactivePowerSetpoint, lossFactor, and voltageSetpoint, but the bus-breaker path passes them unconditionally. A VSC with optional values omitted can fail only in bus-breaker topology.
Proposed fix
- VscConverterStation vscConverterStation = voltageLevel.newVscConverterStation()
+ VscConverterStationAdder converterStationAdder = voltageLevel.newVscConverterStation()
.setId(vscConverterStationCreation.getEquipmentId())
.setName(vscConverterStationCreation.getEquipmentName())
.setVoltageRegulatorOn(vscConverterStationCreation.getVoltageRegulationOn())
- .setReactivePowerSetpoint(vscConverterStationCreation.getReactivePowerSetpoint())
- .setBus(bus.getId())
- .setLossFactor(vscConverterStationCreation.getLossFactor())
- .setVoltageSetpoint(vscConverterStationCreation.getVoltageSetpoint())
- .add();
+ .setBus(bus.getId());
+
+ if (vscConverterStationCreation.getReactivePowerSetpoint() != null) {
+ converterStationAdder.setReactivePowerSetpoint(vscConverterStationCreation.getReactivePowerSetpoint());
+ }
+ if (vscConverterStationCreation.getLossFactor() != null) {
+ converterStationAdder.setLossFactor(vscConverterStationCreation.getLossFactor());
+ }
+ if (vscConverterStationCreation.getVoltageSetpoint() != null) {
+ converterStationAdder.setVoltageSetpoint(vscConverterStationCreation.getVoltageSetpoint());
+ }
+
+ VscConverterStation vscConverterStation = converterStationAdder.add();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| VscConverterStation vscConverterStation = voltageLevel.newVscConverterStation() | |
| .setId(converterStationCreationInfos.getEquipmentId()) | |
| .setName(converterStationCreationInfos.getEquipmentName()) | |
| .setVoltageRegulatorOn(converterStationCreationInfos.getVoltageRegulationOn()) | |
| .setReactivePowerSetpoint(converterStationCreationInfos.getReactivePowerSetpoint()) | |
| .setId(vscConverterStationCreation.getEquipmentId()) | |
| .setName(vscConverterStationCreation.getEquipmentName()) | |
| .setVoltageRegulatorOn(vscConverterStationCreation.getVoltageRegulationOn()) | |
| .setReactivePowerSetpoint(vscConverterStationCreation.getReactivePowerSetpoint()) | |
| .setBus(bus.getId()) | |
| .setLossFactor(converterStationCreationInfos.getLossFactor()) | |
| .setVoltageSetpoint(converterStationCreationInfos.getVoltageSetpoint()) | |
| .setLossFactor(vscConverterStationCreation.getLossFactor()) | |
| .setVoltageSetpoint(vscConverterStationCreation.getVoltageSetpoint()) | |
| .add(); | |
| VscConverterStationAdder converterStationAdder = voltageLevel.newVscConverterStation() | |
| .setId(vscConverterStationCreation.getEquipmentId()) | |
| .setName(vscConverterStationCreation.getEquipmentName()) | |
| .setVoltageRegulatorOn(vscConverterStationCreation.getVoltageRegulationOn()) | |
| .setBus(bus.getId()); | |
| if (vscConverterStationCreation.getReactivePowerSetpoint() != null) { | |
| converterStationAdder.setReactivePowerSetpoint(vscConverterStationCreation.getReactivePowerSetpoint()); | |
| } | |
| if (vscConverterStationCreation.getLossFactor() != null) { | |
| converterStationAdder.setLossFactor(vscConverterStationCreation.getLossFactor()); | |
| } | |
| if (vscConverterStationCreation.getVoltageSetpoint() != null) { | |
| converterStationAdder.setVoltageSetpoint(vscConverterStationCreation.getVoltageSetpoint()); | |
| } | |
| VscConverterStation vscConverterStation = converterStationAdder.add(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/org/gridsuite/modification/modifications/VscCreation.java`
around lines 276 - 284, The bus-breaker VSC converter station creation in the
VscCreation class is setting reactivePowerSetpoint, lossFactor, and
voltageSetpoint unconditionally in the newVscConverterStation() builder chain,
while the node-breaker path properly checks these fields for null before setting
them. Add null guards for these three optional fields in the bus-breaker
creation logic, only calling the setter methods for reactivePowerSetpoint,
lossFactor, and voltageSetpoint when they are not null, mirroring the defensive
null-checking pattern used in the node-breaker path.
Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com> Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
…ion5-redeclare-business-fields-in-modifications
Signed-off-by: Joris Mancini <joris.mancini_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 (1)
src/main/java/org/gridsuite/modification/modifications/data/VscConverterStationModification.java (1)
32-32:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDTO type leakage reintroduces reverse coupling.
VscConverterStationModification(Line 32) still exposesReactiveCapabilityCurvePointsInfosfrom the DTO package, which conflicts with the stated one-way dependency direction goal. Prefer a modification-layer type here and keep DTO mapping at the boundary layer.This is based on the PR objective to avoid modification → DTO dependencies.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/gridsuite/modification/modifications/data/VscConverterStationModification.java` at line 32, The field reactiveCapabilityCurvePoints in the VscConverterStationModification class is using ReactiveCapabilityCurvePointsInfos, which is a DTO type, creating an unwanted reverse dependency from the modification layer back to the DTO package. Replace the ReactiveCapabilityCurvePointsInfos type with an appropriate modification-layer type that does not reference DTO classes. Move the DTO-to-modification mapping logic to the boundary layer (service or converter classes) where DTO dependencies are expected.
🤖 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/modification/modifications/data/VscConverterStationModification.java`:
- Line 32: The field reactiveCapabilityCurvePoints in the
VscConverterStationModification class is using
ReactiveCapabilityCurvePointsInfos, which is a DTO type, creating an unwanted
reverse dependency from the modification layer back to the DTO package. Replace
the ReactiveCapabilityCurvePointsInfos type with an appropriate
modification-layer type that does not reference DTO classes. Move the
DTO-to-modification mapping logic to the boundary layer (service or converter
classes) where DTO dependencies are expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db4b8c37-a710-4603-8c86-5d977880f0a9
📒 Files selected for processing (27)
README.mdsrc/main/java/org/gridsuite/modification/dto/LineCreationInfos.javasrc/main/java/org/gridsuite/modification/dto/LineModificationInfos.javasrc/main/java/org/gridsuite/modification/modifications/AbstractBranchCreation.javasrc/main/java/org/gridsuite/modification/modifications/AbstractBranchModification.javasrc/main/java/org/gridsuite/modification/modifications/AbstractEquipmentBase.javasrc/main/java/org/gridsuite/modification/modifications/AbstractEquipmentCreation.javasrc/main/java/org/gridsuite/modification/modifications/AbstractEquipmentModification.javasrc/main/java/org/gridsuite/modification/modifications/AbstractInjectionCreation.javasrc/main/java/org/gridsuite/modification/modifications/AbstractInjectionModification.javasrc/main/java/org/gridsuite/modification/modifications/GenerationDispatch.javasrc/main/java/org/gridsuite/modification/modifications/LccModification.javasrc/main/java/org/gridsuite/modification/modifications/LineCreation.javasrc/main/java/org/gridsuite/modification/modifications/LineModification.javasrc/main/java/org/gridsuite/modification/modifications/ShuntCompensatorCreation.javasrc/main/java/org/gridsuite/modification/modifications/TwoWindingsTransformerModification.javasrc/main/java/org/gridsuite/modification/modifications/VoltageInitModification.javasrc/main/java/org/gridsuite/modification/modifications/VscCreation.javasrc/main/java/org/gridsuite/modification/modifications/VscModification.javasrc/main/java/org/gridsuite/modification/modifications/data/AbstractInjectionCreation.javasrc/main/java/org/gridsuite/modification/modifications/data/AbstractInjectionModification.javasrc/main/java/org/gridsuite/modification/modifications/data/LccConverterStationCreation.javasrc/main/java/org/gridsuite/modification/modifications/data/LccConverterStationModification.javasrc/main/java/org/gridsuite/modification/modifications/data/VscConverterStationCreation.javasrc/main/java/org/gridsuite/modification/modifications/data/VscConverterStationModification.javasrc/main/java/org/gridsuite/modification/utils/MeasurementUtils.javasrc/test/java/org/gridsuite/modification/modifications/VscModificationTest.java
💤 Files with no reviewable changes (2)
- src/main/java/org/gridsuite/modification/dto/LineCreationInfos.java
- src/main/java/org/gridsuite/modification/dto/LineModificationInfos.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/gridsuite/modification/modifications/AbstractEquipmentCreation.java
🚧 Files skipped from review as they are similar to previous changes (18)
- src/main/java/org/gridsuite/modification/modifications/AbstractEquipmentBase.java
- src/main/java/org/gridsuite/modification/modifications/AbstractEquipmentModification.java
- src/main/java/org/gridsuite/modification/modifications/AbstractBranchCreation.java
- src/main/java/org/gridsuite/modification/modifications/data/AbstractInjectionCreation.java
- src/test/java/org/gridsuite/modification/modifications/VscModificationTest.java
- src/main/java/org/gridsuite/modification/modifications/AbstractInjectionCreation.java
- src/main/java/org/gridsuite/modification/modifications/data/LccConverterStationCreation.java
- src/main/java/org/gridsuite/modification/modifications/data/LccConverterStationModification.java
- src/main/java/org/gridsuite/modification/modifications/data/VscConverterStationCreation.java
- src/main/java/org/gridsuite/modification/modifications/data/AbstractInjectionModification.java
- src/main/java/org/gridsuite/modification/modifications/VoltageInitModification.java
- src/main/java/org/gridsuite/modification/modifications/ShuntCompensatorCreation.java
- src/main/java/org/gridsuite/modification/modifications/LineCreation.java
- src/main/java/org/gridsuite/modification/modifications/GenerationDispatch.java
- src/main/java/org/gridsuite/modification/modifications/LccModification.java
- src/main/java/org/gridsuite/modification/modifications/VscCreation.java
- src/main/java/org/gridsuite/modification/modifications/VscModification.java
- src/main/java/org/gridsuite/modification/modifications/TwoWindingsTransformerModification.java
EtienneLt
reviewed
Jun 19, 2026
…rmonize code-style Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
…-fields-in-modifications
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
| ShuntCompensatorModificationInfos shuntCompensatorModificationInfos = (ShuntCompensatorModificationInfos) modificationInfos; | ||
| List<ReportNode> reports = new ArrayList<>(); | ||
| ShuntCompensatorLinearModel model = shuntCompensator.getModel(ShuntCompensatorLinearModel.class); | ||
| var shuntCompensatorType = model.getBPerSection() > 0 ? ShuntCompensatorType.CAPACITOR : ShuntCompensatorType.REACTOR; |
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
| TwoWindingsTransformer twt, | ||
| ReportNode subReportNode, | ||
| boolean isModification) { | ||
| PhaseTapChanger phaseTapChanger = isModification ? twt.getPhaseTapChanger() : null; |
EtienneLt
reviewed
Jun 22, 2026
| boolean isModification) { | ||
| RatioTapChangerModificationInfos ratioTapChangerInfos = twoWindingsTransformerModificationInfos.getRatioTapChanger(); | ||
| RatioTapChangerModificationInfos ratioTapChangerInfos = ratioTapChanger; | ||
| RatioTapChanger ratioTapChanger = isModification ? twt.getRatioTapChanger() : null; |
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
| @Override | ||
| public void check(Network network) throws NetworkModificationException { | ||
| if (voltageInitModificationInfos == null) { | ||
| if (generators.isEmpty() && transformers.isEmpty() && staticVarCompensators.isEmpty() |
Contributor
There was a problem hiding this comment.
if you do that here you should do it in other modification class
Contributor
Author
There was a problem hiding this comment.
I did that towards a Rabbit remark, but you're right that's a touchy topic so I will just let it as it was before with non-null checks on lists
Contributor
Author
There was a problem hiding this comment.
As I said in the oher comment it deserves a whole dedicated topic on how to handle null values in modifications
Contributor
Author
There was a problem hiding this comment.
I don't really know how to replace this check though, for me it does not make sense to check that voltageModificationInfos is null, we used to build the modification from the dto itself how could it be null then ? ^^
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
EtienneLt
reviewed
Jun 22, 2026
benrejebmoh
requested changes
Jun 22, 2026
Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
TheMaskedTurtle
force-pushed
the
jorism/refactor/solution5-redeclare-business-fields-in-modifications
branch
from
74b3bc0 to
a8ab4dd
Compare
…ion5-redeclare-business-fields-in-modifications
|
benrejebmoh
approved these changes
Jun 24, 2026
TheMaskedTurtle
deleted the
jorism/refactor/solution5-redeclare-business-fields-in-modifications
branch
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
3 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
The point here is to reduce dependencies between modifications (AbstractModification inheritance) and DTOs (ModificationInfos inheritance) and to make it only one-way : only DTOs depend on modifications.
This is not achieved completely here for concision sake, but, for now (except for CompositeModification and TabularModification, see below) at least no inheritor of AbstractModification imports any inheritor of ModificationInfos.