Skip to content

Commit

Permalink
[DROOLS-7493] [DROOLS-7497] executable model wrongly rewrites modify …
Browse files Browse the repository at this point in the history 
…in if-block (apache#5380)

* [DROOLS-7493] executable model wrongly rewrites modify in if-block
- Additional tests to cover setter order for properperty reactivity [DROOLS-7497]

* - analyze whole RHS and make all modification as property reactive

* - Add docs about fact modification after modify or update in RHS

* - fixing code smells
  • Loading branch information
@tkobayas
tkobayas committed Oct 11, 2023
1 parent 5b13798 commit 1014efd
Show file tree
Hide file tree
Showing 6 changed files with 244 additions and 47 deletions.
29 changes: 9 additions & 20 deletions ...-model-compiler/src/main/java/org/drools/modelcompiler/builder/generator/Consequence.java
View file
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.mvelcompiler.MvelCompilerException;

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.core.util.ClassUtils.getter2property;
import static org.drools.core.util.ClassUtils.isGetter;
Expand Down Expand Up @@ -153,7 +151,7 @@ public MethodCallExpr createCall(RuleDescr ruleDescr, String consequenceString,
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 @@ -209,9 +207,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 = rewriteModifyBlock(consequence.trim());
Expand Down Expand Up @@ -268,16 +266,7 @@ public static boolean containsWord(String word, String body) {
return m.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 @@ -361,8 +350,8 @@ private boolean rewriteRHS(BlockStmt ruleBlock, BlockStmt rhs) {
if (context.isPropertyReactive(updatedClass)) {

if ( !initializedBitmaskFields.contains( updatedVar ) ) {
Set<String> modifiedProps = findModifiedProperties( methodCallExprs, updateExpr, updatedVar, updatedClass );
modifiedProps.addAll(findModifiedPropertiesFromAssignment( assignExprs, 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);
if (!DrlxParseUtil.hasDuplicateExpr(ruleBlock, bitMaskAssign)) {
Expand Down Expand Up @@ -412,9 +401,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 Expand Up @@ -464,7 +453,7 @@ private Set<String> findModifiedProperties( List<MethodCallExpr> methodCallExprs
return modifiedProps;
}

private Set<String> findModifiedPropertiesFromAssignment(List<AssignExpr> assignExprs, MethodCallExpr updateExpr, String updatedVar, Class<?> updatedClass) {
private Set<String> findModifiedPropertiesFromAssignment(List<AssignExpr> assignExprs, String updatedVar) {
Set<String> modifiedProps = new HashSet<>();
for (AssignExpr assignExpr : assignExprs) {
Expression target = assignExpr.getTarget();
Expand Down
143 changes: 142 additions & 1 deletion ...s-model-compiler/src/test/java/org/drools/modelcompiler/PropertyReactivityMatrixTest.java
View file
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -104,17 +104,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 @@ -128,7 +133,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 @@ -141,16 +146,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 @@ -191,8 +190,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

0 comments on commit 1014efd

Please sign in to comment.