Copilot commented on code in PR #6387: URL: https://github.com/apache/incubator-kie-drools/pull/6387#discussion_r2789056234
########## kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/compiler/RuntimeModeOption.java: ########## @@ -0,0 +1,75 @@ +/* + * 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.kie.dmn.core.compiler; + +import java.io.Serial; +import org.kie.dmn.core.assembler.DMNAssemblerService; + +public class RuntimeModeOption implements DMNOption { + + public static final String PROPERTY_NAME = DMNAssemblerService.ORG_KIE_DMN_PREFIX + ".runtime.mode"; + + @Serial + private static final long serialVersionUID = -372562279892008329L; + + public enum MODE { + LENIENT("lenient"), + STRICT("strict"); + + private final String mode; + + MODE(String mode) { + this.mode = mode; + } + + public String getMode() { + return mode; + } + } + + /** + * The default <code>MODE</code> for this option + */ + public static final MODE DEFAULT_VALUE = MODE.LENIENT; + + private final MODE runtimeMode; + + public RuntimeModeOption(String runtimeMode) { + MODE toSet = null; + try { + toSet = MODE.valueOf(runtimeMode); + } catch (Exception e) { + toSet = DEFAULT_VALUE; Review Comment: `RuntimeModeOption(String)` uses `MODE.valueOf(runtimeMode)`, which only accepts enum constants ("STRICT"/"LENIENT"). With the documented system property values ("strict"/"lenient"), parsing falls into the exception path and silently defaults to LENIENT, so strict mode can never be enabled via property. Parse case-insensitively (e.g., normalize to upper-case) or map using `MODE.getMode()`. ```suggestion MODE toSet = DEFAULT_VALUE; if (runtimeMode != null) { for (MODE modeValue : MODE.values()) { if (modeValue.getMode().equalsIgnoreCase(runtimeMode) || modeValue.name().equalsIgnoreCase(runtimeMode)) { toSet = modeValue; break; } } ``` ########## kie-dmn/kie-dmn-core/src/test/java/org/kie/dmn/core/DMNInputRuntimeTest.java: ########## @@ -27,6 +27,7 @@ import java.util.Set; import java.util.stream.Collectors; +import org.junit.jupiter.api.Disabled; Review Comment: Unused import `org.junit.jupiter.api.Disabled` will cause compilation to fail (javac errors on unused imports). Remove it or actually use `@Disabled` on a test. ```suggestion ``` ########## kie-dmn/kie-dmn-core/src/test/java/org/kie/dmn/core/DMNInputRuntimeTest.java: ########## @@ -697,7 +700,138 @@ void multipleInvalidElements(boolean useExecModelCompiler) { retrievedResult = dmnResult.getDecisionResultById(decision2SourceId); assertThat(retrievedResult).isNotNull(); assertThat(retrievedResult.getEvaluationStatus()).isEqualTo(DMNDecisionResult.DecisionEvaluationStatus.FAILED); + } + + @ParameterizedTest + @MethodSource("params") + void errorHandlingWithDefaultMode(boolean useExecModelCompiler) { + init(useExecModelCompiler); + String nameSpace = "https://kie.org/dmn/_79591DB5-1EE1-4CBD-AA5D-2E3EDF31155E";; + final DMNRuntime runtime = DMNRuntimeUtil.createRuntime("invalid_models/DMNv1_6/DMN-MultipleInvalidElements" + + ".dmn", this.getClass()); + final DMNModel dmnModel = runtime.getModel( + nameSpace, + "DMN_8F7C4323-412A-4E0B-9AEF-0F24C8F55282"); + assertThat(dmnModel).isNotNull(); + assertThat(dmnModel.hasErrors()).as(DMNRuntimeUtil.formatMessages(dmnModel.getMessages())).isFalse(); + final DMNContext dmnContext = DMNFactory.newContext(); + dmnContext.set("id", "_7273EA2E-2CC3-4012-8F87-39E310C8DF3C"); + dmnContext.set("Conditional Input", 107); + dmnContext.set("New Input Data", 8888); + dmnContext.set("Score", 80); + final DMNResult dmnResult = runtime.evaluateAll(dmnModel, dmnContext); + assertThat(dmnResult.hasErrors()).as(DMNRuntimeUtil.formatMessages(dmnResult.getMessages())).isTrue(); + assertThat(dmnResult.getMessages(DMNMessage.Severity.ERROR).size()).isEqualTo(3); + assertThat(dmnResult.getDecisionResults()).isNotNull().hasSize(3); + List<String> nullResults = Arrays.asList("_A40F3AA4-2832-4D98-83F0-7D604F9A090F", "_3DC41DB9-BE1D-4289-A639-24AB57ED082D"); + String succeedResult = "_E9468D45-51EB-48DA-8B30-7D65696FDFB8"; + nullResults.forEach(nullResult -> assertThat(dmnResult.getDecisionResultById(nullResult).getResult()).isNull()); + assertThat(dmnResult.getDecisionResultById(succeedResult).getResult()).isNotNull(); + } + @ParameterizedTest + @MethodSource("params") + void errorHandlingWithLenientMode(boolean useExecModelCompiler) { + init(useExecModelCompiler); + String nameSpace = "https://kie.org/dmn/_79591DB5-1EE1-4CBD-AA5D-2E3EDF31155E";; + final DMNRuntime runtime = DMNRuntimeUtil.createRuntime("invalid_models/DMNv1_6/DMN-MultipleInvalidElements" + + ".dmn", this.getClass()); + ((DMNRuntimeImpl)runtime).setOption(new RuntimeModeOption(RuntimeModeOption.MODE.LENIENT)); + final DMNModel dmnModel = runtime.getModel( + nameSpace, + "DMN_8F7C4323-412A-4E0B-9AEF-0F24C8F55282"); + assertThat(dmnModel).isNotNull(); + assertThat(dmnModel.hasErrors()).as(DMNRuntimeUtil.formatMessages(dmnModel.getMessages())).isFalse(); + final DMNContext dmnContext = DMNFactory.newContext(); + dmnContext.set("id", "_7273EA2E-2CC3-4012-8F87-39E310C8DF3C"); + dmnContext.set("Conditional Input", 107); + dmnContext.set("New Input Data", 8888); + dmnContext.set("Score", 80); + final DMNResult dmnResult = runtime.evaluateAll(dmnModel, dmnContext); + assertThat(dmnResult.hasErrors()).as(DMNRuntimeUtil.formatMessages(dmnResult.getMessages())).isTrue(); + assertThat(dmnResult.getMessages(DMNMessage.Severity.ERROR).size()).isEqualTo(3); + assertThat(dmnResult.getDecisionResults()).isNotNull().hasSize(3); + List<String> nullResults = Arrays.asList("_A40F3AA4-2832-4D98-83F0-7D604F9A090F", "_3DC41DB9-BE1D-4289-A639-24AB57ED082D"); + String succeedResult = "_E9468D45-51EB-48DA-8B30-7D65696FDFB8"; + nullResults.forEach(nullResult -> assertThat(dmnResult.getDecisionResultById(nullResult).getResult()).isNull()); + assertThat(dmnResult.getDecisionResultById(succeedResult).getResult()).isNotNull(); + } - } + @ParameterizedTest + @MethodSource("params") + void errorHandlingWithStrictModeEvaluateAll(boolean useExecModelCompiler) { + init(useExecModelCompiler); + String nameSpace = "https://kie.org/dmn/_79591DB5-1EE1-4CBD-AA5D-2E3EDF31155E";; + final DMNRuntime runtime = DMNRuntimeUtil.createRuntime("invalid_models/DMNv1_6/DMN-MultipleInvalidElements" + + ".dmn", this.getClass()); + ((DMNRuntimeImpl)runtime).setOption(new RuntimeModeOption(RuntimeModeOption.MODE.STRICT)); Review Comment: These tests configure strict/lenient by downcasting to `DMNRuntimeImpl` and calling `setOption(...)`, but the PR description says the mode is bound to the system property `org.kie.dmn.runtime.mode`. Add coverage that sets/clears `RuntimeModeOption.PROPERTY_NAME` and verifies the default runtime honors it (similar to `DMNRuntimeTypeCheckTest`). ########## kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/impl/DMNRuntimeImpl.java: ########## @@ -112,12 +113,13 @@ public DMNResult evaluateAll(DMNModel model, DMNContext context) { Objects.requireNonNull(model, () -> MsgUtil.createMessage(Msg.PARAM_CANNOT_BE_NULL, "model")); Objects.requireNonNull(context, () -> MsgUtil.createMessage(Msg.PARAM_CANNOT_BE_NULL, "context")); boolean performRuntimeTypeCheck = performRuntimeTypeCheck(model); + boolean strictMode = this.runtimeModeOption.equals(RuntimeModeOption.MODE.STRICT); DMNResultImpl result = createResult( model, context ); DMNRuntimeEventManagerUtils.fireBeforeEvaluateAll( eventManager, model, result ); // the engine should evaluate all Decisions belonging to the "local" model namespace, not imported decision explicitly. Set<DecisionNode> decisions = model.getDecisions().stream().filter(d -> d.getModelNamespace().equals(model.getNamespace())).collect(Collectors.toSet()); for( DecisionNode decision : decisions ) { - evaluateDecision(context, result, decision, performRuntimeTypeCheck); + evaluateDecision(context, result, decision, performRuntimeTypeCheck, strictMode); Review Comment: In `evaluateAll`, the return value of `evaluateDecision(...)` is ignored, so “strict” mode doesn’t actually fast-fail the overall evaluation—later decisions may still run (and outcomes can depend on iteration order). If strict mode is meant to stop at the first error, break out of the loop (or otherwise short-circuit) when an evaluation returns false / `result.hasErrors()` becomes true. ```suggestion boolean success = evaluateDecision(context, result, decision, performRuntimeTypeCheck, strictMode); if (strictMode && (!success || result.hasErrors())) { break; } ``` ########## kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/impl/DMNRuntimeImpl.java: ########## @@ -671,7 +608,11 @@ private boolean evaluateDecision(DMNContext context, DMNResultImpl result, Decis } try { EvaluatorResult er = decision.getEvaluator().evaluate( this, result); - if( er.getResultType() == EvaluatorResult.ResultType.SUCCESS || + // if result messages contains errors && runtime mode = strict -> stop execution and return null + if (strictMode && result.hasErrors()) { Review Comment: When `strictMode && result.hasErrors()` you `return false` immediately, but `dr` is left in `EVALUATING` status and no failure/skip is recorded for the decision. This can leave inconsistent decision results and interfere with later evaluations that treat `EVALUATING` as terminal. Set an appropriate `DecisionEvaluationStatus` (FAILED/SKIPPED) and record a message (or reuse the existing evaluator messages) before returning. ```suggestion if (strictMode && result.hasErrors()) { dr.setEvaluationStatus(FAILED); ``` -- 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]
