feat(filter-rework): introduce reworked expert filter#115
Merged
Conversation
📝 WalkthroughWalkthroughThis PR implements a comprehensive expert-rule filtering framework for the gridsuite filter module. It introduces a typed ExpertRule interface with polymorphic Jackson serialization, adds seven concrete rule implementations (boolean, enum, string, number, properties, combinator, and filter-reference), refactors the Filter interface to use default evaluate() methods delegating to rule predicates, and provides extensive test coverage via parameterized validators. Test network fixtures are enriched with additional properties and tap-changer configurations to support rule evaluation scenarios. ChangesExpert Filter Framework
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
KoloMenek
force-pushed
the
marutk/feat/introduce_reworked_expert_filter
branch
from
c30a4f0 to
c1654ca
Compare
Member
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: 11
🧹 Nitpick comments (7)
src/test/java/org/gridsuite/filter/wip/expert/rules/StringExpertRuleTest.java (1)
86-91: 💤 Low valueMisleading test name.
testGetDataTypeReturnsEnumassertsDataType.STRING— looks copy-pasted fromEnumExpertRuleTest. Rename totestGetDataTypeReturnsStringfor clarity when the test fails.♻️ Rename
- void testGetDataTypeReturnsEnum() { + void testGetDataTypeReturnsString() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/filter/wip/expert/rules/StringExpertRuleTest.java` around lines 86 - 91, Rename the misleading test method in StringExpertRuleTest: change the test method currently named testGetDataTypeReturnsEnum to testGetDataTypeReturnsString; update the method declaration in the class containing the assertion that StringExpertRule.of(...).getDataType() equals DataType.STRING so the name accurately reflects the asserted behavior (ensure any references or annotations remain intact and only the method name is changed).src/test/java/org/gridsuite/filter/wip/TestNetworkUtils.java (3)
25-43: ⚖️ Poor tradeoff
getEquipmentFromTestNetworkrebuilds the entire network on every call.Each parameterized case calls this and triggers a full
createTestNetwork()build (substations, VLs, dozens of equipments). Across all rule test suites this is hundreds of redundant builds. Since the network is only read during evaluation, consider building it once and reusing it (e.g., a lazily-initialized static holder), keeping per-test isolation in mind if any test ever mutates it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/filter/wip/TestNetworkUtils.java` around lines 25 - 43, getEquipmentFromTestNetwork currently calls createTestNetwork() on every invocation causing expensive repeated builds; change it to reuse a lazily-initialized static Network instance (e.g., a private static volatile Network TEST_NETWORK or an AtomicReference) that is constructed once on first access and returned by a getter used by getEquipmentFromTestNetwork, ensure initialization is thread-safe (double-checked locking or synchronized block) and document that tests must not mutate TEST_NETWORK (or create a copy when mutation is needed) so per-test isolation is preserved.
212-233: 💤 Low valueCommented-out tap-changer setup.
This block is intentionally disabled (the
NOT_EXISTScases forRATIO_TARGET_V*/LOAD_TAP_CHANGING_CAPABILITIES_*rely on its absence), but leaving large commented blocks in test fixtures is a maintenance hazard. Consider removing it or replacing with a brief comment explaining why 3WT legs deliberately have no ratio tap changer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/filter/wip/TestNetworkUtils.java` around lines 212 - 233, The large commented-out tap-changer setup for three-winding transformers (the network.getThreeWindingsTransformer("THREE_WINDINGS_TRANSFORMER_" + i) blocks) is a maintenance hazard; either remove the dead code or replace it with a concise explanatory comment stating that the three-winding transformer legs intentionally have no RatioTapChanger because tests assert NOT_EXISTS for RATIO_TARGET_V* / LOAD_TAP_CHANGING_CAPABILITIES_*. Edit TestNetworkUtils.java to delete the commented block or add a one-line comment near the creation of THREE_WINDINGS_TRANSFORMER_* explaining the deliberate absence of ratio tap changers and referencing the related assertions (RATIO_TARGET_V*, LOAD_TAP_CHANGING_CAPABILITIES_*).
256-264: ⚡ Quick winRemove compile-break concern for boundary/dangling line APIs; consider caching the test network creation.
TestNetworkUtilsusesnetwork.getBoundaryLine(equipmentId)andvl1.newBoundaryLine()(lines 38, 256-264) and there are no*DanglingLine*usages in the repo, matching the IIDM 1.16 “DanglingLine → BoundaryLine” rename.- Optional:
getEquipmentFromTestNetwork(lines 25-43) recreates the full test network on each call; cache/create once per test to reduce overhead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/filter/wip/TestNetworkUtils.java` around lines 256 - 264, The code should consistently use the IIDM 1.16 BoundaryLine API instead of any DanglingLine types: ensure calls use network.getBoundaryLine(equipmentId) and call VoltageLevel.newBoundaryLine() (as in vl1.newBoundaryLine()) so compilation won't break due to the rename; also optimize getEquipmentFromTestNetwork by memoizing/ caching the created test network (e.g. a private static field returned on subsequent calls) so the full network isn't rebuilt on each invocation.src/main/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRule.java (1)
43-48: ⚡ Quick winDefensively copy
referenceValuesfor immutability.Unlike
EnumExpertRule, which storesSet.copyOf(referenceValues), this constructor retains the caller-provided set by reference. A caller mutating the original set after construction would silently alter this rule's behavior. Copy it to keep the rule immutable and consistent with the sibling implementation.♻️ Proposed fix
- this.referenceValues = referenceValues; + this.referenceValues = Set.copyOf(referenceValues);🤖 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/filter/wip/expert/rules/PropertiesExpertRule.java` around lines 43 - 48, The PropertiesExpertRule constructor currently assigns the caller-provided Set directly to the referenceValues field, risking external mutation; change the constructor in PropertiesExpertRule to defensively copy the incoming referenceValues (e.g., via Set.copyOf(referenceValues) or a new HashSet<>(referenceValues)) before assigning to the field so the rule is immutable and consistent with EnumExpertRule.src/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.java (1)
65-72: 💤 Low valueConsider validating propertyName for FREE_PROPERTIES.
When
fieldTypeisFREE_PROPERTIES, the code callsidentifiable.getProperty(propertyName)without validating thatpropertyNameis non-null. If the caller passesnull, this could lead to unexpected behavior depending on the IIDM API's handling of null property names.🛡️ Proposed validation
static String getStringFieldValue(Identifiable<?> identifiable, String propertyName, FieldType fieldType) { return switch (fieldType) { case ID -> identifiable.getId(); case NAME -> identifiable.getOptionalName().orElse(null); - case FREE_PROPERTIES -> identifiable.getProperty(propertyName); + case FREE_PROPERTIES -> { + if (propertyName == null) { + throw new PowsyblException("Property name must not be null for FREE_PROPERTIES field type"); + } + yield identifiable.getProperty(propertyName); + } default -> getIdentifiableStringFieldValue(identifiable, propertyName, fieldType); }; }🤖 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/filter/wip/expert/rules/ExpertRuleUtils.java` around lines 65 - 72, The FREE_PROPERTIES branch in getStringFieldValue calls identifiable.getProperty(propertyName) without checking propertyName; add a null-check for propertyName in getStringFieldValue and if null throw an IllegalArgumentException (or return null per project convention) with a clear message referencing propertyName and FREE_PROPERTIES so callers know they passed an invalid argument; update the FREE_PROPERTIES case to validate propertyName before calling identifiable.getProperty(propertyName).src/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.java (1)
88-106: ⚡ Quick winClarify EXISTS/NOT_EXISTS semantics for empty strings.
Lines 100-101 define
EXISTSto return!fieldValue.isEmpty()andNOT_EXISTSto returnfieldValue.isEmpty(). This means:
- A field with an empty string
""is treated as not existing (EXISTS→ false)- However, line 92 treats
nullas not existing (NOT_EXISTS→ true)This creates an ambiguity: does "exists" mean "non-null" or "non-null and non-empty"? For consistency with
BooleanExpertRuleandNumberExpertRule(which return true/false for EXISTS/NOT_EXISTS based solely on null), consider whether empty strings should be treated as existing.♻️ Alternative: treat empty strings as existing
return switch (operatorType) { case IS -> Strings.CI.equals(fieldValue, referenceValue); case CONTAINS -> Strings.CI.contains(fieldValue, referenceValue); case BEGINS_WITH -> Strings.CI.startsWith(fieldValue, referenceValue); case ENDS_WITH -> Strings.CI.endsWith(fieldValue, referenceValue); - case EXISTS -> !fieldValue.isEmpty(); - case NOT_EXISTS -> fieldValue.isEmpty(); + case EXISTS -> true; + case NOT_EXISTS -> false; case IN -> referenceValues.stream().anyMatch(fieldValue::equalsIgnoreCase); case NOT_IN -> referenceValues.stream().noneMatch(fieldValue::equalsIgnoreCase); default -> throw unsupportedOperatorException(); };This aligns with the behavior of
BooleanExpertRuleandNumberExpertRule, whereEXISTSchecks for non-null andNOT_EXISTSchecks for null.🤖 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/filter/wip/expert/rules/StringExpertRule.java` around lines 88 - 106, StringExpertRule.evaluateRule currently treats empty strings as non-existing while nulls are treated as not existing inconsistently with BooleanExpertRule/NumberExpertRule; change the EXISTS/NOT_EXISTS branches to treat any non-null value (including empty string) as existing: in the switch inside evaluateRule update case EXISTS to return true and case NOT_EXISTS to return false (keeping the initial null check that returns operatorType.equals(OperatorType.NOT_EXISTS)); this aligns semantics with ExpertRuleUtils.getStringFieldValue and the other ExpertRule classes.
🤖 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.
Inline comments:
In `@src/main/java/org/gridsuite/filter/wip/AbstractFilter.java`:
- Around line 37-39: AbstractFilter's `@JsonSubTypes` only registers
IdentifierListFilter, so incoming JSON with "type":"EXPERT" (FilterType.EXPERT)
won't deserialize to wip.expert.ExpertFilter; fix by importing
org.gridsuite.filter.wip.expert.ExpertFilter and add a `@JsonSubTypes.Type` entry
for ExpertFilter with name "EXPERT" alongside the existing IdentifierListFilter
registration in the AbstractFilter class so Jackson can map the "EXPERT" type to
ExpertFilter.
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/BooleanExpertRule.java`:
- Around line 32-40: The of factory in BooleanExpertRule currently rejects null
referenceValue for all operators; update BooleanExpertRule.of so that
referenceValue is only required non-null for operators other than EXISTS and
NOT_EXISTS (i.e., allow null when operatorType is EXISTS or NOT_EXISTS), while
keeping the existing checks that fieldType is non-null, operatorType is
non-null, DataType.BOOLEAN and operatorType ∈ SUPPORTED_OPERATORS; throw the
same PowsyblException when the adjusted validation fails to mirror
NumberExpertRule and StringExpertRule behavior.
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.java`:
- Around line 281-292: In getShuntCompensatorStringFieldValue, guard against a
null ShuntCompensatorLinearModel before calling getBPerSection(): after
obtaining shuntCompensatorLinearModel from
shuntCompensator.getModel(ShuntCompensatorLinearModel.class) check if it is null
and handle it (e.g., throw a PowsyblException with a clear message using
FIELD_RETRIEVAL_NOT_SUPPORTED_TEMPLATE or another descriptive message) instead
of calling shuntCompensatorLinearModel.getBPerSection() when the model is
absent.
- Around line 450-473: getStaticVarCompensatorDoubleFieldValue uses nominalV
(from getTerminalDoubleFieldValue) without null checking in the
FIX_Q_AT_NOMINAL_V case; change that case in the switch inside
getStaticVarCompensatorDoubleFieldValue to guard against nominalV == null (e.g.,
return Double.NaN when nominalV is null) before calling Math.pow, so replace
Math.pow(nominalV, 2) * standbyAutomaton.getB0() with a null-safe expression
using nominalV (and keep the existing pattern of returning Double.NaN for
unavailable values).
- Around line 649-660: In getHvdcLineStringFieldValue, the switch arm handling
CONVERTER_STATION_ID_2, COUNTRY_2, VOLTAGE_LEVEL_ID_2,
VOLTAGE_LEVEL_PROPERTIES_2, SUBSTATION_ID_2, SUBSTATION_PROPERTIES_2 incorrectly
calls
getHvdcConverterStationStringFieldValue(hvdcLine.getConverterStation(TwoSides.ONE),
...); change that to call
getHvdcConverterStationStringFieldValue(hvdcLine.getConverterStation(TwoSides.TWO),
propertyName, fieldType) so side-2 fields use TwoSides.TWO; leave the converters
mode and default behavior unchanged.
- Around line 261-279: In getShuntCompensatorDoubleFieldValue, add null checks
for the linear model returned by
shuntCompensator.getModel(ShuntCompensatorLinearModel.class) and for
nominalVoltage from getTerminalDoubleFieldValue(...) before using them: compute
susceptancePerSection and qAtNominalVoltage only if model != null and
nominalVoltage != null, and for cases that depend on those values
(MAX_Q_AT_NOMINAL_V, SWITCHED_ON_Q_AT_NOMINAL_V, MAX_SUSCEPTANCE,
SWITCHED_ON_SUSCEPTANCE) return null (or a safe fallback) when the model or
nominalVoltage is missing; still handle NOMINAL_VOLTAGE, SECTION_COUNT and
MAXIMUM_SECTION_COUNT as before and throw the existing PowsyblException only in
the default branch. Ensure no Math.pow is called when nominalVoltage is null.
In
`@src/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtilsToRemove.java`:
- Around line 1-621: The file contains an entirely commented-out class
ExpertRuleUtilsToRemove; delete this file completely (remove the
src/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtilsToRemove.java
entry) since the active implementation lives in ExpertRuleUtils and keeping this
dead commented file only adds noise.
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/FilterExpertRule.java`:
- Around line 68-75: The evaluateIsPartOfOperator currently recomputes
referenceFilters.stream().map(filter ->
filter.evaluate(identifiable.getNetwork())) for every Identifiable; instead
compute the union of reference filter IDs once per Network and reuse it (e.g.,
add a private helper referencedIds(Network) that maps referenceFilters →
Set<String> of Identifiable::getId or memoize by Network), then change
evaluateIsPartOfOperator to accept or look up the precomputed Set for the given
Network and simply call contains(identifiable.getId()); update the call sites
(evaluateFilterRule / evaluateRule / AbstractFilter.evaluate) to compute or pass
the cached set at the start of the evaluation pass so referenceFilters are not
re-evaluated for each candidate.
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.java`:
- Around line 42-47: The exception message in the EXISTS/NOT_EXISTS branch of
StringExpertRule is a copy-paste from the number rule; update the
PowsyblException text in the method inside class StringExpertRule (the block
that checks OperatorType.EXISTS/NOT_EXISTS and validates
referenceValue/referenceValues) to mention "Invalid string expert rule" (or
otherwise reference StringExpertRule) and keep the rest of the message unchanged
so it reads correctly for string rules.
In `@src/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.java`:
- Around line 1-118: The entire ExpertFilterTest class is commented out so it
isn't compiled or run; uncomment the file so the class and its imports are
active (remove the leading /* ... */ and per-line // comments), ensuring the
package declaration, imports (e.g.,
org.gridsuite.filter.wip.expert.ExpertFilter, AbstractExpertRule,
TestNetworkUtils, JUnit and Mockito imports) and the class ExpertFilterTest with
its test methods (setUp, testCreateFilterWithNullEquipmentTypeThrowsException,
testFilterEvaluationWithMockRuleDelegatesEvaluationToRuleForEachEquipment,
provideMockFilterEvaluationArguments, provideTopologyDependedFilterArguments,
etc.) are restored; after uncommenting, run the test suite and fix any missing
imports or compilation errors (adjust references to Identifiable, Line,
EquipmentType, FilterType, TopologyKind) so the tests compile and execute.
In
`@src/test/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtilsTest.java`:
- Around line 1-88: Uncomment the entire ExpertRuleUtilsTest class so the tests
run: restore the package and import statements and the class body including the
provideEquipmentsWithBooleanFields() method and the two test methods
testGetIdentifiableBooleanFieldValueThrowsExceptionWhenUnsupportedIdentifiable()
and testGetExistingIdentifiableBooleanFieldValueReturnsExpected(); ensure
imports for Identifiable, PowsyblException, TestNetworkUtils, EquipmentType,
FieldType, JUnit and parameterized test providers are present and that
method/parameter names match the production API
(ExpertRuleUtils.getBooleanFieldValue) so the test compiles and executes.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.java`:
- Around line 65-72: The FREE_PROPERTIES branch in getStringFieldValue calls
identifiable.getProperty(propertyName) without checking propertyName; add a
null-check for propertyName in getStringFieldValue and if null throw an
IllegalArgumentException (or return null per project convention) with a clear
message referencing propertyName and FREE_PROPERTIES so callers know they passed
an invalid argument; update the FREE_PROPERTIES case to validate propertyName
before calling identifiable.getProperty(propertyName).
In
`@src/main/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRule.java`:
- Around line 43-48: The PropertiesExpertRule constructor currently assigns the
caller-provided Set directly to the referenceValues field, risking external
mutation; change the constructor in PropertiesExpertRule to defensively copy the
incoming referenceValues (e.g., via Set.copyOf(referenceValues) or a new
HashSet<>(referenceValues)) before assigning to the field so the rule is
immutable and consistent with EnumExpertRule.
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.java`:
- Around line 88-106: StringExpertRule.evaluateRule currently treats empty
strings as non-existing while nulls are treated as not existing inconsistently
with BooleanExpertRule/NumberExpertRule; change the EXISTS/NOT_EXISTS branches
to treat any non-null value (including empty string) as existing: in the switch
inside evaluateRule update case EXISTS to return true and case NOT_EXISTS to
return false (keeping the initial null check that returns
operatorType.equals(OperatorType.NOT_EXISTS)); this aligns semantics with
ExpertRuleUtils.getStringFieldValue and the other ExpertRule classes.
In
`@src/test/java/org/gridsuite/filter/wip/expert/rules/StringExpertRuleTest.java`:
- Around line 86-91: Rename the misleading test method in StringExpertRuleTest:
change the test method currently named testGetDataTypeReturnsEnum to
testGetDataTypeReturnsString; update the method declaration in the class
containing the assertion that StringExpertRule.of(...).getDataType() equals
DataType.STRING so the name accurately reflects the asserted behavior (ensure
any references or annotations remain intact and only the method name is
changed).
In `@src/test/java/org/gridsuite/filter/wip/TestNetworkUtils.java`:
- Around line 25-43: getEquipmentFromTestNetwork currently calls
createTestNetwork() on every invocation causing expensive repeated builds;
change it to reuse a lazily-initialized static Network instance (e.g., a private
static volatile Network TEST_NETWORK or an AtomicReference) that is constructed
once on first access and returned by a getter used by
getEquipmentFromTestNetwork, ensure initialization is thread-safe
(double-checked locking or synchronized block) and document that tests must not
mutate TEST_NETWORK (or create a copy when mutation is needed) so per-test
isolation is preserved.
- Around line 212-233: The large commented-out tap-changer setup for
three-winding transformers (the
network.getThreeWindingsTransformer("THREE_WINDINGS_TRANSFORMER_" + i) blocks)
is a maintenance hazard; either remove the dead code or replace it with a
concise explanatory comment stating that the three-winding transformer legs
intentionally have no RatioTapChanger because tests assert NOT_EXISTS for
RATIO_TARGET_V* / LOAD_TAP_CHANGING_CAPABILITIES_*. Edit TestNetworkUtils.java
to delete the commented block or add a one-line comment near the creation of
THREE_WINDINGS_TRANSFORMER_* explaining the deliberate absence of ratio tap
changers and referencing the related assertions (RATIO_TARGET_V*,
LOAD_TAP_CHANGING_CAPABILITIES_*).
- Around line 256-264: The code should consistently use the IIDM 1.16
BoundaryLine API instead of any DanglingLine types: ensure calls use
network.getBoundaryLine(equipmentId) and call VoltageLevel.newBoundaryLine() (as
in vl1.newBoundaryLine()) so compilation won't break due to the rename; also
optimize getEquipmentFromTestNetwork by memoizing/ caching the created test
network (e.g. a private static field returned on subsequent calls) so the full
network isn't rebuilt on each invocation.
🪄 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: 5ca1a662-dfb7-455c-bac2-95b6d21de498
📒 Files selected for processing (30)
src/main/java/org/gridsuite/filter/wip/AbstractFilter.javasrc/main/java/org/gridsuite/filter/wip/data/EquipmentType.javasrc/main/java/org/gridsuite/filter/wip/data/FilterType.javasrc/main/java/org/gridsuite/filter/wip/expert/ExpertFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/data/CombinatorType.javasrc/main/java/org/gridsuite/filter/wip/expert/data/DataType.javasrc/main/java/org/gridsuite/filter/wip/expert/data/FieldType.javasrc/main/java/org/gridsuite/filter/wip/expert/data/OperatorType.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/AbstractExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/BooleanExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/CombinatorExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/EnumExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtilsToRemove.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/FilterExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.javasrc/main/java/org/gridsuite/filter/wip/identifier/IdentifierListFilter.javasrc/test/java/org/gridsuite/filter/wip/TestNetworkUtils.javasrc/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/BooleanExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/CombinatorExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/EnumExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtilsTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/FilterExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/StringExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/identifier/IdentifierListFilterTest.java
KoloMenek
force-pushed
the
marutk/feat/introduce_reworked_expert_filter
branch
2 times, most recently
from
16d6925 to
ab314ae
Compare
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
KoloMenek
force-pushed
the
marutk/feat/introduce_reworked_expert_filter
branch
2 times, most recently
from
f92e687 to
9177e51
Compare
KoloMenek
changed the title
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.java (3)
265-267:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard against non-linear model and null nominal voltage (NPE).
getModel(ShuntCompensatorLinearModel.class)returnsnullfor non-linear shunt compensators, so.getBPerSection()will throw NPE. LikewisenominalVoltageis unboxed from aDoublethatgetTerminalDoubleFieldValuemay return asnull, triggering NPE on autounboxing.🐛 Proposed fix
- double susceptancePerSection = shuntCompensator.getModel(ShuntCompensatorLinearModel.class).getBPerSection(); - double nominalVoltage = getTerminalDoubleFieldValue(shuntCompensator.getTerminal(), FieldType.NOMINAL_VOLTAGE); - double qAtNominalVoltage = Math.pow(nominalVoltage, 2) * Math.abs(susceptancePerSection); + ShuntCompensatorLinearModel model = shuntCompensator.getModel(ShuntCompensatorLinearModel.class); + if (model == null) { + throw new PowsyblException(String.format("ShuntCompensator %s does not have a linear model", shuntCompensator.getId())); + } + double susceptancePerSection = model.getBPerSection(); + Double nominalVoltage = getTerminalDoubleFieldValue(shuntCompensator.getTerminal(), FieldType.NOMINAL_VOLTAGE); + if (nominalVoltage == null) { + return Double.NaN; + } + double qAtNominalVoltage = Math.pow(nominalVoltage, 2) * Math.abs(susceptancePerSection);🤖 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/filter/wip/expert/rules/ExpertRuleUtils.java` around lines 265 - 267, The code assumes shuntCompensator.getModel(ShuntCompensatorLinearModel.class) and getTerminalDoubleFieldValue(...) are non-null; guard both: check the model returned from getModel(...) is not null before calling getBPerSection() and handle the non-linear case (e.g., skip computation or use a safe default/report), and check that Double nominalVoltage from getTerminalDoubleFieldValue(shuntCompensator.getTerminal(), FieldType.NOMINAL_VOLTAGE) is not null before unboxing (handle null by skipping, throwing a clear exception, or using a default). Update the qAtNominalVoltage calculation to only run when both model and nominalVoltage are present and document/log if you skip computation.
455-466:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winNull-guard
nominalVandstandbyAutomatonbefore use (NPE).
nominalVis unboxed from a possibly-nullDouble(NPE on autounboxing). Additionally,FIX_Q_AT_NOMINAL_VcallsstandbyAutomaton.getB0()without a null check, unlike the otherStandbyAutomatonbranches — NPE when the extension is absent.🐛 Proposed fix
StandbyAutomaton standbyAutomaton = staticVarCompensator.getExtension(StandbyAutomaton.class); - double nominalV = getTerminalDoubleFieldValue(staticVarCompensator.getTerminal(), FieldType.NOMINAL_VOLTAGE); + Double nominalV = getTerminalDoubleFieldValue(staticVarCompensator.getTerminal(), FieldType.NOMINAL_VOLTAGE); + if (nominalV == null) { + return Double.NaN; + } return switch (fieldType) { @@ - case FIX_Q_AT_NOMINAL_V -> Math.pow(nominalV, 2) * standbyAutomaton.getB0(); + case FIX_Q_AT_NOMINAL_V -> standbyAutomaton != null ? Math.pow(nominalV, 2) * standbyAutomaton.getB0() : Double.NaN;🤖 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/filter/wip/expert/rules/ExpertRuleUtils.java` around lines 455 - 466, The switch unboxes nominalV and calls standbyAutomaton.getB0() without null checks which can cause NPEs; update the code in ExpertRuleUtils so nominalV is obtained safely (capture the Double from getTerminalDoubleFieldValue and check for null before using or treat as Double.NaN to avoid autounboxing), and change the FIX_Q_AT_NOMINAL_V branch to first verify standbyAutomaton != null (return Double.NaN or handle accordingly) before calling getB0(); keep existing null-safety pattern used in other FieldType branches (e.g., LOW_VOLTAGE_SET_POINT) and reference staticVarCompensator, standbyAutomaton, getTerminalDoubleFieldValue, FieldType.FIX_Q_AT_NOMINAL_V and getB0() when making the edits.
285-289:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard against non-linear model before
getBPerSection()(NPE).If the shunt compensator uses a non-linear model,
getModel(ShuntCompensatorLinearModel.class)returnsnulland line 289 throws NPE.🐛 Proposed fix
ShuntCompensatorLinearModel shuntCompensatorLinearModel = shuntCompensator.getModel(ShuntCompensatorLinearModel.class); + if (shuntCompensatorLinearModel == null) { + throw new PowsyblException(String.format("ShuntCompensator %s does not have a linear model", shuntCompensator.getId())); + }🤖 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/filter/wip/expert/rules/ExpertRuleUtils.java` around lines 285 - 289, Guard against a null ShuntCompensatorLinearModel before calling getBPerSection(): after obtaining shuntCompensatorLinearModel via shuntCompensator.getModel(ShuntCompensatorLinearModel.class) check if it is null and handle that case (e.g., return a safe default like "UNKNOWN" or compute the type from another model/property) instead of calling shuntCompensatorLinearModel.getBPerSection(); update the SHUNT_COMPENSATOR_TYPE branch to use this null-checked value so no NPE occurs when the compensator uses a non-linear model.
🧹 Nitpick comments (3)
src/main/java/org/gridsuite/filter/wip/expert/rules/AbstractExpertRule.java (1)
23-25: ⚡ Quick winDrop redundant abstract redeclarations of interface methods.
evaluateRuleandgetDataTypeare already declared inExpertRule; re-declaring them here is redundant (flagged by SonarCloud). The meaningful additions aregetOperatorType()andunsupportedOperatorException().♻️ Proposed cleanup
public abstract class AbstractExpertRule implements ExpertRule { - public abstract boolean evaluateRule(Identifiable<?> identifiable); - - public abstract DataType getDataType(); - protected abstract OperatorType getOperatorType();Note: the
IdentifiableandDataTypeimports may become unused after this change.🤖 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/filter/wip/expert/rules/AbstractExpertRule.java` around lines 23 - 25, Remove the redundant abstract method declarations evaluateRule(Identifiable<?>) and getDataType() from AbstractExpertRule since they are already declared in the ExpertRule interface; keep the meaningful additions getOperatorType() and unsupportedOperatorException() and update imports (Identifiable, DataType) if they become unused after removal.src/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.java (1)
35-61: ⚡ Quick winReduce cognitive complexity to clear the SonarCloud gate.
Same as
NumberExpertRule.of: SonarCloud reports complexity 18 (limit 15). Flatten the trailingelse/ extract the reference-value validation to a helper so behavior is unchanged and the gate passes.🤖 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/filter/wip/expert/rules/StringExpertRule.java` around lines 35 - 61, SonarCloud flags StringExpertRule.of for high cognitive complexity; refactor by flattening branching and extracting the reference-value checks into a private helper. Keep all current behavior: handle null checks and DataType/SUPPORTED_OPERATORS first, then early-return for EXISTS/NOT_EXISTS (ensure referenceValue/referenceValues are null), then early-return for IN/NOT_IN (ensure referenceValues non-null/non-empty), and for remaining operators call a new private method (e.g., validateReferenceValue(String referenceValue)) to assert non-null/non-blank before constructing the StringExpertRule. Update StringExpertRule.of to remove the trailing else and call the helper; add the private validateReferenceValue method to centralize validation.src/main/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRule.java (1)
35-61: ⚡ Quick winReduce cognitive complexity to clear the SonarCloud gate.
SonarCloud reports cognitive complexity 17 (limit 15) for
of(...). Extracting the reference-value validation into a small private helper keeps behavior identical and clears the quality gate.♻️ Suggested extraction
- if (OperatorType.EXISTS.equals(operatorType) || OperatorType.NOT_EXISTS.equals(operatorType)) { - if (referenceValue != null || referenceValues != null) { - throw new PowsyblException("Invalid number expert rule, referenceValue and referenceValues must be null for EXISTS and NOT_EXISTS operators"); - } - return new NumberExpertRule(fieldType, operatorType); - } - - if (OperatorType.BETWEEN.equals(operatorType) || OperatorType.IN.equals(operatorType) || OperatorType.NOT_IN.equals(operatorType)) { - if (referenceValues == null || referenceValues.isEmpty()) { - throw new PowsyblException("Invalid number expert rule, referenceValues must be defined (not null and not empty)"); - } - return new NumberExpertRule(fieldType, operatorType, referenceValues); - } else { - if (referenceValue == null) { - throw new PowsyblException("Invalid number expert rule, referenceValue must be defined (not null)"); - } - return new NumberExpertRule(fieldType, operatorType, referenceValue); - } + if (OperatorType.EXISTS.equals(operatorType) || OperatorType.NOT_EXISTS.equals(operatorType)) { + if (referenceValue != null || referenceValues != null) { + throw new PowsyblException("Invalid number expert rule, referenceValue and referenceValues must be null for EXISTS and NOT_EXISTS operators"); + } + return new NumberExpertRule(fieldType, operatorType); + } + if (OperatorType.BETWEEN.equals(operatorType) || OperatorType.IN.equals(operatorType) || OperatorType.NOT_IN.equals(operatorType)) { + if (referenceValues == null || referenceValues.isEmpty()) { + throw new PowsyblException("Invalid number expert rule, referenceValues must be defined (not null and not empty)"); + } + return new NumberExpertRule(fieldType, operatorType, referenceValues); + } + if (referenceValue == null) { + throw new PowsyblException("Invalid number expert rule, referenceValue must be defined (not null)"); + } + return new NumberExpertRule(fieldType, operatorType, referenceValue);(Splitting validation into a helper method is also an option if the inline change is insufficient.)
🤖 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/filter/wip/expert/rules/NumberExpertRule.java` around lines 35 - 61, The of(...) method in NumberExpertRule exceeds SonarCloud's cognitive complexity limit; extract the reference validation logic into one or two private helpers (e.g., validateExistenceReferences(FieldType, OperatorType, Double, Set<Double>) and validateValueReferences(FieldType, OperatorType, Double, Set<Double>) or a single validateReferenceParameters(...)) and call them from NumberExpertRule.of; move the EXISTS/NOT_EXISTS checks and the BETWEEN/IN/NOT_IN vs single-value checks into those helpers so of(...) only contains high-level branching deciding which constructor to call (new NumberExpertRule(...)), preserving all current exception messages and behavior for OperatorType and null checks.
🤖 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.
Inline comments:
In
`@src/test/java/org/gridsuite/filter/wip/expert/rules/FilterExpertRuleTest.java`:
- Around line 143-150: The test method in FilterExpertRuleTest named
testMockRuleEvaluationReturnsExpected duplicates the mock-based test name and
should be renamed to reflect it exercises the real/reference-filter path; locate
the method using the MethodSource provideRealRuleEvaluationArguments and the
class FilterExpertRuleTest (the method that constructs ExpertRule via
FilterExpertRule.of and calls rule.evaluateRule), and rename the method to
something like testRealRuleEvaluationReturnsExpected (or
testReferenceFilterEvaluationReturnsExpected) so test output and maintenance are
unambiguous.
In
`@src/test/java/org/gridsuite/filter/wip/expert/rules/StringExpertRuleTest.java`:
- Around line 86-91: Rename the misleading test method in StringExpertRuleTest:
change the method name testGetDataTypeReturnsEnum to
testGetDataTypeReturnsString (or similar) so it reflects the assertion that
StringExpertRule.getDataType() returns DataType.STRING; update the `@Test` method
signature in class StringExpertRuleTest (the method that constructs
StringExpertRule.of(...) and asserts getDataType()) to use the new name and
adjust any references/imports if necessary.
---
Duplicate comments:
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.java`:
- Around line 265-267: The code assumes
shuntCompensator.getModel(ShuntCompensatorLinearModel.class) and
getTerminalDoubleFieldValue(...) are non-null; guard both: check the model
returned from getModel(...) is not null before calling getBPerSection() and
handle the non-linear case (e.g., skip computation or use a safe
default/report), and check that Double nominalVoltage from
getTerminalDoubleFieldValue(shuntCompensator.getTerminal(),
FieldType.NOMINAL_VOLTAGE) is not null before unboxing (handle null by skipping,
throwing a clear exception, or using a default). Update the qAtNominalVoltage
calculation to only run when both model and nominalVoltage are present and
document/log if you skip computation.
- Around line 455-466: The switch unboxes nominalV and calls
standbyAutomaton.getB0() without null checks which can cause NPEs; update the
code in ExpertRuleUtils so nominalV is obtained safely (capture the Double from
getTerminalDoubleFieldValue and check for null before using or treat as
Double.NaN to avoid autounboxing), and change the FIX_Q_AT_NOMINAL_V branch to
first verify standbyAutomaton != null (return Double.NaN or handle accordingly)
before calling getB0(); keep existing null-safety pattern used in other
FieldType branches (e.g., LOW_VOLTAGE_SET_POINT) and reference
staticVarCompensator, standbyAutomaton, getTerminalDoubleFieldValue,
FieldType.FIX_Q_AT_NOMINAL_V and getB0() when making the edits.
- Around line 285-289: Guard against a null ShuntCompensatorLinearModel before
calling getBPerSection(): after obtaining shuntCompensatorLinearModel via
shuntCompensator.getModel(ShuntCompensatorLinearModel.class) check if it is null
and handle that case (e.g., return a safe default like "UNKNOWN" or compute the
type from another model/property) instead of calling
shuntCompensatorLinearModel.getBPerSection(); update the SHUNT_COMPENSATOR_TYPE
branch to use this null-checked value so no NPE occurs when the compensator uses
a non-linear model.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/AbstractExpertRule.java`:
- Around line 23-25: Remove the redundant abstract method declarations
evaluateRule(Identifiable<?>) and getDataType() from AbstractExpertRule since
they are already declared in the ExpertRule interface; keep the meaningful
additions getOperatorType() and unsupportedOperatorException() and update
imports (Identifiable, DataType) if they become unused after removal.
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRule.java`:
- Around line 35-61: The of(...) method in NumberExpertRule exceeds SonarCloud's
cognitive complexity limit; extract the reference validation logic into one or
two private helpers (e.g., validateExistenceReferences(FieldType, OperatorType,
Double, Set<Double>) and validateValueReferences(FieldType, OperatorType,
Double, Set<Double>) or a single validateReferenceParameters(...)) and call them
from NumberExpertRule.of; move the EXISTS/NOT_EXISTS checks and the
BETWEEN/IN/NOT_IN vs single-value checks into those helpers so of(...) only
contains high-level branching deciding which constructor to call (new
NumberExpertRule(...)), preserving all current exception messages and behavior
for OperatorType and null checks.
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.java`:
- Around line 35-61: SonarCloud flags StringExpertRule.of for high cognitive
complexity; refactor by flattening branching and extracting the reference-value
checks into a private helper. Keep all current behavior: handle null checks and
DataType/SUPPORTED_OPERATORS first, then early-return for EXISTS/NOT_EXISTS
(ensure referenceValue/referenceValues are null), then early-return for
IN/NOT_IN (ensure referenceValues non-null/non-empty), and for remaining
operators call a new private method (e.g., validateReferenceValue(String
referenceValue)) to assert non-null/non-blank before constructing the
StringExpertRule. Update StringExpertRule.of to remove the trailing else and
call the helper; add the private validateReferenceValue method to centralize
validation.
🪄 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: c2e08bd9-6f4c-4267-a675-9f0c39bb5541
📒 Files selected for processing (24)
src/main/java/org/gridsuite/filter/wip/AbstractFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/ExpertFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/data/DataType.javasrc/main/java/org/gridsuite/filter/wip/expert/data/FieldType.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/AbstractExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/BooleanExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/CombinatorExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/EnumExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/FilterExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.javasrc/test/java/org/gridsuite/filter/wip/TestNetworkUtils.javasrc/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/BooleanExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/CombinatorExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/EnumExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtilsTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/FilterExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/StringExpertRuleTest.java
KoloMenek
force-pushed
the
marutk/feat/introduce_reworked_expert_filter
branch
2 times, most recently
from
6026d5e to
f7c4a72
Compare
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/test/java/org/gridsuite/filter/wip/TestNetworkUtils.java (1)
25-43: ⚖️ Poor tradeoffNetwork rebuilt on every lookup.
getEquipmentFromTestNetworkcallscreateTestNetwork()per invocation, so each parameterized case across all rule test suites reconstructs the full network (substations, VLs, all equipment, tap changers). Since evaluation is read-only, consider building the network once and caching it (e.g., a lazily-initialized static), then resolving equipment from the shared instance to cut redundant work.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/filter/wip/TestNetworkUtils.java` around lines 25 - 43, getEquipmentFromTestNetwork currently calls createTestNetwork() on every invocation which rebuilds the entire Network; change this to build the Network once and reuse it by adding a lazily-initialized static cache (e.g., a private static volatile Network TEST_NETWORK and a synchronized/init-if-null accessor or use an AtomicReference/initializer holder) and have getEquipmentFromTestNetwork resolve equipment from that cached Network instead of calling createTestNetwork() each time; keep the method signature and all switch branches (VOLTAGE_LEVEL, LINE, GENERATOR, etc.) but replace the local Network network = createTestNetwork() with fetching the shared TEST_NETWORK via the new accessor to avoid repeated reconstruction.src/main/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRule.java (1)
43-48: 💤 Low valueDefensively copy
referenceValuesfor immutability consistency.Unlike
EnumExpertRule,NumberExpertRule,CombinatorExpertRule, andFilterExpertRule, this constructor stores the caller-supplied set directly, so external mutation after construction would leak into rule evaluation.♻️ Proposed change
- this.referenceValues = referenceValues; + this.referenceValues = Set.copyOf(referenceValues);🤖 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/filter/wip/expert/rules/PropertiesExpertRule.java` around lines 43 - 48, The constructor PropertiesExpertRule currently assigns the caller-supplied Set referenceValues directly, allowing external mutation; change the constructor implementation in PropertiesExpertRule(FieldType, OperatorType, String, Set<String>) to defensively copy and freeze the set (e.g., this.referenceValues = Collections.unmodifiableSet(new HashSet<>(referenceValues == null ? Collections.emptySet() : referenceValues))) so the internal referenceValues field is immutable and null-safe during rule evaluation.src/main/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRule.java (1)
35-61: ⚖️ Poor tradeoffReduce cognitive complexity of
of(...)to clear the failing SonarCloud gate.SonarCloud reports this factory at complexity 17 (limit 15). The nested operator-group branches can be flattened by extracting reference-value validation into a small private helper (e.g.
validateReferences(operatorType, referenceValue, referenceValues)), which also keeps the three sibling factories consistent.🤖 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/filter/wip/expert/rules/NumberExpertRule.java` around lines 35 - 61, The of(...) method in NumberExpertRule is too complex; extract the reference validation logic into a private helper method (e.g. validateReferences(OperatorType operatorType, Double referenceValue, Set<Double> referenceValues)) and call it from NumberExpertRule.of to flatten nested branches: keep initial null/type checks in of, then call validateReferences to enforce the EXISTS/NOT_EXISTS, BETWEEN/IN/NOT_IN and default reference rules, and finally return the appropriate constructor (new NumberExpertRule(fieldType, operatorType), new NumberExpertRule(fieldType, operatorType, referenceValues), or new NumberExpertRule(fieldType, operatorType, referenceValue)); ensure validateReferences references the same OperatorType constants (EXISTS, NOT_EXISTS, BETWEEN, IN, NOT_IN) and throws the same PowsyblException messages so behavior remains identical.src/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.java (1)
35-61: ⚖️ Poor tradeoffReduce cognitive complexity of
of(...)to clear the failing SonarCloud gate.SonarCloud reports complexity 18 (limit 15). Extracting the EXISTS/IN/single-value reference validation into a private helper shared in spirit with the other rule factories would bring it under the threshold and de-duplicate the validation pattern.
🤖 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/filter/wip/expert/rules/StringExpertRule.java` around lines 35 - 61, Refactor StringExpertRule.of(...) to reduce cognitive complexity by extracting the reference-parameter validation into a private helper method (e.g. private static void validateReferenceForOperator(OperatorType operatorType, String referenceValue, Set<String> referenceValues)); have of(...) call this helper after the fieldType/operatorType basic checks and before constructing the rule; the helper should encapsulate the EXISTS/NOT_EXISTS checks (both refs must be null), the IN/NOT_IN checks (referenceValues non-null and non-empty), and the single-value checks (referenceValue non-null and not blank), throwing the same PowsyblException messages used today so the rest of of(...) simply returns the appropriate new StringExpertRule(...) without repeated validation logic.
🤖 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.
Inline comments:
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.java`:
- Around line 466-481: The FIX_Q_AT_NOMINAL_V branch currently calls
standbyAutomaton.getB0() without a null check, which risks an NPE; update the
switch case for FIX_Q_AT_NOMINAL_V to first verify standbyAutomaton != null
(like the LOW_VOLTAGE_* and SUSCEPTANCE_FIX cases) and return Double.NaN when
null, otherwise compute Math.pow(nominalV, 2) * standbyAutomaton.getB0(); ensure
you reference the FIX_Q_AT_NOMINAL_V enum case, the standbyAutomaton variable
and its getB0() method, and keep the behavior consistent with sibling cases that
use Double.NaN as the fallback.
- Around line 770-801: In getRatioTapChangerStringFieldValue and
getPhaseTapChangerStringFieldValue, avoid calling getRegulationMode().name()
unconditionally—first obtain the regulation mode
(ratioTapChanger.getRegulationMode() / phaseTapChanger.getRegulationMode()),
check it for null, and only call .name() if non-null; if null, return null (or
the appropriate empty value) for the matching FieldType branches (e.g.,
RATIO_REGULATION_MODE*, PHASE_REGULATION_MODE*). Update the switch branches in
both methods to perform this null check (or use Optional) before returning the
name to prevent NPEs.
In `@src/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.java`:
- Around line 181-192: Rename the two misleading test methods to match what they
actually verify: change testCreateFilterWithNullEquipmentIdsThrowsException to
testCreateFilterWithNullRuleThrowsException (it constructs new
ExpertFilter(EquipmentType.LINE, null) and expects NullPointerException) and
change testGetFilterTypeReturnsIdentifierList to testGetFilterTypeReturnsExpert
(it creates ExpertFilter(EquipmentType.LINE, mockRule) and asserts
FilterType.EXPERT); update the test method names where they are declared
(keeping the same test bodies, references to ExpertFilter, AbstractExpertRule,
EquipmentType.LINE and FilterType.EXPERT).
---
Nitpick comments:
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRule.java`:
- Around line 35-61: The of(...) method in NumberExpertRule is too complex;
extract the reference validation logic into a private helper method (e.g.
validateReferences(OperatorType operatorType, Double referenceValue, Set<Double>
referenceValues)) and call it from NumberExpertRule.of to flatten nested
branches: keep initial null/type checks in of, then call validateReferences to
enforce the EXISTS/NOT_EXISTS, BETWEEN/IN/NOT_IN and default reference rules,
and finally return the appropriate constructor (new NumberExpertRule(fieldType,
operatorType), new NumberExpertRule(fieldType, operatorType, referenceValues),
or new NumberExpertRule(fieldType, operatorType, referenceValue)); ensure
validateReferences references the same OperatorType constants (EXISTS,
NOT_EXISTS, BETWEEN, IN, NOT_IN) and throws the same PowsyblException messages
so behavior remains identical.
In
`@src/main/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRule.java`:
- Around line 43-48: The constructor PropertiesExpertRule currently assigns the
caller-supplied Set referenceValues directly, allowing external mutation; change
the constructor implementation in PropertiesExpertRule(FieldType, OperatorType,
String, Set<String>) to defensively copy and freeze the set (e.g.,
this.referenceValues = Collections.unmodifiableSet(new HashSet<>(referenceValues
== null ? Collections.emptySet() : referenceValues))) so the internal
referenceValues field is immutable and null-safe during rule evaluation.
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.java`:
- Around line 35-61: Refactor StringExpertRule.of(...) to reduce cognitive
complexity by extracting the reference-parameter validation into a private
helper method (e.g. private static void
validateReferenceForOperator(OperatorType operatorType, String referenceValue,
Set<String> referenceValues)); have of(...) call this helper after the
fieldType/operatorType basic checks and before constructing the rule; the helper
should encapsulate the EXISTS/NOT_EXISTS checks (both refs must be null), the
IN/NOT_IN checks (referenceValues non-null and non-empty), and the single-value
checks (referenceValue non-null and not blank), throwing the same
PowsyblException messages used today so the rest of of(...) simply returns the
appropriate new StringExpertRule(...) without repeated validation logic.
In `@src/test/java/org/gridsuite/filter/wip/TestNetworkUtils.java`:
- Around line 25-43: getEquipmentFromTestNetwork currently calls
createTestNetwork() on every invocation which rebuilds the entire Network;
change this to build the Network once and reuse it by adding a
lazily-initialized static cache (e.g., a private static volatile Network
TEST_NETWORK and a synchronized/init-if-null accessor or use an
AtomicReference/initializer holder) and have getEquipmentFromTestNetwork resolve
equipment from that cached Network instead of calling createTestNetwork() each
time; keep the method signature and all switch branches (VOLTAGE_LEVEL, LINE,
GENERATOR, etc.) but replace the local Network network = createTestNetwork()
with fetching the shared TEST_NETWORK via the new accessor to avoid repeated
reconstruction.
🪄 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: 2eef8a40-5e80-4c23-a35f-e3bf47886fe6
📒 Files selected for processing (27)
src/main/java/org/gridsuite/filter/wip/AbstractFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/ExpertFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/data/DataType.javasrc/main/java/org/gridsuite/filter/wip/expert/data/FieldType.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/AbstractExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/BooleanExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/CombinatorExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/EnumExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/FilterExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.javasrc/test/java/org/gridsuite/filter/wip/SimpleFilterTest.javasrc/test/java/org/gridsuite/filter/wip/TestNetworkUtils.javasrc/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/BooleanExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/CombinatorExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/EnumExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtilsTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/FilterExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/SimpleExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/StringExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/identifier/IdentifierListFilterTest.java
Comment on lines
+181
to
+192
| void testCreateFilterWithNullEquipmentIdsThrowsException() { | ||
| assertThatThrownBy(() -> new ExpertFilter(EquipmentType.LINE, null)) | ||
| .isInstanceOf(NullPointerException.class); | ||
| } | ||
|
|
||
| @Test | ||
| void testGetFilterTypeReturnsIdentifierList() { | ||
| AbstractExpertRule mockRule = Mockito.mock(AbstractExpertRule.class); | ||
| ExpertFilter filter = new ExpertFilter(EquipmentType.LINE, mockRule); | ||
|
|
||
| assertThat(filter.getFilterType()).isEqualTo(FilterType.EXPERT); | ||
| } |
There was a problem hiding this comment.
Misleading test method names (copy-paste from the IdentifierList suite).
- Line 187
testGetFilterTypeReturnsIdentifierListassertsFilterType.EXPERT, not identifier-list — rename to reflect the EXPERT assertion. - Line 181
testCreateFilterWithNullEquipmentIdsThrowsExceptionactually validates the nullruleargument (ExpertFilterhas no equipment IDs); rename to e.g.testCreateFilterWithNullRuleThrowsException.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.java` around
lines 181 - 192, Rename the two misleading test methods to match what they
actually verify: change testCreateFilterWithNullEquipmentIdsThrowsException to
testCreateFilterWithNullRuleThrowsException (it constructs new
ExpertFilter(EquipmentType.LINE, null) and expects NullPointerException) and
change testGetFilterTypeReturnsIdentifierList to testGetFilterTypeReturnsExpert
(it creates ExpertFilter(EquipmentType.LINE, mockRule) and asserts
FilterType.EXPERT); update the test method names where they are declared
(keeping the same test bodies, references to ExpertFilter, AbstractExpertRule,
EquipmentType.LINE and FilterType.EXPERT).
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.java (3)
457-472:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winNPE risk:
standbyAutomatondereferenced without null guard inFIX_Q_AT_NOMINAL_V.Line 473 calls
standbyAutomaton.getB0()directly without checking for null. When theStandbyAutomatonextension is absent, this throws aNullPointerExceptionrather than returning theDouble.NaNfallback used by sibling cases at lines 468–472.🐛 Proposed fix
- case FIX_Q_AT_NOMINAL_V -> Math.pow(nominalV, 2) * standbyAutomaton.getB0(); + case FIX_Q_AT_NOMINAL_V -> standbyAutomaton != null ? Math.pow(nominalV, 2) * standbyAutomaton.getB0() : Double.NaN;🤖 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/filter/wip/expert/rules/ExpertRuleUtils.java` around lines 457 - 472, The FIX: In getStaticVarCompensatorDoubleFieldValue, the switch case for FIX_Q_AT_NOMINAL_V currently calls standbyAutomaton.getB0() without a null check; update that case to mirror the other cases by returning standbyAutomaton != null ? standbyAutomaton.getB0() : Double.NaN so it returns Double.NaN when the StandbyAutomaton extension is missing (method: getStaticVarCompensatorDoubleFieldValue, symbol: StandbyAutomaton, case: FIX_Q_AT_NOMINAL_V).
770-779:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard against nullable
getRegulationMode()before calling.name().Line 776 unconditionally calls
ratioTapChanger.getRegulationMode().name(). In the PowSyBl IIDM API,RatioTapChanger#getRegulationMode()can returnnullwhen the regulation mode is unset, which would cause an NPE. Other methods in this file (e.g., line 492 forstaticVarCompensator.getRegulationMode()and line 666 forhvdcLine.getConvertersMode()) already guard against null before calling.name().🐛 Proposed fix
- case RATIO_REGULATION_MODE, RATIO_REGULATION_MODE_1, RATIO_REGULATION_MODE_2, RATIO_REGULATION_MODE_3 -> ratioTapChanger.getRegulationMode().name(); + case RATIO_REGULATION_MODE, RATIO_REGULATION_MODE_1, RATIO_REGULATION_MODE_2, RATIO_REGULATION_MODE_3 -> + ratioTapChanger.getRegulationMode() != null ? ratioTapChanger.getRegulationMode().name() : null;🤖 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/filter/wip/expert/rules/ExpertRuleUtils.java` around lines 770 - 779, In getRatioTapChangerStringFieldValue, guard against a null result from ratioTapChanger.getRegulationMode() before calling .name(): when fieldType is one of RATIO_REGULATION_MODE, RATIO_REGULATION_MODE_1, RATIO_REGULATION_MODE_2, RATIO_REGULATION_MODE_3, fetch ratioTapChanger.getRegulationMode() into a local, return null if that local is null, otherwise return its name(); keep the default branch throwing PowsyblException as-is and use the existing RATIO_TAP_CHANGER_TYPE and FIELD_RETRIEVAL_NOT_SUPPORTED_TEMPLATE symbols to locate the code.
781-801:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard against nullable
getRegulationMode()before calling.name().Line 798 unconditionally calls
phaseTapChanger.getRegulationMode().name(). In the PowSyBl IIDM API,PhaseTapChanger#getRegulationMode()can returnnullwhen the regulation mode is unset, which would cause an NPE. Other methods in this file already guard against null before calling.name()on nullable enum getters.🐛 Proposed fix
- case PHASE_REGULATION_MODE, PHASE_REGULATION_MODE_1, PHASE_REGULATION_MODE_2, PHASE_REGULATION_MODE_3 -> phaseTapChanger.getRegulationMode().name(); + case PHASE_REGULATION_MODE, PHASE_REGULATION_MODE_1, PHASE_REGULATION_MODE_2, PHASE_REGULATION_MODE_3 -> + phaseTapChanger.getRegulationMode() != null ? phaseTapChanger.getRegulationMode().name() : null;🤖 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/filter/wip/expert/rules/ExpertRuleUtils.java` around lines 781 - 801, The getPhaseTapChangerStringFieldValue method calls phaseTapChanger.getRegulationMode().name() without null-checking; update getPhaseTapChangerStringFieldValue to first retrieve PhaseTapChanger#getRegulationMode() into a local variable, return null if that variable is null for the PHASE_REGULATION_MODE, PHASE_REGULATION_MODE_1/2/3 cases, otherwise call .name(); keep the existing default branch that throws the PowsyblException for unsupported FieldType.src/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.java (1)
180-192:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading test method names (still unaddressed).
- Line 181
testCreateFilterWithNullEquipmentIdsThrowsExceptionactually validates a nullrule;ExpertFilterhas no equipment IDs.- Line 187
testGetFilterTypeReturnsIdentifierListassertsFilterType.EXPERT, not identifier-list.Rename to e.g.
testCreateFilterWithNullRuleThrowsExceptionandtestGetFilterTypeReturnsExpert.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.java` around lines 180 - 192, Rename the two misleading test methods to reflect what they actually assert: change testCreateFilterWithNullEquipmentIdsThrowsException to testCreateFilterWithNullRuleThrowsException (it constructs new ExpertFilter(EquipmentType.LINE, null) and expects NPE) and change testGetFilterTypeReturnsIdentifierList to testGetFilterTypeReturnsExpert (it creates an ExpertFilter and asserts getFilterType() == FilterType.EXPERT); update only the method names in ExpertFilterTest to match these clearer identifiers so the test names align with the assertions involving ExpertFilter and FilterType.EXPERT.
🧹 Nitpick comments (2)
src/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.java (1)
35-61: ⚡ Quick winReduce cognitive complexity of
of(SonarCloud gate failing).SonarCloud flags this factory at complexity 18 (limit 15). Extracting the reference-value validation into a small private helper keeps the public factory readable and clears the gate.
♻️ Proposed extraction
if (!DataType.STRING.equals(fieldType.getDataType()) || !SUPPORTED_OPERATORS.contains(operatorType)) { throw new PowsyblException("Invalid string expert rule, fieldType must be STRING and operatorType must be one of " + SUPPORTED_OPERATORS); } - - if (OperatorType.EXISTS.equals(operatorType) || OperatorType.NOT_EXISTS.equals(operatorType)) { - if (referenceValue != null || referenceValues != null) { - throw new PowsyblException("Invalid string expert rule, referenceValue and referenceValues must be null for EXISTS and NOT_EXISTS operators"); - } - return new StringExpertRule(fieldType, operatorType); - } - - if (OperatorType.IN.equals(operatorType) || OperatorType.NOT_IN.equals(operatorType)) { - if (referenceValues == null || referenceValues.isEmpty()) { - throw new PowsyblException("Invalid string expert rule, referenceValues must be defined (not null and not empty)"); - } - return new StringExpertRule(fieldType, operatorType, referenceValues); - } else { - if (referenceValue == null || referenceValue.isBlank()) { - throw new PowsyblException("Invalid string expert rule, referenceValue must be defined (not null and not blank)"); - } - return new StringExpertRule(fieldType, operatorType, referenceValue); - } + if (OperatorType.EXISTS.equals(operatorType) || OperatorType.NOT_EXISTS.equals(operatorType)) { + requireNoReferences(referenceValue, referenceValues); + return new StringExpertRule(fieldType, operatorType); + } + if (OperatorType.IN.equals(operatorType) || OperatorType.NOT_IN.equals(operatorType)) { + requireNonEmptyValues(referenceValues); + return new StringExpertRule(fieldType, operatorType, referenceValues); + } + requireNonBlankValue(referenceValue); + return new StringExpertRule(fieldType, operatorType, referenceValue); }🤖 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/filter/wip/expert/rules/StringExpertRule.java` around lines 35 - 61, The factory method StringExpertRule.of is too complex; extract the operator-specific reference validation into a private helper (e.g., validateReferenceForOperator) that takes (OperatorType operatorType, String referenceValue, Set<String> referenceValues) and performs the EXISTS/NOT_EXISTS null checks, the IN/NOT_IN non-null non-empty referenceValues check, and the default non-blank referenceValue check, throwing the same PowsyblException messages; keep the public of(...) method focused on parameter null checks, dataType/operator support check, call the new helper to validate references, and then construct and return the appropriate StringExpertRule using the same constructors (new StringExpertRule(fieldType, operatorType), new StringExpertRule(fieldType, operatorType, referenceValues), new StringExpertRule(fieldType, operatorType, referenceValue)).src/main/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRule.java (1)
35-61: ⚡ Quick winReduce cognitive complexity of
of(...)to satisfy the quality gate.SonarCloud reports cognitive complexity of 17 (limit 15) for this factory, which is failing CI. Extracting the operator-specific reference-value validation into a helper keeps
of(...)flat and below the threshold.♻️ Proposed refactor
public static NumberExpertRule of(FieldType fieldType, OperatorType operatorType, Double referenceValue, Set<Double> referenceValues) { if (fieldType == null || operatorType == null) { throw new PowsyblException("Invalid number expert rule, parameters must be non null"); } if (!DataType.NUMBER.equals(fieldType.getDataType()) || !SUPPORTED_OPERATORS.contains(operatorType)) { throw new PowsyblException("Invalid number expert rule, fieldType must be NUMBER and operatorType must be one of " + SUPPORTED_OPERATORS); } - - if (OperatorType.EXISTS.equals(operatorType) || OperatorType.NOT_EXISTS.equals(operatorType)) { - if (referenceValue != null || referenceValues != null) { - throw new PowsyblException("Invalid number expert rule, referenceValue and referenceValues must be null for EXISTS and NOT_EXISTS operators"); - } - return new NumberExpertRule(fieldType, operatorType); - } - - if (OperatorType.BETWEEN.equals(operatorType) || OperatorType.IN.equals(operatorType) || OperatorType.NOT_IN.equals(operatorType)) { - if (referenceValues == null || referenceValues.isEmpty()) { - throw new PowsyblException("Invalid number expert rule, referenceValues must be defined (not null and not empty)"); - } - return new NumberExpertRule(fieldType, operatorType, referenceValues); - } else { - if (referenceValue == null) { - throw new PowsyblException("Invalid number expert rule, referenceValue must be defined (not null)"); - } - return new NumberExpertRule(fieldType, operatorType, referenceValue); - } + return buildValidatedRule(fieldType, operatorType, referenceValue, referenceValues); + } + + private static NumberExpertRule buildValidatedRule(FieldType fieldType, OperatorType operatorType, Double referenceValue, Set<Double> referenceValues) { + if (OperatorType.EXISTS.equals(operatorType) || OperatorType.NOT_EXISTS.equals(operatorType)) { + if (referenceValue != null || referenceValues != null) { + throw new PowsyblException("Invalid number expert rule, referenceValue and referenceValues must be null for EXISTS and NOT_EXISTS operators"); + } + return new NumberExpertRule(fieldType, operatorType); + } + if (OperatorType.BETWEEN.equals(operatorType) || OperatorType.IN.equals(operatorType) || OperatorType.NOT_IN.equals(operatorType)) { + if (referenceValues == null || referenceValues.isEmpty()) { + throw new PowsyblException("Invalid number expert rule, referenceValues must be defined (not null and not empty)"); + } + return new NumberExpertRule(fieldType, operatorType, referenceValues); + } + if (referenceValue == null) { + throw new PowsyblException("Invalid number expert rule, referenceValue must be defined (not null)"); + } + return new NumberExpertRule(fieldType, operatorType, referenceValue); }🤖 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/filter/wip/expert/rules/NumberExpertRule.java` around lines 35 - 61, The factory method NumberExpertRule.of(...) is over the cognitive complexity limit; extract the operator-specific reference validation into a new private static helper (e.g., validateReferenceForOperator(OperatorType operatorType, Double referenceValue, Set<Double> referenceValues)) that contains the EXISTS/NOT_EXISTS checks (ensure both references are null), the BETWEEN/IN/NOT_IN checks (ensure referenceValues non-null and non-empty), and the default single-value check (ensure referenceValue non-null), throwing PowsyblException exactly as currently done; then simplify NumberExpertRule.of to perform only null and DataType.NUMBER/SUPPORTED_OPERATORS checks and call the new helper before returning the appropriate NumberExpertRule constructors so behavior and error messages remain unchanged.
🤖 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.
Duplicate comments:
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.java`:
- Around line 457-472: The FIX: In getStaticVarCompensatorDoubleFieldValue, the
switch case for FIX_Q_AT_NOMINAL_V currently calls standbyAutomaton.getB0()
without a null check; update that case to mirror the other cases by returning
standbyAutomaton != null ? standbyAutomaton.getB0() : Double.NaN so it returns
Double.NaN when the StandbyAutomaton extension is missing (method:
getStaticVarCompensatorDoubleFieldValue, symbol: StandbyAutomaton, case:
FIX_Q_AT_NOMINAL_V).
- Around line 770-779: In getRatioTapChangerStringFieldValue, guard against a
null result from ratioTapChanger.getRegulationMode() before calling .name():
when fieldType is one of RATIO_REGULATION_MODE, RATIO_REGULATION_MODE_1,
RATIO_REGULATION_MODE_2, RATIO_REGULATION_MODE_3, fetch
ratioTapChanger.getRegulationMode() into a local, return null if that local is
null, otherwise return its name(); keep the default branch throwing
PowsyblException as-is and use the existing RATIO_TAP_CHANGER_TYPE and
FIELD_RETRIEVAL_NOT_SUPPORTED_TEMPLATE symbols to locate the code.
- Around line 781-801: The getPhaseTapChangerStringFieldValue method calls
phaseTapChanger.getRegulationMode().name() without null-checking; update
getPhaseTapChangerStringFieldValue to first retrieve
PhaseTapChanger#getRegulationMode() into a local variable, return null if that
variable is null for the PHASE_REGULATION_MODE, PHASE_REGULATION_MODE_1/2/3
cases, otherwise call .name(); keep the existing default branch that throws the
PowsyblException for unsupported FieldType.
In `@src/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.java`:
- Around line 180-192: Rename the two misleading test methods to reflect what
they actually assert: change testCreateFilterWithNullEquipmentIdsThrowsException
to testCreateFilterWithNullRuleThrowsException (it constructs new
ExpertFilter(EquipmentType.LINE, null) and expects NPE) and change
testGetFilterTypeReturnsIdentifierList to testGetFilterTypeReturnsExpert (it
creates an ExpertFilter and asserts getFilterType() == FilterType.EXPERT);
update only the method names in ExpertFilterTest to match these clearer
identifiers so the test names align with the assertions involving ExpertFilter
and FilterType.EXPERT.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRule.java`:
- Around line 35-61: The factory method NumberExpertRule.of(...) is over the
cognitive complexity limit; extract the operator-specific reference validation
into a new private static helper (e.g.,
validateReferenceForOperator(OperatorType operatorType, Double referenceValue,
Set<Double> referenceValues)) that contains the EXISTS/NOT_EXISTS checks (ensure
both references are null), the BETWEEN/IN/NOT_IN checks (ensure referenceValues
non-null and non-empty), and the default single-value check (ensure
referenceValue non-null), throwing PowsyblException exactly as currently done;
then simplify NumberExpertRule.of to perform only null and
DataType.NUMBER/SUPPORTED_OPERATORS checks and call the new helper before
returning the appropriate NumberExpertRule constructors so behavior and error
messages remain unchanged.
In `@src/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.java`:
- Around line 35-61: The factory method StringExpertRule.of is too complex;
extract the operator-specific reference validation into a private helper (e.g.,
validateReferenceForOperator) that takes (OperatorType operatorType, String
referenceValue, Set<String> referenceValues) and performs the EXISTS/NOT_EXISTS
null checks, the IN/NOT_IN non-null non-empty referenceValues check, and the
default non-blank referenceValue check, throwing the same PowsyblException
messages; keep the public of(...) method focused on parameter null checks,
dataType/operator support check, call the new helper to validate references, and
then construct and return the appropriate StringExpertRule using the same
constructors (new StringExpertRule(fieldType, operatorType), new
StringExpertRule(fieldType, operatorType, referenceValues), new
StringExpertRule(fieldType, operatorType, referenceValue)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ad3d4b7-dcb8-4748-9239-7e85c522d09a
📒 Files selected for processing (27)
src/main/java/org/gridsuite/filter/wip/AbstractFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/ExpertFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/data/DataType.javasrc/main/java/org/gridsuite/filter/wip/expert/data/FieldType.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/AbstractExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/BooleanExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/CombinatorExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/EnumExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtils.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/FilterExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rules/StringExpertRule.javasrc/test/java/org/gridsuite/filter/wip/SimpleFilterTest.javasrc/test/java/org/gridsuite/filter/wip/TestNetworkUtils.javasrc/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/BooleanExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/CombinatorExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/EnumExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/ExpertRuleUtilsTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/FilterExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/NumberExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/PropertiesExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/SimpleExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rules/StringExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/identifier/IdentifierListFilterTest.java
KoloMenek
force-pushed
the
marutk/feat/introduce_reworked_expert_filter
branch
2 times, most recently
from
089c05d to
6ac623e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/test/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRuleTest.java (1)
34-42: 💤 Low valueAdd missing OR truth-table case for completeness.
The parameterized test covers OR with (true, false) and (false, false), but omits OR with (true, true) → true. While this case is less critical (the outcome is obvious), including it ensures complete truth-table coverage.
📋 Proposed addition
private static Stream<Arguments> provideMockRuleEvaluationArguments() { return Stream.of( + Arguments.of(CombinatorType.OR, Set.of(mockExpertRule(true), mockExpertRule(true)), true), Arguments.of(CombinatorType.OR, Set.of(mockExpertRule(true), mockExpertRule(false)), true), Arguments.of(CombinatorType.OR, Set.of(mockExpertRule(false), mockExpertRule(false)), false),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRuleTest.java` around lines 34 - 42, Update the parameterized test data in provideMockRuleEvaluationArguments to include the missing OR truth-table case: add an Arguments.of(CombinatorType.OR, Set.of(mockExpertRule(true), mockExpertRule(true)), true) entry so the OR combinator is tested for (true, true) as well; modify the Stream.of list in provideMockRuleEvaluationArguments to include this new Arguments element alongside the existing OR and AND cases.src/main/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRule.java (1)
34-36: ⚡ Quick winReplace immutable
Collections.emptySet()with mutable default.
Collections.emptySet()returns an unmodifiable empty set. If code later retrievessubRulesvia the Lombok-generated getter and attempts to add elements (e.g.,rule.getSubRules().add(...)), it will throwUnsupportedOperationExceptionat runtime.♻️ Proposed fix: Use mutable HashSet
`@Builder.Default` `@JsonDeserialize`(as = HashSet.class) - private Set<ExpertRule> subRules = Collections.emptySet(); + private Set<ExpertRule> subRules = new HashSet<>();🤖 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/filter/wip/expert/rule/CombinatorExpertRule.java` around lines 34 - 36, The field subRules in CombinatorExpertRule is initialized with Collections.emptySet(), which is immutable; replace that default with a mutable empty Set (e.g., new HashSet<>()) so callers using the Lombok getter (subRules) can add elements without throwing UnsupportedOperationException; update the `@Builder.Default` initializer for subRules (and keep `@JsonDeserialize`(as = HashSet.class)) to use a mutable HashSet instance instead of Collections.emptySet().src/test/java/org/gridsuite/filter/wip/TestNetworkUtils.java (1)
113-117: 💤 Low valueConsider consolidating generator initialization logic.
The current pattern sets
targetQtoDouble.NaNfor all generators in the loop (line 113), then overwritesGENERATOR_3's value outside the loop (line 117). This works but could be clearer:for (int i = 1; i <= 3; i++) { vl1.newGenerator() .setId("GENERATOR_" + i) .setNode(connect(vl1, nodeCounter1)) .setEnergySource(EnergySource.NUCLEAR) .setMinP(0.0).setMaxP(1000.0) .setTargetP(100.0 * i).setTargetV(400.0) - .setTargetQ(Double.NaN) + .setTargetQ(i == 3 ? 100.0 : Double.NaN) .setVoltageRegulatorOn(true) .add(); } -network.getGenerator("GENERATOR_3").setTargetQ(100.0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/filter/wip/TestNetworkUtils.java` around lines 113 - 117, Consolidate the generator initialization by setting each generator's target reactive power at creation instead of first using Double.NaN and then overwriting GENERATOR_3 later: when building generators in the loop (the builder that calls setTargetQ, setVoltageRegulatorOn, add), detect the generator id/name (or pass a map/condition) and call setTargetQ(100.0) for "GENERATOR_3" while using Double.NaN for others, or capture the created generator reference from add() (or network.getGenerator(...)) and immediately call setTargetQ on that reference; update references to setTargetQ, setVoltageRegulatorOn, add and network.getGenerator("GENERATOR_3") accordingly so initialization is done in one place.
🤖 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.
Inline comments:
In `@src/main/java/org/gridsuite/filter/wip/expert/rule/BooleanExpertRule.java`:
- Around line 52-53: In BooleanExpertRule, the EXISTS/NOT_EXISTS cases currently
use parsedFieldValue but should reflect presence only; update the switch
handling for EXISTS and NOT_EXISTS (in the method containing the switch over the
operator) to return true for EXISTS and false for NOT_EXISTS (since the field is
already checked non-null earlier), replacing references to parsedFieldValue in
those two branches.
In
`@src/main/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRule.java`:
- Around line 44-49: In CombinatorExpertRule.evaluateRule, validate that the
identifiable parameter is non-null before invoking subRules; add an explicit
null-check at the start of evaluateRule (for the method that switches on
combinatorType and iterates subRules) and either throw an
IllegalArgumentException/NullPointerException with a clear message (e.g.,
"identifiable must not be null") or handle null by returning false, so
subRules.stream().allMatch/anyMatch is never called with a null argument.
- Line 33: The field combinatorType in CombinatorExpertRule may be null and
evaluateRule performs a switch on it; update CombinatorExpertRule to guarantee
non-null by either annotating the field with a non-null annotation (e.g.,
`@NonNull`) and ensuring constructors/setters enforce it, or add an explicit
null-check at the start of evaluateRule that throws a clear
IllegalStateException (or returns a sensible default) when combinatorType is
null; reference the combinatorType field and the evaluateRule method when making
the change so the switch never receives a null value.
In `@src/main/java/org/gridsuite/filter/wip/expert/rule/FilterExpertRule.java`:
- Around line 36-43: The transient evaluation fields networkIdCache and
filterEvaluationCache should be excluded from Jackson serialization and
Lombok-generated equals/hashCode/toString: annotate both fields with `@JsonIgnore`
and with Lombok exclusions `@EqualsAndHashCode.Exclude` and `@ToString.Exclude`,
remove the `@JsonDeserialize`(as = HashSet.class) on filterEvaluationCache and
drop the now-unused JsonDeserialize/HashSet imports, and add import
com.fasterxml.jackson.annotation.JsonIgnore; keep their current default values
and getters unchanged so evaluateRule can still mutate them.
In
`@src/test/java/org/gridsuite/filter/wip/expert/rule/PropertiesExpertRuleTest.java`:
- Around line 541-555: The testPropertiesValue() method is asserting against
org.gridsuite.filter.expertfilter.expertrule.PropertiesExpertRule (incorrect
package); update the test to reference the intended class
org.gridsuite.filter.wip.expert.rule.PropertiesExpertRule everywhere it appears
(constructor/builder, type references, and assertions) so the test exercises the
wip class, or if this test truly belongs to a different test file remove the
method from this file; ensure all references (builder(), getPropertyValues(),
getStringValue(), getField(), getOperator()) point to the correct class after
the change.
---
Nitpick comments:
In
`@src/main/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRule.java`:
- Around line 34-36: The field subRules in CombinatorExpertRule is initialized
with Collections.emptySet(), which is immutable; replace that default with a
mutable empty Set (e.g., new HashSet<>()) so callers using the Lombok getter
(subRules) can add elements without throwing UnsupportedOperationException;
update the `@Builder.Default` initializer for subRules (and keep
`@JsonDeserialize`(as = HashSet.class)) to use a mutable HashSet instance instead
of Collections.emptySet().
In
`@src/test/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRuleTest.java`:
- Around line 34-42: Update the parameterized test data in
provideMockRuleEvaluationArguments to include the missing OR truth-table case:
add an Arguments.of(CombinatorType.OR, Set.of(mockExpertRule(true),
mockExpertRule(true)), true) entry so the OR combinator is tested for (true,
true) as well; modify the Stream.of list in provideMockRuleEvaluationArguments
to include this new Arguments element alongside the existing OR and AND cases.
In `@src/test/java/org/gridsuite/filter/wip/TestNetworkUtils.java`:
- Around line 113-117: Consolidate the generator initialization by setting each
generator's target reactive power at creation instead of first using Double.NaN
and then overwriting GENERATOR_3 later: when building generators in the loop
(the builder that calls setTargetQ, setVoltageRegulatorOn, add), detect the
generator id/name (or pass a map/condition) and call setTargetQ(100.0) for
"GENERATOR_3" while using Double.NaN for others, or capture the created
generator reference from add() (or network.getGenerator(...)) and immediately
call setTargetQ on that reference; update references to setTargetQ,
setVoltageRegulatorOn, add and network.getGenerator("GENERATOR_3") accordingly
so initialization is done in one place.
🪄 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: 04b773d0-bd18-48b4-90f7-d2a4b855978f
📒 Files selected for processing (22)
src/main/java/org/gridsuite/filter/wip/AbstractFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/ExpertFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/data/DataType.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/AbstractExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/BooleanExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/EnumExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/ExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/FilterExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/NumberExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/PropertiesExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/StringExpertRule.javasrc/test/java/org/gridsuite/filter/wip/SimpleFilterTest.javasrc/test/java/org/gridsuite/filter/wip/TestNetworkUtils.javasrc/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/BooleanExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/EnumExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/FilterExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/NumberExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/PropertiesExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/StringExpertRuleTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/gridsuite/filter/wip/expert/rule/NumberExpertRule.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/org/gridsuite/filter/wip/expert/data/DataType.java
- src/main/java/org/gridsuite/filter/wip/expert/ExpertFilter.java
- src/test/java/org/gridsuite/filter/wip/SimpleFilterTest.java
- src/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.java
Signed-off-by: Kamil MARUT <[email protected]>
Signed-off-by: Kamil MARUT <[email protected]>
KoloMenek
force-pushed
the
marutk/feat/introduce_reworked_expert_filter
branch
from
63b24e0 to
8914d76
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRuleTest.java (1)
191-197: 💤 Low valueConsider using AssertJ for consistency.
The test uses
assertEqualsfrom JUnit, while the rest of the test class uses AssertJ assertions (assertThat). For consistency, consider:- assertEquals(expected, rule.evaluateRule(equipment)); + assertThat(rule.evaluateRule(equipment)).isEqualTo(expected);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRuleTest.java` around lines 191 - 197, In testEvaluateRule inside CombinatorExpertRuleTest, replace the JUnit assertEquals with an AssertJ assertion to match the rest of the class: call assertThat on rule.evaluateRule(equipment) and assert equality to the expected boolean (use isTrue/isFalse or isEqualTo(expected) as appropriate); this keeps assertions consistent with other tests that use assertThat and references the CombinatorExpertRule builder usage and evaluateRule method to find the assertion to change.src/main/java/org/gridsuite/filter/wip/expert/rule/FilterExpertRule.java (1)
81-88: 💤 Low valueConsider reusing the cache
Setinstance rather than replacing it.Line 87 replaces
filterEvaluationCachewith a newSeton every cache miss, discarding the old instance (whichclearCache()had emptied). While correct, this allocates a newHashSetper evaluation.For a micro-optimization, clear and populate the existing set:
♻️ Optional refactor
- Set<String> filterEvaluation = referenceFilters.stream() + filterEvaluationCache.clear(); + filterEvaluationCache.addAll(referenceFilters.stream() .map(filter -> filter.evaluate(network)) .flatMap(List::stream) .map(Identifiable::getId) - .collect(Collectors.toSet()); - - filterEvaluationCache = filterEvaluation; - return filterEvaluation.contains(targetId); + .collect(Collectors.toSet())); + return filterEvaluationCache.contains(targetId);🤖 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/filter/wip/expert/rule/FilterExpertRule.java` around lines 81 - 88, The code currently replaces filterEvaluationCache with a newly collected Set every evaluation; instead, reuse the existing filterEvaluationCache instance: after computing the new Set from referenceFilters.stream().map(filter -> filter.evaluate(network)).flatMap(...).map(Identifiable::getId).collect(...), call filterEvaluationCache.clear() and then filterEvaluationCache.addAll(...) to populate it, and finally return filterEvaluationCache.contains(targetId); ensure filterEvaluationCache is non-null (initialize if necessary) and reference the methods/variables filterEvaluationCache, referenceFilters, evaluate(...), Identifiable::getId, and targetId when making the change.
🤖 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.
Inline comments:
In `@src/main/java/org/gridsuite/filter/wip/AbstractFilter.java`:
- Around line 54-61: The evaluate method currently builds and consumes the
stream returned by getConsideredEquipmentStream(...) and then calls
clearEvaluationCache(), but if the stream evaluation (via evaluateFilterRule)
throws an exception the cache isn't cleared; modify AbstractFilter.evaluate to
wrap the filtering/collection step in a try-finally so clearEvaluationCache() is
always executed. Ensure the try covers the call to
getConsideredEquipmentStream(...).filter(this::evaluateFilterRule).toList() and
the finally invokes clearEvaluationCache(); this guarantees
FilterExpertRule/CombinatorExpertRule’s filterEvaluationCache and nested caches
are cleared even on errors.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/filter/wip/expert/rule/FilterExpertRule.java`:
- Around line 81-88: The code currently replaces filterEvaluationCache with a
newly collected Set every evaluation; instead, reuse the existing
filterEvaluationCache instance: after computing the new Set from
referenceFilters.stream().map(filter ->
filter.evaluate(network)).flatMap(...).map(Identifiable::getId).collect(...),
call filterEvaluationCache.clear() and then filterEvaluationCache.addAll(...) to
populate it, and finally return filterEvaluationCache.contains(targetId); ensure
filterEvaluationCache is non-null (initialize if necessary) and reference the
methods/variables filterEvaluationCache, referenceFilters, evaluate(...),
Identifiable::getId, and targetId when making the change.
In
`@src/test/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRuleTest.java`:
- Around line 191-197: In testEvaluateRule inside CombinatorExpertRuleTest,
replace the JUnit assertEquals with an AssertJ assertion to match the rest of
the class: call assertThat on rule.evaluateRule(equipment) and assert equality
to the expected boolean (use isTrue/isFalse or isEqualTo(expected) as
appropriate); this keeps assertions consistent with other tests that use
assertThat and references the CombinatorExpertRule builder usage and
evaluateRule method to find the assertion to change.
🪄 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: d8b56b0f-f15c-4367-b662-e17bfe406ca8
📒 Files selected for processing (24)
src/main/java/org/gridsuite/filter/wip/AbstractFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/ExpertFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/data/DataType.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/AbstractCachingExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/AbstractExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/BooleanExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/EnumExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/ExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/FilterExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/NumberExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/PropertiesExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/StringExpertRule.javasrc/main/java/org/gridsuite/filter/wip/identifier/IdentifierListFilter.javasrc/test/java/org/gridsuite/filter/wip/SimpleFilterTest.javasrc/test/java/org/gridsuite/filter/wip/TestNetworkUtils.javasrc/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/BooleanExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/EnumExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/FilterExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/NumberExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/PropertiesExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/StringExpertRuleTest.java
🚧 Files skipped from review as they are similar to previous changes (15)
- src/main/java/org/gridsuite/filter/wip/identifier/IdentifierListFilter.java
- src/main/java/org/gridsuite/filter/wip/expert/rule/ExpertRule.java
- src/main/java/org/gridsuite/filter/wip/expert/ExpertFilter.java
- src/main/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRule.java
- src/main/java/org/gridsuite/filter/wip/expert/rule/BooleanExpertRule.java
- src/main/java/org/gridsuite/filter/wip/expert/data/DataType.java
- src/main/java/org/gridsuite/filter/wip/expert/rule/AbstractExpertRule.java
- src/test/java/org/gridsuite/filter/wip/SimpleFilterTest.java
- src/main/java/org/gridsuite/filter/wip/expert/rule/PropertiesExpertRule.java
- src/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.java
- src/main/java/org/gridsuite/filter/wip/expert/rule/NumberExpertRule.java
- src/main/java/org/gridsuite/filter/wip/expert/rule/StringExpertRule.java
- src/test/java/org/gridsuite/filter/wip/expert/rule/BooleanExpertRuleTest.java
- src/test/java/org/gridsuite/filter/wip/expert/rule/PropertiesExpertRuleTest.java
- src/test/java/org/gridsuite/filter/wip/expert/rule/EnumExpertRuleTest.java
KoloMenek
force-pushed
the
marutk/feat/introduce_reworked_expert_filter
branch
from
597bcd9 to
1d9480f
Compare
Signed-off-by: Kamil MARUT <[email protected]>
KoloMenek
force-pushed
the
marutk/feat/introduce_reworked_expert_filter
branch
from
1d9480f to
251de53
Compare
Signed-off-by: Kamil MARUT <[email protected]>
KoloMenek
changed the title
Signed-off-by: Kamil MARUT <[email protected]>
|
Reworked annotations across filters Added support for equals/hashcode Added tests for serialization/deserialization Signed-off-by: Kamil MARUT <[email protected]>
KoloMenek
force-pushed
the
marutk/feat/introduce_reworked_expert_filter
branch
from
dbeb119 to
9902f9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In
`@src/main/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRule.java`:
- Around line 30-36: Make subRules immutable and prevent reassignment: declare
the field subRules as final, keep the constructor CombinatorExpertRule(...)
initializing it with Set.copyOf(Objects.requireNonNull(subRules)), remove any
setters or later assignments to subRules, and ensure any methods that iterate
over subRules (the code blocks referencing subRules after construction) operate
on this immutable field or on a local immutable snapshot to avoid concurrent
mutation during iteration.
In
`@src/main/java/org/gridsuite/filter/wip/expert/rule/PropertiesExpertRule.java`:
- Around line 24-40: The class enforces non-null/immutable invariants only in
the `@Builder` constructor but exposes mutable fields via Lombok-generated
setters, so add validation in the mutators: implement explicit
setFieldType(FieldType), setOperatorType(OperatorType),
setTargetProperty(String) and setReferenceValues(Set<String>) that each
requireNonNull their argument (and for referenceValues wrap with Set.copyOf) to
preserve invariants at runtime; keep the existing constructor and builder but
replace or override Lombok-generated setters for these fields so null/invalid
assignments cannot break later logic (e.g., the code paths around
fieldType/operatorType/referenceValues usage).
In `@src/main/java/org/gridsuite/filter/wip/identifier/IdentifierListFilter.java`:
- Around line 26-37: Make equipmentIds immutable and non-null for the lifetime
of IdentifierListFilter by changing the field to private final Set<String>
equipmentIds, initializing it in the constructor with
Set.copyOf(Objects.requireNonNull(equipmentIds)) (already done) and preventing
Lombok from generating a public setter; replace `@Data` (which creates setters)
with `@Getter` (and keep `@EqualsAndHashCode`(callSuper = true)) or otherwise remove
the setter so external code cannot reassign equipmentIds, ensuring the
builder/constructor is the only way to set it and the set remains unmodifiable.
🪄 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: 363061a0-61a6-4a5c-93e7-8e9dce6e7099
📒 Files selected for processing (23)
src/main/java/org/gridsuite/filter/wip/AbstractFilter.javasrc/main/java/org/gridsuite/filter/wip/Filter.javasrc/main/java/org/gridsuite/filter/wip/expert/ExpertFilter.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/AbstractCachingExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/AbstractExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/BooleanExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/EnumExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/ExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/FilterExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/NumberExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/PropertiesExpertRule.javasrc/main/java/org/gridsuite/filter/wip/expert/rule/StringExpertRule.javasrc/main/java/org/gridsuite/filter/wip/identifier/IdentifierListFilter.javasrc/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/BooleanExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/CombinatorExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/EnumExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/FilterExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/NumberExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/PropertiesExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/expert/rule/StringExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/identifier/IdentifierListFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (9)
- src/main/java/org/gridsuite/filter/wip/expert/rule/EnumExpertRule.java
- src/main/java/org/gridsuite/filter/wip/expert/rule/BooleanExpertRule.java
- src/main/java/org/gridsuite/filter/wip/expert/rule/NumberExpertRule.java
- src/test/java/org/gridsuite/filter/wip/expert/rule/BooleanExpertRuleTest.java
- src/test/java/org/gridsuite/filter/wip/expert/rule/EnumExpertRuleTest.java
- src/test/java/org/gridsuite/filter/wip/expert/ExpertFilterTest.java
- src/main/java/org/gridsuite/filter/wip/expert/ExpertFilter.java
- src/main/java/org/gridsuite/filter/wip/expert/rule/FilterExpertRule.java
- src/test/java/org/gridsuite/filter/wip/expert/rule/PropertiesExpertRuleTest.java
TheMaskedTurtle
requested changes
Jun 10, 2026
Signed-off-by: Kamil MARUT <[email protected]>
KoloMenek
force-pushed
the
marutk/feat/introduce_reworked_expert_filter
branch
from
4533217 to
e9b4f0a
Compare
Signed-off-by: Kamil MARUT <[email protected]>
TheMaskedTurtle
requested changes
Jun 10, 2026
TheMaskedTurtle
left a comment
TheMaskedTurtle
left a comment
Contributor
There was a problem hiding this comment.
Same remark as for field names, it would be nicer to keep Lists as Lists and not Sets. Futhermore, we might need to conserve some ordring for current server features, so I think for now we should rather keep Lists.
Signed-off-by: Kamil MARUT <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/org/gridsuite/filter/wip/IdentifierListFilter.java (1)
25-31:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
@Datagenerates setters that violate non-null invariants.
@Datagenerates public setters forequipmentTypeandequipmentIds, allowing external code to reassign them to null or mutable collections after construction. This bypasses the defensive copy and null-check in the constructor (lines 35-36) and will cause NPE whenevaluateFilterRuleis called (line 48).🔒 Recommended fix
Replace
@Datawith@Getterto prevent setter generation, or make fieldsfinal:Option 1 (preferred): Replace
@Datawith@Getter-@Data +@Getter `@EqualsAndHashCode` `@NoArgsConstructor`(access = AccessLevel.PRIVATE) public class IdentifierListFilter implements Filter {Option 2: Make fields final
- private EquipmentType equipmentType; - private Set<String> equipmentIds; + private final EquipmentType equipmentType; + private final Set<String> equipmentIds;🤖 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/filter/wip/IdentifierListFilter.java` around lines 25 - 31, The class IdentifierListFilter uses Lombok's `@Data` which generates public setters that can break the constructor's non-null/defensive-copy guarantees for equipmentType and equipmentIds; fix by removing `@Data` and using `@Getter` (or alternatively make equipmentType and equipmentIds final) so setters are not generated, ensuring the constructor's null-checks and defensive copy remain effective and evaluateFilterRule cannot encounter a null or mutated collection.src/test/java/org/gridsuite/filter/wip/ExpertFilterTest.java (1)
256-259:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading test method name (copy-paste artifact).
The method name
testCreateFilterWithNullEquipmentIdsThrowsExceptionsuggests it tests anequipmentIdsparameter, butExpertFilterhas no such field. It actually validates the nullruleargument.📝 Suggested fix
- void testCreateFilterWithNullEquipmentIdsThrowsException() { + void testCreateFilterWithNullRuleThrowsException() { assertThatThrownBy(() -> new ExpertFilter(EquipmentType.LINE, null)) .isInstanceOf(NullPointerException.class); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/org/gridsuite/filter/wip/ExpertFilterTest.java` around lines 256 - 259, Rename this misleading test method and align it with the validated parameter: change the method name `testCreateFilterWithNullEquipmentIdsThrowsException` to something like `testCreateFilterWithNullRuleThrowsException` (or similar) to reflect that the second constructor argument is the `rule` being validated; keep the assertion that constructing `new ExpertFilter(EquipmentType.LINE, null)` throws a NullPointerException and update any javadoc/comments referencing `equipmentIds` accordingly so the test clearly targets `rule`.src/main/java/org/gridsuite/filter/wip/rule/NumberExpertRule.java (1)
38-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent unchecked runtime failures in numeric rule evaluation.
Malformed numeric field content and missing operands currently surface as unchecked exceptions (
NumberFormatException,NullPointerException,NoSuchElementException). Validate/normalize before comparisons and fail with a controlled domain exception.🔧 Suggested fix
+import com.powsybl.commons.PowsyblException; +import java.util.EnumSet; @@ public static Double getNumberValue(String value) { - return value == null ? Double.NaN : Double.parseDouble(value); + if (value == null) { + return Double.NaN; + } + try { + return Double.parseDouble(value); + } catch (NumberFormatException e) { + throw new PowsyblException("Invalid numeric field value: " + value, e); + } } @@ public boolean evaluateRule(Identifiable<?> identifiable) { Double fieldValue = getNumberValue(ExpertFilterUtils.getFieldValue(field, null, identifiable)); if (fieldValue.isNaN()) { return OperatorType.NOT_EXISTS.equals(operator); } + if (EnumSet.of(OperatorType.GREATER_OR_EQUALS, OperatorType.GREATER, + OperatorType.LOWER_OR_EQUALS, OperatorType.LOWER).contains(operator) && value == null) { + throw new PowsyblException(operator + " requires non-null value"); + } + if (OperatorType.BETWEEN.equals(operator) && (values == null || values.isEmpty())) { + throw new PowsyblException("BETWEEN requires non-empty values"); + } @@ private boolean evaluateBetweenOperator(Double fieldValue) { Double lowerLimit = Collections.min(values); Double upperLimit = Collections.max(values); return fieldValue.compareTo(lowerLimit) >= 0 && fieldValue.compareTo(upperLimit) <= 0; }Also applies to: 57-63, 78-81
🤖 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/filter/wip/rule/NumberExpertRule.java` around lines 38 - 40, The getNumberValue method (and other numeric-evaluating code paths in NumberExpertRule) currently throws unchecked exceptions for null/malformed input; change these to validate and normalize inputs and throw a controlled domain exception instead: update getNumberValue(String) to check for null/empty and use a safe parse with try/catch around Double.parseDouble, returning a sentinel (e.g., OptionalDouble or Double.NaN) only after validation, and on parse failure throw a custom checked/unchecked DomainValidationException with a clear message; apply the same pattern to the other numeric comparison helpers referenced in the file (the blocks around lines 57-63 and 78-81) so all NumberExpertRule numeric conversions and comparisons guard against NumberFormatException, NullPointerException, and missing operands and surface a consistent domain exception.
🧹 Nitpick comments (3)
src/main/java/org/gridsuite/filter/wip/rule/FilterExpertRule.java (2)
49-54: ⚡ Quick winConsider making
field,operator, andfiltersfinal to prevent setter-based mutation.Lombok's
@Datagenerates public setters that can reassign these fields after construction, breaking the immutability guarantee established byList.copyOfon line 53 and the non-null validation on lines 51-52.🔒 Proposed fix: declare fields as final
- private FieldType field; - private OperatorType operator; - private List<Filter> filters; + private final FieldType field; + private final OperatorType operator; + private final List<Filter> filters;This prevents Lombok from generating setters and ensures the defensive copy is never replaced. Cache fields (
cachedFilterEvaluationandfilterEvaluationCache) should remain mutable.🤖 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/filter/wip/rule/FilterExpertRule.java` around lines 49 - 54, Mark the instance fields field, operator, and filters in class FilterExpertRule as final to prevent Lombok-generated setters from allowing post-construction mutation; keep cachedFilterEvaluation and filterEvaluationCache non-final so they remain mutable. This preserves the defensive List.copyOf and non-null checks in the FilterExpertRule constructor while avoiding setter-based replacement of those fields.
80-93: 💤 Low valueConsider assigning the set directly instead of clearing and adding.
Line 88 uses
addAllto populate an empty cache set. SincefilterEvaluationCacheis always cleared inclearCache()and the check on line 81 guards against re-population, you can assign the new set directly.♻️ Proposed refactor
private boolean evaluateIsPartOfOperator(Network network, String targetId) { if (!cachedFilterEvaluation) { - Set<String> filterEvaluation = filters.stream() + filterEvaluationCache = filters.stream() .map(filter -> filter.evaluate(network)) .flatMap(List::stream) .map(Identifiable::getId) .collect(Collectors.toSet()); - filterEvaluationCache.addAll(filterEvaluation); cachedFilterEvaluation = true; } return filterEvaluationCache.contains(targetId); }This eliminates the intermediate variable and the
addAllcall.🤖 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/filter/wip/rule/FilterExpertRule.java` around lines 80 - 93, In evaluateIsPartOfOperator, replace building a local Set and calling filterEvaluationCache.addAll(...) with directly assigning the collected set to filterEvaluationCache (e.g. filterEvaluationCache = filters.stream()...collect(Collectors.toSet())); keep the cachedFilterEvaluation = true flag and the contains check, remove the intermediate variable and the addAll call to simplify and avoid unnecessary copying; reference symbols: evaluateIsPartOfOperator, filterEvaluationCache, cachedFilterEvaluation, filters, clearCache.src/main/java/org/gridsuite/filter/wip/rule/CombinatorExpertRule.java (1)
35-39: ⚡ Quick winConsider making fields
finalto prevent setter-based mutation.Lombok's
@Datagenerates public setters forcombinatorandrules. After construction, callers can reassignruleswith a different (potentially mutable) list, breaking the immutability guarantee established byList.copyOfon line 38.🔒 Proposed fix: declare fields as final
- private CombinatorType combinator; - private List<ExpertRule> rules; + private final CombinatorType combinator; + private final List<ExpertRule> rules;This prevents Lombok from generating setters and ensures the defensive copy is never replaced.
🤖 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/filter/wip/rule/CombinatorExpertRule.java` around lines 35 - 39, The CombinatorExpertRule class exposes mutable setters because Lombok's `@Data` generates them for non-final fields; to preserve immutability make the fields final by declaring combinator and rules as final (i.e., change the field declarations for combinator and rules to be final) so Lombok will not generate setters and the defensive List.copyOf in the CombinatorExpertRule(CombinatorType combinator, List<ExpertRule> rules) constructor remains effective; keep the existing constructor and `@Builder` so object construction still works.
🤖 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.
Inline comments:
In `@src/main/java/org/gridsuite/filter/wip/ExpertFilter.java`:
- Around line 25-31: The class uses Lombok's `@Data` which generates public
setters that allow equipmentType and rule to be nulled out; replace `@Data` with
`@Getter` to prevent setter generation (keep `@EqualsAndHashCode` and
`@NoArgsConstructor` as-is) so the constructor's non-null invariants are
preserved, ensuring methods evaluateFilterRule and clearEvaluationCache cannot
encounter null fields due to external mutation.
---
Duplicate comments:
In `@src/main/java/org/gridsuite/filter/wip/IdentifierListFilter.java`:
- Around line 25-31: The class IdentifierListFilter uses Lombok's `@Data` which
generates public setters that can break the constructor's
non-null/defensive-copy guarantees for equipmentType and equipmentIds; fix by
removing `@Data` and using `@Getter` (or alternatively make equipmentType and
equipmentIds final) so setters are not generated, ensuring the constructor's
null-checks and defensive copy remain effective and evaluateFilterRule cannot
encounter a null or mutated collection.
In `@src/main/java/org/gridsuite/filter/wip/rule/NumberExpertRule.java`:
- Around line 38-40: The getNumberValue method (and other numeric-evaluating
code paths in NumberExpertRule) currently throws unchecked exceptions for
null/malformed input; change these to validate and normalize inputs and throw a
controlled domain exception instead: update getNumberValue(String) to check for
null/empty and use a safe parse with try/catch around Double.parseDouble,
returning a sentinel (e.g., OptionalDouble or Double.NaN) only after validation,
and on parse failure throw a custom checked/unchecked DomainValidationException
with a clear message; apply the same pattern to the other numeric comparison
helpers referenced in the file (the blocks around lines 57-63 and 78-81) so all
NumberExpertRule numeric conversions and comparisons guard against
NumberFormatException, NullPointerException, and missing operands and surface a
consistent domain exception.
In `@src/test/java/org/gridsuite/filter/wip/ExpertFilterTest.java`:
- Around line 256-259: Rename this misleading test method and align it with the
validated parameter: change the method name
`testCreateFilterWithNullEquipmentIdsThrowsException` to something like
`testCreateFilterWithNullRuleThrowsException` (or similar) to reflect that the
second constructor argument is the `rule` being validated; keep the assertion
that constructing `new ExpertFilter(EquipmentType.LINE, null)` throws a
NullPointerException and update any javadoc/comments referencing `equipmentIds`
accordingly so the test clearly targets `rule`.
---
Nitpick comments:
In `@src/main/java/org/gridsuite/filter/wip/rule/CombinatorExpertRule.java`:
- Around line 35-39: The CombinatorExpertRule class exposes mutable setters
because Lombok's `@Data` generates them for non-final fields; to preserve
immutability make the fields final by declaring combinator and rules as final
(i.e., change the field declarations for combinator and rules to be final) so
Lombok will not generate setters and the defensive List.copyOf in the
CombinatorExpertRule(CombinatorType combinator, List<ExpertRule> rules)
constructor remains effective; keep the existing constructor and `@Builder` so
object construction still works.
In `@src/main/java/org/gridsuite/filter/wip/rule/FilterExpertRule.java`:
- Around line 49-54: Mark the instance fields field, operator, and filters in
class FilterExpertRule as final to prevent Lombok-generated setters from
allowing post-construction mutation; keep cachedFilterEvaluation and
filterEvaluationCache non-final so they remain mutable. This preserves the
defensive List.copyOf and non-null checks in the FilterExpertRule constructor
while avoiding setter-based replacement of those fields.
- Around line 80-93: In evaluateIsPartOfOperator, replace building a local Set
and calling filterEvaluationCache.addAll(...) with directly assigning the
collected set to filterEvaluationCache (e.g. filterEvaluationCache =
filters.stream()...collect(Collectors.toSet())); keep the cachedFilterEvaluation
= true flag and the contains check, remove the intermediate variable and the
addAll call to simplify and avoid unnecessary copying; reference symbols:
evaluateIsPartOfOperator, filterEvaluationCache, cachedFilterEvaluation,
filters, clearCache.
🪄 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: 136e661d-f73f-4376-a4d7-34e5eeec2c94
📒 Files selected for processing (23)
src/main/java/org/gridsuite/filter/wip/ExpertFilter.javasrc/main/java/org/gridsuite/filter/wip/Filter.javasrc/main/java/org/gridsuite/filter/wip/IdentifierListFilter.javasrc/main/java/org/gridsuite/filter/wip/NetworkUtils.javasrc/main/java/org/gridsuite/filter/wip/data/DataType.javasrc/main/java/org/gridsuite/filter/wip/rule/BooleanExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/CombinatorExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/EnumExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/ExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/FilterExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/NumberExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/PropertiesExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/StringExpertRule.javasrc/test/java/org/gridsuite/filter/wip/ExpertFilterTest.javasrc/test/java/org/gridsuite/filter/wip/IdentifierListFilterTest.javasrc/test/java/org/gridsuite/filter/wip/NetworkUtilsTest.javasrc/test/java/org/gridsuite/filter/wip/rule/BooleanExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/CombinatorExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/EnumExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/FilterExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/NumberExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/PropertiesExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/StringExpertRuleTest.java
TheMaskedTurtle
requested changes
Jun 11, 2026
TheMaskedTurtle
left a comment
TheMaskedTurtle
left a comment
Contributor
There was a problem hiding this comment.
Just had an idea for tests, you could put all the providers in distinct classes BooleanExpertRuleProviders, etc... for example, so that you can reuse them in both tests classes. In @MethodSource the syntax is like that "org.gridsuite.filter.utils.providers.BooleanProviders#provideArgumentsForGeneratorTest" to use a method from another class. It will produce less code, less duplication and we will be 100% sure it keeps the same behavior.
Signed-off-by: Kamil MARUT <[email protected]>
Signed-off-by: Kamil MARUT <[email protected]>
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/java/org/gridsuite/filter/wip/rule/NumberExpertRule.java (2)
57-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate operator operands before comparison dispatch.
Lines 57-64 and 78-80 still evaluate without strict operand checks: invalid configs (e.g.,
valuemissing for relational operators, or emptyvaluesforBETWEEN) can produce incorrect NaN-based outcomes or runtime failures.🔧 Suggested patch
+import com.powsybl.commons.PowsyblException; +import java.util.EnumSet; @@ public boolean evaluateRule(Identifiable<?> identifiable) { Double fieldValue = getNumberValue(ExpertFilterUtils.getFieldValue(field, null, identifiable)); if (fieldValue.isNaN()) { return OperatorType.NOT_EXISTS.equals(operator); } + + if (EnumSet.of(OperatorType.EQUALS, OperatorType.GREATER_OR_EQUALS, OperatorType.GREATER, + OperatorType.LOWER_OR_EQUALS, OperatorType.LOWER).contains(operator) && value.isNaN()) { + throw new PowsyblException(operator + " requires a non-NaN value"); + } + if (OperatorType.BETWEEN.equals(operator) && (values == null || values.isEmpty() || values.stream().anyMatch(Double::isNaN))) { + throw new PowsyblException("BETWEEN requires non-empty numeric values"); + } @@ private boolean evaluateBetweenOperator(Double fieldValue) { Double lowerLimit = Collections.min(values); Double upperLimit = Collections.max(values); return fieldValue.compareTo(lowerLimit) >= 0 && fieldValue.compareTo(upperLimit) <= 0; }Also applies to: 78-80
🤖 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/filter/wip/rule/NumberExpertRule.java` around lines 57 - 64, Add strict operand validation in NumberExpertRule before the switch that uses operator: verify that for relational operators (EQUALS, GREATER, GREATER_OR_EQUALS, LOWER, LOWER_OR_EQUALS) both value and fieldValue are non-null and parseable as numbers (not NaN), for BETWEEN ensure values is non-null and has at least two entries and that evaluateBetweenOperator can safely parse them, and for EXISTS handle nulls explicitly; if validations fail, throw an IllegalArgumentException or return a safe false result instead of proceeding. Locate the switch using operator in the method within NumberExpertRule and add these pre-checks (referencing operator, value, values, fieldValue, and evaluateBetweenOperator) before dispatching to prevent NaN comparisons or runtime errors.
38-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle malformed numeric field values with a controlled domain error.
At Line 39,
Double.parseDoubleexceptions still bubble as rawNumberFormatException, which can crash evaluation on misconfigured numeric fields.🔧 Suggested patch
+import com.powsybl.commons.PowsyblException; @@ public static Double getNumberValue(String value) { - return value == null ? Double.NaN : Double.parseDouble(value); + if (value == null) { + return Double.NaN; + } + try { + return Double.parseDouble(value); + } catch (NumberFormatException e) { + throw new PowsyblException("Invalid numeric field value: " + value, e); + } }🤖 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/filter/wip/rule/NumberExpertRule.java` around lines 38 - 40, The method getNumberValue currently lets NumberFormatException bubble up; update getNumberValue to catch NumberFormatException around Double.parseDouble(value) and instead throw a controlled unchecked domain error (e.g., new IllegalArgumentException("Malformed numeric field value: '"+value+"'", e) or a project-specific DomainException if available) so callers receive a clear, contextual error (include the original exception as the cause). Ensure null handling remains unchanged and reference getNumberValue to locate and modify the code.
🤖 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.
Duplicate comments:
In `@src/main/java/org/gridsuite/filter/wip/rule/NumberExpertRule.java`:
- Around line 57-64: Add strict operand validation in NumberExpertRule before
the switch that uses operator: verify that for relational operators (EQUALS,
GREATER, GREATER_OR_EQUALS, LOWER, LOWER_OR_EQUALS) both value and fieldValue
are non-null and parseable as numbers (not NaN), for BETWEEN ensure values is
non-null and has at least two entries and that evaluateBetweenOperator can
safely parse them, and for EXISTS handle nulls explicitly; if validations fail,
throw an IllegalArgumentException or return a safe false result instead of
proceeding. Locate the switch using operator in the method within
NumberExpertRule and add these pre-checks (referencing operator, value, values,
fieldValue, and evaluateBetweenOperator) before dispatching to prevent NaN
comparisons or runtime errors.
- Around line 38-40: The method getNumberValue currently lets
NumberFormatException bubble up; update getNumberValue to catch
NumberFormatException around Double.parseDouble(value) and instead throw a
controlled unchecked domain error (e.g., new IllegalArgumentException("Malformed
numeric field value: '"+value+"'", e) or a project-specific DomainException if
available) so callers receive a clear, contextual error (include the original
exception as the cause). Ensure null handling remains unchanged and reference
getNumberValue to locate and modify the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57af4513-bea6-4185-bb81-e241b0107f20
📒 Files selected for processing (16)
src/main/java/org/gridsuite/filter/wip/ExpertFilter.javasrc/main/java/org/gridsuite/filter/wip/IdentifierListFilter.javasrc/main/java/org/gridsuite/filter/wip/rule/BooleanExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/CombinatorExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/EnumExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/FilterExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/NumberExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/PropertiesExpertRule.javasrc/main/java/org/gridsuite/filter/wip/rule/StringExpertRule.javasrc/test/java/org/gridsuite/filter/wip/rule/BooleanExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/CombinatorExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/EnumExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/FilterExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/NumberExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/PropertiesExpertRuleTest.javasrc/test/java/org/gridsuite/filter/wip/rule/StringExpertRuleTest.java
💤 Files with no reviewable changes (2)
- src/test/java/org/gridsuite/filter/wip/rule/NumberExpertRuleTest.java
- src/test/java/org/gridsuite/filter/wip/rule/StringExpertRuleTest.java
🚧 Files skipped from review as they are similar to previous changes (9)
- src/main/java/org/gridsuite/filter/wip/rule/CombinatorExpertRule.java
- src/main/java/org/gridsuite/filter/wip/rule/PropertiesExpertRule.java
- src/main/java/org/gridsuite/filter/wip/rule/EnumExpertRule.java
- src/main/java/org/gridsuite/filter/wip/ExpertFilter.java
- src/main/java/org/gridsuite/filter/wip/rule/StringExpertRule.java
- src/main/java/org/gridsuite/filter/wip/rule/BooleanExpertRule.java
- src/test/java/org/gridsuite/filter/wip/rule/CombinatorExpertRuleTest.java
- src/main/java/org/gridsuite/filter/wip/IdentifierListFilter.java
- src/main/java/org/gridsuite/filter/wip/rule/FilterExpertRule.java
TheMaskedTurtle
approved these changes
Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary