add not/exists operator for properties filter#119
Conversation
Signed-off-by: Etienne LESOT <[email protected]>
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements property existence checking in ChangesProperty Existence Operators
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
flomillot
left a comment
flomillot
left a comment
There was a problem hiding this comment.
Even completed the parameterized tests 💯
Signed-off-by: Etienne LESOT <[email protected]>
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/rule/PropertiesExpertRule.java`:
- Around line 39-44: The constructor PropertiesExpertRule currently maps null
propertyValues to an empty list unconditionally which makes NOT_IN rules with
null/empty values match everything; change the constructor
(PropertiesExpertRule) to branch on operator (OperatorType): for EXISTS and
NOT_EXISTS allow null/empty by converting null to Collections.emptyList(), but
for IN and NOT_IN require a non-null, non-empty propertyValues and throw an
IllegalArgumentException (or similar) if missing/empty; keep the use of
List.copyOf for defensive copying when a non-empty list is provided.
🪄 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: 64957163-2b00-4246-a3fa-9b34cba2ec74
📒 Files selected for processing (1)
src/main/java/org/gridsuite/filter/wip/rule/PropertiesExpertRule.java
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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/rule/PropertiesExpertRule.java`:
- Around line 39-44: The constructor PropertiesExpertRule currently maps null
propertyValues to an empty list unconditionally which makes NOT_IN rules with
null/empty values match everything; change the constructor
(PropertiesExpertRule) to branch on operator (OperatorType): for EXISTS and
NOT_EXISTS allow null/empty by converting null to Collections.emptyList(), but
for IN and NOT_IN require a non-null, non-empty propertyValues and throw an
IllegalArgumentException (or similar) if missing/empty; keep the use of
List.copyOf for defensive copying when a non-empty list is provided.
🪄 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: 64957163-2b00-4246-a3fa-9b34cba2ec74
📒 Files selected for processing (1)
src/main/java/org/gridsuite/filter/wip/rule/PropertiesExpertRule.java
🛑 Comments failed to post (1)
src/main/java/org/gridsuite/filter/wip/rule/PropertiesExpertRule.java (1)
39-44:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
propertyValuesby operator to avoid silent match-allNOT_INrules.Line 43 currently converts
nullto empty for every operator. That makes malformedNOT_INrules evaluate totruefor any non-null property (becausenoneMatchon empty is true), which can silently over-select results. Keep null/empty tolerance only forEXISTS/NOT_EXISTS, and require non-empty values forIN/NOT_IN.💡 Proposed fix
`@Builder` public PropertiesExpertRule(FieldType field, OperatorType operator, String propertyName, List<String> propertyValues) { this.field = Objects.requireNonNull(field); this.operator = Objects.requireNonNull(operator); this.propertyName = Objects.requireNonNull(propertyName); - this.propertyValues = propertyValues == null ? Collections.emptyList() : List.copyOf(propertyValues); + if (operator == OperatorType.EXISTS || operator == OperatorType.NOT_EXISTS) { + this.propertyValues = Collections.emptyList(); + } else { + List<String> values = List.copyOf(Objects.requireNonNull(propertyValues, "propertyValues is required for " + operator)); + if (values.isEmpty()) { + throw new IllegalArgumentException("propertyValues must not be empty for " + operator); + } + this.propertyValues = values; + } }🤖 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/PropertiesExpertRule.java` around lines 39 - 44, The constructor PropertiesExpertRule currently maps null propertyValues to an empty list unconditionally which makes NOT_IN rules with null/empty values match everything; change the constructor (PropertiesExpertRule) to branch on operator (OperatorType): for EXISTS and NOT_EXISTS allow null/empty by converting null to Collections.emptyList(), but for IN and NOT_IN require a non-null, non-empty propertyValues and throw an IllegalArgumentException (or similar) if missing/empty; keep the use of List.copyOf for defensive copying when a non-empty list is provided.
2 participants
PR Summary