This is an automated email from the ASF dual-hosted git repository.
mariofusco pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/incubator-kie-drools.git
The following commit(s) were added to refs/heads/main by this push:
new 0081e03187 [incubator-kie-issues-1946] Rule engine half constraint or
condition is not working as expected (#6345)
0081e03187 is described below
commit 0081e031875120d562db7ef80bda6d556fb6d041
Author: Toshiya Kobayashi <[email protected]>
AuthorDate: Fri May 16 22:17:06 2025 +0900
[incubator-kie-issues-1946] Rule engine half constraint or condition is not
working as expected (#6345)
* Use NullSafeExpressions for halfconstraintOr
Signed-off-by: soniyaabraham <[email protected]>
* Using equals method for halfConstraints
* Using equals method for halfConstraints
* Using equals method for halfConstraints and Updated the test cases
* Using equals method for halfConstraints and Updated the test cases
* Removing unwanted comments
* Updating the method name
* Reverting the formatting
* - complement HalfBinaryExpr
- fix tests because complementing HalfBinaryExpr introduces parentheses for
the combined binary expression. Logical result shouldn't change
* - copilot suggestions
---------
Signed-off-by: soniyaabraham <[email protected]>
Co-authored-by: soniyaabraham <[email protected]>
---
.../generator/drlxparse/ConstraintParser.java | 83 ++++++++++++++++---
.../generator/drlxparse/ConstraintParserTest.java | 92 ++++++++++++++++++++--
2 files changed, 154 insertions(+), 21 deletions(-)
diff --git
a/drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParser.java
b/drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParser.java
index 5bb7999cfd..1e0b9cf669 100644
---
a/drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParser.java
+++
b/drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParser.java
@@ -62,8 +62,6 @@ import
org.drools.model.codegen.execmodel.generator.TypedExpression;
import
org.drools.model.codegen.execmodel.generator.expressiontyper.ExpressionTyper;
import
org.drools.model.codegen.execmodel.generator.expressiontyper.ExpressionTyperContext;
import
org.drools.model.codegen.execmodel.generator.expressiontyper.TypedExpressionResult;
-import org.drools.mvel.parser.ast.expr.BigDecimalLiteralExpr;
-import org.drools.mvel.parser.ast.expr.BigIntegerLiteralExpr;
import org.drools.mvel.parser.ast.expr.DrlNameExpr;
import org.drools.mvel.parser.ast.expr.DrlxExpression;
import org.drools.mvel.parser.ast.expr.FullyQualifiedInlineCastExpr;
@@ -412,7 +410,7 @@ public class ConstraintParser {
return combo;
}
- private Expression combineExpressions(List<Expression>
leftPrefixExpresssions, List<Expression> rightPrefixExpresssions, Expression
combo) {
+ private Expression combineExpressions(List<Expression>
leftPrefixExpressions, List<Expression> rightPrefixExpressions, Expression
combo) {
Expression inner = combo;
if (combo.isEnclosedExpr()) {
EnclosedExpr enclosedExpr = combo.asEnclosedExpr();
@@ -427,13 +425,13 @@ public class ConstraintParser {
}
Expression left = binaryExpr.getLeft();
- for (Expression prefixExpression : leftPrefixExpresssions) {
+ for (Expression prefixExpression : leftPrefixExpressions) {
left = new BinaryExpr(prefixExpression, left,
BinaryExpr.Operator.AND);
}
binaryExpr.setLeft(left);
Expression right = binaryExpr.getRight();
- for (Expression prefixExpression : rightPrefixExpresssions) {
+ for (Expression prefixExpression : rightPrefixExpressions) {
right = new BinaryExpr(prefixExpression, right,
BinaryExpr.Operator.AND);
}
binaryExpr.setRight(right);
@@ -630,11 +628,20 @@ public class ConstraintParser {
boolean isOrBinary = operator == BinaryExpr.Operator.OR;
if ( isLogicalOperator( operator ) && isCombinable( binaryExpr ) ) {
- DrlxParseResult leftResult = compileToJavaRecursive(patternType,
bindingId, constraint, binaryExpr.getLeft(), hasBind, isPositional );
- Expression rightExpr = binaryExpr.getRight() instanceof
HalfPointFreeExpr ?
- completeHalfExpr( (( PointFreeExpr )
binaryExpr.getLeft()).getLeft(), ( HalfPointFreeExpr ) binaryExpr.getRight(),
context) :
- binaryExpr.getRight();
- DrlxParseResult rightResult = compileToJavaRecursive(patternType,
bindingId, constraint, rightExpr, hasBind, isPositional );
+ Expression left = binaryExpr.getLeft();
+ if (left instanceof HalfBinaryExpr leftHalfBinaryExpr) {
+ left = completeHalfBinaryExpr(leftHalfBinaryExpr, context);
+ }
+ DrlxParseResult leftResult = compileToJavaRecursive(patternType,
bindingId, constraint, left, hasBind, isPositional );
+
+ Expression right = binaryExpr.getRight();
+ if (right instanceof HalfPointFreeExpr rightHalfPointFreeExpr) {
+ right = completeHalfPointFreeExpr(((PointFreeExpr)
binaryExpr.getLeft()).getLeft(), rightHalfPointFreeExpr, context);
+ } else if (right instanceof HalfBinaryExpr rightHalfBinaryExpr) {
+ right = completeHalfBinaryExpr(rightHalfBinaryExpr, context);
+ }
+
+ DrlxParseResult rightResult = compileToJavaRecursive(patternType,
bindingId, constraint, right, hasBind, isPositional );
return isMultipleResult(leftResult, operator, rightResult) ?
createMultipleDrlxParseSuccess( operator, (
DrlxParseSuccess ) leftResult, ( DrlxParseSuccess ) rightResult ) :
leftResult.combineWith( rightResult, operator );
@@ -824,14 +831,64 @@ public class ConstraintParser {
}
private boolean isCombinable( BinaryExpr binaryExpr ) {
- return !(binaryExpr.getRight() instanceof HalfBinaryExpr) && (
!(binaryExpr.getRight() instanceof HalfPointFreeExpr) || binaryExpr.getLeft()
instanceof PointFreeExpr );
+ if (binaryExpr.getLeft() instanceof HalfBinaryExpr ||
binaryExpr.getRight() instanceof HalfBinaryExpr) {
+ // if the leftmost operand exists, we can complete the
HalfBinaryExpr
+ return findLeftmostOperand(Optional.of(binaryExpr)).isPresent();
+ }
+ // if left is PointFreeExpr, we can complete HalfPointFreeExpr
+ return !(binaryExpr.getRight() instanceof HalfPointFreeExpr) ||
binaryExpr.getLeft() instanceof PointFreeExpr;
}
- private static PointFreeExpr completeHalfExpr(Expression left,
HalfPointFreeExpr halfRight, RuleContext context) {
+ private static PointFreeExpr completeHalfPointFreeExpr(Expression left,
HalfPointFreeExpr halfRight, RuleContext context) {
ParserLogUtils.logHalfConstraintWarn(halfRight, Optional.of(context));
return new PointFreeExpr( halfRight.getTokenRange().orElse( null ),
left, halfRight.getRight(), halfRight.getOperator(), halfRight.isNegated(),
halfRight.getArg1(), halfRight.getArg2(), halfRight.getArg3(),
halfRight.getArg4() );
}
+ private static BinaryExpr completeHalfBinaryExpr(HalfBinaryExpr halfRight,
RuleContext context) {
+ ParserLogUtils.logHalfConstraintWarn(halfRight, Optional.of(context));
+ Optional<Expression> leftOperandOpt =
findLeftmostOperand(halfRight.getParentNode());
+ if (leftOperandOpt.isEmpty()) {
+ throw new IllegalStateException("isCombinable should have ensured
that leftmostOperand exists: halfRight = " + PrintUtil.printNode(halfRight));
+ }
+ return new BinaryExpr(leftOperandOpt.get(), halfRight.getRight(),
halfRight.getOperator().toBinaryExprOperator());
+ }
+
+ private static Optional<Expression> findLeftmostOperand(Optional<Node>
parentOpt) {
+ // find the leftmost operand under the tree
+ if (parentOpt.isEmpty()) {
+ return Optional.empty();
+ }
+ Node parent = parentOpt.get();
+ if (parent instanceof EnclosedExpr enclosedExpr) {
+ parent = stripEnclosedExpr(enclosedExpr);
+ }
+ if (parent instanceof BinaryExpr binaryExpr) {
+ Optional<Expression> leftmostOpt =
findLeftmostOperandInSubTree(binaryExpr);
+ if (leftmostOpt.isPresent()) {
+ return leftmostOpt;
+ } else {
+ // go up one level
+ return findLeftmostOperand(parent.getParentNode());
+ }
+ } else {
+ // Stop if it's not a BinaryExpr
+ return Optional.empty();
+ }
+ }
+
+ private static Optional<Expression>
findLeftmostOperandInSubTree(BinaryExpr binaryExpr) {
+ Expression left = binaryExpr.getLeft();
+ left = stripEnclosedExpr(left);
+ if (left instanceof HalfBinaryExpr) {
+ // HalfBinary are chained. Go up the tree
+ return Optional.empty();
+ } else if (left instanceof BinaryExpr leftBinaryExpr) {
+ return findLeftmostOperandInSubTree(leftBinaryExpr);
+ } else {
+ return Optional.of(left);
+ }
+ }
+
private static String getExpressionSymbol(Expression expr) {
if (expr instanceof MethodCallExpr) {
Optional<Expression> scopeExpression = (( MethodCallExpr )
expr).getScope();
@@ -1046,4 +1103,4 @@ public class ConstraintParser {
public static boolean isArithmeticOperator(BinaryExpr.Operator operator) {
return ARITHMETIC_OPERATORS.contains(operator);
}
-}
+}
\ No newline at end of file
diff --git
a/drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParserTest.java
b/drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParserTest.java
index 6dd9391eda..7794cc7cd6 100644
---
a/drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParserTest.java
+++
b/drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParserTest.java
@@ -65,10 +65,87 @@ public class ConstraintParserTest {
public void testNullSafeExpressionsWithOr() {
SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "name == \"John\" || == address!.city");
- assertThat(result.getNullSafeExpressions().size()).isEqualTo(0); //
not using NullSafeExpressions for complex OR cases
-
// null check is done after the first constraint
- assertThat(result.getExpr().toString()).isEqualTo("_this.getName() ==
\"John\" || _this.getAddress() != null && _this.getName() ==
_this.getAddress().getCity()");
+
assertThat(result.getExpr().toString()).isEqualTo("(org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"John\")" +
+ " ||
_this.getAddress() != null" +
+ " &&
org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
_this.getAddress().getCity()))");
+ }
+
+ @Test
+ public void testOrWithMultipleFullConstraints() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "name == \"John\" || name == \"Jacob\" ||
name == \"Peter\"");
+
+
assertThat(result.getExpr().toString()).isEqualTo("((org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"John\")" +
+ " ||
org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"Jacob\"))" +
+ " ||
org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"Peter\"))");
+ }
+
+ @Test
+ public void testOrWithHalfConstraint() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "name == \"John\" || == \"Jacob\"");
+
+
assertThat(result.getExpr().toString()).isEqualTo("(org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"John\")" +
+ " ||
org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"Jacob\"))");
+ }
+
+ @Test
+ public void testAndWithHalfConstraint() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "name != \"John\" && != \"Jacob\"");
+
+
assertThat(result.getExpr().toString()).isEqualTo("(!org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"John\")" +
+ " &&
!org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"Jacob\"))");
+ }
+
+ @Test
+ public void testOrWithMultipleHalfConstraints() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "name == \"John\" || == \"Jacob\" || ==
\"Peter\" || == \"Ann\"");
+
+
assertThat(result.getExpr().toString()).isEqualTo("(((org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"John\")" +
+ " ||
org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"Jacob\"))" +
+ " ||
org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"Peter\"))" +
+ " ||
org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"Ann\"))");
+ }
+
+ @Test
+ public void testOrWithHalfAndFullConstraints() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "name == \"John\" || == \"Jacob\" || name
== \"Peter\" || == \"Ann\"");
+
+
assertThat(result.getExpr().toString()).isEqualTo("(((org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"John\")" +
+ " ||
org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"Jacob\"))" +
+ " ||
org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"Peter\"))" +
+ " ||
org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"Ann\"))");
+ }
+
+ @Test
+ public void testOrWithNumberConstraints() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "age <= 19 || age >= 60");
+
+
assertThat(result.getExpr().toString()).isEqualTo("(org.drools.modelcompiler.util.EvaluationUtil.lessOrEqualNumbers(_this.getAge(),
19) ||
org.drools.modelcompiler.util.EvaluationUtil.greaterOrEqualNumbers(_this.getAge(),
60))");
+ }
+
+ @Test
+ public void testHalfConstraintOrWithNumberConstraints() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "age <= 19 || >= 60");
+
+
assertThat(result.getExpr().toString()).isEqualTo("(org.drools.modelcompiler.util.EvaluationUtil.lessOrEqualNumbers(_this.getAge(),
19)" +
+ " ||
org.drools.modelcompiler.util.EvaluationUtil.greaterOrEqualNumbers(_this.getAge(),
60))");
+ }
+
+ @Test
+ public void testWithNumberConstraint() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "age == 60");
+
+
assertThat(result.getExpr().toString()).isEqualTo("org.drools.modelcompiler.util.EvaluationUtil.areNumbersNullSafeEquals(_this.getAge(),
60)");
+ }
+
+ @Test
+ public void testOrWithComplexConstraints() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "(name == \"John\" || != \"Peter\") &&
(age <= 19 || >= 60)");
+
+
assertThat(result.getExpr().toString()).isEqualTo("((org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"John\")" +
+ " ||
!org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getName(),
\"Peter\"))" +
+ " &&
(org.drools.modelcompiler.util.EvaluationUtil.lessOrEqualNumbers(_this.getAge(),
19)" +
+ " ||
org.drools.modelcompiler.util.EvaluationUtil.greaterOrEqualNumbers(_this.getAge(),
60)))");
}
@Test
@@ -123,12 +200,11 @@ public class ConstraintParserTest {
public void testImplicitCastExpressionWithOr() {
SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Object.class, "$o", "\"Mark\" == this.toString() || ==
this#Person.address.city");
- Optional<Expression> implicitCastExpression =
result.getImplicitCastExpression();
- assertThat(implicitCastExpression.isPresent()).isTrue();
- assertThat(implicitCastExpression.get().toString()).isEqualTo("_this
instanceof Person"); // will be added as the first predicate
-
// instanceof check is done after the first constraint
- assertThat(result.getExpr().toString()).isEqualTo("\"Mark\" ==
_this.toString() || _this instanceof " + Person.class.getCanonicalName() + " &&
\"Mark\" == ((" + Person.class.getCanonicalName() + ")
_this).getAddress().getCity()");
+
assertThat(result.getExpr().toString()).isEqualTo("(org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(\"Mark\",
_this.toString())" +
+ " || _this
instanceof Person" +
+ " && _this
instanceof org.drools.model.codegen.execmodel.domain.Person" +
+ " &&
org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(\"Mark\",
((org.drools.model.codegen.execmodel.domain.Person)
_this).getAddress().getCity()))");
}
@Test
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
