Copilot commented on code in PR #7092:
URL: https://github.com/apache/incubator-kie/pull/7092#discussion_r3965736490
##########
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 `acceptingEventType(String signalName, String id)` from
`ProcessInstances` is a breaking change for external API consumers (this
interface lives in `kogito-api`). Consider keeping it as a deprecated default
method (possibly implemented via `findById(...)` +
`ProcessInstance.eventTypes()`/`adHocFragments()`), and remove it in a future
major release. If the intent is immediate removal, it should be called out
explicitly as an API break (and versioning/release notes should reflect it).
##########
kogito-jbpm/jbpm-flow/src/main/java/org/kie/kogito/process/impl/ProcessServiceImpl.java:
##########
@@ -162,9 +162,15 @@ 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 -> {
+ boolean isWaitingForSignal =
pi.eventTypes().stream()
+ .anyMatch(e -> signalName.equals(e) ||
("Message-" + signalName).equals(e));
+ boolean isAdHocNode = pi.adHocFragments().stream()
+ .anyMatch(f ->
f.getName().equals(signalName));
+ return isWaitingForSignal || isAdHocNode;
+ })
Review Comment:
This change introduces a new NPE hazard when `signalName` is null:
`signalName.equals(e)` will throw. The previous implementation would not NPE in
this case (it would effectively treat it as a non-matching signal). Either (a)
explicitly enforce `signalName` as non-null at the API boundary (e.g., fail
fast with a clear exception / annotation contract), or (b) preserve prior
behavior by making the equality null-safe (e.g., `Objects.equals(signalName,
e)` and precomputing the message key only when `signalName != null`).
##########
kogito-jbpm/jbpm-flow/src/main/java/org/kie/kogito/process/impl/AbstractProcessInstance.java:
##########
@@ -753,6 +754,11 @@ public Set<EventDescription<?>> events() {
});
}
+ @Override
+ public Collection<String> eventTypes() {
+ return executeInWorkflowProcessInstanceRead(pi ->
Arrays.asList(pi.getEventTypes()));
Review Comment:
`Arrays.asList(pi.getEventTypes())` will throw if `pi.getEventTypes()`
returns `null`. Returning an empty collection in that case would make
`eventTypes()` safer for callers (including the new `signalProcessInstance`
logic which streams over this result). A small adjustment inside the lambda
(null -> empty array/empty list) would avoid unexpected runtime failures.
##########
kogito-api/kogito-api/src/main/java/org/kie/kogito/process/ProcessInstance.java:
##########
@@ -271,6 +271,15 @@ default ProcessInstance<T> checkError() {
Set<EventDescription<?>> events();
+ /**
+ * Returns the resolved event types this process instance is currently
waiting for.
+ * Uses runtime-resolved values, so variable expressions (e.g. {@code
#{myVar}}) are
+ * returned as their actual value rather than the raw expression.
+ *
+ * @return resolved event type strings
Review Comment:
The Javadoc promises that variable expressions are returned as resolved
runtime values. The provided implementation in `AbstractProcessInstance`
currently delegates to `WorkflowProcessInstance#getEventTypes()` without any
explicit resolution step. If `getEventTypes()` is guaranteed to be resolved
already, it would help to reference that contract in the Javadoc; otherwise the
Javadoc should be adjusted to match actual behavior or the implementation
should be updated to perform resolution.
##########
kogito-jbpm/jbpm-flow/src/main/java/org/kie/kogito/process/impl/AbstractProcessInstance.java:
##########
@@ -753,6 +754,11 @@ public Set<EventDescription<?>> events() {
});
}
+ @Override
+ public Collection<String> eventTypes() {
+ return executeInWorkflowProcessInstanceRead(pi ->
Arrays.asList(pi.getEventTypes()));
+ }
Review Comment:
This introduces a new public behavior (`ProcessInstance.eventTypes()`) with
a concrete runtime implementation, but the updated tests only mock
`eventTypes()` rather than exercising the real implementation and edge cases
(e.g., `getEventTypes()` returning null/empty, arrays containing null entries).
Adding a focused unit test around `AbstractProcessInstance.eventTypes()` would
help prevent regressions and validate the method’s contract.
##########
kogito-jbpm/jbpm-flow/src/main/java/org/kie/kogito/process/impl/ProcessServiceImpl.java:
##########
@@ -162,9 +162,15 @@ 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 -> {
+ boolean isWaitingForSignal =
pi.eventTypes().stream()
+ .anyMatch(e -> signalName.equals(e) ||
("Message-" + signalName).equals(e));
Review Comment:
The message-event prefix `\"Message-\"` is a protocol detail that’s now
hard-coded in this method. To reduce future drift and improve readability,
consider centralizing this prefix as a constant (or reusing an existing
constant if the engine already defines one) and precomputing the message key
once per invocation rather than concatenating inside the stream predicate.
--
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]
