Copilot commented on code in PR #6343:
URL:
https://github.com/apache/incubator-kie-drools/pull/6343#discussion_r2086739609
##########
drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParserTest.java:
##########
@@ -61,14 +61,78 @@ public void testNullSafeExpressions() {
assertThat(result.getExpr().toString()).isEqualTo("org.drools.modelcompiler.util.EvaluationUtil.areNullSafeEquals(_this.getAddress().getCity(),
\"London\")");
}
+ @Test
+ public void testNullSafeExpressionsWithMultipleConstraints() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "name == \"John\" || name == \"Jacob\" ||
name == \"Peter\"");
+
+ assertThat(result.getNullSafeExpressions().size()).isEqualTo(0); //
not using NullSafeExpressions for complex OR cases
+
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 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 testOrWithHalfConstraint() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "name == \"John\" || == \"Jacob\"");
+
+ assertThat(result.getNullSafeExpressions().size()).isEqualTo(0); //
not using NullSafeExpressions for complex OR cases
+
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.getNullSafeExpressions().size()).isEqualTo(0); //
not using NullSafeExpressions for complex OR cases
+
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.getNullSafeExpressions().size()).isEqualTo(0); //
not using NullSafeExpressions for complex OR cases
+
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.getNullSafeExpressions().size()).isEqualTo(0); //
not using NullSafeExpressions for complex OR case
+
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.getNullSafeExpressions().size()).isEqualTo(0); //
not using NullSafeExpressions for complex OR case
+
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.getNullSafeExpressions().size()).isEqualTo(0); //
not using NullSafeExpressions for complex OR case
+
assertThat(result.getExpr().toString()).isEqualTo("org.drools.modelcompiler.util.EvaluationUtil.areNumbersNullSafeEquals(_this.getAge(),
60)");
+ }
+
+ @Test
+ public void testOrWithMultipleConstraints() {
+ SingleDrlxParseSuccess result = (SingleDrlxParseSuccess)
parser.drlxParse(Person.class, "$p", "(name == \"John\" || != \"Peter\") &&
(age <= 19 || >= 60)");
+
+ assertThat(result.getNullSafeExpressions().size()).isEqualTo(0); //
not using NullSafeExpressions for complex OR case
+
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))");
Review Comment:
[nitpick] The expected expression in this test mixes '||' and '&&' without
explicit grouping, which could lead to ambiguities in operator precedence;
consider adding parentheses to clearly indicate the intended evaluation order.
```suggestion
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)))");
```
##########
drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/drlxparse/ConstraintParser.java:
##########
@@ -426,20 +424,34 @@ private Expression combineExpressions(List<Expression>
leftPrefixExpresssions, L
throw new RuntimeException(combo + " is not nor contains
BinaryExpr");
}
- Expression left = binaryExpr.getLeft();
- for (Expression prefixExpression : leftPrefixExpresssions) {
+ Expression left =
getConstraintEqualityExpression(binaryExpr.getLeft());
+ for (Expression prefixExpression : leftPrefixExpressions) {
left = new BinaryExpr(prefixExpression, left,
BinaryExpr.Operator.AND);
}
binaryExpr.setLeft(left);
- Expression right = binaryExpr.getRight();
- for (Expression prefixExpression : rightPrefixExpresssions) {
+ Expression right =
getConstraintEqualityExpression(binaryExpr.getRight());
+ for (Expression prefixExpression : rightPrefixExpressions) {
right = new BinaryExpr(prefixExpression, right,
BinaryExpr.Operator.AND);
}
binaryExpr.setRight(right);
return combo;
}
Review Comment:
[nitpick] Consider adding a comment to explain the recursive handling in
getConstraintEqualityExpression, especially its purpose in transforming binary
equality expressions, to aid future maintainers.
```suggestion
/**
* Transforms binary equality expressions into a specific format for
further processing.
* This method recursively traverses the left and right operands of a
BinaryExpr,
* applying transformations to nested binary expressions. It
specifically handles
* equality (EQUALS) and inequality (NOT_EQUALS) operators by converting
them into
* a TypedExpression-based equality expression.
*
* @param expr The expression to process, which may be a BinaryExpr or
another type of expression.
* @return The transformed expression.
*/
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
