Expose the fields that caused a rule to fail in the rule evaluation result#8201
Open
kaviska wants to merge 9 commits into
Open
Expose the fields that caused a rule to fail in the rule evaluation result#8201kaviska wants to merge 9 commits into
kaviska wants to merge 9 commits into
Conversation
Track and expose the fields that caused a rule to fail evaluation. - RuleEvaluationResult: add a failedFields list, a constructor accepting it, and a getFailedFields() accessor (empty when the rule is satisfied) - RuleEvaluator: collect the field names of failing expressions per OR branch, clearing them when a branch passes, and expose them via getFailedFields() - RuleEvaluationServiceImpl: pass the evaluator's failed fields into the result
Assert that getFailedFields() reports the failing field when a rule fails, and is empty when the rule passes.
Contributor
|
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:
📝 WalkthroughWalkthroughRule evaluation now captures failed field names, carries them through ChangesFailed field reporting
🚥 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8201 +/- ##
=========================================
Coverage 53.76% 53.77%
- Complexity 21354 21357 +3
=========================================
Files 2199 2199
Lines 129636 129648 +12
Branches 19366 19367 +1
=========================================
+ Hits 69704 69712 +8
- Misses 51437 51439 +2
- Partials 8495 8497 +2
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:
|
Address review feedback on the failed fields feature. - RuleEvaluator: remove the shared failedFields instance field, which was unsafe under concurrent evaluation, and add an evaluate(ruleId, rule, data) overload that returns a RuleEvaluationResult carrying the failed fields. The boolean evaluate(rule, data) delegates to it. - RuleEvaluationServiceImpl: use the new evaluate overload and return its result directly, so callers read the failed fields from the returned object. - RuleEvaluationResult: deprecate the two-arg constructor and keep the failed fields as-is (null when not set) so callers can distinguish 'not computed' from 'no failed fields'. - Update tests to read the failed fields from the returned result.
![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: 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
`@components/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/main/java/org/wso2/carbon/identity/rule/evaluation/internal/service/impl/RuleEvaluationServiceImpl.java`:
- Line 80: Guard the concatenated debug statement in RuleEvaluationServiceImpl
with if (LOG.isDebugEnabled()) before constructing the message and invoking
result.isRuleSatisfied().
🪄 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: 4289d3b9-5e00-4da3-af6e-8d4f0d9aeca7
📒 Files selected for processing (4)
components/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/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.evaluation/src/test/java/org/wso2/carbon/identity/rule/evaluation/core/RuleEvaluatorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- components/rule-mgt/org.wso2.carbon.identity.rule.evaluation/src/test/java/org/wso2/carbon/identity/rule/evaluation/core/RuleEvaluatorTest.java
Replace the ruleId-overloaded evaluate with a distinctly named evaluateResult(Rule, Map) method that returns the full result. The rule id is derived from rule.getId(), so the extra ruleId parameter is no longer needed. The existing boolean evaluate(Rule, Map) is unchanged and delegates to evaluateResult, keeping the change purely additive.
RuleEvaluator is an internal class that is excluded from Export-Package, so it is not reachable outside the bundle and its only caller is RuleEvaluationServiceImpl. Change the existing evaluate(Rule, Map) to return RuleEvaluationResult instead of a boolean, and drop the extra method, leaving a single entry point. Callers that need the boolean use isRuleSatisfied().
|
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.
Proposed changes in this pull request
This PR adds the ability to know which fields caused a rule to fail during evaluation, and exposes them on the
RuleEvaluationResult.Previously,
RuleEvaluationResultonly reported whether a rule was satisfied (a boolean). Callers had no way to tell why a rule did not match. This PR adds afailedFieldslist so callers can see the specific field names that failed, which is useful for diagnostics, user feedback, and debugging.The change is confined to the rule evaluation module and is additive and backward compatible:
RuleEvaluationResult— add afailedFieldslist, a constructor that accepts it, and agetFailedFields()accessor. The existing constructor is kept (it delegates with an empty list), and the accessor returns an unmodifiable, never-null list (empty when the rule is satisfied).RuleEvaluator— while evaluating the OR-of-ANDs rule tree, collect the field names of the failing expressions per OR branch. If a branch passes, the collected failed fields are cleared (the rule is satisfied, so nothing failed); if all branches fail, the failing fields from the branches are retained. Exposed viagetFailedFields().RuleEvaluationServiceImpl— pass the evaluator's failed fields into the returnedRuleEvaluationResult.The component provides:
failedFieldsis an additive side output and does not change the evaluation outcome.Unit tests assert that the failed fields are reported when a rule fails and are empty when the rule passes.
When should this PR be merged
No preconditions.
Follow up actions
N/A