Repository navigation
Add value-less operators and value input to rule operators - #8346
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Change: Feature Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description gives detailed purpose, goals, implementation approach, behavior, tests, and follow-up work, but it does not follow the required template and omits several required sections, including… | Update the description to include every required template section. Mark non-applicable sections as “N/A” with a brief explanation, and provide the required security-check responses, test-environment details, documentation status, release no… |
| Docstring Coverage | |
Docstring coverage is 41.96% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 112 functions across 26 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the main changes: value-less operators and operator value-input metadata. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description gives detailed purpose, goals, implementation approach, behavior, tests, and follow-up work, but it does not follow the required template and omits several required sections, including user stories, release note, documentation, training, certification, marketing, security checks, samples, related PRs, migrations, test environment, and learning.
Resolution
Update the description to include every required template section. Mark non-applicable sections as “N/A” with a brief explanation, and provide the required security-check responses, test-environment details, documentation status, release note, and user-story summary.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8346 +/- ##
============================================
- Coverage 54.72% 54.69% -0.03%
+ Complexity 22521 22507 -14
============================================
Files 2267 2268 +1
Lines 133008 133482 +474
Branches 21870 21940 +70
============================================
+ Hits 72785 73012 +227
- Misses 51465 51707 +242
- Partials 8758 8763 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/38354054898
ff13925 to
19e46ca
Compare
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/38380034286
| Map<String, Operator> loaded = new HashMap<>(); | ||
| for (org.wso2.carbon.identity.rule.metadata.api.model.Operator operator | ||
| : ruleMetadataService.getApplicableOperatorsInExpressions()) { | ||
| String name = operator.getName(); |
There was a problem hiding this comment.
In order for the operator to be null in here, the operator should be defined in the operators.json file like below,
{
"equals": {
"name": "equals",
"displayName": "equals"
},
"notEquals": null
}
So we can consider this as a configuration issue of the server. However updated the logic to ignore null operators to reduce the impact for the other operators.
| private boolean applyOperatorForList(Operator operator, Object fieldValue, Object expressionValue) { | ||
| private static Boolean parseBoolean(String value) throws RuleEvaluationException { | ||
|
|
||
| if (!"true".equalsIgnoreCase(value) && !"false".equalsIgnoreCase(value)) { |
19e46ca to
c9beb37
Compare
c9beb37 to
6b2ede9
Compare
6b2ede9 to
50e5e1a
Compare
|
PR builder started |
|
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/38556541810
Proposed changes in this pull request
Why
A rule can compare a field with a value (
country equals Sri Lanka), but cannot ask whether a field has a value at all, for example "the user has no groups" or "the phone number was left blank". Some conditions also failed quietly: a malformed stored condition, an unknown operator or a referenced field that was never resolved evaluated tofalse(or threw aNullPointerException) instead of reporting an error.This PR follows up on #8334 with:
valueInputon every operator, saying what value a condition using it takes;in/notInlimited to multi-valued fields;What changes
1. Operators that check the field alone
isBlank/isNotBlankisEmpty/isNotEmptyA condition using one of these has no
value:{ "field": "user.groups", "operator": "isEmpty" }These checks run before the "a missing value never holds" rule, so
isEmptyholds on a field with no value.isEmpty/isNotEmptyon a single-valued field is an error;isBlankcovers that case.2. Operators that need a multi-valued field
in,notIn,isEmptyandisNotEmptycheck the entries of a multi-valued field, so they apply to multi-valued fields only:groups in [admins, staff]holds when the field and the list share an entry;country in [LK, IN]on a single-valued field is an error at evaluation (useequalsfor one value);groups in <another field>still intersects the two, and a single-valued field on the right counts as a set of one.The field metadata has no multi-valued type, so this is checked at evaluation; a product offering these operators (such as flows) restricts them to multi-valued fields in its own metadata and validation.
3.
valueInputon operators (rule.metadata)Each operator in
operators.jsondeclares the value a condition using it takes, which is also what a client renders after the operator is picked:valueInputnonesingle(default)listvalueInputtakes a single value, so existing configs load unchanged. An unknown value fails the config load.valueInputfrom the operator configuration; a field cannot declare otherwise.inconfigured assingle.valueInputinstead of the operator namesin/notIn.4. Validation when a rule is built (
RuleBuilder)valueInput: nonemust have no value; any other operator needs a value of a known type.listoperator, and alistoperator needs a list or another field.5. Evaluation errors instead of a quiet false (
RuleEvaluator)A stored condition that the builder would have rejected now raises
RuleEvaluationException:singleoperator, or a single value with alistoperator;in,notIn,isEmptyorisNotEmptyon a single-valued field;A missing (null) value on either side of a comparison still makes the condition not hold.
6. Other fixes
qualifierinfields.jsonis ignored rather than failing to load the file.FieldDefinitionerror message for an unknown operator no longer throws aNullPointerException.Tests
RuleBuilderTest: each rejected condition, value-less conditions built without a value, blank qualifiers stored as none.RuleEvaluatorTest: each value-less operator over text, number, boolean and list fields,in/notInon single-valued fields as errors, set comparisons between fields, mismatched and malformed stored values as errors, blank qualifiers.OperatorRegistryTest: the value input of each operator, the operators that need a multi-valued field, and configurations that disagree with the implementation.OperatorConfigTest,FieldDefinitionConfigTest:valueInputfrom configuration, an unknown value input, a qualifier infields.json.AuditLogBuilderForRuleTest,RuleSerializationCompatibilityTest: conditions without a value.When should this PR be merged
Any time. Existing operator configurations and stored rules keep working.
in/notInwere added in #8334 and are not released yet, so limiting them to multi-valued fields affects no released rule.Follow up actions
in,notIn,isEmptyandisNotEmptyonly on multi-valued fields (multi-valued claims, groups, roles) and validate them per claim.valuein conditions andvalueInputin the operator metadata (identity-api-server).valueInput(identity-apps).Related Issues
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.