added definition variable container to support EL Resolvers to use th…#4169
added definition variable container to support EL Resolvers to use th…#4169WelschChristopher wants to merge 2 commits intoflowable:mainfrom
Conversation
…e definition if no VariableScope is available (during deployment)
...api/src/main/java/org/flowable/common/engine/api/definition/DefinitionVariableContainer.java
Show resolved
Hide resolved
| * | ||
| * @author Christopher Welsch | ||
| */ | ||
| public class DefinitionVariableContainer implements VariableContainer { |
There was a problem hiding this comment.
This should not be in the flowable-engine-common-api module. Instead it should be part of the flowable-engine-common module in org.flowable.common.engine.impl.el. Additionally, I think that it'll also be good if we can pass the definitionKey to the container as well.
| public DefinitionVariableContainer() { | ||
| } |
| /** | ||
| * Determines signal name from definition or expression | ||
| */ | ||
| public static String determineSignalName(CommandContext commandContext, SignalEventDefinition signalEventDefinition, BpmnModel bpmnModel, |
There was a problem hiding this comment.
Why don't we change the existing determiniSignalName to take in VariableContainer instead of the DelegateExecution?
| /** | ||
| * Determines message name from definition or expression | ||
| */ | ||
| public static String determineMessageName(CommandContext commandContext, MessageEventDefinition messageEventDefinition, |
There was a problem hiding this comment.
Why don't we change the existing determineMessageName to take VariableContainer instead of DelegateExecution?
| VariableScope scopeForExpression = executionEntity; | ||
| if (scopeForExpression == null) { | ||
| scopeForExpression = NoExecutionVariableScope.getSharedInstance(); | ||
| } |
There was a problem hiding this comment.
The comment above this should also be removed I think
| * | ||
| * @author Christopher Welsch | ||
| */ | ||
| public class StartEventExpressionWithDefinitionContainerTest extends PluggableFlowableTestCase { |
There was a problem hiding this comment.
Would be good if we can have some test cases that use something like ${variableContainer.scopeType} and / or ${variableContainer.definitionKey}. I believe that some of the tests cases like the ones that only use static strings would pass even without the newly added logic.
| /** | ||
| * @author Christopher Welsch | ||
| */ | ||
| class DefinitionVariableContainerTest { |
There was a problem hiding this comment.
I do not see the value in this test. Most of the things should be implicitly tested in StartEventExpressionWithDefinitionContainerTest by validating the tenant id, definition id, etc. on the jobs.
There was a problem hiding this comment.
I can't see the expression that is being used here? From what I can see it is using a static timeDuration. You could do something like ${variableContainer.definitionKey == 'timerExpressionProcess' ? 'PT1H' : 'PT2H'} or something in that area
| * Takes in a {@link DefinitionVariableContainer} for expression evaluation at deployment time (eg Timer start event). | ||
| */ | ||
| public static TimerJobEntity createTimerEntityForTimerEventDefinition(TimerEventDefinition timerEventDefinition, | ||
| FlowElement currentFlowElement, boolean isInterruptingTimer, DefinitionVariableContainer definitionVariableContainer, |
There was a problem hiding this comment.
We can slightly improve this by passing the ProcessDefinition instead of the DefinitionVariableContainer and remove the instantiation of the container on the caller side and also the setting of the definition and tenant id on the caller side.
| assertThat(instances).hasSize(1); | ||
|
|
||
| } finally { | ||
| processEngineConfiguration.resetClock(); |
There was a problem hiding this comment.
This is obsolete since our test harness resets it automatically.
3 participants
…e definition if no VariableScope is available (during deployment)
Check List: