BAL-3498: enhance rule-engine evaluation logic with path comparison #3069
Conversation
- Introduce isPathComparison flag in Rule schema for clearer distinction - Update extractValue method to handle operators with and without path comparison - Adjust validation logic to accommodate new extracted value structure
- Ensure 'isPathComparison' is only true when present in the rule - Update condition to prevent false positives in comparison checks
|
WalkthroughThis pull request updates the rule engine by modifying how values are extracted and compared. The Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/common/src/rule-engine/rules/schemas.ts (1)
42-42: Consider adding JSDoc documentation for theisPathComparisonproperty.While the implementation is correct, adding documentation would help other developers understand when and why to use path comparison in rules.
Add documentation like this:
z.object({ key: z.string(), operator: z.literal(OPERATION.EQUALS), value: PrimitiveSchema, + /** When true, treats the value as a path to compare against instead of a literal value */ isPathComparison: z.boolean().default(false), }),Also applies to: 48-48, 59-59, 65-65, 71-71, 77-77, 88-88, 94-94, 100-100
packages/common/src/rule-engine/operators/helpers.ts (1)
42-46: Consider moving OPERATORS_WITHOUT_PATH_COMPARISON to a constants file.The constant would be better placed in a dedicated constants file since it defines operator behavior.
Consider moving it to the existing constants file:
// In ./constants.ts +export const OPERATORS_WITHOUT_PATH_COMPARISON = [ + OPERATION.AML_CHECK, + OPERATION.BETWEEN, + OPERATION.LAST_YEAR, +] as const; // In this file -const OPERATORS_WITHOUT_PATH_COMPARISON = [...] +import { OPERATORS_WITHOUT_PATH_COMPARISON } from './constants';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/common/src/rule-engine/operators/helpers.ts(2 hunks)packages/common/src/rule-engine/rules/schemas.ts(3 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/rule-engine/core/rule-engine.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
services/workflows-service/src/rule-engine/core/rule-engine.ts (1)
26-35: LGTM! Clear and robust value extraction logic.The new implementation properly handles both path comparison and regular value comparison cases with clear variable naming and type checking.
packages/common/src/rule-engine/operators/helpers.ts (1)
48-71: LGTM! Robust path comparison implementation.The implementation:
- Properly checks for path comparison eligibility
- Includes appropriate null/undefined checks
- Provides clear error messages
…s usage - Move OPERATORS_WITHOUT_PATH_COMPARISON to constants for better reusability - Refactor extraction logic to use isObject utility for clarity - Update imports to reflect the new constants structure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/common/src/rule-engine/operators/helpers.ts (1)
43-49: Consider extracting the path comparison check into a helper method.The path comparison check logic could be more readable if extracted into a separate method like
isPathComparisonOperation.+ private isPathComparisonOperation(rule: Rule): boolean { + return ( + !OPERATORS_WITHOUT_PATH_COMPARISON.includes( + rule.operator as (typeof OPERATORS_WITHOUT_PATH_COMPARISON)[number], + ) && + 'isPathComparison' in rule && + rule.isPathComparison + ); + } + extractValue(data: unknown, rule: Rule) { const value = get(data, rule.key); - const isPathComparison = - !OPERATORS_WITHOUT_PATH_COMPARISON.includes( - rule.operator as (typeof OPERATORS_WITHOUT_PATH_COMPARISON)[number], - ) && - 'isPathComparison' in rule && - rule.isPathComparison; + const isPathComparison = this.isPathComparisonOperation(rule);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/common/src/rule-engine/operators/constants.ts(1 hunks)packages/common/src/rule-engine/operators/helpers.ts(2 hunks)services/workflows-service/prisma/data-migrations(1 hunks)services/workflows-service/src/rule-engine/core/rule-engine.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/workflows-service/src/rule-engine/core/rule-engine.ts
- services/workflows-service/prisma/data-migrations
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze (javascript)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: lint
🔇 Additional comments (2)
packages/common/src/rule-engine/operators/constants.ts (1)
35-39: LGTM! Well-defined constant for operators without path comparison support.The constant correctly identifies operators that have special value handling and therefore don't support path comparison.
packages/common/src/rule-engine/operators/helpers.ts (1)
62-64: LGTM! Proper null check for comparison value.The code correctly validates the comparison value and throws a descriptive error when the path is invalid.
- Refactor validation logic for extracted values - Utilize 'in' operator for better readability and accuracy
…lity - Add isPathComparison flag to various rule definitions - Implement new unit tests for path comparison scenarios - Update integration tests to include path comparison in validation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (1)
1062-1125: LGTM! The new test suite thoroughly validates path comparison functionality.The test suite covers:
- Successful path comparison between different paths.
- Error handling for invalid paths.
The test cases are well-structured and provide good coverage of the new feature.
However, consider adding more test cases to cover:
- Path comparison with arrays
- Path comparison with nested objects
- Path comparison with different data types
- Edge cases like empty objects or null values
Here's an example test case for array comparison:
+ it('should compare array values from two different paths', () => { + const ruleSetExample: RuleSet = { + operator: OPERATOR.AND, + rules: [ + { + key: 'pluginsOutput.businessInformation.data', + operator: OPERATION.EQUALS, + value: 'entity.data.businesses', + isPathComparison: true, + }, + ], + }; + + const testContext = { + pluginsOutput: { + businessInformation: { + data: ['Business1', 'Business2'], + }, + }, + entity: { + data: { + businesses: ['Business1', 'Business2'], + }, + }, + }; + + const engine = RuleEngine(ruleSetExample); + const result = engine.run(testContext); + expect(result).toBeDefined(); + expect(result).toHaveLength(1); + expect(result[0]?.status).toBe('PASSED'); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/workflows-service/src/rule-engine/core/rule-engine.ts(2 hunks)services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts(20 hunks)services/workflows-service/src/rule-engine/rule-engine.service.intg.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/workflows-service/src/rule-engine/core/rule-engine.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test_windows
- GitHub Check: test_linux
- GitHub Check: build (windows-latest)
- GitHub Check: lint
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
services/workflows-service/src/rule-engine/rule-engine.service.intg.test.ts (1)
24-24: LGTM! The changes correctly specify that these rules don't use path comparison.The addition of
isPathComparison: falseto the rule definitions is consistent with the PR's objective of enhancing the rule engine with path comparison functionality.Also applies to: 30-30
services/workflows-service/src/rule-engine/core/test/rule-engine.unit.test.ts (1)
29-29: LGTM! The changes consistently specify that these rules don't use path comparison.The addition of
isPathComparison: falseto all existing rule definitions is thorough and consistent with the PR's objective.Also applies to: 47-47, 53-53, 80-80, 103-103, 505-505, 560-560, 625-625, 652-652, 681-681
Summary by CodeRabbit