Copilot commented on code in PR #7096:
URL: https://github.com/apache/incubator-kie/pull/7096#discussion_r3967018621
##########
drools-compiler/src/main/java/org/drools/compiler/rule/builder/PatternBuilder.java:
##########
@@ -1818,6 +1823,27 @@ protected ConstraintConnectiveDescr
parseExpression(final RuleBuildContext conte
return result;
}
+ static boolean containsTernaryOperator(String expr) {
+ for (int i = 0; i < expr.length(); i++) {
+ char c = expr.charAt(i);
+ if (c == '"' || c == '\'') {
+ char quote = c;
+ i++;
+ while (i < expr.length()) {
+ if (expr.charAt(i) == '\\') {
+ i++;
+ } else if (expr.charAt(i) == quote) {
+ break;
+ }
+ i++;
+ }
+ } else if (c == '?' && (i + 1 >= expr.length() || expr.charAt(i +
1) != '.')) {
+ return true;
+ }
+ }
+ return false;
+ }
+
Review Comment:
`containsTernaryOperator` currently returns `true` for any `?` outside
quotes (except `?.`), even when there is no `:` (e.g., `x?`). This can produce
false positives and skip `normalizeEval` for non-ternary expressions that
legitimately contain `?` (e.g., regex-like constructs outside quotes, or other
dialect-specific operators), changing parsing behavior unnecessarily. A more
accurate check should confirm a matching `:` exists after the `?` (outside
quotes) before returning `true`.
##########
drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/EvalTest.java:
##########
@@ -775,4 +775,86 @@ private void
testModifyEvalAfterJoinWithMatchingAlphaSharingJoin(RUN_TYPE runTyp
assertThat(list).as("R1 should not
fire").containsExactly("ModifyingRule");
}
}
+
+ @ParameterizedTest
+ @MethodSource("parametersStandardOnly") // exec-model doesn't support
ternary constraint
+ void testTernaryEvalInsidePattern(RUN_TYPE runType) {
+ String str =
+ "import " + MyPerson.class.getCanonicalName() + ";\n" +
+ "rule \"TernaryEvalLoop\"\n" +
+ "dialect \"mvel\"\n" +
+ "when\n" +
+ " String(this == \"go\")\n" +
+ " $p : MyPerson( eval(\"foo\" == \"foo\" ? \"foo\" ==
flag1 : \"foo\" == flag2) )\n" +
+ "then\n" +
+ " modify($p) {\n" +
+ " setOtherAttribute(\"done\")\n" +
+ " }\n" +
+ "end";
+
+ //-- 1st round
+
+ KieSession ksession = getKieSession(runType, str);
+
+ MyPerson person = new MyPerson();
+ person.setFlag1("foo"); // matches the rule
+ person.setFlag2("bar");
+
+ ksession.insert("go");
+ ksession.insert(person);
+ int fired = ksession.fireAllRules(10);
+
+ assertThat(fired).isEqualTo(1);
+ assertThat(person.getOtherAttribute()).isEqualTo("done");
+
+ ksession.dispose();
+
+ //-- 2nd round
+
+ ksession = getKieSession(runType, str);
+
+ person = new MyPerson();
+ person.setFlag1("bar"); // doesn't match the rule
+ person.setFlag2("bar");
+
+ ksession.insert("go");
+ ksession.insert(person);
+ fired = ksession.fireAllRules(10); // do not fire
+
+ assertThat(fired).isZero();
+ assertThat(person.getOtherAttribute()).isNull();
+
+ ksession.dispose();
Review Comment:
`KieSession.dispose()` is called at the end of each round, but if an
assertion fails earlier, the session may not be disposed, which can leak
resources and potentially interfere with subsequent tests in the same JVM. Wrap
each round in a `try/finally` to guarantee `dispose()` is always executed.
##########
drools-compiler/src/test/java/org/drools/compiler/rule/builder/PatternBuilderTest.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.compiler.rule.builder;
+
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class PatternBuilderTest {
+
+ @Test
+ void ternaryOperator() {
+ assertThat(PatternBuilder.containsTernaryOperator("\"foo\" == \"foo\"
? \"foo\" == flag1 : \"foo\" == flag2")).isTrue();
+ }
+
+ @Test
+ void simpleTernary() {
+ assertThat(PatternBuilder.containsTernaryOperator("x > 0 ? y :
z")).isTrue();
+ }
+
+ @Test
+ void noTernary() {
+ assertThat(PatternBuilder.containsTernaryOperator("age >
10")).isFalse();
+ }
+
+ @Test
+ void equalityExpression() {
+ assertThat(PatternBuilder.containsTernaryOperator("length ==
4")).isFalse();
+ }
+
+ @Test
+ void nullSafeOperator() {
+ assertThat(PatternBuilder.containsTernaryOperator("address?.city ==
\"London\"")).isFalse();
+ }
+
+ @Test
+ void questionMarkInsideStringLiteral() {
+ assertThat(PatternBuilder.containsTernaryOperator("name ==
\"what?\"")).isFalse();
+ }
+
+ @Test
+ void escapedQuoteBeforeTernary() {
+ assertThat(PatternBuilder.containsTernaryOperator("\"val\\\"ue\" ==
flag ? x : y")).isTrue();
+ }
+
+ @Test
+ void questionMarkInsideStringWithEscapedQuote() {
+ assertThat(PatternBuilder.containsTernaryOperator("name ==
\"is\\\"this?real\"")).isFalse();
+ }
+
+ @Test
+ void singleQuotedStringWithQuestionMark() {
+ assertThat(PatternBuilder.containsTernaryOperator("name ==
'what?'")).isFalse();
+ }
+
+ @Test
+ void ternaryAtEndOfExpression() {
+ assertThat(PatternBuilder.containsTernaryOperator("x?")).isTrue();
Review Comment:
This test asserts that `x?` is a ternary operator case, but `x?` is not a
ternary `?:` expression. If the intent is to validate ternary detection, the
input should include both `?` and `:` outside of literals (e.g., `x ? y : z`).
If the intent is to validate detection of any non-null-safe `?`, the
test/method naming should be updated accordingly to avoid encoding incorrect
semantics.
##########
drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/EvalTest.java:
##########
@@ -775,4 +775,86 @@ private void
testModifyEvalAfterJoinWithMatchingAlphaSharingJoin(RUN_TYPE runTyp
assertThat(list).as("R1 should not
fire").containsExactly("ModifyingRule");
}
}
+
+ @ParameterizedTest
+ @MethodSource("parametersStandardOnly") // exec-model doesn't support
ternary constraint
+ void testTernaryEvalInsidePattern(RUN_TYPE runType) {
+ String str =
+ "import " + MyPerson.class.getCanonicalName() + ";\n" +
+ "rule \"TernaryEvalLoop\"\n" +
+ "dialect \"mvel\"\n" +
+ "when\n" +
+ " String(this == \"go\")\n" +
+ " $p : MyPerson( eval(\"foo\" == \"foo\" ? \"foo\" ==
flag1 : \"foo\" == flag2) )\n" +
+ "then\n" +
+ " modify($p) {\n" +
+ " setOtherAttribute(\"done\")\n" +
+ " }\n" +
+ "end";
+
+ //-- 1st round
+
+ KieSession ksession = getKieSession(runType, str);
+
+ MyPerson person = new MyPerson();
+ person.setFlag1("foo"); // matches the rule
+ person.setFlag2("bar");
+
+ ksession.insert("go");
+ ksession.insert(person);
+ int fired = ksession.fireAllRules(10);
+
+ assertThat(fired).isEqualTo(1);
+ assertThat(person.getOtherAttribute()).isEqualTo("done");
+
+ ksession.dispose();
+
+ //-- 2nd round
+
+ ksession = getKieSession(runType, str);
+
+ person = new MyPerson();
+ person.setFlag1("bar"); // doesn't match the rule
+ person.setFlag2("bar");
+
+ ksession.insert("go");
+ ksession.insert(person);
+ fired = ksession.fireAllRules(10); // do not fire
+
+ assertThat(fired).isZero();
+ assertThat(person.getOtherAttribute()).isNull();
+
+ ksession.dispose();
Review Comment:
`KieSession.dispose()` is called at the end of each round, but if an
assertion fails earlier, the session may not be disposed, which can leak
resources and potentially interfere with subsequent tests in the same JVM. Wrap
each round in a `try/finally` to guarantee `dispose()` is always executed.
##########
drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/EvalTest.java:
##########
@@ -775,4 +775,86 @@ private void
testModifyEvalAfterJoinWithMatchingAlphaSharingJoin(RUN_TYPE runTyp
assertThat(list).as("R1 should not
fire").containsExactly("ModifyingRule");
}
}
+
+ @ParameterizedTest
+ @MethodSource("parametersStandardOnly") // exec-model doesn't support
ternary constraint
+ void testTernaryEvalInsidePattern(RUN_TYPE runType) {
+ String str =
+ "import " + MyPerson.class.getCanonicalName() + ";\n" +
+ "rule \"TernaryEvalLoop\"\n" +
+ "dialect \"mvel\"\n" +
+ "when\n" +
+ " String(this == \"go\")\n" +
+ " $p : MyPerson( eval(\"foo\" == \"foo\" ? \"foo\" ==
flag1 : \"foo\" == flag2) )\n" +
+ "then\n" +
+ " modify($p) {\n" +
+ " setOtherAttribute(\"done\")\n" +
+ " }\n" +
+ "end";
+
+ //-- 1st round
+
+ KieSession ksession = getKieSession(runType, str);
+
+ MyPerson person = new MyPerson();
+ person.setFlag1("foo"); // matches the rule
+ person.setFlag2("bar");
+
+ ksession.insert("go");
+ ksession.insert(person);
+ int fired = ksession.fireAllRules(10);
+
+ assertThat(fired).isEqualTo(1);
+ assertThat(person.getOtherAttribute()).isEqualTo("done");
+
+ ksession.dispose();
+
+ //-- 2nd round
+
+ ksession = getKieSession(runType, str);
+
+ person = new MyPerson();
+ person.setFlag1("bar"); // doesn't match the rule
+ person.setFlag2("bar");
+
+ ksession.insert("go");
+ ksession.insert(person);
+ fired = ksession.fireAllRules(10); // do not fire
+
+ assertThat(fired).isZero();
+ assertThat(person.getOtherAttribute()).isNull();
+
+ ksession.dispose();
Review Comment:
`KieSession.dispose()` is called at the end of each round, but if an
assertion fails earlier, the session may not be disposed, which can leak
resources and potentially interfere with subsequent tests in the same JVM. Wrap
each round in a `try/finally` to guarantee `dispose()` is always executed.
##########
drools-compiler/src/main/java/org/drools/compiler/rule/builder/PatternBuilder.java:
##########
@@ -1818,6 +1823,27 @@ protected ConstraintConnectiveDescr
parseExpression(final RuleBuildContext conte
return result;
}
+ static boolean containsTernaryOperator(String expr) {
Review Comment:
The helper name `containsTernaryOperator` is misleading given the current
implementation flags any standalone `?` (except `?.`) even without a `:`.
Either tighten the detection to actual ternary `?:` expressions, or rename the
method to reflect that it detects a non-null-safe question-mark operator (e.g.,
`containsQuestionMarkOperatorOutsideLiterals`).
##########
drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/EvalTest.java:
##########
@@ -775,4 +775,86 @@ private void
testModifyEvalAfterJoinWithMatchingAlphaSharingJoin(RUN_TYPE runTyp
assertThat(list).as("R1 should not
fire").containsExactly("ModifyingRule");
}
}
+
+ @ParameterizedTest
+ @MethodSource("parametersStandardOnly") // exec-model doesn't support
ternary constraint
+ void testTernaryEvalInsidePattern(RUN_TYPE runType) {
+ String str =
+ "import " + MyPerson.class.getCanonicalName() + ";\n" +
+ "rule \"TernaryEvalLoop\"\n" +
+ "dialect \"mvel\"\n" +
+ "when\n" +
+ " String(this == \"go\")\n" +
+ " $p : MyPerson( eval(\"foo\" == \"foo\" ? \"foo\" ==
flag1 : \"foo\" == flag2) )\n" +
+ "then\n" +
+ " modify($p) {\n" +
+ " setOtherAttribute(\"done\")\n" +
+ " }\n" +
+ "end";
+
+ //-- 1st round
+
+ KieSession ksession = getKieSession(runType, str);
+
+ MyPerson person = new MyPerson();
+ person.setFlag1("foo"); // matches the rule
+ person.setFlag2("bar");
+
+ ksession.insert("go");
+ ksession.insert(person);
+ int fired = ksession.fireAllRules(10);
+
+ assertThat(fired).isEqualTo(1);
+ assertThat(person.getOtherAttribute()).isEqualTo("done");
+
+ ksession.dispose();
+
+ //-- 2nd round
+
+ ksession = getKieSession(runType, str);
+
+ person = new MyPerson();
+ person.setFlag1("bar"); // doesn't match the rule
+ person.setFlag2("bar");
+
+ ksession.insert("go");
+ ksession.insert(person);
+ fired = ksession.fireAllRules(10); // do not fire
+
+ assertThat(fired).isZero();
+ assertThat(person.getOtherAttribute()).isNull();
+
+ ksession.dispose();
Review Comment:
`KieSession.dispose()` is called at the end of each round, but if an
assertion fails earlier, the session may not be disposed, which can leak
resources and potentially interfere with subsequent tests in the same JVM. Wrap
each round in a `try/finally` to guarantee `dispose()` is always executed.
##########
drools-model/drools-model-codegen/src/test/java/org/drools/model/codegen/execmodel/EvalTest.java:
##########
@@ -775,4 +775,86 @@ private void
testModifyEvalAfterJoinWithMatchingAlphaSharingJoin(RUN_TYPE runTyp
assertThat(list).as("R1 should not
fire").containsExactly("ModifyingRule");
}
}
+
+ @ParameterizedTest
+ @MethodSource("parametersStandardOnly") // exec-model doesn't support
ternary constraint
Review Comment:
The comment says 'exec-model doesn't support ternary constraint', but this
test uses a ternary inside `eval(...)` in a pattern. Consider clarifying the
wording to match the actual limitation (e.g., ternary in constraints vs ternary
inside eval, or whether it's a parser/codegen limitation) so future readers
understand why `parametersStandardOnly` is used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
