From 6ad83805154d4b79b3d50dc172ba08409e4e6e3a Mon Sep 17 00:00:00 2001 From: Toshiya Kobayashi Date: Wed, 12 Jul 2023 15:46:09 +0900 Subject: [PATCH] [DROOLS-7492] Property reactivity inconsistent behavior when modify-block is placed inside a block like if-block in RHS (#5370) (#5384) --- .../org/drools/core/util/StringUtils.java | 9 + .../org/drools/core/util/StringUtilsTest.java | 85 ++++++++ .../PropertyReactivityMatrixTest.java | 195 ++++++++++++++++++ .../java/org/drools/mvel/asm/AsmUtil.java | 6 +- .../mvel/builder/MVELConsequenceBuilder.java | 3 +- 5 files changed, 294 insertions(+), 4 deletions(-) create mode 100644 drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/PropertyReactivityMatrixTest.java diff --git a/drools-core/src/main/java/org/drools/core/util/StringUtils.java b/drools-core/src/main/java/org/drools/core/util/StringUtils.java index a7bce173081..78794ddb1be 100644 --- a/drools-core/src/main/java/org/drools/core/util/StringUtils.java +++ b/drools-core/src/main/java/org/drools/core/util/StringUtils.java @@ -32,6 +32,7 @@ import java.util.Iterator; import java.util.List; import java.util.UUID; +import java.util.stream.Collectors; import org.kie.api.builder.ReleaseId; @@ -1007,6 +1008,14 @@ public static List splitStatements(CharSequence string) { return codeAwareSplitOnChar(string, true, ';', '\n'); } + public static List splitStatementsAcrossBlocks(CharSequence string) { + List statements = codeAwareSplitOnChar(string, true, ';', '\n', '{', '}'); + return statements.stream() + .filter(stmt -> !(stmt.isEmpty())) + .filter(stmt -> !(stmt.startsWith("//"))) + .collect(Collectors.toList()); + } + public static List splitArgumentsList(CharSequence string) { return splitArgumentsList(string, true); } diff --git a/drools-core/src/test/java/org/drools/core/util/StringUtilsTest.java b/drools-core/src/test/java/org/drools/core/util/StringUtilsTest.java index 0d8df12983c..60edd44f677 100644 --- a/drools-core/src/test/java/org/drools/core/util/StringUtilsTest.java +++ b/drools-core/src/test/java/org/drools/core/util/StringUtilsTest.java @@ -25,6 +25,7 @@ import static org.drools.core.util.StringUtils.indexOfOutOfQuotes; import static org.drools.core.util.StringUtils.md5Hash; import static org.drools.core.util.StringUtils.splitStatements; +import static org.drools.core.util.StringUtils.splitStatementsAcrossBlocks; public class StringUtilsTest { @@ -309,4 +310,88 @@ public void testSplitStatements() { assertThat(statements.get(1)).isEqualTo("$visaApplication.setValidation( Validation.FAILED )"); assertThat(statements.get(2)).isEqualTo("drools.update($visaApplication)"); } + + @Test + public void splitStatementsAcrossBlocksIf() { + String text = "if (true) {\n" + + " $fact.value1 = 2;\n" + + " drools.update($fact);\n" + + "}"; + List statements = splitStatementsAcrossBlocks(text); + assertThat(statements.get(0)).isEqualTo("if (true)"); + assertThat(statements.get(1)).isEqualTo("$fact.value1 = 2"); + assertThat(statements.get(2)).isEqualTo("drools.update($fact)"); + } + + @Test + public void splitStatementsAcrossBlocksDoWhile() { + String text = "do {\n" + + " $fact.value1 = 2;\n" + + " drools.update($fact);\n" + + "} while (false);"; + List statements = splitStatementsAcrossBlocks(text); + assertThat(statements.get(0)).isEqualTo("do"); + assertThat(statements.get(1)).isEqualTo("$fact.value1 = 2"); + assertThat(statements.get(2)).isEqualTo("drools.update($fact)"); + assertThat(statements.get(3)).isEqualTo("while (false)"); + } + + @Test + public void splitStatementsAcrossBlocksFor() { + String text = "for (int i = 0; i < 1; i++) {\n" + + " $fact.value1 = 2;\n" + + " drools.update($fact);\n" + + "}"; + List statements = splitStatementsAcrossBlocks(text); + assertThat(statements.get(0)).isEqualTo("for (int i = 0; i < 1; i++)"); + assertThat(statements.get(1)).isEqualTo("$fact.value1 = 2"); + assertThat(statements.get(2)).isEqualTo("drools.update($fact)"); + } + + @Test + public void splitStatementsAcrossBlocksCommentedIf() { + String text = "// if (true) {\n" + + " $fact.value1 = 2;\n" + + " drools.update($fact);\n" + + "// }"; + List statements = splitStatementsAcrossBlocks(text); + assertThat(statements.get(0)).isEqualTo("$fact.value1 = 2"); + assertThat(statements.get(1)).isEqualTo("drools.update($fact)"); + } + + @Test + public void splitStatementsAcrossBlocksCommentedIfMissingStartingBrace() { + String text = "// if (true)\n" + + " $fact.value1 = 2;\n" + + " drools.update($fact);\n" + + "// }"; + List statements = splitStatementsAcrossBlocks(text); + assertThat(statements.get(0)).isEqualTo("$fact.value1 = 2"); + assertThat(statements.get(1)).isEqualTo("drools.update($fact)"); + } + + @Test + public void splitStatementsAcrossBlocksCommentedIfMissingEndingBrace() { + String text = "// if (true) {\n" + + " $fact.value1 = 2;\n" + + " drools.update($fact);\n" + + "//"; + List statements = splitStatementsAcrossBlocks(text); + assertThat(statements.get(0)).isEqualTo("$fact.value1 = 2"); + assertThat(statements.get(1)).isEqualTo("drools.update($fact)"); + } + + @Test + public void splitStatementsAcrossBlocksIfInsideAndOutsideAssginment() { + String text = "$fact.value1 = 2;\n" + + "if (true) {\n" + + " $fact.value2 = 2;\n" + + " drools.update($fact);\n" + + "}"; + List statements = splitStatementsAcrossBlocks(text); + assertThat(statements.get(0)).isEqualTo("$fact.value1 = 2"); + assertThat(statements.get(1)).isEqualTo("if (true)"); + assertThat(statements.get(2)).isEqualTo("$fact.value2 = 2"); + assertThat(statements.get(3)).isEqualTo("drools.update($fact)"); + } } 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 new file mode 100644 index 00000000000..c18e87024e8 --- /dev/null +++ b/drools-model/drools-model-compiler/src/test/java/org/drools/modelcompiler/PropertyReactivityMatrixTest.java @@ -0,0 +1,195 @@ +/* + * Copyright 2023 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; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; + +import org.drools.core.util.StringUtils; +import org.junit.Test; +import org.junit.runners.Parameterized.Parameters; +import org.kie.api.runtime.KieSession; + +import static org.assertj.core.api.Assertions.assertThat; + +public class PropertyReactivityMatrixTest extends BaseModelTest { + + enum Dialect { + JAVA, + MVEL + } + + private Dialect dialect; + + @Parameters(name = "{0} {1}") + public static Collection parameters() { + List parameterData = new ArrayList(); + for (Object runType : PLAIN) { + for (Dialect dialect : Dialect.values()) { + parameterData.add(new Object[]{runType, dialect}); + } + } + return parameterData; + } + + public PropertyReactivityMatrixTest(RUN_TYPE testRunType, Dialect testDialect) { + super(testRunType); + this.dialect = testDialect; + } + + public static class Fact { + + private int value1; + private int value2; + + public Fact(int value1) { + this.value1 = value1; + } + + public Fact(int value1, int value2) { + this.value1 = value1; + this.value2 = value2; + } + + public int getValue1() { + return value1; + } + + public void setValue1(int value1) { + this.value1 = value1; + } + + public int getValue2() { + return value2; + } + + public void setValue2(int value2) { + this.value2 = value2; + } + + } + + private String setValueStatement(String property, int value) { + if (dialect == Dialect.JAVA) { + return "set" + StringUtils.ucFirst(property) + "(" + value + ");"; + } else { + return property + " = " + value + ";"; + } + } + + @Test + public void modifyInsideIfTrueBlock_shouldTriggerReactivity() { + statementInsideBlock_shouldTriggerReactivity(" if (true) {\n" + + " modify($fact) {\n" + + " " + setValueStatement("value1", 2) + "\n" + + " }\n" + + " }\n"); + } + + @Test + public void modifyInsideForBlock_shouldTriggerReactivity() { + statementInsideBlock_shouldTriggerReactivity(" for (int i = 0; i < 1; i++) {\n" + + " modify($fact) {\n" + + " " + setValueStatement("value1", 2) + "\n" + + " }\n" + + " }\n"); + } + + @Test + public void modifyInsideCommentedIfTrueBlock_shouldTriggerReactivity() { + statementInsideBlock_shouldTriggerReactivity(" // if (true) {\n" + + " modify($fact) {\n" + + " " + setValueStatement("value1", 2) + "\n" + + " }\n" + + " // }\n"); + } + + @Test + public void modifyInsideIfBlockInsideAndOutsideAssignment_shouldTriggerReactivity() { + statementInsideBlock_shouldTriggerReactivity(" $fact." + setValueStatement("value1", 2) + "\n" + + " if (true) {\n" + + " modify($fact) {\n" + + " " + setValueStatement("value2", 2) + "\n" + + " }\n" + + " }"); + } + + @Test + public void updateInsideIfTrueBlock_shouldTriggerReactivity() { + statementInsideBlock_shouldTriggerReactivity(" if (true) {\n" + + " $fact." + setValueStatement("value1", 2) + ";\n" + + " update($fact);\n" + + " }\n"); + } + + @Test + public void updateInsideForBlock_shouldTriggerReactivity() { + statementInsideBlock_shouldTriggerReactivity(" for (int i = 0; i < 1; i++) {\n" + + " $fact." + setValueStatement("value1", 2) + "\n" + + " update($fact);\n" + + " }\n"); + } + + @Test + public void updateInsideCommentedIfTrueBlock_shouldTriggerReactivity() { + statementInsideBlock_shouldTriggerReactivity(" // if (true) {\n" + + " $fact." + setValueStatement("value1", 2) + "\n" + + " update($fact);\n" + + " // }\n"); + } + + @Test + public void updateInsideIfBlockInsideAndOutsideAssignment_shouldTriggerReactivity() { + statementInsideBlock_shouldTriggerReactivity(" $fact." + setValueStatement("value1", 2) + "\n" + + " if (true) {\n" + + " $fact." + setValueStatement("value2", 2) + "\n" + + " update($fact);\n" + + " }"); + } + + private void statementInsideBlock_shouldTriggerReactivity(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); + ksession.fireAllRules(); + + assertThat(results).as("Should trigger property reactivity on value1") + .containsExactly("R2 fired"); + } +} 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 5103a0b9b6b..da91702f5ed 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 @@ -28,7 +28,6 @@ import org.drools.compiler.lang.descr.FunctionDescr; import org.drools.compiler.lang.descr.PackageDescr; import org.drools.compiler.rule.builder.RuleBuildContext; -import org.drools.mvel.java.JavaAnalysisResult; import org.drools.compiler.rule.builder.dialect.java.parser.JavaBlockDescr; import org.drools.compiler.rule.builder.dialect.java.parser.JavaInterfacePointsDescr; import org.drools.compiler.rule.builder.dialect.java.parser.JavaLocalDeclarationDescr; @@ -43,6 +42,7 @@ import org.drools.core.util.bitmask.BitMask; import org.drools.mvel.builder.MVELAnalysisResult; import org.drools.mvel.builder.MVELDialect; +import org.drools.mvel.java.JavaAnalysisResult; import org.kie.api.definition.type.FactField; import org.mvel2.CompileException; import org.mvel2.asm.MethodVisitor; @@ -58,7 +58,7 @@ import static org.drools.core.util.StringUtils.extractFirstIdentifier; import static org.drools.core.util.StringUtils.findEndOfMethodArgsIndex; import static org.drools.core.util.StringUtils.splitArgumentsList; -import static org.drools.core.util.StringUtils.splitStatements; +import static org.drools.core.util.StringUtils.splitStatementsAcrossBlocks; public final class AsmUtil { private static final Pattern LINE_BREAK_FINDER = Pattern.compile( "\\r\\n|\\r|\\n" ); @@ -375,7 +375,7 @@ private static void rewriteUpdateDescr( RuleBuildContext context, private static BitMask getModificationMask( StringBuilder consequence, 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 : splitStatements( consequence )) { + for (String expr : splitStatementsAcrossBlocks( consequence )) { String updateExpr = expr.replaceFirst("^\\Q" + obj + "\\E\\s*\\.", ""); if (!updateExpr.equals(expr)) { parsedExprOnce = true; diff --git a/drools-mvel/src/main/java/org/drools/mvel/builder/MVELConsequenceBuilder.java b/drools-mvel/src/main/java/org/drools/mvel/builder/MVELConsequenceBuilder.java index d316495911e..9bd3c9eda82 100644 --- a/drools-mvel/src/main/java/org/drools/mvel/builder/MVELConsequenceBuilder.java +++ b/drools-mvel/src/main/java/org/drools/mvel/builder/MVELConsequenceBuilder.java @@ -48,6 +48,7 @@ import static org.drools.core.util.StringUtils.findEndOfBlockIndex; import static org.drools.core.util.StringUtils.findEndOfMethodArgsIndex; import static org.drools.core.util.StringUtils.splitStatements; +import static org.drools.core.util.StringUtils.splitStatementsAcrossBlocks; import static org.drools.mvel.asm.AsmUtil.copyErrorLocation; public class MVELConsequenceBuilder @@ -272,7 +273,7 @@ public static String rewriteUpdates( Function> classResolver, F BitMask modificationMask = getEmptyPropertyReactiveMask(settableProperties.size()); boolean directAccess = false; - for (String expr : splitStatements(text)) { + for (String expr : splitStatementsAcrossBlocks(text)) { if (expr.startsWith( identifier + "." )) { int fieldEnd = identifier.length()+1; while (Character.isJavaIdentifierPart( expr.charAt( fieldEnd ) )) fieldEnd++;