From 081fb1152703dc50d8c2872da58d95b7d631f928 Mon Sep 17 00:00:00 2001 From: Toshiya Kobayashi Date: Fri, 11 Oct 2024 15:15:19 +0900 Subject: [PATCH] =?UTF-8?q?[DROOLS-7635]=20ansible-rulebook=20:=20Raise=20?= =?UTF-8?q?an=20error=20when=20a=20condition=20comp=E2=80=A6=20(#117)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [DROOLS-7635] ansible-rulebook : Raise an error when a condition compares incompatible types - alpha index test, beta index test beta index test fix add negate test and any test * refactor * minor comment fix --- .../domain/constraints/NegationOperator.java | 12 + .../RulebookConstraintOperator.java | 38 ++- .../integration/api/TypeMismatchTest.java | 310 +++++++++++++++++- 3 files changed, 339 insertions(+), 21 deletions(-) diff --git a/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/NegationOperator.java b/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/NegationOperator.java index 4e32ff1c..1ff512d8 100644 --- a/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/NegationOperator.java +++ b/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/NegationOperator.java @@ -4,6 +4,8 @@ import org.drools.model.ConstraintOperator; +import static org.drools.ansible.rulebook.integration.api.domain.constraints.RulebookConstraintOperator.isCompatibleType; + public class NegationOperator implements ConstraintOperator { private final ConstraintOperator toBeNegated; @@ -14,6 +16,16 @@ public NegationOperator(ConstraintOperator toBeNegated) { @Override public BiPredicate asPredicate() { + if (toBeNegated instanceof RulebookConstraintOperator rulebookConstraintOperator) { + return (t, v) -> { + // if not compatible type, return false. Use the operator to log the type check error + if (!isCompatibleType(t, v)) { + rulebookConstraintOperator.logTypeCheck(t, v); + return false; + } + return !toBeNegated.asPredicate().test(t, v); + }; + } return (t, v) -> !toBeNegated.asPredicate().test(t, v); } diff --git a/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/RulebookConstraintOperator.java b/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/RulebookConstraintOperator.java index 2dbcbe79..df0ed2f7 100644 --- a/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/RulebookConstraintOperator.java +++ b/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/RulebookConstraintOperator.java @@ -24,6 +24,7 @@ public RulebookConstraintOperator(Index.ConstraintType type) { this.type = type; } + @Override public void setConditionContext(RuleGenerationContext ruleContext, Map expression) { this.conditionContext = new ConditionContext(ruleContext.getRuleSetName(), ruleContext.getRuleName(), expression.toString()); } @@ -64,6 +65,7 @@ public RulebookConstraintOperator negate() { return this; } + @Override public boolean canInverse() { switch (this.type) { case EQUAL: @@ -107,23 +109,35 @@ public BiPredicate asPredicate() { } private boolean predicateWithTypeCheck(T t, V v, BiPredicate predicate) { - if (t == null - || v == null - || t instanceof Number && v instanceof Number - || t.getClass() == v.getClass()) { + if (isCompatibleType(t, v)) { return predicate.test(t, v); } else { - if (!typeCheckLogged) { - LOG.error("Cannot compare values of different types: {} and {}. RuleSet: {}. RuleName: {}. Condition: {}", - convertJavaClassToPythonClass(t.getClass()), - convertJavaClassToPythonClass(v.getClass()), - conditionContext.getRuleSet(), conditionContext.getRuleName(), conditionContext.getConditionExpression()); - typeCheckLogged = true; // Log only once per constraint - } - return false; // Different types are never equal + logTypeCheck(t, v); + return false; // Different types evaluation always return false even if the operator is NOT_EQUAL + } + } + + /* + * Log a type check error once per constraint + */ + void logTypeCheck(T t, V v) { + if (!typeCheckLogged) { + LOG.error("Cannot compare values of different types: {} and {}. RuleSet: {}. RuleName: {}. Condition: {}", + convertJavaClassToPythonClass(t.getClass()), + convertJavaClassToPythonClass(v.getClass()), + conditionContext.getRuleSet(), conditionContext.getRuleName(), conditionContext.getConditionExpression()); + typeCheckLogged = true; // Log only once per constraint } } + public static boolean isCompatibleType(Object t, Object v) { + return t == null + || v == null + || t instanceof Number && v instanceof Number + || t.getClass() == v.getClass(); + } + + @Override public RulebookConstraintOperator inverse() { switch (this.type) { case GREATER_THAN: diff --git a/drools-ansible-rulebook-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/TypeMismatchTest.java b/drools-ansible-rulebook-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/TypeMismatchTest.java index 3ca3a5bc..48c2faca 100644 --- a/drools-ansible-rulebook-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/TypeMismatchTest.java +++ b/drools-ansible-rulebook-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/TypeMismatchTest.java @@ -40,7 +40,7 @@ public void before() { stringPrintStream.getStringList().clear(); } - public static final String JSON1 = + public static final String JSON_MAP_STRING = """ { "name": "ruleSet1", @@ -107,10 +107,10 @@ public void before() { @Test public void mapAndString() { - RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(JSON1); + RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(JSON_MAP_STRING); - // incoming event.mera.headers is a map, not a string - List matchedRules = rulesExecutor.processEvents("{ \"meta\": {\"headers\": {\"Content-Length\": \"36\"}} } }").join(); + // incoming event.meta.headers is a map, not a string + List matchedRules = rulesExecutor.processEvents("{ \"meta\": {\"headers\": {\"Content-Length\": \"36\" } } }").join(); // When comparing mismatched types, the rule should not match (even r2). Logs error for 2 rules. assertNumberOfErrorLogs(2); assertThat(stringPrintStream.getStringList()) @@ -125,7 +125,7 @@ public void mapAndString() { assertEquals(0, matchedRules.size()); // One more time - matchedRules = rulesExecutor.processEvents("{ \"meta\": {\"headers\": {\"Content-Length\": \"25\"}} } }").join(); + matchedRules = rulesExecutor.processEvents("{ \"meta\": {\"headers\": {\"Content-Length\": \"25\" } } }").join(); // not firing. Don't log errors again. assertNumberOfErrorLogs(2); assertEquals(0, matchedRules.size()); @@ -133,7 +133,7 @@ public void mapAndString() { rulesExecutor.dispose(); } - public static final String JSON2 = + public static final String JSON_STRING_INTEGER = """ { "name": "ruleSet1", @@ -172,9 +172,9 @@ public void mapAndString() { @Test public void stringAndInteger() { - RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(JSON2); + RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(JSON_STRING_INTEGER); - // incoming event.i is a string, not a integer + // incoming event.i is a String, not an Integer List matchedRules = rulesExecutor.processEvents("{ \"i\": \"1\" }").join(); assertNumberOfErrorLogs(1); assertThat(stringPrintStream.getStringList()) @@ -290,7 +290,7 @@ public void typeMismatchWithNodeSharing() { .filter(constraint -> constraint.getExprId().equals("expr:i:EQUAL:1")) .count()).isEqualTo(1); - // i is a string, not an integer + // i is a String, not an Integer List matchedRules = rulesExecutor.processEvents("{ \"i\": \"1\", \"j\": 1 }").join(); assertNumberOfErrorLogs(1); assertEquals(0, matchedRules.size()); @@ -322,4 +322,296 @@ private static void collectAlphaNodes(NetworkNode[] sinks, List alpha private static void assertNumberOfErrorLogs(int expected) { assertThat(stringPrintStream.getStringList().stream().filter(s -> s.contains("ERROR")).count()).isEqualTo(expected); } + + public static final String JSON_ALPHA_INDEX = + """ + { + "name": "ruleSet1", + "rules": [ + { + "Rule": { + "name": "r1", + "condition": { + "AllCondition": [ + { + "EqualsExpression": { + "lhs": { + "Event": "meta.headers" + }, + "rhs": { + "String": "AAA" + } + } + } + ] + }, + "actions": [ + { + "Action": { + "action": "debug", + "action_args": {} + } + } + ], + "enabled": true + } + }, + { + "Rule": { + "name": "r2", + "condition": { + "AllCondition": [ + { + "EqualsExpression": { + "lhs": { + "Event": "meta.headers" + }, + "rhs": { + "String": "BBB" + } + } + } + ] + }, + "actions": [ + { + "Action": { + "action": "debug", + "action_args": {} + } + } + ], + "enabled": true + } + }, + { + "Rule": { + "name": "r3", + "condition": { + "AllCondition": [ + { + "EqualsExpression": { + "lhs": { + "Event": "meta.headers" + }, + "rhs": { + "String": "CCC" + } + } + } + ] + }, + "actions": [ + { + "Action": { + "action": "debug", + "action_args": {} + } + } + ], + "enabled": true + } + } + ] + } + """; + + @Test + public void alphaIndex() { + RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(JSON_ALPHA_INDEX); + + // incoming event.meta.headers is a map, not a string + List matchedRules = rulesExecutor.processEvents("{ \"meta\": {\"headers\": {\"Content-Length\": \"36\" } } }").join(); + // When alpha index is effective (= more than 3 Equals for the same property with literal values), constraint is not evaluated, so no error log. Just don't match. + assertNumberOfErrorLogs(0); + assertEquals(0, matchedRules.size()); + + rulesExecutor.dispose(); + } + + public static final String JSON_BETA_INDEX = + """ + { + "name": "ruleSet1", + "rules": [ + { + "Rule": { + "name": "r1", + "condition": { + "AllCondition": [ + { + "EqualsExpression": { + "lhs": { + "Event": "mydata" + }, + "rhs": { + "String": "AAA" + } + } + }, + { + "EqualsExpression": { + "lhs": { + "Event": "meta.headers" + }, + "rhs": { + "Events": "m_0.mydata" + } + } + } + ] + }, + "actions": [ + { + "Action": { + "action": "debug", + "action_args": {} + } + } + ], + "enabled": true + } + } + ] + } + """; + + @Test + public void betaIndex() { + RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(JSON_BETA_INDEX); + + // incoming event.meta.headers is a map, not a string + rulesExecutor.processEvents("{ \"mydata\": \"AAA\" }").join(); + List matchedRules = rulesExecutor.processEvents("{ \"meta\": {\"headers\": {\"Content-Length\": \"36\" } } }").join(); + + // When beta index is effective (= comparing 2 events properties with Equals), constraint is not evaluated, so no error log. Just doesn't match. + assertNumberOfErrorLogs(0); + assertEquals(0, matchedRules.size()); + + rulesExecutor.dispose(); + } + + public static final String JSON_NEGATE = + """ + { + "name": "ruleSet1", + "rules": [ + { + "Rule": { + "name": "r1", + "condition": { + "AllCondition": [ + { + "NegateExpression": { + "EqualsExpression": { + "lhs": { + "Event": "meta.headers" + }, + "rhs": { + "String": "Hello Testing World" + } + } + } + } + ] + }, + "actions": [ + { + "Action": { + "action": "debug", + "action_args": {} + } + } + ], + "enabled": true + } + } + ] + } + """; + + @Test + public void negation() { + RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(JSON_NEGATE); + + // incoming event.meta.headers is a map, not a string + List matchedRules = rulesExecutor.processEvents("{ \"meta\": {\"headers\": {\"Content-Length\": \"36\"} } }").join(); + // When comparing mismatched types, the rule should not match, even with negation. Logs error for 1 rule. + assertNumberOfErrorLogs(1); + assertThat(stringPrintStream.getStringList()) + .anyMatch(s -> s.contains("Cannot compare values of different types: dict and str." + + " RuleSet: ruleSet1." + + " RuleName: r1." + + " Condition: {lhs={Event=meta.headers}, rhs={String=Hello Testing World}}")); + + assertEquals(0, matchedRules.size()); + + rulesExecutor.dispose(); + } + + public static final String JSON_ANY = + """ + { + "name": "ruleSet1", + "rules": [ + { + "Rule": { + "name": "r1", + "condition": { + "AnyCondition": [ + { + "EqualsExpression": { + "lhs": { + "Event": "i" + }, + "rhs": { + "Integer": 1 + } + } + }, + { + "EqualsExpression": { + "lhs": { + "Event": "j" + }, + "rhs": { + "Integer": 1 + } + } + } + ] + }, + "actions": [ + { + "Action": { + "action": "debug", + "action_args": {} + } + } + ], + "enabled": true + } + } + ] + } + """; + + @Test + public void any() { + RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(JSON_ANY); + + rulesExecutor.processEvents("{ \"i\": \"AAA\"}").join(); + List matchedRules = rulesExecutor.processEvents("{ \"j\": 1 }").join(); + assertNumberOfErrorLogs(1); + assertThat(stringPrintStream.getStringList()) + .anyMatch(s -> s.contains("Cannot compare values of different types: str and int." + + " RuleSet: ruleSet1." + + " RuleName: r1." + + " Condition: {lhs={Event=i}, rhs={Integer=1}}")); + + // j == 1 matches, so the rule should match + assertEquals(1, matchedRules.size()); + + rulesExecutor.dispose(); + } } \ No newline at end of file