Skip to content
This repository has been archived by the owner on Jan 9, 2024. It is now read-only.

[7.x] [DROOLS-7493] [DROOLS-7497] executable model wrongly rewrites modify … #7

Merged
merged 1 commit into from
Oct 11, 2023
Merged

[7.x] [DROOLS-7493] [DROOLS-7497] executable model wrongly rewrites modify … #7

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 9 additions & 20 deletions ...-model-compiler/src/main/java/org/drools/modelcompiler/builder/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.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
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 @@ -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
Loading