[8.40.x][kie-issues#941] Fix executable model generation for binding enclosed in parentheses

drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/drlxparse/ArithmeticCoercedExpression.java

@@ -51,6 +51,10 @@ public ArithmeticCoercedExpression(TypedExpression left, TypedExpression right,
         this.operator = operator;
     }
 
+    /*
+     * This coercion only deals with String vs Numeric types.
+     * BigDecimal arithmetic operation is handled by ExpressionTyper.convertArithmeticBinaryToMethodCall()
+     */
     public ArithmeticCoercedExpressionResult coerce() {
 
         if (!requiresCoercion()) {

drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParser.java

@@ -94,6 +94,9 @@
 import static org.drools.model.codegen.execmodel.generator.ConstraintUtil.GREATER_THAN_PREFIX;
 import static org.drools.model.codegen.execmodel.generator.ConstraintUtil.LESS_OR_EQUAL_PREFIX;
 import static org.drools.model.codegen.execmodel.generator.ConstraintUtil.LESS_THAN_PREFIX;
+import static org.drools.model.codegen.execmodel.generator.expressiontyper.ExpressionTyper.convertArithmeticBinaryToMethodCall;
+import static org.drools.model.codegen.execmodel.generator.expressiontyper.ExpressionTyper.getBinaryTypeAfterConversion;
+import static org.drools.model.codegen.execmodel.generator.expressiontyper.ExpressionTyper.shouldConvertArithmeticBinaryToMethodCall;
 import static org.drools.util.StringUtils.lcFirstForBean;
 import static org.drools.model.codegen.execmodel.generator.DrlxParseUtil.THIS_PLACEHOLDER;
 import static org.drools.model.codegen.execmodel.generator.DrlxParseUtil.createConstraintCompiler;
@@ -196,11 +199,28 @@ private void logWarnIfNoReactOnCausedByVariableFromDifferentPattern(DrlxParseRes
     }
 
     private void addDeclaration(DrlxExpression drlx, SingleDrlxParseSuccess singleResult, String bindId) {
-        DeclarationSpec decl = context.addDeclaration( bindId, singleResult.getLeftExprTypeBeforeCoercion() );
+        DeclarationSpec decl = context.addDeclaration(bindId, getDeclarationType(drlx, singleResult));
         if (drlx.getExpr() instanceof NameExpr) {
-            decl.setBoundVariable( PrintUtil.printNode(drlx.getExpr()) );
+            decl.setBoundVariable(PrintUtil.printNode(drlx.getExpr()));
+        } else if (drlx.getExpr() instanceof EnclosedExpr && drlx.getBind() != null) {
+            ExpressionTyperContext expressionTyperContext = new ExpressionTyperContext();
+            ExpressionTyper expressionTyper = new ExpressionTyper(context, singleResult.getPatternType(), bindId, false, expressionTyperContext);
+            TypedExpressionResult typedExpressionResult = expressionTyper.toTypedExpression(drlx.getExpr());
+            singleResult.setBoundExpr(typedExpressionResult.typedExpressionOrException());
         } else if (drlx.getExpr() instanceof BinaryExpr) {
-            decl.setBoundVariable(PrintUtil.printNode(drlx.getExpr().asBinaryExpr().getLeft()));
+            Expression leftMostExpression = getLeftMostExpression(drlx.getExpr().asBinaryExpr());
+            decl.setBoundVariable(PrintUtil.printNode(leftMostExpression));
+            if (singleResult.getExpr() instanceof MethodCallExpr) {
+                // BinaryExpr was converted to MethodCallExpr. Create a TypedExpression for the leftmost expression of the BinaryExpr
+                ExpressionTyperContext expressionTyperContext = new ExpressionTyperContext();
+                ExpressionTyper expressionTyper = new ExpressionTyper(context, singleResult.getPatternType(), bindId, false, expressionTyperContext);
+                TypedExpressionResult leftTypedExpressionResult = expressionTyper.toTypedExpression(leftMostExpression);
+                Optional<TypedExpression> optLeft = leftTypedExpressionResult.getTypedExpression();
+                if (optLeft.isEmpty()) {
+                    throw new IllegalStateException("Cannot create TypedExpression for " + drlx.getExpr().asBinaryExpr().getLeft());
+                }
+                singleResult.setBoundExpr(optLeft.get());
+            }
         }
         decl.setBelongingPatternDescr(context.getCurrentPatternDescr());
         singleResult.setExprBinding( bindId );
@@ -210,6 +230,24 @@ private void addDeclaration(DrlxExpression drlx, SingleDrlxParseSuccess singleRe
         }
     }
 
+    private static Class<?> getDeclarationType(DrlxExpression drlx, SingleDrlxParseSuccess singleResult) {
+        if (drlx.getBind() != null && drlx.getExpr() instanceof EnclosedExpr) {
+            // in case of enclosed, bind type should be the calculation result type
+            // If drlx.getBind() == null, a bind variable is inside the enclosed expression, leave it to the default behavior
+            return (Class<?>)singleResult.getExprType();
+        } else {
+            return singleResult.getLeftExprTypeBeforeCoercion();
+        }
+    }
+
+    private Expression getLeftMostExpression(BinaryExpr binaryExpr) {
+        Expression left = binaryExpr.getLeft();
+        if (left instanceof BinaryExpr) {
+            return getLeftMostExpression((BinaryExpr) left);
+        }
+        return left;
+    }
+
     /*
         This is the entry point for Constraint Transformation from a parsed MVEL constraint
         to a Java Expression
@@ -656,17 +694,16 @@ private DrlxParseResult parseBinaryExpr(BinaryExpr binaryExpr, Class<?> patternT
 
         Expression combo;
 
-        boolean arithmeticExpr = ARITHMETIC_OPERATORS.contains(operator);
         boolean isBetaConstraint = right.getExpression() != null && hasDeclarationFromOtherPattern( expressionTyperContext );
         boolean requiresSplit = operator == BinaryExpr.Operator.AND && binaryExpr.getRight() instanceof HalfBinaryExpr && !isBetaConstraint;
 
+        Type exprType = isBooleanOperator( operator ) ? boolean.class : left.getType();
+
         if (equalityExpr) {
             combo = getEqualityExpression( left, right, operator ).expression;
-        } else if (arithmeticExpr && (left.isBigDecimal())) {
-            ConstraintCompiler constraintCompiler = createConstraintCompiler(this.context, of(patternType));
-            CompiledExpressionResult compiledExpressionResult = constraintCompiler.compileExpression(binaryExpr);
-
-            combo = compiledExpressionResult.getExpression();
+        } else if (shouldConvertArithmeticBinaryToMethodCall(operator, left.getType(), right.getType())) {
+            combo = convertArithmeticBinaryToMethodCall(binaryExpr, of(patternType), this.context);
+            exprType = getBinaryTypeAfterConversion(left.getType(), right.getType());
         } else {
             if (left.getExpression() == null || right.getExpression() == null) {
                 return new DrlxParseFail(new ParseExpressionErrorResult(drlxExpr));
@@ -694,7 +731,7 @@ private DrlxParseResult parseBinaryExpr(BinaryExpr binaryExpr, Class<?> patternT
             constraintType = Index.ConstraintType.FORALL_SELF_JOIN;
         }
 
-        return new SingleDrlxParseSuccess(patternType, bindingId, combo, isBooleanOperator( operator ) ? boolean.class : left.getType())
+        return new SingleDrlxParseSuccess(patternType, bindingId, combo, exprType)
                 .setDecodeConstraintType( constraintType )
                 .setUsedDeclarations( expressionTyperContext.getUsedDeclarations() )
                 .setUsedDeclarationsOnLeft( usedDeclarationsOnLeft )
@@ -1007,4 +1044,8 @@ private Optional<DrlxParseFail> convertBigDecimalArithmetic(MethodCallExpr metho
         }
         return Optional.empty();
     }
+
+    public static boolean isArithmeticOperator(BinaryExpr.Operator operator) {
+        return ARITHMETIC_OPERATORS.contains(operator);
+    }
 }

drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/drlxparse/SingleDrlxParseSuccess.java

@@ -355,8 +355,26 @@ public boolean isPredicate() {
         return this.isPredicate;
     }
 
+    /*
+     * This method finds out, if the parse result is a predicate enclosed in parentheses, bound to a variable.
+     * Example: Person($booleanVariable: (name != null))
+     * This shouldn't apply to any other form of predicate. So e.g.
+     * Person($booleanVariable: (name != null) == "someName") should be properly generated as a constraint.
+     * After discussions, to align the executable model behaviour with the old non-executable model,
+     * such predicate is not generated as a rule constraint, and just bound to a variable. This behaviour needs more
+     * discussions to revisit this behaviour.
+     */
+    private boolean isEnclosedPredicateBoundToVariable() {
+        final TypedExpression boundExpr = getBoundExpr();
+        return boundExpr != null
+                && boundExpr.getExpression() instanceof EnclosedExpr
+                && getExprBinding() != null
+                && !getLeft().getExpression().equals(boundExpr.getExpression())
+                && !getRight().getExpression().equals(boundExpr.getExpression());
+    }
+
     public SingleDrlxParseSuccess setIsPredicate(boolean predicate) {
-        this.isPredicate = predicate;
+        this.isPredicate = predicate && !isEnclosedPredicateBoundToVariable();
         return this;
     }
 

drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/expression/AbstractExpressionBuilder.java

@@ -113,8 +113,17 @@ protected Expression getBindingExpression(SingleDrlxParseSuccess drlxParseResult
         } else {
             final TypedExpression boundExpr = drlxParseResult.getBoundExpr();
             // Can we unify it? Sometimes expression is in the left sometimes in expression
-            final Expression e = boundExpr != null ? findLeftmostExpression(boundExpr.getExpression()) : drlxParseResult.getExpr();
-            return buildConstraintExpression(drlxParseResult, drlxParseResult.getUsedDeclarationsOnLeft(), e);
+            final Expression expression;
+            if (boundExpr != null) {
+                if (boundExpr.getExpression() instanceof EnclosedExpr) {
+                    expression = boundExpr.getExpression();
+                } else {
+                    expression = findLeftmostExpression(boundExpr.getExpression());
+                }
+            } else {
+                expression = drlxParseResult.getExpr();
+            }
+            return buildConstraintExpression(drlxParseResult, drlxParseResult.getUsedDeclarationsOnLeft(), expression);
         }
     }
 

drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/expressiontyper/ExpressionTyper.java

@@ -22,6 +22,7 @@
 import java.lang.reflect.Modifier;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.TypeVariable;
+import java.math.BigDecimal;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -89,6 +90,8 @@
 import org.drools.mvel.parser.ast.expr.OOPathExpr;
 import org.drools.mvel.parser.ast.expr.PointFreeExpr;
 import org.drools.mvel.parser.printer.PrintUtil;
+import org.drools.mvelcompiler.CompiledExpressionResult;
+import org.drools.mvelcompiler.ConstraintCompiler;
 import org.drools.mvelcompiler.util.BigDecimalArgumentCoercion;
 import org.drools.util.MethodUtils;
 import org.drools.util.TypeResolver;
@@ -99,6 +102,7 @@
 import static java.util.Optional.empty;
 import static java.util.Optional.of;
 import static org.drools.model.codegen.execmodel.generator.DrlxParseUtil.THIS_PLACEHOLDER;
+import static org.drools.model.codegen.execmodel.generator.DrlxParseUtil.createConstraintCompiler;
 import static org.drools.model.codegen.execmodel.generator.DrlxParseUtil.findRootNodeViaParent;
 import static org.drools.model.codegen.execmodel.generator.DrlxParseUtil.getClassFromContext;
 import static org.drools.model.codegen.execmodel.generator.DrlxParseUtil.getClassFromType;
@@ -113,6 +117,7 @@
 import static org.drools.model.codegen.execmodel.generator.DrlxParseUtil.toStringLiteral;
 import static org.drools.model.codegen.execmodel.generator.DrlxParseUtil.transformDrlNameExprToNameExpr;
 import static org.drools.model.codegen.execmodel.generator.DrlxParseUtil.trasformHalfBinaryToBinary;
+import static org.drools.model.codegen.execmodel.generator.drlxparse.ConstraintParser.isArithmeticOperator;
 import static org.drools.model.codegen.execmodel.generator.expressiontyper.FlattenScope.flattenScope;
 import static org.drools.model.codegen.execmodel.generator.expressiontyper.FlattenScope.transformFullyQualifiedInlineCastExpr;
 import static org.drools.mvel.parser.MvelParser.parseType;
@@ -229,7 +234,14 @@ private Optional<TypedExpression> toTypedExpressionRec(Expression drlxExpr) {
             right = coerced.getCoercedRight();
 
             final BinaryExpr combo = new BinaryExpr(left.getExpression(), right.getExpression(), operator);
-            return of(new TypedExpression(combo, left.getType()));
+
+            if (shouldConvertArithmeticBinaryToMethodCall(operator, left.getType(), right.getType())) {
+                Expression expression = convertArithmeticBinaryToMethodCall(combo, of(typeCursor), ruleContext);
+                java.lang.reflect.Type binaryType = getBinaryTypeAfterConversion(left.getType(), right.getType());
+                return of(new TypedExpression(expression, binaryType));
+            } else {
+                return of(new TypedExpression(combo, left.getType()));
+            }
         }
 
         if (drlxExpr instanceof HalfBinaryExpr) {
@@ -800,7 +812,38 @@ private TypedExpressionCursor binaryExpr(BinaryExpr binaryExpr) {
         TypedExpression rightTypedExpression = right.getTypedExpression()
                                                     .orElseThrow(() -> new NoSuchElementException("TypedExpressionResult doesn't contain TypedExpression!"));
         binaryExpr.setRight(rightTypedExpression.getExpression());
-        return new TypedExpressionCursor(binaryExpr, getBinaryType(leftTypedExpression, rightTypedExpression, binaryExpr.getOperator()));
+        if (shouldConvertArithmeticBinaryToMethodCall(binaryExpr.getOperator(), leftTypedExpression.getType(), rightTypedExpression.getType())) {
+            Expression compiledExpression = convertArithmeticBinaryToMethodCall(binaryExpr, leftTypedExpression.getOriginalPatternType(), ruleContext);
+            java.lang.reflect.Type binaryType = getBinaryTypeAfterConversion(leftTypedExpression.getType(), rightTypedExpression.getType());
+            return new TypedExpressionCursor(compiledExpression, binaryType);
+        } else {
+            java.lang.reflect.Type binaryType = getBinaryType(leftTypedExpression, rightTypedExpression, binaryExpr.getOperator());
+            return new TypedExpressionCursor(binaryExpr, binaryType);
+        }
+    }
+
+    /*
+     * Converts arithmetic binary expression (including coercion) to method call using ConstraintCompiler.
+     * This method can be generic, so we may centralize the calls in drools-model
+     */
+    public static Expression convertArithmeticBinaryToMethodCall(BinaryExpr binaryExpr,  Optional<Class<?>> originalPatternType, RuleContext ruleContext) {
+        ConstraintCompiler constraintCompiler = createConstraintCompiler(ruleContext, originalPatternType);
+        CompiledExpressionResult compiledExpressionResult = constraintCompiler.compileExpression(printNode(binaryExpr));
+        return compiledExpressionResult.getExpression();
+    }
+
+    /*
+     * BigDecimal arithmetic operations should be converted to method calls. We may also apply this to BigInteger.
+     */
+    public static boolean shouldConvertArithmeticBinaryToMethodCall(BinaryExpr.Operator operator, java.lang.reflect.Type leftType, java.lang.reflect.Type rightType) {
+        return isArithmeticOperator(operator) && (leftType.equals(BigDecimal.class) || rightType.equals(BigDecimal.class));
+    }
+
+    /*
+     * After arithmetic to method call conversion, BigDecimal should take precedence regardless of left or right. We may also apply this to BigInteger.
+     */
+    public static java.lang.reflect.Type getBinaryTypeAfterConversion(java.lang.reflect.Type leftType, java.lang.reflect.Type rightType) {
+        return (leftType.equals(BigDecimal.class) || rightType.equals(BigDecimal.class)) ? BigDecimal.class : leftType;
     }
 
     private java.lang.reflect.Type getBinaryType(TypedExpression leftTypedExpression, TypedExpression rightTypedExpression, Operator operator) {
@@ -907,6 +950,9 @@ private void promoteBigDecimalParameters(MethodCallExpr methodCallExpr, Class[]
                 Expression argumentExpression = methodCallExpr.getArgument(i);
 
                 if (argumentType != actualArgumentType) {
+                    // unbind the original argumentExpression first, otherwise setArgument() will remove the argumentExpression from coercedExpression.childrenNodes
+                    // It will result in failing to find DrlNameExpr in AST at DrlsParseUtil.transformDrlNameExprToNameExpr()
+                    methodCallExpr.replace(argumentExpression, new NameExpr("placeholder"));
                     Expression coercedExpression = new BigDecimalArgumentCoercion().coercedArgument(argumentType, actualArgumentType, argumentExpression);
                     methodCallExpr.setArgument(i, coercedExpression);
                 }

drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/BindingTest.java

@@ -471,4 +471,69 @@ public void test3BindingOn3ConditionsWithOrAnd() {
         ksession.fireAllRules();
         assertThat(result).isEmpty();
     }
+
+    @Test
+    public void testConstraintExpression() {
+        String str = "package constraintexpression\n" +
+                "\n" +
+                "import " + Person.class.getCanonicalName() + "\n" +
+                "import java.util.List; \n" +
+                "global List<Boolean> booleanListGlobal; \n" +
+                "rule \"r1\"\n" +
+                "when \n" +
+                "    $p : Person($booleanVariable: (name != null))\n" +
+                "then \n" +
+                "    System.out.println($booleanVariable); \n" +
+                "    System.out.println($p); \n" +
+                "    booleanListGlobal.add($booleanVariable); \n " +
+                "end \n";
+
+        KieSession ksession = getKieSession(str);
+        try {
+            final List<Boolean> booleanListGlobal = new ArrayList<>();
+            ksession.setGlobal("booleanListGlobal", booleanListGlobal);
+            Person person = new Person("someName");
+            ksession.insert(person);
+            int rulesFired = ksession.fireAllRules();
+            assertThat(rulesFired).isEqualTo(1);
+            assertThat(booleanListGlobal).isNotEmpty().containsExactly(Boolean.TRUE);
+        } finally {
+            ksession.dispose();
+        }
+    }
+
+    /**
+     * This test checks that a rule is not fired, when a binding is
+     * enclosed in parentheses. This is intentional behaviour, agreed in discussions,
+     * which may be revised in the future.
+     */
+    @Test
+    public void testIgnoreConstraintInParentheses() {
+        String str = "package constraintexpression\n" +
+                "\n" +
+                "import " + Person.class.getCanonicalName() + "\n" +
+                "import java.util.List; \n" +
+                "global List<Boolean> booleanListGlobal; \n" +
+                "rule \"r1\"\n" +
+                "when \n" +
+                "    $p : Person($booleanVariable: (name == null))\n" +
+                "then \n" +
+                "    System.out.println($booleanVariable); \n" +
+                "    System.out.println($p); \n" +
+                "    booleanListGlobal.add($booleanVariable); \n " +
+                "end \n";
+
+        KieSession ksession = getKieSession(str);
+        try {
+            final List<Boolean> booleanListGlobal = new ArrayList<>();
+            ksession.setGlobal("booleanListGlobal", booleanListGlobal);
+            Person person = new Person("someName");
+            ksession.insert(person);
+            int rulesFired = ksession.fireAllRules();
+            assertThat(rulesFired).isEqualTo(1);
+            assertThat(booleanListGlobal).isNotEmpty().containsExactly(Boolean.FALSE);
+        } finally {
+            ksession.dispose();
+        }
+    }
 }