From ab41fe4dc6f913c1d8b5bbf97650bfad7880a43d Mon Sep 17 00:00:00 2001 From: Toshiya Kobayashi Date: Mon, 30 Sep 2024 19:29:03 +0900 Subject: [PATCH] Remove OperatorWrapper. Add more assertion. Better error message --- .../rulebook/integration/api/LogUtil.java | 27 ++ .../RulebookConstraintOperator.java | 14 +- .../domain/constraints/RulebookOperator.java | 49 +-- .../integration/api/StringPrintStream.java | 36 ++ .../integration/api/TypeMismatchTest.java | 345 +++++++++++------- 5 files changed, 291 insertions(+), 180 deletions(-) create mode 100644 drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/LogUtil.java create mode 100644 drools-ansible-rulebook-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/StringPrintStream.java diff --git a/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/LogUtil.java b/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/LogUtil.java new file mode 100644 index 0000000..b83f13b --- /dev/null +++ b/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/LogUtil.java @@ -0,0 +1,27 @@ +package org.drools.ansible.rulebook.integration.api; + +import java.util.Map; + +public class LogUtil { + + private LogUtil() { + // utility class + } + + // convert java class to python class + private static Map, String> convertMap = Map.of( + java.lang.Integer.class, "int", + java.lang.Boolean.class, "bool", + java.lang.String.class, "str", + java.lang.Double.class, "float", + java.util.List.class, "list", + java.util.ArrayList.class, "list", + java.util.Map.class, "dict", + java.util.LinkedHashMap.class, "dict", + java.util.HashMap.class, "dict" + ); + + public static String convertJavaClassToPythonClass(Class javaClass) { + return convertMap.getOrDefault(javaClass, javaClass.getSimpleName()); + } +} 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 258f20a..2dbcbe7 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 @@ -4,15 +4,15 @@ import java.util.function.BiPredicate; import org.drools.ansible.rulebook.integration.api.domain.RuleGenerationContext; -import org.drools.model.ConstraintOperator; import org.drools.model.Index; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import static org.drools.ansible.rulebook.integration.api.LogUtil.convertJavaClassToPythonClass; import static org.drools.model.util.OperatorUtils.areEqual; import static org.drools.model.util.OperatorUtils.compare; -public class RulebookConstraintOperator implements ConstraintOperator { +public class RulebookConstraintOperator implements RulebookOperator { private static final Logger LOG = LoggerFactory.getLogger(RulebookConstraintOperator.class); @@ -29,17 +29,18 @@ public void setConditionContext(RuleGenerationContext ruleContext, Map exp } @Override - public boolean containsConstraintType() { + public boolean hasIndex() { return true; } @Override - public Index.ConstraintType getConstraintType() { + public Index.ConstraintType getIndexType() { return type; } public RulebookConstraintOperator negate() { switch (this.type) { + case FORALL_SELF_JOIN: case EQUAL: this.type = Index.ConstraintType.NOT_EQUAL; return this; @@ -113,9 +114,10 @@ private boolean predicateWithTypeCheck(T t, V v, BiPredicate predic return predicate.test(t, v); } else { if (!typeCheckLogged) { - // TODO: rewrite the message to be python friendly LOG.error("Cannot compare values of different types: {} and {}. RuleSet: {}. RuleName: {}. Condition: {}", - t.getClass(), v.getClass(), conditionContext.getRuleSet(), conditionContext.getRuleName(), conditionContext.getConditionExpression()); + 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 diff --git a/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/RulebookOperator.java b/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/RulebookOperator.java index 5b383fa..d298fc4 100644 --- a/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/RulebookOperator.java +++ b/drools-ansible-rulebook-integration-api/src/main/java/org/drools/ansible/rulebook/integration/api/domain/constraints/RulebookOperator.java @@ -22,67 +22,34 @@ default ConstraintOperator asConstraintOperator() { } static RulebookOperator newEqual() { - return new OperatorWrapper(new RulebookConstraintOperator(Index.ConstraintType.EQUAL)); + return new RulebookConstraintOperator(Index.ConstraintType.EQUAL); } static RulebookOperator newNotEqual() { - return new OperatorWrapper(new RulebookConstraintOperator(Index.ConstraintType.NOT_EQUAL)); + return new RulebookConstraintOperator(Index.ConstraintType.NOT_EQUAL); } static RulebookOperator newGreaterThan() { - return new OperatorWrapper(new RulebookConstraintOperator(Index.ConstraintType.GREATER_THAN)); + return new RulebookConstraintOperator(Index.ConstraintType.GREATER_THAN); } static RulebookOperator newGreaterOrEqual() { - return new OperatorWrapper(new RulebookConstraintOperator(Index.ConstraintType.GREATER_OR_EQUAL)); + return new RulebookConstraintOperator(Index.ConstraintType.GREATER_OR_EQUAL); } static RulebookOperator newLessThan() { - return new OperatorWrapper(new RulebookConstraintOperator(Index.ConstraintType.LESS_THAN)); + return new RulebookConstraintOperator(Index.ConstraintType.LESS_THAN); } static RulebookOperator newLessOrEqual() { - return new OperatorWrapper(new RulebookConstraintOperator(Index.ConstraintType.LESS_OR_EQUAL)); + return new RulebookConstraintOperator(Index.ConstraintType.LESS_OR_EQUAL); } default void setConditionContext(RuleGenerationContext ruleContext, Map expression) { - if (this instanceof OperatorWrapper operatorWrapper) { - operatorWrapper.setConditionContext(ruleContext, expression); + if (this instanceof RulebookConstraintOperator rulebookConstraintOperator) { + rulebookConstraintOperator.setConditionContext(ruleContext, expression); } else { // do nothing } } - - class OperatorWrapper implements RulebookOperator { - private final RulebookConstraintOperator delegate; - - public OperatorWrapper(RulebookConstraintOperator delegate) { - this.delegate = delegate; - } - - @Override - public BiPredicate asPredicate() { - return delegate.asPredicate(); - } - - @Override - public boolean canInverse() { - return delegate.canInverse(); - } - - @Override - public RulebookOperator inverse() { - return new OperatorWrapper(delegate.inverse()); - } - - @Override - public ConstraintOperator asConstraintOperator() { - return delegate; - } - - @Override - public void setConditionContext(RuleGenerationContext ruleContext, Map expression) { - delegate.setConditionContext(ruleContext, expression); - } - } } diff --git a/drools-ansible-rulebook-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/StringPrintStream.java b/drools-ansible-rulebook-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/StringPrintStream.java new file mode 100644 index 0000000..faa72dd --- /dev/null +++ b/drools-ansible-rulebook-integration-api/src/test/java/org/drools/ansible/rulebook/integration/api/StringPrintStream.java @@ -0,0 +1,36 @@ +package org.drools.ansible.rulebook.integration.api; + +import java.io.PrintStream; +import java.util.ArrayList; +import java.util.List; + +public class StringPrintStream extends PrintStream { + + public static final String LINE_SEP = System.getProperty("line.separator"); + PrintStream other; + List stringList = new ArrayList<>(); + + public StringPrintStream(PrintStream ps) { + super(ps); + other = ps; + } + + public void print(String s) { + other.print(s); + stringList.add(s); + } + + public void println(String s) { + other.println(s); + stringList.add(s); + } + + public void println(Object o) { + other.println(o); + stringList.add(o.toString()); + } + + public List getStringList() { + return stringList; + } +} 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 16ae46a..3ca3a5b 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 @@ -1,92 +1,133 @@ package org.drools.ansible.rulebook.integration.api; +import java.io.PrintStream; +import java.util.ArrayList; import java.util.List; +import org.drools.base.common.NetworkNode; +import org.drools.core.reteoo.AlphaNode; +import org.drools.core.reteoo.ObjectSink; +import org.drools.kiesession.rulebase.InternalKnowledgeBase; +import org.drools.modelcompiler.constraints.LambdaConstraint; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; import org.junit.Test; +import org.kie.api.KieBase; import org.kie.api.runtime.rule.Match; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; public class TypeMismatchTest { + // For logging assertion + static PrintStream originalOut = System.out; + static StringPrintStream stringPrintStream = new StringPrintStream(System.out); + + @BeforeClass + public static void beforeClass() { + System.setOut(stringPrintStream); + } + + @AfterClass + public static void afterClass() { + System.setOut(originalOut); + } + + @Before + public void before() { + stringPrintStream.getStringList().clear(); + } + public static final String JSON1 = """ - { - "name": "ruleSet1", - "rules": [ - { - "Rule": { - "name": "r1", - "condition": { - "AllCondition": [ - { - "EqualsExpression": { - "lhs": { - "Event": "meta.headers" - }, - "rhs": { - "String": "Hello Testing World" + { + "name": "ruleSet1", + "rules": [ + { + "Rule": { + "name": "r1", + "condition": { + "AllCondition": [ + { + "EqualsExpression": { + "lhs": { + "Event": "meta.headers" + }, + "rhs": { + "String": "Hello Testing World" + } } } - } - ] - }, - "actions": [ - { - "Action": { - "action": "debug", - "action_args": {} - } - } - ], - "enabled": true - } - }, - { - "Rule": { - "name": "r2", - "condition": { - "AllCondition": [ + ] + }, + "actions": [ { - "NotEqualsExpression": { - "lhs": { - "Event": "meta.headers" - }, - "rhs": { - "String": "Hello Testing World" - } + "Action": { + "action": "debug", + "action_args": {} } } - ] - }, - "actions": [ - { - "Action": { - "action": "debug", - "action_args": {} + ], + "enabled": true + } + }, + { + "Rule": { + "name": "r2", + "condition": { + "AllCondition": [ + { + "NotEqualsExpression": { + "lhs": { + "Event": "meta.headers" + }, + "rhs": { + "String": "Hello Testing World" + } + } + } + ] + }, + "actions": [ + { + "Action": { + "action": "debug", + "action_args": {} + } } - } - ], - "enabled": true + ], + "enabled": true + } } - } - ] - } - """; + ] + } + """; @Test public void mapAndString() { RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(JSON1); - // mera.headers is a map, not a string + // incoming event.mera.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. - // TODO: Add log assertion + assertNumberOfErrorLogs(2); + 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}}")) + .anyMatch(s -> s.contains("Cannot compare values of different types: dict and str." + + " RuleSet: ruleSet1." + + " RuleName: r2." + + " Condition: {lhs={Event=meta.headers}, rhs={String=Hello Testing World}}")); assertEquals(0, matchedRules.size()); // One more time matchedRules = rulesExecutor.processEvents("{ \"meta\": {\"headers\": {\"Content-Length\": \"25\"}} } }").join(); // not firing. Don't log errors again. + assertNumberOfErrorLogs(2); assertEquals(0, matchedRules.size()); rulesExecutor.dispose(); @@ -95,6 +136,7 @@ public void mapAndString() { public static final String JSON2 = """ { + "name": "ruleSet1", "rules": [ { "Rule": { @@ -132,115 +174,152 @@ public void mapAndString() { public void stringAndInteger() { RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(JSON2); - // mera.headers is a map, not a string + // incoming event.i is a string, not a integer List matchedRules = rulesExecutor.processEvents("{ \"i\": \"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}}")); assertEquals(0, matchedRules.size()); rulesExecutor.dispose(); } - @Test public void typeMismatchWithNodeSharing() { String json = """ - { - "rules": [ { - "Rule": { - "name": "r1", - "condition": { - "AllCondition": [ - { - "AndExpression": { - "lhs": { - "EqualsExpression": { + "rules": [ + { + "Rule": { + "name": "r1", + "condition": { + "AllCondition": [ + { + "AndExpression": { "lhs": { - "Event": "i" + "EqualsExpression": { + "lhs": { + "Event": "i" + }, + "rhs": { + "Integer": 1 + } + } }, "rhs": { - "Integer": 1 - } - } - }, - "rhs": { - "EqualsExpression": { - "lhs": { - "Event": "j" - }, - "rhs": { - "Integer": 1 + "EqualsExpression": { + "lhs": { + "Event": "j" + }, + "rhs": { + "Integer": 1 + } + } } } } + ] + }, + "actions": [ + { + "Action": { + "action": "debug", + "action_args": {} + } } - } - ] - }, - "actions": [ - { - "Action": { - "action": "debug", - "action_args": {} - } + ], + "enabled": true } - ], - "enabled": true - } - }, - { - "Rule": { - "name": "r2", - "condition": { - "AllCondition": [ - { - "AndExpression": { - "lhs": { - "EqualsExpression": { - "lhs": { - "Event": "i" - }, - "rhs": { - "Integer": 1 - } - } - }, - "rhs": { - "EqualsExpression": { + }, + { + "Rule": { + "name": "r2", + "condition": { + "AllCondition": [ + { + "AndExpression": { "lhs": { - "Event": "j" + "EqualsExpression": { + "lhs": { + "Event": "i" + }, + "rhs": { + "Integer": 1 + } + } }, "rhs": { - "Integer": 2 + "EqualsExpression": { + "lhs": { + "Event": "j" + }, + "rhs": { + "Integer": 2 + } + } } } } + ] + }, + "actions": [ + { + "Action": { + "action": "debug", + "action_args": {} + } } - } - ] - }, - "actions": [ - { - "Action": { - "action": "debug", - "action_args": {} - } + ], + "enabled": true } - ], - "enabled": true - } + } + ] } - ] - } - """; + """; RulesExecutor rulesExecutor = RulesExecutorFactory.createFromJson(json); - - // TODO: add node sharing assertion + KieBase kieBase = rulesExecutor.asKieSession().getKieBase(); + List alphaNodes = collectAlphaNodes(kieBase); + // assert node sharing + assertThat(alphaNodes.stream() + .map(node -> ((LambdaConstraint) node.getConstraint()).getEvaluator().getConstraint()) + .filter(constraint -> constraint.getExprId().equals("expr:i:EQUAL:1")) + .count()).isEqualTo(1); // i is a string, not an integer List matchedRules = rulesExecutor.processEvents("{ \"i\": \"1\", \"j\": 1 }").join(); + assertNumberOfErrorLogs(1); assertEquals(0, matchedRules.size()); + + rulesExecutor.dispose(); + } + + private List collectAlphaNodes(KieBase kieBase) { + List alphaNodes = new ArrayList<>(); + ((InternalKnowledgeBase) kieBase).getRete().getObjectTypeNodes().forEach(otn -> { + ObjectSink[] sinks = otn.getObjectSinkPropagator().getSinks(); + collectAlphaNodes(sinks, alphaNodes); + }); + return alphaNodes; + } + + private static void collectAlphaNodes(NetworkNode[] sinks, List alphaNodes) { + if (sinks == null) { + return; + } + for (NetworkNode sink : sinks) { + if (sink instanceof AlphaNode alphaNode) { + alphaNodes.add(alphaNode); + } + collectAlphaNodes(sink.getSinks(), alphaNodes); + } + } + + private static void assertNumberOfErrorLogs(int expected) { + assertThat(stringPrintStream.getStringList().stream().filter(s -> s.contains("ERROR")).count()).isEqualTo(expected); } } \ No newline at end of file