Copilot commented on code in PR #7092:
URL: https://github.com/apache/incubator-kie/pull/7092#discussion_r3967261380
##########
kogito-api/kogito-api/src/main/java/org/kie/kogito/process/ProcessInstances.java:
##########
@@ -56,20 +56,4 @@ default Stream<ProcessInstance<T>>
waitingForEventType(String eventType) {
}
Stream<ProcessInstance<T>> waitingForEventType(String eventType,
ProcessInstanceReadMode mode);
Review Comment:
Removing the `acceptingEventType(String signalName, String id)` default
method from a public interface is a source/binary breaking change for
downstream consumers and extensions that may call it. If the goal is to stop
using it internally, consider restoring it (possibly re-implemented more
efficiently) and deprecating it first, or moving it to an internal SPI/helper
while keeping API compatibility.
##########
kogito-jbpm/jbpm-flow/src/main/java/org/kie/kogito/process/impl/ProcessServiceImpl.java:
##########
Review Comment:
This method sends a signal (a mutating operation) but now loads the instance
via `findById(id)` without an explicit read mode. If any `ProcessInstances`
implementation defaults `findById(String)` to a read-only instance,
`pi.send(...)` may fail or mutate a non-persisted view. Prefer using an
explicit mutable lookup (e.g., `findById(id, ProcessInstanceReadMode.MUTABLE)`)
to make the requirement unambiguous and consistent with the former behavior.
##########
kogito-jbpm/jbpm-flow/src/main/java/org/jbpm/workflow/instance/node/EventNodeInstance.java:
##########
@@ -272,12 +275,20 @@ public String[] getEventTypes() {
@Override
public Set<EventDescription<?>> getEventDescriptions() {
NamedDataType dataType = null;
- if (getEventNode().getVariableName() != null) {
- VariableScope variableScope = (VariableScope)
getEventNode().getContext(VariableScope.VARIABLE_SCOPE);
- Variable variable =
variableScope.findVariable(getEventNode().getVariableName());
- dataType = new NamedDataType(variable.getName(),
variable.getType());
+ String variableName = getEventNode().getVariableName();
+ if (variableName != null) {
+ VariableScopeInstance variableScopeInstance =
(VariableScopeInstance) resolveContextInstance(VariableScope.VARIABLE_SCOPE,
variableName);
+ if (variableScopeInstance == null) {
+ variableScopeInstance = (VariableScopeInstance)
getProcessInstance().getContextInstance(VariableScope.VARIABLE_SCOPE);
+ }
+ Variable variable =
variableScopeInstance.getVariableScope().findVariable(variableName);
+ if (variable != null) {
+ dataType = new NamedDataType(variable.getName(),
variable.getType());
Review Comment:
`variableScopeInstance` can still be `null` after the fallback assignment
(e.g., if there is no `VariableScopeInstance` on the process instance for some
reason), which would lead to a `NullPointerException` at
`variableScopeInstance.getVariableScope()`. Consider guarding that access and
treating the `dataType` as unknown (`null`) when the scope instance isn't
available.
##########
kogito-jbpm/jbpm-flow/src/test/java/org/kie/kogito/process/impl/ProcessServiceImplSignalTest.java:
##########
@@ -19,219 +19,204 @@
package org.kie.kogito.process.impl;
import java.util.Collections;
+import java.util.List;
import java.util.Optional;
-import java.util.stream.Stream;
+import java.util.Set;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.kie.kogito.Application;
import org.kie.kogito.MappableToModel;
import org.kie.kogito.Model;
import org.kie.kogito.config.ConfigBean;
+import org.kie.kogito.process.BaseEventDescription;
+import org.kie.kogito.process.EventDescription;
import org.kie.kogito.process.Process;
import org.kie.kogito.process.ProcessInstance;
-import org.kie.kogito.process.ProcessInstanceReadMode;
import org.kie.kogito.process.ProcessInstances;
+import org.kie.kogito.process.flexible.AdHocFragment;
import org.kie.kogito.uow.UnitOfWorkManager;
-import org.kie.kogito.uow.WorkUnit;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
/**
- * Integration tests for ProcessServiceImpl signal handling.
- * Tests validation of signals for both traditional signal events and ad hoc
nodes.
+ * Unit tests for ProcessServiceImpl.signalProcessInstance.
+ * Verifies signal routing for traditional signal events, message events, ad
hoc nodes,
+ * non-existent instances, and non-matching signals.
*/
class ProcessServiceImplSignalTest {
private ProcessServiceImpl processService;
- private Application application;
private Process<TestModel> process;
private ProcessInstances<TestModel> processInstances;
private ProcessInstance<TestModel> processInstance;
- private UnitOfWorkManager unitOfWorkManager;
- private ConfigBean configBean;
@BeforeEach
+ @SuppressWarnings("unchecked")
void setup() {
- application = mock(Application.class);
+ Application application = mock(Application.class);
process = mock(Process.class);
processInstances = mock(ProcessInstances.class);
processInstance = mock(ProcessInstance.class);
- unitOfWorkManager = mock(UnitOfWorkManager.class);
- configBean = mock(ConfigBean.class);
+ UnitOfWorkManager unitOfWorkManager = mock(UnitOfWorkManager.class);
+ ConfigBean configBean = mock(ConfigBean.class);
when(application.unitOfWorkManager()).thenReturn(unitOfWorkManager);
when(application.config()).thenReturn(mock(org.kie.kogito.Config.class));
when(application.config().get(ConfigBean.class)).thenReturn(configBean);
when(configBean.processInstanceLimit()).thenReturn((short) 100);
when(process.instances()).thenReturn(processInstances);
- // Setup UnitOfWorkManager to execute code immediately
+ // Make the UoW execute the supplied callable immediately
org.kie.kogito.uow.UnitOfWork unitOfWork =
mock(org.kie.kogito.uow.UnitOfWork.class);
when(unitOfWorkManager.newUnitOfWork()).thenReturn(unitOfWork);
when(unitOfWorkManager.currentUnitOfWork()).thenReturn(unitOfWork);
doAnswer(invocation -> {
- org.kie.kogito.uow.WorkUnit<?> workUnit =
invocation.getArgument(0);
- workUnit.perform();
+ invocation.<org.kie.kogito.uow.WorkUnit<?>>
getArgument(0).perform();
return null;
}).when(unitOfWork).intercept(any());
processService = new ProcessServiceImpl(application);
}
+ // --- helpers ---
+
+ private static EventDescription<?> eventDesc(String eventName) {
+ return new BaseEventDescription(eventName, "node-1", "Node", "signal",
"ni-1", "pi-1", null);
+ }
Review Comment:
If `BaseEventDescription` is generic, constructing it without type
parameters here may introduce raw-type warnings. Prefer using an explicitly
parameterized construction (or `new BaseEventDescription<>(...)` if available)
to keep the test code type-safe and avoid suppressing warnings more broadly
than necessary.
##########
kogito-jbpm/jbpm-flow/src/main/java/org/kie/kogito/process/impl/ProcessServiceImpl.java:
##########
@@ -162,9 +162,16 @@ public <T extends MappableToModel<R>, R> Optional<R>
updatePartial(Process<T> pr
public <T extends MappableToModel<R>, R> Optional<R>
signalProcessInstance(Process<T> process, String id, Object data, String
signalName) {
return UnitOfWorkExecutor.executeInUnitOfWork(
application.unitOfWorkManager(),
- () -> process
- .instances().acceptingEventType(signalName, id)
- .findFirst()
+ () -> process.instances()
+ .findById(id)
+ .filter(pi -> {
+ if (pi.events().stream().anyMatch(e ->
signalName.equals(e.getEvent()) || ("Message-" +
signalName).equals(e.getEvent()))) {
+ return true;
+ }
+ return pi.adHocFragments()
+ .stream()
+ .anyMatch(f ->
f.getName().equals(signalName));
+ })
.map(pi -> {
pi.send(SignalFactory.of(signalName, data));
Review Comment:
The acceptance logic hard-codes message matching via the `\"Message-\" +
signalName` convention. Since `EventDescription` already carries an event type
(and the engine is already populating it from metadata elsewhere in this PR),
consider matching using structured fields (e.g., event type + event name)
rather than encoding message-ness in the event string. This reduces coupling to
string conventions and makes future event naming changes less risky.
--
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]
