Skip to content

Commit

Permalink
[8.40.x][kie-issues#941] Fix executable model generation for binding …
Browse files Browse the repository at this point in the history 
…enclosed in parentheses (#5753)
  • Loading branch information
@baldimir
baldimir authored Mar 4, 2024
1 parent 8d8a7f0 commit 8de49a3
Show file tree
Hide file tree
Showing 8 changed files with 333 additions and 15 deletions.
4 changes: 4 additions & 0 deletions ...a/org/drools/model/codegen/execmodel/generator/drlxparse/ArithmeticCoercedExpression.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down
61 changes: 51 additions & 10 deletions ...rc/main/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 );
Expand All @@ -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
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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 )
Expand Down Expand Up @@ -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);
}
}
20 changes: 19 additions & 1 deletion ...n/java/org/drools/model/codegen/execmodel/generator/drlxparse/SingleDrlxParseSuccess.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
13 changes: 11 additions & 2 deletions ...va/org/drools/model/codegen/execmodel/generator/expression/AbstractExpressionBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
50 changes: 48 additions & 2 deletions ...in/java/org/drools/model/codegen/execmodel/generator/expressiontyper/ExpressionTyper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
Expand Down
65 changes: 65 additions & 0 deletions ...el/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/BindingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}
Loading

0 comments on commit 8de49a3

Please sign in to comment.