Skip to content

Commit

Permalink
[DROOLS-7492] Property reactivity inconsistent behavior when modify-b…
Browse files Browse the repository at this point in the history
…lock is placed inside a block like if-block in RHS (#5370) (#5384)
  • Loading branch information
tkobayas committed Jul 12, 2023
1 parent 630de7e commit 6ad8380
Show file tree
Hide file tree
Showing 5 changed files with 294 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -1007,6 +1008,14 @@ public static List<String> splitStatements(CharSequence string) {
return codeAwareSplitOnChar(string, true, ';', '\n');
}

public static List<String> splitStatementsAcrossBlocks(CharSequence string) {
List<String> statements = codeAwareSplitOnChar(string, true, ';', '\n', '{', '}');
return statements.stream()
.filter(stmt -> !(stmt.isEmpty()))
.filter(stmt -> !(stmt.startsWith("//")))
.collect(Collectors.toList());
}

public static List<String> splitArgumentsList(CharSequence string) {
return splitArgumentsList(string, true);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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<String> 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<String> 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<String> 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<String> 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<String> 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<String> 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<String> 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)");
}
}
Original file line number Diff line number Diff line change
@@ -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<Object[]> parameters() {
List<Object[]> parameterData = new ArrayList<Object[]>();
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<String> 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");
}
}
6 changes: 3 additions & 3 deletions drools-mvel/src/main/java/org/drools/mvel/asm/AsmUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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" );
Expand Down Expand Up @@ -375,7 +375,7 @@ private static void rewriteUpdateDescr( RuleBuildContext context,
private static BitMask getModificationMask( StringBuilder consequence, String obj, BitMask modificationMask, TypeDeclaration typeDeclaration, List<String> 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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -272,7 +273,7 @@ public static String rewriteUpdates( Function<String, Class<?>> 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++;
Expand Down

0 comments on commit 6ad8380

Please sign in to comment.