In our class that implements FlowableEventListener we access ExecutionEntity (instead of Execution) for the following reasons:
To retrieve the process definition id.
To set/get a local variable.
To check whether the execution is a multi-instance root.
To retrieve the parent execution.
We would prefer to move away from using ExecutionEntity since it resides in an ‘impl’ package. One option that would remove a number of ExecutionEntity references in our class would be to expose the following methods from ExecutionEntity in the Execution interface:
However, adding just these methods would still leave us with a compile time dependency on ExecutionEntity for setting/getting a local variable.
Another option would be to add the following method to the DelegateExecution interface and cast to it rather than ExecutionEntity since it provides access to variables.
getParent()
getActivityId()
I’m not sure I completely understand the distinction being made between the Execution and DelegateExecution interfaces so I’m not sure with which option to proceed with.
The idea behind the various interfaces is as follows:
ExecutionEntity is the interface for the data/entity manager layer. It contains all the data fields and related entity getters
DelegateExecution is what’s passed when delegating to custom logic, like for a a JavaDelegate. On one hand it constrains certain data not to be accessible and on the other hand it adds convenience methods around variable handling (which otherwise would have to be done via the entityManagers).
Execution is what’s returned by the query api’s and constrains the data even more so nothing internal is exposed. It also hides the methods that could throw an exception when not running within a command context.
With this in mind, the use case that you mention, in an event listener it probably mandates the use of the DelegateExecution. One option could be to enrich the event listeners with a convenience method to retrieve the DelegateExecution. Adding those two methods to the DelegateExecution is allright (if the getParent() returns a DelegateExecution too).
The FlowableEventListener provides a FlowableEvent instance in the onEvent method. The FlowableEventImpl class already contains an execution id so I think it would make sense to add a method to get the execution from it and expose it in the interface.
I agree, adding the two methods to the DelegateExecution seems like the right approach based on your explanation of the different interfaces.
Adding a getExecution method that returns a DelegateExecution to the FlowableEvent interface is not possible without additional refactoring since the FlowableEvent interface resides in the flowable-engine-common-api project and the DelegateExecution interface resides in the flowable-engine project. Are there any plans to move the DelegateExecution class to the flowable-engine-common-api project? It seems like the interface could be considered part of the API for users that extend Flowable.
No, moving it engine-common wouldn’t be good: engine-common is the module shared with the other engines (dmn, form, etc.) and Executions are only used by the bpmn engine, which also explains why the FlowableEvent interface doesn’t expose anything around the execution id and the fact FlowableEventImpl is in the flowable-engine module.
Maybe one way is to add a new event interface like ‘FlowableProcessEngineEvent’ (above FlowableEventImpl), part of the api package, that exposes these and extends the FlowableEvent interface? Still would mean a cast, but at least it’s API from that point on?
It looks like there is a FlowableEngineEvent interface already. Maybe FlowableEventImpl should extend FlowableEngineEvent and then the getExecution method added there. The one issue I see with this change is that the FlowableEngineEvent interface lives in the o.f.e.impl.delegate.event package which is not considered the API. Should we consider moving it to the o.f.e.delegate.event package?
Moving FlowableEngineEvent to the api package sounds like a good idea, all the actual event interfaces are already in the public packages , and they inherit from FlowableEngineEvent (which is now impl, what doesnt make sense).
Not sure if that’s needed. It seems that all the event interfaces already extend from it?
Regarding your question, I agree it’s not necessary but it seems like a good idea since any new event implementations should implement FlowableEngineEvent. After you review the patch let me know what your thoughts are. I don’t mind removing that change if you have a different opinion.