Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DROOLS-7493] [DROOLS-7497] executable model wrongly rewrites modify in if-block #5380

Merged
merged 4 commits into from
Jul 14, 2023
Merged

[DROOLS-7493] [DROOLS-7497] executable model wrongly rewrites modify in if-block #5380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7087d2a
[DROOLS-7493] executable model wrongly rewrites modify in if-block
tkobayas Jul 10, 2023
38427f6
- analyze whole RHS and make all modification as property reactive
tkobayas Jul 11, 2023
8fc9c83
- Add docs about fact modification after modify or update in RHS
tkobayas Jul 11, 2023
b16c73e
- fixing code smells
tkobayas Jul 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion drools-docs/src/modules/ROOT/pages/language-reference/_drl-rules.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2595,9 +2595,21 @@ update ( <object> ) // Causes `KieSession` to search for a fact handle of the o
[source]
----
$application.setAmount( 100 );
update( LoanApplication );
update( $application );
----

[NOTE]
====
When `modify` or `update` is called in a rule, its actual evaluation happens after executing the whole RHS code. It means all modifications in the RHS will affect in the evaluation. For example,
[source]
----
$application.setAmount( 100 );
update( $application );
$application.setApproved( true );
----
The modifications of both `amount` and `approved` are considered in the evaluation of the update.
====

NOTE: If you provide property-change listeners, you do not need to call this method when an object changes. For more information about property-change listeners, see
xref:rule-engine/index.adoc#property-change-listeners-con_rule-engine[Property-change settings and listeners for fact types].

Expand Down
24 changes: 7 additions & 17 deletions ...model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/Consequence.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -65,7 +64,6 @@
import org.drools.util.StringUtils;

import static com.github.javaparser.StaticJavaParser.parseExpression;
import static com.github.javaparser.ast.NodeList.nodeList;
import static java.util.stream.Collectors.toSet;
import static org.drools.model.codegen.execmodel.PackageModel.DOMAIN_CLASSESS_METADATA_FILE_NAME;
import static org.drools.model.codegen.execmodel.PackageModel.DOMAIN_CLASS_METADATA_INSTANCE;
Expand Down Expand Up @@ -165,7 +163,7 @@ public MethodCallExpr createCall(String consequenceString, BlockStmt ruleVariabl
switch (context.getRuleDialect()) {
case JAVA:
rewriteReassignedDeclarations(ruleConsequence, usedDeclarationInRHS);
executeCall = executeCall(ruleVariablesBlock, ruleConsequence, usedDeclarationInRHS, onCall, Collections.emptySet());
executeCall = executeCall(ruleVariablesBlock, ruleConsequence, usedDeclarationInRHS, onCall);
break;
case MVEL:
executeCall = createExecuteCallMvel(consequenceString, ruleVariablesBlock, usedDeclarationInRHS, onCall);
Expand Down Expand Up @@ -221,9 +219,9 @@ private MethodCallExpr createExecuteCallMvel(String consequenceString, BlockStmt
return executeCall(ruleVariablesBlock,
compile.statementResults(),
usedDeclarationInRHS,
onCall,
compile.getUsedBindings());
onCall);
}

private BlockStmt rewriteConsequence(String consequence) {
try {
String ruleConsequenceAsBlock = preprocessConsequence(consequence.trim());
Expand Down Expand Up @@ -279,15 +277,7 @@ public static boolean containsWord(String word, String body) {
return p.matcher(bodyWithDollarReplaced).find();
}

private MethodCallExpr executeCall(BlockStmt ruleVariablesBlock, BlockStmt ruleConsequence, Collection<String> verifiedDeclUsedInRHS, MethodCallExpr onCall, Set<String> modifyProperties) {

for (String modifiedProperty : modifyProperties) {
NodeList<Expression> arguments = nodeList(new NameExpr(modifiedProperty));
MethodCallExpr update = new MethodCallExpr(new NameExpr("drools"), "update", arguments);
ruleConsequence.getStatements().add(new ExpressionStmt(update));
}


private MethodCallExpr executeCall(BlockStmt ruleVariablesBlock, BlockStmt ruleConsequence, Collection<String> verifiedDeclUsedInRHS, MethodCallExpr onCall) {
boolean requireDrools = rewriteRHS(ruleVariablesBlock, ruleConsequence);
MethodCallExpr executeCall = new MethodCallExpr(onCall != null ? onCall : new NameExpr("D"), EXECUTE_CALL);
LambdaExpr executeLambda = new LambdaExpr();
Expand Down Expand Up @@ -401,7 +391,7 @@ private void addUpdateBitMask(BlockStmt ruleBlock, List<MethodCallExpr> methodCa
if (context.isPropertyReactive(updatedClass)) {

if ( !initializedBitmaskFields.contains( updatedVar ) ) {
Set<String> modifiedProps = findModifiedProperties(methodCallExprs, updateExpr, updatedVar, updatedClass );
Set<String> modifiedProps = findModifiedProperties(methodCallExprs, updatedVar, updatedClass );
modifiedProps.addAll(findModifiedPropertiesFromAssignment(assignExprs, updatedVar));
MethodCallExpr bitMaskCreation = createBitMaskInitialization( updatedClass, modifiedProps );
AssignExpr bitMaskAssign = createBitMaskField(updatedVar, bitMaskCreation);
Expand Down Expand Up @@ -454,9 +444,9 @@ private AssignExpr createBitMaskField(String updatedVar, MethodCallExpr bitMaskC
return new AssignExpr(bitMaskVar, bitMaskCreation, AssignExpr.Operator.ASSIGN);
}

private Set<String> findModifiedProperties( List<MethodCallExpr> methodCallExprs, MethodCallExpr updateExpr, String updatedVar, Class<?> updatedClass ) {
private Set<String> findModifiedProperties( List<MethodCallExpr> methodCallExprs, String updatedVar, Class<?> updatedClass ) {
Set<String> modifiedProps = new HashSet<>();
for (MethodCallExpr methodCall : methodCallExprs.subList(0, methodCallExprs.indexOf(updateExpr))) {
for (MethodCallExpr methodCall : methodCallExprs) {
if (!isDirectExpression(methodCall)) {
continue; // don't evaluate a method which is a part of other expression
}
Expand Down
143 changes: 142 additions & 1 deletion ...odegen/src/test/java/org/drools/model/codegen/execmodel/PropertyReactivityMatrixTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ public int getValue2() {
public void setValue2(int value2) {
this.value2 = value2;
}

}

private String setValueStatement(String property, int value) {
Expand Down Expand Up @@ -163,6 +162,52 @@ public void updateInsideIfBlockInsideAndOutsideAssignment_shouldTriggerReactivit
" }");
}

@Test
public void assignmentAfterModify_shouldTriggerReactivity() {
// DROOLS-7497
statementInsideBlock_shouldTriggerReactivity(" modify($fact) {\n" +
" " + setValueStatement("value2", 2) + "\n" +
" }\n" +
" $fact." + setValueStatement("value1", 2) + "\n");
}

@Test
public void assignmentBeforeAndAfterModify_shouldTriggerReactivity() {
// DROOLS-7497
statementInsideBlock_shouldTriggerReactivity(" $fact." + setValueStatement("value2", 2) + "\n" +
" modify($fact) {\n" +
" }\n" +
" $fact." + setValueStatement("value1", 2) + "\n");
}

@Test
public void assignmentAfterIfBlockModify_shouldTriggerReactivity() {
// DROOLS-7497
statementInsideBlock_shouldTriggerReactivity(" if (true) {\n" +
" modify($fact) {\n" +
" " + setValueStatement("value2", 2) + "\n" +
" }\n" +
" }\n" +
" $fact." + setValueStatement("value1", 2) + "\n");
}

@Test
public void assignmentBeforeAndAfterUpdate_shouldTriggerReactivity() {
// DROOLS-7497
statementInsideBlock_shouldTriggerReactivity(" $fact." + setValueStatement("value2", 2) + "\n" +
" update($fact);\n" +
" $fact." + setValueStatement("value1", 2) + "\n");
}

@Test
public void assignmentAfterIfBlockUpdate_shouldTriggerReactivity() {
// DROOLS-7497
statementInsideBlock_shouldTriggerReactivity(" if (true) {\n" +
" update($fact);\n" +
" }\n" +
" $fact." + setValueStatement("value1", 2) + "\n");
}

private void statementInsideBlock_shouldTriggerReactivity(String rhs) {
final String str =
"import " + Fact.class.getCanonicalName() + ";\n" +
Expand Down Expand Up @@ -192,4 +237,100 @@ private void statementInsideBlock_shouldTriggerReactivity(String rhs) {
assertThat(results).as("Should trigger property reactivity on value1")
.containsExactly("R2 fired");
}

@Test
public void modifyInsideIfFalseAndTrueBlock_shouldTriggerReactivity() {
// DROOLS-7493
statementInsideBlock_shouldTriggerReactivityWithLoop(" if (false) {\n" +
" $fact." + setValueStatement("value1", 2) + "\n" + // this line is not executed, but analyzed as a property reactivity
" }\n" +
" if (true) {\n" +
" modify($fact) {\n" +
" }\n" +
" }\n");
}

@Test
public void updateInsideIfFalseAndTrueBlock_shouldTriggerReactivity() {
statementInsideBlock_shouldTriggerReactivityWithLoop(" if (false) {\n" +
" $fact." + setValueStatement("value1", 2) + "\n" + // this line is not executed, but analyzed as a property reactivity
" }\n" +
" if (true) {\n" +
" update($fact);\n" +
" }\n");
}

private void statementInsideBlock_shouldTriggerReactivityWithLoop(String rhs) {
final String str =
"import " + Fact.class.getCanonicalName() + ";\n" +
"dialect \"" + dialect.name().toLowerCase() + "\"\n" +
"global java.util.List results;\n" +
"rule R1\n" +
" when\n" +
" $fact : Fact( value1 == 1 )\n" +
" then\n" +
rhs +
"end\n";

KieSession ksession = getKieSession(str);
List<String> results = new ArrayList<>();
ksession.setGlobal("results", results);

Fact fact = new Fact(1);
ksession.insert(fact);
int fired = ksession.fireAllRules(10);

assertThat(fired).as("Should trigger property reactivity on value1 and result in a loop")
.isEqualTo(10);
}

@Test
public void modifyInsideIfFalseBlock_shouldNotTriggerReactivity() {
// DROOLS-7493
statementInsideIfFalseBlock_shouldNotTriggerReactivityNorSelfLoop(" if (false) {\n" +
" modify($fact) {\n" +
" " + setValueStatement("value1", 2) + "\n" +
" }\n" +
" }\n");
}

@Test
public void updateInsideIfFalseBlock_shouldNotTriggerReactivity() {
statementInsideIfFalseBlock_shouldNotTriggerReactivityNorSelfLoop(" if (false) {\n" +
" $fact." + setValueStatement("value1", 2) + "\n" +
" update($fact);\n" +
" }\n");
}

private void statementInsideIfFalseBlock_shouldNotTriggerReactivityNorSelfLoop(String rhs) {
final String str =
"import " + Fact.class.getCanonicalName() + ";\n" +
"dialect \"" + dialect.name().toLowerCase() + "\"\n" +
"global java.util.List results;\n" +
"rule R1\n" +
" when\n" +
" $fact : Fact( value1 == 1 )\n" +
" then\n" +
rhs +
"end\n" +
"rule R2\n" +
" when\n" +
" $fact : Fact( value1 == 2 )\n" +
" then\n" +
" results.add(\"R2 fired\");\n" +
"end\n";

KieSession ksession = getKieSession(str);
List<String> results = new ArrayList<>();
ksession.setGlobal("results", results);

Fact fact = new Fact(1);
ksession.insert(fact);
int fired = ksession.fireAllRules(10);
assertThat(fired).as("Don't cause a loop")
.isEqualTo(1);

assertThat(results).as("Shouldn't trigger R2, because modify is not executed")
.isEmpty();
}
}
14 changes: 12 additions & 2 deletions drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/MvelCompiler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,20 @@

import java.util.HashSet;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Stream;

import com.github.javaparser.ast.Node;
import com.github.javaparser.ast.expr.MethodCallExpr;
import com.github.javaparser.ast.expr.NameExpr;
import com.github.javaparser.ast.stmt.BlockStmt;
import com.github.javaparser.ast.stmt.Statement;
import org.drools.mvel.parser.MvelParser;
import org.drools.mvel.parser.ast.expr.ModifyStatement;
import org.drools.mvelcompiler.ast.TypedExpression;
import org.drools.mvelcompiler.context.MvelCompilerContext;

import static com.github.javaparser.ast.NodeList.nodeList;
import static java.util.stream.Collectors.toList;

public class MvelCompiler {
Expand Down Expand Up @@ -72,8 +75,15 @@ public CompiledBlockResult compileStatement(String mvelBlock) {
.setUsedBindings(allUsedBindings);
}

private Stream<String> transformStatementWithPreprocessing(Statement s) {
private Stream<String> transformStatementWithPreprocessing(ModifyStatement s) {
Optional<Node> parentNode = s.getParentNode();
PreprocessPhase.PreprocessPhaseResult invoke = preprocessPhase.invoke(s);
parentNode.ifPresent(p -> {
BlockStmt parentBlock = (BlockStmt) p;
for (String modifiedFact : invoke.getUsedBindings()) {
parentBlock.addStatement(new MethodCallExpr(new NameExpr("drools"), "update", nodeList(new NameExpr(modifiedFact))));
}
});
s.remove();
return invoke.getUsedBindings().stream();
}
Expand Down
20 changes: 9 additions & 11 deletions drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/PreprocessPhase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,17 +107,22 @@ private PreprocessPhaseResult modifyPreprocessor(ModifyStatement modifyStatement
final Expression scope = modifyStatement.getModifyObject();
modifyStatement
.findAll(AssignExpr.class)
.replaceAll(assignExpr -> assignToFieldAccess(result, scope, assignExpr));
.replaceAll(assignExpr -> assignToFieldAccess(scope, assignExpr));

// Do not use findAll as we should only process top level expressions
modifyStatement
.getExpressions()
.replaceAll(e -> addScopeToMethodCallExpr(result, scope, e));
.replaceAll(e -> addScopeToMethodCallExpr(scope, e));

NodeList<Statement> statements = wrapToExpressionStmt(modifyStatement.getExpressions());
// delete modify statement and replace its own block of statements
modifyStatement.replace(new BlockStmt(statements));

// even if no property is modified inside modify block, need to call update because its properties may be modified in RHS
if (scope.isNameExpr() || scope instanceof DrlNameExpr) {
result.addUsedBinding(printNode(scope));
}

return result;
}

Expand All @@ -131,7 +136,7 @@ private NodeList<Statement> wrapToExpressionStmt(NodeList<Statement> expressions
.collect(Collectors.toList()));
}

private Statement addScopeToMethodCallExpr(PreprocessPhaseResult result, Expression scope, Statement e) {
private Statement addScopeToMethodCallExpr(Expression scope, Statement e) {
if (e != null && e.isExpressionStmt()) {
Expression expression = e.asExpressionStmt().getExpression();
if (expression.isMethodCallExpr()) {
Expand All @@ -144,16 +149,10 @@ private Statement addScopeToMethodCallExpr(PreprocessPhaseResult result, Express
MethodCallExpr rootMcExpr = rootExpr.asMethodCallExpr();
Expression enclosed = new EnclosedExpr(scope);
rootMcExpr.setScope(enclosed);

if (scope.isNameExpr() || scope instanceof DrlNameExpr) { // some classes such "AtomicInteger" have a setter called "set"
result.addUsedBinding(printNode(scope));
}

return new ExpressionStmt(mcExpr);
} else if (rootExpr instanceof DrlNameExpr) {
throwExceptionIfSameDrlName(rootExpr, scope);
// Unknown name. Assume a property of the fact
result.addUsedBinding(printNode(scope));
replaceRootExprWithFieldAccess(scope, (DrlNameExpr) rootExpr);
}
}
Expand Down Expand Up @@ -194,8 +193,7 @@ private Expression findRootScope(Expression expr) {
return scope;
}

private AssignExpr assignToFieldAccess(PreprocessPhaseResult result, Expression scope, AssignExpr assignExpr) {
result.addUsedBinding(printNode(scope));
private AssignExpr assignToFieldAccess(Expression scope, AssignExpr assignExpr) {
Expression target = assignExpr.getTarget();

if (target instanceof DrlNameExpr) { // e.g. age = 10
Expand Down
Loading