apache / incubator-kie-kogito-runtimes

Kogito Runtimes - Kogito is a cloud-native business automation technology for building cloud-ready business applications.
http://kogito.kie.org
Apache License 2.0
544 stars 209 forks source link

Codegen Message Start Event: Incorrect endpoint generation: endpoint is requiring instance id #1893

Closed StephenOTT closed 2 years ago

StephenOTT commented 2 years ago

Describe the bug

Docs on Message Start Event: https://docs.jboss.org/kogito/release/latest/html_single/#ref-bpmn-start-events_kogito-developing-process-services

Consider a process such as:

image

This configuration is generating endpoints with:

image

Based on how Message Start Events are supposed to be used ("to start process instances"), this generated endpoint is unexpected and appears wrong.

Expected behavior

Would expect a POST endpoint to be generated that sends the message and filtered by the specific application and not the process instance, as there would not no instance (as it is a start event)

Actual behavior

The endpoint is generating the following code:

return processService.signalProcessInstance(process, id, null, "My Signal").orElseThrow(() -> new NotFoundException());

which runs:

    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().findById(id)
                        .map(pi -> {
                            pi.send(Sig.of(signalName, data));
                            return pi.checkError().variables().toModel();
                        }));
    }

This does not seem to be the intended behaviour given it is a Start Event.

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Kogito version or git rev (or at least Quarkus version if you are using Kogito via Quarkus platform BOM)

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

StephenOTT commented 2 years ago

This code also makes me ~worry about performance on large number of instances? What if there were 500,000 active instances of a specific process definition. Are all of these instances loaded into memory for something like signal? The underlying process.instances().... code seems to query everything from a concurrentMap

fjtirado commented 2 years ago

Nice catch!!! Opened JIRA

fjtirado commented 2 years ago

As per the performance concern, @StephenOTT, when persistence is active, process.instance() will be translated to a DB query (which I do not know if it worse or not, just kidding), so in real deployment scenario not all process instances will be in memory.

StephenOTT commented 2 years ago

Some further comments that I have added into the JIRA ticket:

Some further thoughts on this:

Experimenting with a Message and Signals with the Codegen, some thoughts on the semantics of the design:

When a Signal or Message is detected in an App's BPMNs:

Should there be generic Message and Signal endpoints generated which allow a Message/Signal Name param (+var inputs).  This would allow easy semantics of Cross BPMN messaging. Should Message/Signal endpoints be generated under the specific BPMN section of endpoints which allow the same as #1, BUT are filtered for the BPMN definition. + has param to filter against a specific process instance. The difference between the two are, the detection of messages or signal events in BPMNs means it could be a event sent to one or many instances across one or many definitions, where #1 handles the Multiple BPMN, and #2 is handled for target messaging within a single BPMN scope.

This would also help from a security standpoint: allowing control at the endpoint level to ensure that people who have access to one BPMN are not messaging into other BPMNs.