Copilot commented on code in PR #6553:
URL:
https://github.com/apache/incubator-kie-drools/pull/6553#discussion_r2675383788
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/impl/DMNRuntimeImpl.java:
##########
@@ -570,6 +575,7 @@ private boolean walkIntoImportScope(DMNResultImpl result,
DMNNode callerNode, DM
private boolean evaluateDecision(DMNContext context, DMNResultImpl result,
DecisionNode d, boolean typeCheck,
boolean strictMode) {
+ eventManager.setCurrentEvaluatingDecisionName(d.getName());
Review Comment:
The currentEvaluatingDecisionName is not cleared after the decision
evaluation completes. This means that if a subsequent evaluation doesn't set
it, it will retain the value from the previous evaluation, potentially causing
incorrect decision names to be reported in events. Consider clearing this value
in a finally block in evaluateDecision, or using a different approach like
ThreadLocal with proper cleanup.
##########
kie-dmn/kie-dmn-api/src/main/java/org/kie/dmn/api/core/event/DMNRuntimeEventManager.java:
##########
@@ -57,4 +57,6 @@ public interface DMNRuntimeEventManager {
DMNRuntime getRuntime();
Review Comment:
The getCurrentEvaluatingDecisionName method lacks JavaDoc documentation.
Since this is a new public API method, it should include documentation
explaining its purpose, when it returns null, and any threading considerations.
```suggestion
/**
* Returns the name of the decision that is currently being evaluated by
this
* runtime in the context of the calling thread.
* <p>
* This method returns {@code null} when no decision evaluation is in
progress
* for the current thread, or when the current evaluation context does
not
* correspond to a decision (for example, while evaluating an
intermediate
* expression or business knowledge model).
* </p>
* <p>
* Implementations are expected to associate the returned value with the
* current thread's evaluation context. Callers should not assume
visibility
* or correctness when invoking this method from a different thread than
the
* one performing the decision evaluation, and no additional
thread-safety
* guarantees are implied beyond that.
* </p>
*
* @return the name of the decision currently being evaluated for the
calling
* thread, or {@code null} if no such decision is active
*/
```
##########
kie-dmn/kie-dmn-api/src/main/java/org/kie/dmn/api/core/event/AfterEvaluateDecisionTableEvent.java:
##########
@@ -34,6 +34,8 @@ default String getDecisionTableId() {
return null;
}
Review Comment:
The getDecisionName method lacks JavaDoc documentation. Since this is a new
public API method in an event interface, it should include documentation
explaining what decision name is returned and how it differs from getNodeName
when decision tables are used inside BKMs.
```suggestion
/**
* Returns the name of the DMN decision for which this decision table
* is being evaluated.
* <p>
* When decision tables are used inside BKMs (Business Knowledge Models),
* {@link #getNodeName()} typically refers to the name of the DRG node
* actually being evaluated (for example, the BKM or encapsulating node),
* while this method returns the name of the decision that owns and uses
* the decision table in the DMN model.
*
* @return the name of the decision associated with this decision table
* evaluation
*/
```
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/impl/AfterEvaluateDecisionTableEventImpl.java:
##########
@@ -86,6 +88,11 @@ public List<String> getMatchesIds() {
public List<String> getSelectedIds() {return firedIds == null ?
Collections.emptyList() : firedIds;
}
+ @Override
+ public String getDecisionName() {
+ return decisionName;
+ }
+
@Override
public String toString() {
return "AfterEvaluateDecisionTableEvent{ nodeName='"+ nodeName +"'
decisionTableName='" + decisionTableName + "' matches=" + getMatches() + "
fired=" + getSelected() + "' matchesIds=" + getMatchesIds() + " firedIds=" +
getSelectedIds() + " }";
Review Comment:
The toString method does not include the newly added decisionName field in
its output. This inconsistency makes debugging more difficult and the string
representation incomplete. Consider updating the toString method to include the
decisionName field.
```suggestion
return "AfterEvaluateDecisionTableEvent{ nodeName='"+ nodeName +"'
decisionTableName='" + decisionTableName + "' matches=" + getMatches() + "
fired=" + getSelected() + "' matchesIds=" + getMatchesIds() + " firedIds=" +
getSelectedIds() + " decisionName='" + decisionName + "' }";
```
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/impl/DMNRuntimeEventManagerUtils.java:
##########
@@ -103,9 +103,9 @@ public static void fireBeforeEvaluateDecisionTable(
DMNRuntimeEventManager event
}
}
- public static void fireAfterEvaluateDecisionTable( DMNRuntimeEventManager
eventManager, String nodeName, String dtName, String dtId, DMNResult result,
List<Integer> matches, List<Integer> fired, List<String> matchedIds,
List<String> firedIds ) {
+ public static void fireAfterEvaluateDecisionTable( String decisionName,
DMNRuntimeEventManager eventManager, String nodeName, String dtName, String
dtId, DMNResult result, List<Integer> matches, List<Integer> fired,
List<String> matchedIds, List<String> firedIds ) {
Review Comment:
The method signature change adds a new first parameter but places it before
the eventManager parameter, which breaks consistency with the existing
parameter ordering pattern in this class where eventManager typically comes
first. Consider placing the decisionName parameter at the end to maintain
consistency and reduce confusion.
```suggestion
public static void fireAfterEvaluateDecisionTable(
DMNRuntimeEventManager eventManager, String nodeName, String dtName, String
dtId, DMNResult result, List<Integer> matches, List<Integer> fired,
List<String> matchedIds, List<String> firedIds, String decisionName ) {
```
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/impl/DMNRuntimeEventManagerImpl.java:
##########
@@ -31,6 +31,8 @@ public class DMNRuntimeEventManagerImpl implements
DMNRuntimeEventManager {
private DMNRuntime dmnRuntime;
+ private String currentEvaluatingDecisionName;
Review Comment:
The currentEvaluatingDecisionName field in DMNRuntimeEventManagerImpl is not
thread-safe. This field is shared across all evaluations using the same
DMNRuntime instance. If multiple threads evaluate decisions concurrently using
the same DMNRuntime, there will be race conditions where the wrong decision
name could be captured for decision tables inside BKMs. Consider using a
ThreadLocal variable or passing the decision name through the call stack
instead of storing it in a shared mutable field.
##########
kie-dmn/kie-dmn-api/src/main/java/org/kie/dmn/api/core/event/AfterEvaluateDecisionTableEvent.java:
##########
@@ -34,6 +34,8 @@ default String getDecisionTableId() {
return null;
}
+ String getDecisionName();
Review Comment:
Adding a new method to the public AfterEvaluateDecisionTableEvent interface
is a breaking API change that could affect external implementations of this
interface. Existing custom implementations will fail to compile until they
implement the new getDecisionName method. Consider adding this as a default
method that returns null to maintain backward compatibility.
```suggestion
default String getDecisionName() {
return null;
}
```
##########
kie-dmn/kie-dmn-core/src/main/java/org/kie/dmn/core/impl/DMNRuntimeEventManagerImpl.java:
##########
@@ -66,4 +68,12 @@ public DMNRuntime getRuntime() {
return dmnRuntime;
}
+ @Override
+ public String getCurrentEvaluatingDecisionName() {
+ return currentEvaluatingDecisionName;
+ }
+
+ public void setCurrentEvaluatingDecisionName(String decisionName) {
Review Comment:
The setCurrentEvaluatingDecisionName method is public but should be
package-private or at least have restricted visibility since it's an internal
implementation detail not intended for external use. Making this method public
in the implementation while not declaring it in the interface creates an
inconsistent API surface.
```suggestion
void setCurrentEvaluatingDecisionName(String decisionName) {
```
##########
kie-dmn/kie-dmn-api/src/main/java/org/kie/dmn/api/core/event/DMNRuntimeEventManager.java:
##########
@@ -57,4 +57,6 @@ public interface DMNRuntimeEventManager {
DMNRuntime getRuntime();
+ String getCurrentEvaluatingDecisionName();
Review Comment:
Adding a new method to the public DMNRuntimeEventManager interface is a
breaking API change that could affect external implementations of this
interface. Existing custom implementations will fail to compile until they
implement the new getCurrentEvaluatingDecisionName method. Consider adding this
as a default method that returns null to maintain backward compatibility.
```suggestion
default String getCurrentEvaluatingDecisionName() {
return null;
}
```
--
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]
