Copilot commented on code in PR #6556:
URL:
https://github.com/apache/incubator-kie-drools/pull/6556#discussion_r2711363419
##########
kie-dmn/kie-dmn-legacy-tests/src/test/java/org/kie/dmn/legacy/tests/core/v1_1/DMNDecisionTableRuntimeTest.java:
##########
@@ -163,7 +163,17 @@ void decisionTableInvalidInputErrorMessage(VariantTestConf
conf) {
context.set( "Branches dispersion", "Province" );
context.set( "Number of Branches", BigDecimal.valueOf( 10 ) );
- testDecisionTableInvalidInput( context );
+ final DMNRuntime runtime = DMNRuntimeUtil.createRuntime(
"InvalidInput.dmn", this.getClass() );
+ final DMNModel dmnModel = runtime.getModel(
"http://www.trisotech.com/dmn/definitions/_cdf29af2-959b-4004-8271-82a9f5a62147";,
"Dessin 1" );
+ assertThat(dmnModel).isNotNull();
+
+ final DMNResult dmnResult = runtime.evaluateAll( dmnModel, context );
+ // In lenient mode, invalid input value is set to null and evaluation
continues
+ assertThat( dmnResult.hasErrors()).isFalse();
+
+ final DMNContext result = dmnResult.getContext();
+ // A rule matches with null parameter, so result is defined
+ assertThat(result.isDefined( "Branches
distribution")).isEqualTo(Boolean.TRUE);
Review Comment:
The comment says "In lenient mode, invalid input value is set to null and
evaluation continues" but there's no explicit test verification that the input
was actually set to null. The test only checks that a result is defined and no
errors occurred. Consider adding an assertion to verify the actual mechanism -
that when an invalid input like "Province" is provided (which doesn't match the
allowed values), the decision table receives null for that parameter and
matches the appropriate rule.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/impl/FEELImpl.java:
##########
@@ -168,16 +168,16 @@ public Object evaluate(CompiledExpression expr,
EvaluationContext ctx) {
/**
* Creates a new EvaluationContext using this FEEL instance classloader,
and the supplied parameters listeners and inputVariables
*/
Review Comment:
This method signature change breaks backward compatibility. The original
method `newEvaluationContext(Collection<FEELEventListener> listeners,
Map<String, Object> inputVariables)` has been changed to add a third parameter
`String runtimeMode`. This is a breaking API change that could affect external
consumers of this library. Consider adding a new overloaded method instead to
maintain backward compatibility.
```suggestion
*/
public EvaluationContextImpl
newEvaluationContext(Collection<FEELEventListener> listeners, Map<String,
Object> inputVariables) {
return newEvaluationContext(listeners, inputVariables, null);
}
/**
* Creates a new EvaluationContext using this FEEL instance classloader,
and the supplied parameters listeners and inputVariables
*/
```
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/ast/DMNDTExpressionEvaluator.java:
##########
@@ -26,13 +26,12 @@
import org.kie.dmn.api.core.DMNMessage;
import org.kie.dmn.api.core.DMNResult;
+import org.kie.dmn.api.core.EvaluatorResult;
+import org.kie.dmn.api.core.EvaluatorResult.ResultType;
import org.kie.dmn.api.core.ast.DMNNode;
-import org.kie.dmn.api.core.ast.DecisionNode;
import org.kie.dmn.api.core.event.DMNRuntimeEventManager;
import org.kie.dmn.api.feel.runtime.events.FEELEvent;
import org.kie.dmn.core.api.DMNExpressionEvaluator;
-import org.kie.dmn.api.core.EvaluatorResult;
-import org.kie.dmn.api.core.EvaluatorResult.ResultType;
import org.kie.dmn.core.impl.DMNResultImpl;
Review Comment:
The imports have been reorganized with EvaluatorResult and its nested class
ResultType moved and DecisionNode import removed. While the removed import for
DecisionNode appears to be unused in the visible diff, ensure it's not used
elsewhere in the file. The reordering of imports is cosmetic but consider
maintaining consistent alphabetical ordering.
```suggestion
```
##########
kie-dmn/kie-dmn-legacy-tests/src/test/java/org/kie/dmn/legacy/tests/core/v1_1/DMNDecisionTableHitPolicyTest.java:
##########
@@ -67,8 +67,9 @@ void
simpleDecisionTableHitPolicyUniqueSatisfies(VariantTestConf conf) {
final DMNResult dmnResult = runtime.evaluateAll(dmnModel, context);
final DMNContext result = dmnResult.getContext();
- assertThat(result.get("Approval Status")).isNull();
- assertThat(dmnResult.getMessages()).hasSizeGreaterThan(0);
+ // In lenient mode (default), when input doesn't match constraints,
it's set to null and evaluation continues
+ // Rule 4 matches: Age=any(-), RiskCategory=any(-), isAffordable=false
-> "Declined"
+ assertThat(result.get("Approval Status")).isEqualTo("Declined");
Review Comment:
The comment says "Rule 4 matches: Age=any(-), RiskCategory=any(-),
isAffordable=false -> Declined" but there's no verification that this is
actually the rule that matched. The test only checks the final result. Consider
adding assertions about which rules were matched or evaluated to ensure the
lenient behavior is working as documented in the comment.
##########
kie-dmn/kie-dmn-core/src/test/java/org/kie/dmn/core/DMNDecisionTableHitPolicyTest.java:
##########
@@ -66,14 +66,15 @@ void simpleDecisionTableHitPolicyUniqueSatisfies(boolean
useExecModelCompiler) {
assertThat(dmnModel).isNotNull();
// Risk Category is constrained to "High", "Low", "Medium" and "ASD"
is not allowed
+ // In lenient mode (default), invalid input is set to null and
evaluation continues
final DMNContext context =
getSimpleTableContext(BigDecimal.valueOf(18), "ASD", false);
final DMNResult dmnResult = runtime.evaluateAll(dmnModel, context);
final DMNContext result = dmnResult.getContext();
- assertThat(result.get("Approval Status")).isNull();
- assertThat(dmnResult.getMessages()).hasSizeGreaterThan(0);
- DMNMessage message = dmnResult.getMessages().iterator().next();
- assertThat(message.getText()).isEqualTo("DMN: RiskCategory='ASD' does
not match any of the valid values \"High\", \"Low\", \"Medium\" for decision
table '_0004-simpletable-U'. (DMN id: _0004-simpletable-U, FEEL expression
evaluation error) ");
+ // In lenient mode, the decision table evaluates with null parameter
and returns a result
+ assertThat(result.get("Approval Status")).isEqualTo("Declined");
+ // No error messages in lenient mode
+ assertThat(dmnResult.getMessages()).isEmpty();
Review Comment:
The previous implementation had assertions checking for error messages
including a specific error message text on line 77 (removed). The new
implementation expects NO messages at all. However, there's no separate test to
verify that strict mode still produces the appropriate error messages. Consider
adding a test that explicitly sets RuntimeModeOption to STRICT and verifies
that the error message "RiskCategory='ASD' does not match any of the valid
values" is still produced for backwards compatibility.
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/ast/DMNLiteralExpressionEvaluator.java:
##########
@@ -26,14 +26,15 @@
import org.kie.dmn.api.core.DMNMessage;
import org.kie.dmn.api.core.DMNResult;
+import org.kie.dmn.api.core.EvaluatorResult;
+import org.kie.dmn.api.core.EvaluatorResult.ResultType;
Review Comment:
The imports have been reorganized with EvaluatorResult.ResultType moved
before EvaluatorResult. Consider maintaining consistent alphabetical ordering
for imports, which typically places nested classes after their parent class or
imports them together.
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/compiler/DMNFEELHelper.java:
##########
@@ -65,6 +63,9 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.github.javaparser.ast.CompilationUnit;
+import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration;
Review Comment:
The imports have been reorganized, moving com.github.javaparser imports
after org.slf4j imports. While this doesn't affect functionality, it violates
the typical Java import ordering convention where third-party library imports
should be grouped together and separated from application-specific imports. The
standard ordering is: java.*, javax.*, then third-party libraries
(alphabetically), then application packages.
##########
kie-dmn/kie-dmn-core/src/test/java/org/kie/dmn/core/DMNDecisionTableRuntimeTest.java:
##########
@@ -208,18 +208,51 @@ void decisionTableNonexistingInputErrorMessage(boolean
useExecModelCompiler) {
context.set( "Not exists", "Province" );
context.set( "Number of Branches", BigDecimal.valueOf( 10 ) );
- testDecisionTableInvalidInput( context );
+ testDecisionTableMissingDependency( context );
}
- private void testDecisionTableInvalidInput(final DMNContext inputContext) {
+ private void testDecisionTableInvalidInputLenient(final DMNContext
inputContext) {
final DMNRuntime runtime = DMNRuntimeUtil.createRuntime(
"InvalidInput.dmn", this.getClass() );
final DMNModel dmnModel = runtime.getModel(
"http://www.trisotech.com/dmn/definitions/_cdf29af2-959b-4004-8271-82a9f5a62147";,
"Dessin 1" );
assertThat(dmnModel).isNotNull();
final DMNResult dmnResult = runtime.evaluateAll( dmnModel,
inputContext );
+ // In lenient mode (default), invalid input values don't generate
errors
+ // The invalid value is set to null and evaluation continues
+ assertThat( dmnResult.hasErrors()).isFalse();
+
+ final DMNContext result = dmnResult.getContext();
+ // Result is defined - evaluation continues with null for invalid input
+ assertThat( result.isDefined( "Branches distribution"
)).isEqualTo(Boolean.TRUE);
+ }
+
+ private void testDecisionTableInvalidInputType(final DMNContext
inputContext) {
+ final DMNRuntime runtime = DMNRuntimeUtil.createRuntime(
"InvalidInput.dmn", this.getClass() );
+ final DMNModel dmnModel = runtime.getModel(
"http://www.trisotech.com/dmn/definitions/_cdf29af2-959b-4004-8271-82a9f5a62147";,
"Dessin 1" );
+ assertThat(dmnModel).isNotNull();
+
+ final DMNResult dmnResult = runtime.evaluateAll( dmnModel,
inputContext );
+ // Type errors generate errors regardless of lenient/strict mode
+ // Wrong type (integer instead of string) is a type mismatch error
+ assertThat( dmnResult.hasErrors()).isTrue();
+
+ final DMNContext result = dmnResult.getContext();
+ // With type error, the result cannot be evaluated
+ assertThat( result.isDefined( "Branches distribution"
)).isEqualTo(Boolean.FALSE);
+ }
+
+ private void testDecisionTableMissingDependency(final DMNContext
inputContext) {
+ final DMNRuntime runtime = DMNRuntimeUtil.createRuntime(
"InvalidInput.dmn", this.getClass() );
+ final DMNModel dmnModel = runtime.getModel(
"http://www.trisotech.com/dmn/definitions/_cdf29af2-959b-4004-8271-82a9f5a62147";,
"Dessin 1" );
+ assertThat(dmnModel).isNotNull();
+
+ final DMNResult dmnResult = runtime.evaluateAll( dmnModel,
inputContext );
+ // Missing required dependency generates errors regardless of
lenient/strict mode
+ // This is a structural error, not an input validation error
assertThat( dmnResult.hasErrors()).isTrue();
final DMNContext result = dmnResult.getContext();
+ // With missing required dependency, the result cannot be evaluated
assertThat( result.isDefined( "Branches distribution"
)).isEqualTo(Boolean.FALSE);
}
Review Comment:
While this test method has been renamed from testDecisionTableInvalidInput
to testDecisionTableInvalidInputLenient and now tests lenient mode behavior,
there are no corresponding tests for strict mode behavior. The comments on
lines 220-221 and 235-236 mention that type errors and missing dependencies
generate errors "regardless of lenient/strict mode," but this claim is not
verified by tests in strict mode. Consider adding test methods that explicitly
test strict mode behavior to ensure the mode differentiation is working
correctly.
##########
kie-dmn/kie-dmn-legacy-tests/src/test/java/org/kie/dmn/legacy/tests/core/v1_1/DMNDecisionTableRuntimeTest.java:
##########
@@ -163,7 +163,17 @@ void decisionTableInvalidInputErrorMessage(VariantTestConf
conf) {
context.set( "Branches dispersion", "Province" );
context.set( "Number of Branches", BigDecimal.valueOf( 10 ) );
- testDecisionTableInvalidInput( context );
+ final DMNRuntime runtime = DMNRuntimeUtil.createRuntime(
"InvalidInput.dmn", this.getClass() );
+ final DMNModel dmnModel = runtime.getModel(
"http://www.trisotech.com/dmn/definitions/_cdf29af2-959b-4004-8271-82a9f5a62147";,
"Dessin 1" );
+ assertThat(dmnModel).isNotNull();
+
+ final DMNResult dmnResult = runtime.evaluateAll( dmnModel, context );
+ // In lenient mode, invalid input value is set to null and evaluation
continues
+ assertThat( dmnResult.hasErrors()).isFalse();
+
+ final DMNContext result = dmnResult.getContext();
+ // A rule matches with null parameter, so result is defined
+ assertThat(result.isDefined( "Branches
distribution")).isEqualTo(Boolean.TRUE);
Review Comment:
The removed lines are testing for error conditions and messages when an
invalid input is provided. However, the new implementation (lines 166-176) no
longer verifies the error message content. The test now expects NO errors in
lenient mode, but it doesn't verify that the correct behavior occurs (i.e.,
that the invalid value is properly set to null and the right rule matches).
Consider adding explicit assertions about what rule matched and why, to ensure
the lenient behavior is working correctly.
##########
kie-dmn/kie-dmn-legacy-tests/src/test/java/org/kie/dmn/legacy/tests/core/v1_1/DMNDecisionTableHitPolicyTest.java:
##########
@@ -67,8 +67,9 @@ void
simpleDecisionTableHitPolicyUniqueSatisfies(VariantTestConf conf) {
final DMNResult dmnResult = runtime.evaluateAll(dmnModel, context);
final DMNContext result = dmnResult.getContext();
- assertThat(result.get("Approval Status")).isNull();
- assertThat(dmnResult.getMessages()).hasSizeGreaterThan(0);
+ // In lenient mode (default), when input doesn't match constraints,
it's set to null and evaluation continues
+ // Rule 4 matches: Age=any(-), RiskCategory=any(-), isAffordable=false
-> "Declined"
+ assertThat(result.get("Approval Status")).isEqualTo("Declined");
}
Review Comment:
The assertion on line 72 has been removed without replacement, and line 77
now expects an empty messages list instead of checking for messages with size
greater than zero. However, there's no test to verify that the deprecated/old
strict mode behavior still works correctly. Consider adding a separate test
that explicitly sets strict mode and verifies the original error behavior is
preserved for backwards compatibility.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/impl/FEELImpl.java:
##########
@@ -168,16 +168,16 @@ public Object evaluate(CompiledExpression expr,
EvaluationContext ctx) {
/**
* Creates a new EvaluationContext using this FEEL instance classloader,
and the supplied parameters listeners and inputVariables
*/
- public EvaluationContextImpl
newEvaluationContext(Collection<FEELEventListener> listeners, Map<String,
Object> inputVariables) {
- return newEvaluationContext(this.classLoader, listeners,
inputVariables);
+ public EvaluationContextImpl
newEvaluationContext(Collection<FEELEventListener> listeners, Map<String,
Object> inputVariables, String runtimeMode ) {
+ return newEvaluationContext(this.classLoader, listeners,
inputVariables, runtimeMode);
}
/**
Review Comment:
This method signature change breaks backward compatibility. The original
method `newEvaluationContext(ClassLoader cl, Collection<FEELEventListener>
listeners, Map<String, Object> inputVariables)` has been changed to add a
fourth parameter `String runtimeMode`. This is a breaking API change that could
affect external consumers of this library. Consider adding a new overloaded
method instead to maintain backward compatibility.
```suggestion
/**
* Creates a new EvaluationContext with the supplied classloader, and
the supplied parameters listeners and inputVariables.
* This overload is kept for backward compatibility and defaults the
runtimeMode to null.
*/
public EvaluationContextImpl newEvaluationContext(ClassLoader cl,
Collection<FEELEventListener> listeners, Map<String, Object> inputVariables) {
return newEvaluationContext(cl, listeners, inputVariables, null);
}
/**
```
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/ast/DMNInvocationEvaluator.java:
##########
@@ -26,17 +26,18 @@
import javax.xml.namespace.QName;
import org.kie.dmn.api.core.DMNContext;
+import org.kie.dmn.api.core.DMNMessage;
import org.kie.dmn.api.core.DMNResult;
import org.kie.dmn.api.core.DMNType;
-import org.kie.dmn.api.core.EvaluatorResult;
-import org.kie.dmn.api.core.DMNMessage;
import org.kie.dmn.api.core.DMNVersion;
+import org.kie.dmn.api.core.EvaluatorResult;
+import org.kie.dmn.api.core.EvaluatorResult.ResultType;
Review Comment:
The imports have been reorganized to group EvaluatorResult and its nested
class together. While functionally equivalent, the previous ordering where
EvaluatorResult was imported before DMNMessage was already sorted. Consider
maintaining consistent import ordering according to the standard Java
convention (alphabetical by package and class name).
##########
kie-dmn/kie-dmn-core/src/test/java/org/kie/dmn/core/DMNDecisionTableHitPolicyTest.java:
##########
@@ -66,14 +66,15 @@ void simpleDecisionTableHitPolicyUniqueSatisfies(boolean
useExecModelCompiler) {
assertThat(dmnModel).isNotNull();
// Risk Category is constrained to "High", "Low", "Medium" and "ASD"
is not allowed
+ // In lenient mode (default), invalid input is set to null and
evaluation continues
final DMNContext context =
getSimpleTableContext(BigDecimal.valueOf(18), "ASD", false);
final DMNResult dmnResult = runtime.evaluateAll(dmnModel, context);
final DMNContext result = dmnResult.getContext();
- assertThat(result.get("Approval Status")).isNull();
- assertThat(dmnResult.getMessages()).hasSizeGreaterThan(0);
- DMNMessage message = dmnResult.getMessages().iterator().next();
- assertThat(message.getText()).isEqualTo("DMN: RiskCategory='ASD' does
not match any of the valid values \"High\", \"Low\", \"Medium\" for decision
table '_0004-simpletable-U'. (DMN id: _0004-simpletable-U, FEEL expression
evaluation error) ");
+ // In lenient mode, the decision table evaluates with null parameter
and returns a result
+ assertThat(result.get("Approval Status")).isEqualTo("Declined");
Review Comment:
Documentation comment says "In lenient mode (default), invalid input is set
to null and evaluation continues" but there's no explicit assertion that
verifies the input was actually set to null. The test only checks the final
output. Consider adding verification that when an invalid RiskCategory value
like "ASD" is provided, it actually gets transformed to null during evaluation,
to ensure the documented mechanism is working correctly.
--
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]
