From a40a06185cf692a2c96f3bc22c51ff8d52ac1af6 Mon Sep 17 00:00:00 2001 From: Toshiya Kobayashi Date: Fri, 13 Oct 2023 16:28:30 +0900 Subject: [PATCH] [7.67.x] [DROOLS-7493] [DROOLS-7497] executable model wrongly rewrites modify in if-block (#8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [DROOLS-7195] Modify syntax fails when using executable model, works … (#4846) (#4868) * [DROOLS-7195] Modify syntax fails when using executable model, works with mvel runtime (nested properties) * - Dropping 'with' statement support * [DROOLS-7493] [DROOLS-7497] executable model wrongly rewrites modify in if-block (#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 --- .../builder/generator/Consequence.java | 29 +-- .../CompilationFailuresTest.java | 91 ++++++++- .../drools/modelcompiler/MvelDialectTest.java | 131 ++++++++++++ .../PropertyReactivityMatrixTest.java | 143 ++++++++++++- .../drools/modelcompiler/domain/Address.java | 9 + .../drools/modelcompiler/domain/Counter.java | 29 +++ .../org/drools/mvelcompiler/LHSPhase.java | 16 +- .../drools/mvelcompiler/ModifyCompiler.java | 2 +- .../org/drools/mvelcompiler/MvelCompiler.java | 21 +- .../drools/mvelcompiler/PreprocessPhase.java | 192 +++++++----------- .../drools/mvelcompiler/MvelCompilerTest.java | 117 ++++++----- .../java/org/drools/mvel/asm/AsmUtil.java | 13 +- .../integrationtests/RuleChainingTest.java | 10 +- 13 files changed, 580 insertions(+), 223 deletions(-) create mode 100644 drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/domain/Counter.java diff --git a/drools-model/drools-model-compiler/src/main/java/org/drools/modelcompiler/builder/generator/Consequence.java b/drools-model/drools-model-compiler/src/main/java/org/drools/modelcompiler/builder/generator/Consequence.java index d3b857b4844..f3de2b19519 100644 --- a/drools-model/drools-model-compiler/src/main/java/org/drools/modelcompiler/builder/generator/Consequence.java +++ b/drools-model/drools-model-compiler/src/main/java/org/drools/modelcompiler/builder/generator/Consequence.java @@ -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; @@ -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; @@ -152,7 +150,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); @@ -208,9 +206,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()); @@ -267,16 +265,7 @@ public static boolean containsWord(String word, String body) { return m.find(); } - private MethodCallExpr executeCall(BlockStmt ruleVariablesBlock, BlockStmt ruleConsequence, Collection verifiedDeclUsedInRHS, MethodCallExpr onCall, Set modifyProperties) { - - for (String modifiedProperty : modifyProperties) { - NodeList 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 verifiedDeclUsedInRHS, MethodCallExpr onCall) { boolean requireDrools = rewriteRHS(ruleVariablesBlock, ruleConsequence); MethodCallExpr executeCall = new MethodCallExpr(onCall != null ? onCall : new NameExpr("D"), EXECUTE_CALL); LambdaExpr executeLambda = new LambdaExpr(); @@ -360,8 +349,8 @@ private boolean rewriteRHS(BlockStmt ruleBlock, BlockStmt rhs) { if (context.isPropertyReactive(updatedClass)) { if ( !initializedBitmaskFields.contains( updatedVar ) ) { - Set modifiedProps = findModifiedProperties( methodCallExprs, updateExpr, updatedVar, updatedClass ); - modifiedProps.addAll(findModifiedPropertiesFromAssignment( assignExprs, updateExpr, updatedVar, updatedClass )); + Set 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)) { @@ -411,9 +400,9 @@ private AssignExpr createBitMaskField(String updatedVar, MethodCallExpr bitMaskC return new AssignExpr(bitMaskVar, bitMaskCreation, AssignExpr.Operator.ASSIGN); } - private Set findModifiedProperties( List methodCallExprs, MethodCallExpr updateExpr, String updatedVar, Class updatedClass ) { + private Set findModifiedProperties( List methodCallExprs, String updatedVar, Class updatedClass ) { Set 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 } @@ -463,7 +452,7 @@ private Set findModifiedProperties( List methodCallExprs return modifiedProps; } - private Set findModifiedPropertiesFromAssignment(List assignExprs, MethodCallExpr updateExpr, String updatedVar, Class updatedClass) { + private Set findModifiedPropertiesFromAssignment(List assignExprs, String updatedVar) { Set modifiedProps = new HashSet<>(); for (AssignExpr assignExpr : assignExprs) { Expression target = assignExpr.getTarget(); diff --git a/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/CompilationFailuresTest.java b/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/CompilationFailuresTest.java index fb943339143..53222c24f92 100644 --- a/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/CompilationFailuresTest.java +++ b/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/CompilationFailuresTest.java @@ -135,11 +135,12 @@ private void checkCompilationFailureOnMismatchingAccumulate(String type, String @Test - public void testModifyOnFactInScope() { - // DROOLS-5242 + public void modify_factInScope_java() { + // DROOLS-5242, DROOLS-7195 String drl = "import " + Person.class.getCanonicalName() + ";" + - "rule R1 when\n" + + "rule R1\n" + + "when\n" + " $p : Person(name == \"Mario\")\n" + "then\n" + " modify($p) { $p.setName(\"Mark\") }\n" + @@ -147,9 +148,89 @@ public void testModifyOnFactInScope() { Results results = getCompilationResults(drl); assertThat(results.getMessages(Message.Level.ERROR).isEmpty()).isFalse(); + } - // RHS error : line = 1 with STANDARD_FROM_DRL (RuleDescr) - assertThat(results.getMessages().get(0).getLine()).isEqualTo(1); + @Test + public void modify_factInScope_mvel() { + // DROOLS-5242, DROOLS-7195 + String drl = + "import " + Person.class.getCanonicalName() + ";" + + "rule R1\n" + + "dialect 'mvel'\n" + + "when\n" + + " $p : Person(name == \"Mario\")\n" + + "then\n" + + " modify($p) { $p.setName(\"Mark\") }\n" + + "end"; + + Results results = getCompilationResults(drl); + assertThat(results.getMessages(Message.Level.ERROR).isEmpty()).isFalse(); + } + + @Test + public void modify_factInScope_nestedPropertySetter_java() { + // DROOLS-5242, DROOLS-7195 + String drl = + "import " + Person.class.getCanonicalName() + ";" + + "rule R1\n" + + "when\n" + + " $p : Person(name == \"Mario\")\n" + + "then\n" + + " modify($p) { $p.address.setCity(\"London\") }\n" + + "end"; + + Results results = getCompilationResults(drl); + assertThat(results.getMessages(Message.Level.ERROR).isEmpty()).isFalse(); + } + + @Test + public void modify_factInScope_nestedPropertySetter_mvel() { + // DROOLS-5242, DROOLS-7195 + String drl = + "import " + Person.class.getCanonicalName() + ";" + + "rule R1\n" + + "dialect 'mvel'\n" + + "when\n" + + " $p : Person(name == \"Mario\")\n" + + "then\n" + + " modify($p) { $p.address.setCity(\"London\") }\n" + + "end"; + + Results results = getCompilationResults(drl); + assertThat(results.getMessages(Message.Level.ERROR).isEmpty()).isFalse(); + } + + @Test + public void modify_factInScope_nestedPropertyAssign_java() { + // DROOLS-5242, DROOLS-7195 + String drl = + "import " + Person.class.getCanonicalName() + ";" + + "rule R1\n" + + "when\n" + + " $p : Person(name == \"Mario\")\n" + + "then\n" + + " modify($p) { $p.address.city = \"London\" }\n" + + "end"; + + Results results = getCompilationResults(drl); + assertThat(results.getMessages(Message.Level.ERROR).isEmpty()).isFalse(); + } + + @Test + public void modify_factInScope_nestedPropertyAssign_mvel() { + // DROOLS-5242, DROOLS-7195 + String drl = + "import " + Person.class.getCanonicalName() + ";" + + "rule R1\n" + + "dialect 'mvel'\n" + + "when\n" + + " $p : Person(name == \"Mario\")\n" + + "then\n" + + " modify($p) { $p.address.city = \"London\" }\n" + + "end"; + + Results results = getCompilationResults(drl); + assertThat(results.getMessages(Message.Level.ERROR).isEmpty()).isFalse(); } @Test diff --git a/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/MvelDialectTest.java b/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/MvelDialectTest.java index 8a5cc99a7e2..1920dbbcf64 100644 --- a/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/MvelDialectTest.java +++ b/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/MvelDialectTest.java @@ -30,6 +30,7 @@ import org.drools.core.WorkingMemory; import org.drools.core.spi.Tuple; import org.drools.modelcompiler.domain.Address; +import org.drools.modelcompiler.domain.Counter; import org.drools.modelcompiler.domain.InternationalAddress; import org.drools.modelcompiler.domain.Person; @@ -1599,4 +1600,134 @@ public void drools_fieldAccess() { assertThat(results.get("knowledgeRuntime")).isInstanceOf(KieRuntime.class); assertThat(results.get("kieRuntime")).isInstanceOf(KieRuntime.class); } + + public void assign_nestedProperty() { + // DROOLS-7195 + String str = "package com.example.reproducer\n" + + "import " + Person.class.getCanonicalName() + ";\n" + + "rule R\n" + + "dialect \"mvel\"\n" + + "when\n" + + " $p : Person()\n" + + "then\n" + + " $p.address.city = \"Tokyo\";\n" + + "end"; + + KieSession ksession = getKieSession(str); + + Person p = new Person("John"); + p.setAddress(new Address("London")); + ksession.insert(p); + ksession.fireAllRules(); + + assertThat(p.getAddress().getCity()).isEqualTo("Tokyo"); + } + + @Test + public void assign_nestedPropertyInModify() { + // DROOLS-7195 + String str = "package com.example.reproducer\n" + + "import " + Person.class.getCanonicalName() + ";\n" + + "rule R\n" + + "dialect \"mvel\"\n" + + "when\n" + + " $p : Person(name == \"John\")\n" + + "then\n" + + " modify($p) {" + + " address.city = \"Tokyo\";\n" + + " }\n" + + "end"; + + KieSession ksession = getKieSession(str); + + Person p = new Person("John"); + p.setAddress(new Address("London")); + ksession.insert(p); + ksession.fireAllRules(); + + assertThat(p.getAddress().getCity()).isEqualTo("Tokyo"); + } + + @Test + public void setter_nestedPropertyInModify() { + // DROOLS-7195 + String str = "package com.example.reproducer\n" + + "import " + Person.class.getCanonicalName() + ";\n" + + "rule R\n" + + "dialect \"mvel\"\n" + + "when\n" + + " $p : Person(name == \"John\")\n" + + "then\n" + + " modify($p) {" + + " address.setCity(\"Tokyo\");\n" + + " }\n" + + "end"; + + KieSession ksession = getKieSession(str); + + Person p = new Person("John"); + p.setAddress(new Address("London")); + ksession.insert(p); + ksession.fireAllRules(); + + assertThat(p.getAddress().getCity()).isEqualTo("Tokyo"); + } + + @Test + public void assign_deepNestedPropertyInModify() { + // DROOLS-7195 + String str = "package com.example.reproducer\n" + + "import " + Person.class.getCanonicalName() + ";\n" + + "rule R\n" + + "dialect \"mvel\"\n" + + "when\n" + + " $p : Person(name == \"John\")\n" + + "then\n" + + " modify($p) {" + + " address.visitorCounter.value = 1;\n" + + " }\n" + + "end"; + + KieSession ksession = getKieSession(str); + + Person person = new Person("John"); + Address address = new Address("London"); + Counter counter = new Counter(); + counter.setValue(0); + address.setVisitorCounter(counter); + person.setAddress(address); + ksession.insert(person); + ksession.fireAllRules(); + + assertThat(person.getAddress().getVisitorCounter().getValue()).isEqualTo(1); + } + + @Test + public void setter_deepNestedPropertyInModify() { + // DROOLS-7195 + String str = "package com.example.reproducer\n" + + "import " + Person.class.getCanonicalName() + ";\n" + + "rule R\n" + + "dialect \"mvel\"\n" + + "when\n" + + " $p : Person(name == \"John\")\n" + + "then\n" + + " modify($p) {" + + " address.visitorCounter.setValue(1);\n" + + " }\n" + + "end"; + + KieSession ksession = getKieSession(str); + + Person person = new Person("John"); + Address address = new Address("London"); + Counter counter = new Counter(); + counter.setValue(0); + address.setVisitorCounter(counter); + person.setAddress(address); + ksession.insert(person); + ksession.fireAllRules(); + + assertThat(person.getAddress().getVisitorCounter().getValue()).isEqualTo(1); + } } diff --git a/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/PropertyReactivityMatrixTest.java b/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/PropertyReactivityMatrixTest.java index c18e87024e8..719e599e1f7 100644 --- a/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/PropertyReactivityMatrixTest.java +++ b/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/PropertyReactivityMatrixTest.java @@ -82,7 +82,6 @@ public int getValue2() { public void setValue2(int value2) { this.value2 = value2; } - } private String setValueStatement(String property, int value) { @@ -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" + @@ -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 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 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(); + } } diff --git a/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/domain/Address.java b/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/domain/Address.java index a5c24c4a3f9..7f960806111 100644 --- a/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/domain/Address.java +++ b/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/domain/Address.java @@ -6,6 +6,7 @@ public class Address { private int number; private short shortNumber; private String city; + private Counter visitorCounter = new Counter(); public Address() { this("", 0, ""); @@ -58,6 +59,14 @@ public void setShortNumber(short shortNumber) { this.shortNumber = shortNumber; } + public Counter getVisitorCounter() { + return visitorCounter; + } + + public void setVisitorCounter(Counter visitorCounter) { + this.visitorCounter = visitorCounter; + } + public int hashCode() { final int prime = 31; int result = 1; diff --git a/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/domain/Counter.java b/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/domain/Counter.java new file mode 100644 index 00000000000..bb55dc605d6 --- /dev/null +++ b/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/domain/Counter.java @@ -0,0 +1,29 @@ +/* + * Copyright 2022 Red Hat, Inc. and/or its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.drools.modelcompiler.domain; + + +public class Counter { + private int value; + + public int getValue() { + return value; + } + + public void setValue(int value) { + this.value = value; + } +} diff --git a/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/LHSPhase.java b/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/LHSPhase.java index b772e433cf5..c2cadbf670d 100644 --- a/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/LHSPhase.java +++ b/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/LHSPhase.java @@ -126,7 +126,11 @@ public TypedExpression visit(FieldAccessExpr n, Void arg) { TypedExpression fieldAccessScope = n.getScope().accept(this, arg); n.getName().accept(this, arg); - if(parentIsArrayAccessExpr(n)) { + if (n.isInternal()) { + // a part of a larger FieldAccessExpr. e.g. [$p.address] of [$p.address.city] + return tryParseItAsGetter(n, fieldAccessScope) + .orElse(new UnalteredTypedExpression(n)); + } else if(parentIsArrayAccessExpr(n)) { return tryParseItAsMap(n, fieldAccessScope) .map(Optional::of) .orElseGet(() -> tryParseItAsSetter(n, fieldAccessScope, getRHSType())) @@ -262,6 +266,16 @@ private Optional tryParseItAsSetter(FieldAccessExpr n, TypedExp }); } + private Optional tryParseItAsGetter(FieldAccessExpr n, TypedExpression scope) { + return scope.getType().flatMap(scopeType -> { + String propertyName = printNode(n.getName()); + Optional optAccessor = + ofNullable(getAccessor((Class) scopeType, propertyName)); + + return optAccessor.map(accessor -> new FieldToAccessorTExpr(scope, accessor, emptyList())); + }); + } + @Override public TypedExpression visit(MethodCallExpr n, Void arg) { logPhase("MethodCallExpr {}", n); diff --git a/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/ModifyCompiler.java b/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/ModifyCompiler.java index cef1800d82f..778b9174fb1 100644 --- a/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/ModifyCompiler.java +++ b/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/ModifyCompiler.java @@ -32,7 +32,7 @@ // A special case of compiler in which only the modify statements are processed public class ModifyCompiler { - private static final PreprocessPhase preprocessPhase = new PreprocessPhase(true); + private static final PreprocessPhase preprocessPhase = new PreprocessPhase(); public CompiledBlockResult compile(String mvelBlock) { diff --git a/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/MvelCompiler.java b/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/MvelCompiler.java index 3bcad8c1d95..9891bca634a 100644 --- a/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/MvelCompiler.java +++ b/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/MvelCompiler.java @@ -18,18 +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.mvel.parser.ast.expr.WithStatement; 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 { @@ -56,13 +58,7 @@ public CompiledBlockResult compileStatement(String mvelBlock) { .flatMap(this::transformStatementWithPreprocessing) .collect(toList()); - List withUsedBindings = mvelExpression.findAll(WithStatement.class) - .stream() - .flatMap(this::transformStatementWithPreprocessing) - .collect(toList()); - allUsedBindings.addAll(modifyUsedBindings); - allUsedBindings.addAll(withUsedBindings); // Entry point of the compiler TypedExpression compiledRoot = mvelExpression.accept(statementVisitor, null); @@ -79,8 +75,15 @@ public CompiledBlockResult compileStatement(String mvelBlock) { .setUsedBindings(allUsedBindings); } - private Stream transformStatementWithPreprocessing(Statement s) { + private Stream transformStatementWithPreprocessing(ModifyStatement s) { + Optional 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(); } diff --git a/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/PreprocessPhase.java b/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/PreprocessPhase.java index 45dfac5bf99..45ec20bcde3 100644 --- a/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/PreprocessPhase.java +++ b/drools-model/drools-mvel-compiler/src/main/java/org/drools/mvelcompiler/PreprocessPhase.java @@ -16,11 +16,8 @@ package org.drools.mvelcompiler; -import java.util.ArrayDeque; import java.util.ArrayList; -import java.util.Deque; import java.util.HashSet; -import java.util.Iterator; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -34,41 +31,25 @@ import com.github.javaparser.ast.expr.Expression; import com.github.javaparser.ast.expr.FieldAccessExpr; import com.github.javaparser.ast.expr.MethodCallExpr; -import com.github.javaparser.ast.expr.ObjectCreationExpr; -import com.github.javaparser.ast.expr.VariableDeclarationExpr; import com.github.javaparser.ast.stmt.BlockStmt; import com.github.javaparser.ast.stmt.EmptyStmt; import com.github.javaparser.ast.stmt.ExpressionStmt; import com.github.javaparser.ast.stmt.Statement; -import com.github.javaparser.ast.type.ClassOrInterfaceType; import org.drools.mvel.parser.ast.expr.DrlNameExpr; import org.drools.mvel.parser.ast.expr.ModifyStatement; -import org.drools.mvel.parser.ast.expr.WithStatement; import org.drools.mvel.parser.printer.PrintUtil; import static com.github.javaparser.ast.NodeList.nodeList; -import static java.util.Optional.empty; -import static java.util.Optional.of; import static org.drools.mvel.parser.printer.PrintUtil.printNode; /** - * This phase transforms modify and with statements in valid Java code + * This phase transforms modify statements in valid Java code * * It's used both in the MVEL Compiler and also to preprocess Drools' Java consequences that - * use modify and with blocks. + * use modify blocks. */ public class PreprocessPhase { - private final boolean failOnEmptyRootScope; - - public PreprocessPhase() { - this(false); - } - - public PreprocessPhase(boolean failOnEmptyRootScope) { - this.failOnEmptyRootScope = failOnEmptyRootScope; - } - interface PreprocessPhaseResult { Set getUsedBindings(); @@ -112,8 +93,6 @@ public PreprocessPhaseResult invoke(Statement statement) { if (statement instanceof ModifyStatement) { return modifyPreprocessor((ModifyStatement) statement); - } else if (statement instanceof WithStatement) { - return withPreprocessor((WithStatement) statement); } else { return new StatementResult(); } @@ -125,85 +104,25 @@ 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 statements = wrapToExpressionStmt(modifyStatement.getExpressions()); // delete modify statement and replace its own block of statements modifyStatement.replace(new BlockStmt(statements)); - return result; - } - - private PreprocessPhaseResult withPreprocessor(WithStatement withStatement) { - PreprocessPhaseResult result = new StatementResult(); - - Deque allNewStatements = new ArrayDeque<>(); - - Optional initScope = addTypeToInitialization(withStatement, allNewStatements); - final Expression scope = initScope.orElse(withStatement.getWithObject()); - - withStatement - .findAll(AssignExpr.class) - .replaceAll(assignExpr -> assignToFieldAccess(result, scope, assignExpr)); - - // Do not use findAll as we should only process top level expressions - withStatement - .getExpressions() - .replaceAll(e -> addScopeToMethodCallExpr(result, scope, e)); - - NodeList bodyStatements = wrapToExpressionStmt(withStatement.getExpressions()); - - allNewStatements.addAll(bodyStatements); - - // delete modify statement and add the new statements to its children - Node parentNode = withStatement.getParentNode() - .orElseThrow(() -> new MvelCompilerException("A parent node is expected here")); - - // We need to replace the with statement with the statements without nesting the new statements inside - // a BlockStmt otherwise other statements might reference the newly created instance - // See RuleChainingTest.testRuleChainingWithLogicalInserts - if(parentNode instanceof BlockStmt) { - BlockStmt parentBlock = (BlockStmt) parentNode; - - Iterator newStatementsReversed = allNewStatements.descendingIterator(); - while(newStatementsReversed.hasNext()) { - parentBlock.getStatements().addAfter(newStatementsReversed.next(), withStatement); - } - - withStatement.remove(); - } else { - throw new MvelCompilerException("Expecting a BlockStmt as a parent"); + // 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; } - private Optional addTypeToInitialization(WithStatement withStatement, Deque allNewStatements) { - if (withStatement.getWithObject().isAssignExpr()) { - AssignExpr assignExpr = withStatement.getWithObject().asAssignExpr(); - Expression assignExprValue = assignExpr.getValue(); - Expression assignExprTarget = assignExpr.getTarget(); - - if (assignExprValue.isObjectCreationExpr() && assignExprTarget instanceof DrlNameExpr) { - ObjectCreationExpr constructor = assignExprValue.asObjectCreationExpr(); - ClassOrInterfaceType ctorType = constructor.getType(); - - String targetVariableName = ((DrlNameExpr) assignExprTarget).getNameAsString(); - VariableDeclarationExpr variableDeclarationExpr = new VariableDeclarationExpr(ctorType, targetVariableName); - AssignExpr withTypeAssignmentExpr = new AssignExpr(variableDeclarationExpr, assignExprValue, assignExpr.getOperator()); - allNewStatements.add(new ExpressionStmt(withTypeAssignmentExpr)); - return of(new DrlNameExpr(targetVariableName)); - } - } - return empty(); - } - private NodeList wrapToExpressionStmt(NodeList expressions) { return nodeList(expressions .stream() @@ -214,7 +133,7 @@ private NodeList wrapToExpressionStmt(NodeList 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()) { @@ -227,56 +146,87 @@ 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 && !isSameDrlName(rootExpr, scope)) { + } else if (rootExpr instanceof DrlNameExpr) { + throwExceptionIfSameDrlName(rootExpr, scope); // Unknown name. Assume a property of the fact - DrlNameExpr originalFieldAccess = (DrlNameExpr) rootExpr; - String propertyName = originalFieldAccess.getName().asString(); - result.addUsedBinding(printNode(scope)); - FieldAccessExpr fieldAccessWithScope = new FieldAccessExpr(scope, propertyName); - rootExpr.getParentNode().filter(MethodCallExpr.class::isInstance) - .map(MethodCallExpr.class::cast) - .ifPresent(mce -> mce.setScope(fieldAccessWithScope)); + replaceRootExprWithFieldAccess(scope, (DrlNameExpr) rootExpr); } } } return e; } - private boolean isSameDrlName(Expression rootExpr, Expression scope) { - return rootExpr instanceof DrlNameExpr && scope instanceof DrlNameExpr && - ((DrlNameExpr) rootExpr).getName().equals(((DrlNameExpr) scope).getName()); + private void throwExceptionIfSameDrlName(Expression target, Expression scope) { + if (isSameDrlName(target, scope)) { + throw new MvelCompilerException("Invalid modify statement: " + PrintUtil.printNode(target)); + } } - private Expression findRootScope(MethodCallExpr mcExpr) { - Optional opt = mcExpr.getScope(); - if (!opt.isPresent()) { - return mcExpr; - } else { - Expression scope = opt.get(); - if (scope.isMethodCallExpr()) { - return findRootScope(scope.asMethodCallExpr()); + private boolean isSameDrlName(Expression target, Expression scope) { + return target instanceof DrlNameExpr && scope instanceof DrlNameExpr && + ((DrlNameExpr) target).getName().equals(((DrlNameExpr) scope).getName()); + } + + private Expression findRootScope(Expression expr) { + Expression scope; + if (expr.isMethodCallExpr()) { + MethodCallExpr mcExpr = expr.asMethodCallExpr(); + Optional opt = mcExpr.getScope(); + if (!opt.isPresent()) { + return mcExpr; // return MethodCallExpr if no scope + } else { + scope = opt.get(); + return findRootScope(scope); } + } else if (expr.isFieldAccessExpr()) { + FieldAccessExpr faExpr = expr.asFieldAccessExpr(); + scope = faExpr.getScope(); // FieldAccessExpr must have a scope + return findRootScope(scope); + } else { + scope = expr; + } + + return scope; + } + + private AssignExpr assignToFieldAccess(Expression scope, AssignExpr assignExpr) { + Expression target = assignExpr.getTarget(); + + if (target instanceof DrlNameExpr) { // e.g. age = 10 + return propertyNameToFieldAccess(scope, assignExpr, (DrlNameExpr) target); + } else if (target.isFieldAccessExpr()) { // e.g. address.city = 10 + Expression rootScope = findRootScope(target); + throwExceptionIfSameDrlName(rootScope, scope); + replaceRootExprWithFieldAccess(scope, (DrlNameExpr) rootScope); + return assignExpr; + } else { + throw new MvelCompilerException("Unexpected target: " + target.getClass() + ", assignExpr: " + PrintUtil.printNode(assignExpr)); } - if (failOnEmptyRootScope) { - throw new MvelCompilerException("Invalid modify statement: " + PrintUtil.printNode(mcExpr)); + } + + private void replaceRootExprWithFieldAccess(Expression scope, DrlNameExpr rootExpr) { + String propertyName = rootExpr.getName().asString(); + FieldAccessExpr fieldAccessWithScope = new FieldAccessExpr(scope, propertyName); + Optional optRootParent = rootExpr.getParentNode(); + if (optRootParent.isPresent()) { + Node rootParent = optRootParent.get(); + if (rootParent instanceof FieldAccessExpr) { + ((FieldAccessExpr)rootParent).setScope(fieldAccessWithScope); + } else if (rootParent instanceof MethodCallExpr) { + ((MethodCallExpr)rootParent).setScope(fieldAccessWithScope); + } else { + throw new MvelCompilerException(String.format("Unexpected rootParent: %s, rootExpr: %s", rootParent.getClass(), PrintUtil.printNode(rootExpr))); + } + } else { + throw new MvelCompilerException(String.format("rootExpr doesn't have a parent: %s, rootExpr: %s", rootExpr.getClass(), PrintUtil.printNode(rootExpr))); } - return opt.get(); } - private AssignExpr assignToFieldAccess(PreprocessPhaseResult result, Expression scope, AssignExpr assignExpr) { - DrlNameExpr originalFieldAccess = (DrlNameExpr) assignExpr.getTarget(); + private AssignExpr propertyNameToFieldAccess(Expression scope, AssignExpr assignExpr, DrlNameExpr originalFieldAccess) { String propertyName = originalFieldAccess.getName().asString(); - result.addUsedBinding(printNode(scope)); - FieldAccessExpr fieldAccessWithScope = new FieldAccessExpr(scope, propertyName); assignExpr.setTarget(fieldAccessWithScope); - return assignExpr; } diff --git a/drools-model/drools-mvel-compiler/src/test/java/org/drools/mvelcompiler/MvelCompilerTest.java b/drools-model/drools-mvel-compiler/src/test/java/org/drools/mvelcompiler/MvelCompilerTest.java index 989fcd12f3b..814764f2c6a 100644 --- a/drools-model/drools-mvel-compiler/src/test/java/org/drools/mvelcompiler/MvelCompilerTest.java +++ b/drools-model/drools-mvel-compiler/src/test/java/org/drools/mvelcompiler/MvelCompilerTest.java @@ -341,7 +341,7 @@ public void testSetterStringWithNull() { public void testSetterBigDecimalConstantModify() { test(ctx -> ctx.addDeclaration("$p", Person.class), "{ modify ( $p ) { salary = 50000 }; }", - "{ { $p.setSalary(new java.math.BigDecimal(50000)); } }", + "{ { $p.setSalary(new java.math.BigDecimal(50000)); } drools.update($p);}", result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); } @@ -349,7 +349,7 @@ public void testSetterBigDecimalConstantModify() { public void testSetterBigDecimalLiteralModify() { test(ctx -> ctx.addDeclaration("$p", Person.class), "{ modify ( $p ) { salary = 50000B }; }", - "{ { $p.setSalary(new java.math.BigDecimal(\"50000\")); } }", + "{ { $p.setSalary(new java.math.BigDecimal(\"50000\")); } drools.update($p);}", result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); } @@ -399,10 +399,64 @@ public void testDoNotConvertAdditionInStringConcatenation() { " list.add(\"before \" + $p + \", money = \" + $p.getSalary()); " + " { $p.setSalary(new java.math.BigDecimal(50000)); }" + " list.add(\"after \" + $p + \", money = \" + $p.getSalary()); " + + " drools.update($p);" + "}\n", result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); } + @Test + public void modifyInsideIfTrueBlock() { + test(ctx -> ctx.addDeclaration("$p", Person.class), + "{" + + " if (true) { " + + " modify ( $p ) { salary = 50000 }; " + + " }" + + "}", + "{\n" + + " if (true) {" + + " { $p.setSalary(new java.math.BigDecimal(50000)); }" + + " drools.update($p);" + + " }" + + "}", + result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); + } + + @Test + public void modifyInsideIfFalseBlock() { + test(ctx -> ctx.addDeclaration("$p", Person.class), + "{" + + " if (false) { " + + " modify ( $p ) { salary = 50000 }; " + + " }" + + "}", + "{\n" + + " if (false) {" + + " { $p.setSalary(new java.math.BigDecimal(50000)); }" + + " drools.update($p);" + + " }" + + "}", + result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); + } + + @Test + public void emptyModifyInsideIfBlockAndSetterOutside() { + test(ctx -> ctx.addDeclaration("$p", Person.class), + "{" + + " $p.salary = 50000;" + + " if (false) { " + + " modify ( $p ) { }; " + + " }" + + "}", + "{\n" + + " $p.setSalary(new java.math.BigDecimal(50000));" + + " if (false) {" + + " { }" + + " drools.update($p);" + + " }" + + "}", + result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); + } + @Test public void testSetterBigIntegerLiteral() { test(ctx -> { @@ -726,7 +780,7 @@ public void testPromotionOfIntToBigDecimalOnField() { public void testModify() { test(ctx -> ctx.addDeclaration("$p", Person.class), "{ modify ( $p ) { name = \"Luca\", age = 35 }; }", - "{\n {\n $p.setName(\"Luca\");\n $p.setAge(35);\n }\n }", + "{\n {\n $p.setName(\"Luca\");\n $p.setAge(35);\n } drools.update($p);\n }", result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); } @@ -737,7 +791,7 @@ public void testModifyMap() { ctx.addDeclaration("$p2", Person.class); }, "{ modify ( $p ) { items = $p2.items }; }", - "{\n {\n $p.setItems($p2.getItems());\n }\n }", + "{\n {\n $p.setItems($p2.getItems());\n } drools.update($p);\n }", result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); } @@ -745,7 +799,7 @@ public void testModifyMap() { public void testModifySemiColon() { test(ctx -> ctx.addDeclaration("$p", Person.class), "{ modify($p) { setAge(1); }; }", - "{ { $p.setAge(1); } }", + "{ { $p.setAge(1); } drools.update($p);}", result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); } @@ -753,7 +807,7 @@ public void testModifySemiColon() { public void testModifyWithAssignment() { test(ctx -> ctx.addDeclaration("$p", Person.class), "{ modify($p) { age = $p.age+1 }; }", - "{ { $p.setAge($p.getAge() + 1); } }", + "{ { $p.setAge($p.getAge() + 1); } drools.update($p); }", result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); } @@ -761,33 +815,10 @@ public void testModifyWithAssignment() { public void testModifyWithMethodCall() { test(ctx -> ctx.addDeclaration("$p", Person.class), "{ modify($p) { addresses.clear() }; }", - "{ { $p.getAddresses().clear(); } }", + "{ { $p.getAddresses().clear(); } drools.update($p);}", result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); } - @Test - public void testWithSemiColon() { - test("{ with( $l = new ArrayList()) { $l.add(2); }; }", - "{ java.util.ArrayList $l = new java.util.ArrayList(); $l.add(2); }", - result -> assertThat(allUsedBindings(result)).isEmpty()); - } - - @Test - public void testWithWithAssignment() { - test(ctx -> ctx.addDeclaration("$p", Person.class), - "{ with($p = new Person()) { age = $p.age+1 }; }", - "{ org.drools.Person $p = new org.drools.Person(); $p.setAge($p.getAge() + 1); }", - result -> assertThat(allUsedBindings(result)).isEmpty()); - } - - @Test - public void testWithInIf() { - test(ctx -> ctx.addDeclaration("$p", Person.class), - "{ if (true) { with($p = new Person()) { age = $p.age+1 }; } }", - "{ if (true) { org.drools.Person $p = new org.drools.Person(); $p.setAge($p.getAge() + 1); } }", - result -> assertThat(allUsedBindings(result)).isEmpty()); - } - @Test public void testAddCastToMapGet() { test(ctx -> ctx.addDeclaration("$map", Map.class), @@ -830,35 +861,12 @@ public void testModifyInsideIfBlock() { " {\n" + " $p.setName(\"without_parent\");\n" + " }\n" + + " drools.update($p);\n" + " } " + "}", result -> assertThat(allUsedBindings(result)).containsExactlyInAnyOrder("$p")); } - @Test - public void testWithOrdering() { - test(ctx -> ctx.addDeclaration("$p", Person.class), - "{ " + - " with( s0 = new Person() ) {\n" + - " age = 0\n" + - " }\n" + - " insertLogical(s0);\n" + - " with( s1 = new Person() ) {\n" + - " age = 1\n" + - " }\n" + - " insertLogical(s1);\n " + - " }", - - "{ " + - "org.drools.Person s0 = new org.drools.Person(); " + - "s0.setAge(0); " + - "insertLogical(s0);\n" + - "org.drools.Person s1 = new org.drools.Person(); " + - "s1.setAge(1);\n" + - "insertLogical(s1);\n" + - "}"); - } - @Test public void testModifyOrdering() { test(ctx -> ctx.addDeclaration("$person", Person.class), @@ -878,6 +886,7 @@ public void testModifyOrdering() { "{ " + " $person.setAddress($newAddress);\n" + "}\n" + + "drools.update($person);\n" + "}"); } diff --git a/drools-mvel/src/main/java/org/drools/mvel/asm/AsmUtil.java b/drools-mvel/src/main/java/org/drools/mvel/asm/AsmUtil.java index da91702f5ed..db7b4e72f16 100644 --- a/drools-mvel/src/main/java/org/drools/mvel/asm/AsmUtil.java +++ b/drools-mvel/src/main/java/org/drools/mvel/asm/AsmUtil.java @@ -268,7 +268,7 @@ private static void rewriteDescr(final RuleBuildContext context, switch (d.getType()) { case MODIFY: - rewriteModifyDescr(context, d, originalBlock, consequence, declr, obj); + rewriteModifyDescr(context, d, analysis, originalBlock, consequence, declr, obj); break; case UPDATE: rewriteUpdateDescr(context, d, analysis, consequence, declr, obj); @@ -281,6 +281,7 @@ private static void rewriteDescr(final RuleBuildContext context, private static void rewriteModifyDescr( RuleBuildContext context, JavaBlockDescr d, + JavaAnalysisResult analysis, String originalBlock, StringBuilder consequence, Declaration declr, @@ -301,7 +302,8 @@ private static void rewriteModifyDescr( RuleBuildContext context, } BitMask modificationMask = isPropertyReactive ? getEmptyPropertyReactiveMask(settableProperties.size()) : allSetButTraitBitMask(); if (isPropertyReactive) { - modificationMask = getModificationMask( consequence, obj, modificationMask, typeDeclaration, settableProperties, statement, false ); + // collect modification outside modify block + modificationMask = getModificationMask( analysis.getAnalyzedExpr(), obj, modificationMask, typeDeclaration, settableProperties, statement, false ); } int end = originalBlock.indexOf("{"); @@ -327,6 +329,7 @@ private static void rewriteModifyDescr( RuleBuildContext context, start = end + exprStr.length(); if (typeDeclaration != null) { + // collect modification inside modify block modificationMask = parseModifiedProperties(statement, settableProperties, typeDeclaration, isPropertyReactive, modificationMask, exprStr); } } @@ -365,17 +368,17 @@ private static void rewriteUpdateDescr( RuleBuildContext context, context.getRule().getConsequenceMetaData().addStatement(statement); if (isPropertyReactive) { - modificationMask = getModificationMask( consequence, obj, modificationMask, typeDeclaration, settableProperties, statement, true ); + modificationMask = getModificationMask( analysis.getAnalyzedExpr(), obj, modificationMask, typeDeclaration, settableProperties, statement, true ); } } appendUpdateStatement(consequence, declr, obj, modificationMask, typeClass); } - private static BitMask getModificationMask( StringBuilder consequence, String obj, BitMask modificationMask, TypeDeclaration typeDeclaration, List settableProperties, ConsequenceMetaData.Statement statement, boolean isUpdate ) { + private static BitMask getModificationMask( String originalConsequence, String obj, BitMask modificationMask, TypeDeclaration typeDeclaration, List settableProperties, ConsequenceMetaData.Statement statement, boolean isUpdate ) { boolean parsedExprOnce = false; // a late optimization to include this for-loop within this if - for (String expr : splitStatementsAcrossBlocks( consequence )) { + for (String expr : splitStatementsAcrossBlocks( originalConsequence )) { String updateExpr = expr.replaceFirst("^\\Q" + obj + "\\E\\s*\\.", ""); if (!updateExpr.equals(expr)) { parsedExprOnce = true; diff --git a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/RuleChainingTest.java b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/RuleChainingTest.java index 5c49d60e1c0..61cd9656b92 100644 --- a/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/RuleChainingTest.java +++ b/drools-test-coverage/test-compiler-integration/src/test/java/org/drools/compiler/integrationtests/RuleChainingTest.java @@ -68,13 +68,11 @@ public void testRuleChainingWithLogicalInserts() { " dialect \"mvel\"\n" + " when\n" + " then\n" + - " with( s0 = new Some() ) {\n" + - " field = 0\n" + - " }\n" + + " Some s0 = new Some();\n" + + " s0.field = 0;\n" + " insertLogical(s0);\n" + - " with( s1 = new Some() ) {\n" + - " field = 1\n" + - " }\n" + + " Some s1 = new Some();\n" + + " s1.field = 1;\n" + " insertLogical(s1);\n" + "end\n" + "\n" +