Add symbolic value type, resolver registry, and the in operator to the rule management component#8198
Open
kaviska wants to merge 5 commits into
Open
Add symbolic value type, resolver registry, and the in operator to the rule management component#8198kaviska wants to merge 5 commits into
in operator to the rule management component#8198kaviska wants to merge 5 commits into
Conversation
Introduce a new SYMBOLIC value type across the rule metadata, management and evaluation modules, allowing a rule to store a placeholder value (e.g. LATEST_ANDROID) that is resolved to a concrete value at evaluation time. - metadata/management: add SYMBOLIC to the value type enums and the DEVICE_POLICY flow type; RuleBuilder resolves symbolic values and validates option membership per token for the 'in' operator - evaluation: add SymbolicValueResolver contract and registry plus an evaluateSymbolicExpression path; FieldExtractor maps symbolic fields to a numeric evaluation type - add unit tests covering symbolic and 'in' operator happy and failure paths
Add the device policy flow metadata that backs the symbolic value support in the rule management server feature: - operators.json: add the 'in' and 'greaterThanOrEquals' operators - flows.json: add the devicePolicy flow (android/ios/macos/windows OS version) - fields.json: add the symbolic OS version field definitions
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds DEVICE_POLICY flow types, SYMBOLIC and LIST value types, symbolic value resolver infrastructure, evaluator and RuleBuilder support for symbolic values and new operators, plus device policy metadata, flows, and tests. ChangesDevice Policy Rule Feature
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
`@components/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/main/java/org/wso2/carbon/identity/rule/evaluation/internal/service/impl/OperatorRegistry.java`:
- Around line 48-49: The `in` operator in `OperatorRegistry` is inconsistent
with `evaluateInForNumber` because it compares raw comma-separated tokens
without trimming whitespace. Update the `supportedOperators.put("in", ...)`
predicate to trim each split value before checking containment, so
STRING/REFERENCE fields handle inputs like “Android, iOS” consistently. Use the
existing `stringPredicate` setup and keep the behavior aligned with the number
path’s token normalization.
In
`@components/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/main/java/org/wso2/carbon/identity/rule/evaluation/internal/service/impl/RuleEvaluator.java`:
- Around line 149-155: Handle parsing failures in evaluateInForNumber by
validating the commaSeparated input before splitting and by catching
NumberFormatException from Double.parseDouble, then convert those failures into
a RuleEvaluationException instead of letting them escape. Update the
evaluateInForNumber path in RuleEvaluator, and ensure callers such as
evaluateSymbolicExpression receive a handled rule-evaluation error when resolver
output is null or non-numeric.
- Around line 157-174: Validate operator and resolved-type compatibility in
evaluateSymbolicExpression before routing evaluation. In RuleEvaluator, the
current logic only branches on SymbolicValueResolver result type, which lets
NUMBER values fall through to operator.apply and LIST values always use
evaluateInForNumber regardless of the Operator. Update the method to explicitly
handle only the supported operator/type combinations (for example, use
membership logic only for in with LIST, and equality/other numeric operators
only for NUMBER), and throw RuleEvaluationException for unsupported combinations
so wrong results are not silently returned.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5036743e-2e95-4534-a730-1f5211e3b9ed
📒 Files selected for processing (18)
components/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/main/java/org/wso2/carbon/identity/rule/evaluation/api/model/FlowType.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/main/java/org/wso2/carbon/identity/rule/evaluation/api/model/RuleEvaluationResult.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/main/java/org/wso2/carbon/identity/rule/evaluation/api/resolver/SymbolicValueResolver.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/main/java/org/wso2/carbon/identity/rule/evaluation/api/resolver/SymbolicValueResolverRegistry.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/main/java/org/wso2/carbon/identity/rule/evaluation/internal/service/impl/FieldExtractor.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/main/java/org/wso2/carbon/identity/rule/evaluation/internal/service/impl/OperatorRegistry.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/main/java/org/wso2/carbon/identity/rule/evaluation/internal/service/impl/RuleEvaluationServiceImpl.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/main/java/org/wso2/carbon/identity/rule/evaluation/internal/service/impl/RuleEvaluator.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.management/src/main/java/org/wso2/carbon/identity/rule/management/api/model/FlowType.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.management/src/main/java/org/wso2/carbon/identity/rule/management/api/model/Value.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.management/src/main/java/org/wso2/carbon/identity/rule/management/api/util/RuleBuilder.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.management/src/test/java/org/wso2/carbon/identity/rule/management/util/RuleBuilderTest.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.management/src/test/resources/configs/valid-operators.jsoncomponents/rule-mgt/org.wso2.carbon.identity.rule.metadata/src/main/java/org/wso2/carbon/identity/rule/metadata/api/model/FlowType.javacomponents/rule-mgt/org.wso2.carbon.identity.rule.metadata/src/main/java/org/wso2/carbon/identity/rule/metadata/api/model/Value.javafeatures/rule-mgt/org.wso2.carbon.identity.rule.management.server.feature/resources/identity/rulemeta/fields.jsonfeatures/rule-mgt/org.wso2.carbon.identity.rule.management.server.feature/resources/identity/rulemeta/flows.jsonfeatures/rule-mgt/org.wso2.carbon.identity.rule.management.server.feature/resources/identity/rulemeta/operators.json
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8198 +/- ##
============================================
+ Coverage 52.82% 52.85% +0.02%
+ Complexity 21345 21255 -90
============================================
Files 2197 2198 +1
Lines 131721 131828 +107
Branches 19936 19959 +23
============================================
+ Hits 69587 69672 +85
- Misses 53596 53609 +13
- Partials 8538 8547 +9
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:
|
- OperatorRegistry: trim comma-separated tokens in the 'in' operator, for parity with evaluateInForNumber and the save-time option validation - RuleEvaluator: wrap numeric parsing in the 'in' evaluation so a non-numeric value throws a handled RuleEvaluationException instead of an uncaught NumberFormatException - RuleEvaluator: route symbolic expressions by the operator (in vs equals) instead of the resolved value shape, fixing incorrect results for 'in' with a single value and 'equals' with a multi-value - add unit tests covering the symbolic evaluation paths and the symbolic value resolver registry
The rule engine is generic infrastructure, so rename the 'deviceValue' parameter of evaluateInForNumber to 'numericValue' to avoid a device-specific name in the shared component.
…y ValueType import - Remove the unused 2-arg resolveValue overload (dead code; the single call site always passes the originalValue argument to the 3-arg version) - Import Value.ValueType and use the short name instead of the fully qualified name inline
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes in this pull request
This PR introduces a new symbolic value type (
SYMBOLIC) and theinoperator to the Rule Managementcomponent (
rule-mgt), together with the runtime resolver mechanism that turns a symbolic value into aconcrete value at evaluation time.
Symbolic value type
SYMBOLICto the metadata and management value type enums, and add theDEVICE_POLICYflow type.RuleBuilderresolves a symbolic field's value to theSYMBOLICtype at save time (rather than storing itas a plain string/number), so it can be resolved later at evaluation time.
FieldExtractormaps a symbolic field to a numeric evaluation type, because the live value supplied bythe data provider is concrete (numeric) — only the stored rule value is symbolic.
Symbolic value resolver
SymbolicValueResolvercontract:Value resolve(String symbolicValue)— given a placeholder, returnits resolved concrete value (a single
NUMBER, or aLISTfor comma-separated multi-values).SymbolicValueResolverRegistrykeyed by field name (register/deregister/getResolver), so consuming components register their resolver at bundle activation and remove it atdeactivation. This keeps
rule-mgtfully decoupled — it ships no resolvers and depends on no consumingcomponent.
RuleEvaluator.evaluateSymbolicExpressionlooks up the resolver for the field, resolves the storedplaceholder, and compares the result against the runtime value. If no resolver is registered for the field,
it fails with a clear error instead of throwing an unchecked exception.
inoperatorinoperator, which represents a multi-value (comma-separated) membership check(for example
androidOsVersion in LATEST_ANDROID,SECOND_LATEST_ANDROID).invalue is stored as aLIST, andRuleBuildervalidates each token of the list against thefield's allowed options at save time, so an invalid token is rejected early with a clear message rather than
failing at evaluation.
inchecks membership against the comma-separated numbers; for symbolic fields, eachtoken is resolved first and then membership is checked.
Unit tests
inoperator, for happy and failure paths.Rule metadata configuration (JSON) changes
The rule metadata catalog shipped by the rule management server feature
(
features/rule-mgt/.../resources/identity/rulemeta) is updated so the new type and operator are available atruntime:
operators.json— add theinandgreaterThanOrEqualsoperators.flows.json— add thedevicePolicyflow, listing the OS version fields(
androidOsVersion,iosOsVersion,macosOsVersion,windowsOsVersion).fields.json— add the symbolic OS version field definitions. Each field is declared withvalueType: symbolicand an options list that contains the symbolic placeholders (for exampleLATEST_ANDROID,SECOND_LATEST_ANDROID) alongside concrete versions.These are additive configuration changes; existing flows and fields are unaffected.
When should this PR be merged
No preconditions. The change is self-contained within
rule-mgt.Follow up actions
N/A