Hi tobias.metzke,

thanks for revising my proposal, you totally got my point.

Hi jan,

glad to hear that's also what you had in mind.
Let us know if you intend to work on this yourself, happy to help you get started.

Best,
Tobias

Hi tobias.metzke,

I'm glad to get involved. I think this proposal is a good start, I will try to work on this. Thanks for helping.

Hi jan,

happy to hear that! Let us know if you need any input or pointers.
Looking forward to your contribution.

Best,
Tobias

Hi tobias.metzke,

There are 2 set of abstract about el: ExpressionManager/Expression/VariableScope used in BPM engine, and ElProvider/ElExpression/VariableContext used in DMN engine. When init engine, we try to use the same el implement both in bpm and dmn. Here is the problem, DmnEngineConfigurationBuilder depends on Juel-specific implement of ExpresionManager. DmnEngineConfigurationBuilder should only depend on ElProvider instead of ExpresionManager. 2 solution below:

1. Make ExpressionManager/Expression/VariableScope the generalization of ElProvider/ElExpression/VariableContext,  ExpressionManager extends ElProvider, Expression extends ElExpression and VariableScope extends VariableContext.
2. Keep the 2 set of abstract independent, but bpm and dmn engine only share the same expressionManager when expressionManager implements both ExpressionManager and ElProvider.

// ProcessEngineConfigurationImpl.java

  protected void initDmnEngine() {
    if (dmnEngine == null) {

      if (dmnEngineConfiguration == null) {
        dmnEngineConfiguration = (DefaultDmnEngineConfiguration) DmnEngineConfiguration.createDefaultDmnEngineConfiguration();
      }

      dmnEngineConfiguration = new DmnEngineConfigurationBuilder(dmnEngineConfiguration)
          .dmnHistoryEventProducer(dmnHistoryEventProducer)
          .scriptEngineResolver(scriptingEngines)
          //.expressionManager(expressionManager)
          // should be 
          // if (expressionManager instanceof ElProvider) then
          //.elProvider((ElProvider) expressionManager) 
          .feelCustomFunctionProviders(dmnFeelCustomFunctionProviders)
          .enableFeelLegacyBehavior(dmnFeelEnableLegacyBehavior)
          .build();

      dmnEngine = dmnEngineConfiguration.buildEngine();

    } else if (dmnEngineConfiguration == null) {
      dmnEngineConfiguration = (DefaultDmnEngineConfiguration) dmnEngine.getConfiguration();
    }
  }

what's your suggestion?

Hey jan,

thanks for looking into this.

Looking at the code, I think we can still keep the two things separate. There are also roadmap plans on our end to adjust the DMN capabilities in the platform in the future. We might consolidate expression handling in that effort as well.
That being said, do you think we could just keep the two things as separate as they are? The DMN engine configuration is taking care of wrapping the expression manager into a ProcessEngineElProvider in case there is no ElProvider in the DMN engine configuration yet. Do you see any issues with that mechanism, given the new abstraction idea?

Thanks again and best,
Tobias

Hi tobias.metzke,

1. There is no need DmnEngineConfigurationBuilder depends on ExpresionManager.
2. Implement ElProvider by wrapping ExpressionManager is not easy and not reasonable, because ExpressionManager/Expression/VariableScope is more general than ElProvider/ElExpression/VariableContext. The ProcessEngineElProvider is actually not a wrapper of ExpressionManager, instead, there are VariableContext&Juel-related code implemented in ExpressionManager - ExpressionManager.createElContext(VariableContext).

I have created a pull request, check it out.

Hi jan,

thanks for your additional input here and for creating the PR.
I will have a look at this as soon as I can, hopefully by next week.
Sorry for the delay, I'll keep you in the loop.

Best,
Tobias

Hi jan,

I provided some feedback in the PR. Feel free to answer at your leisure in the PR.
If you have any questions, don't hesitate to reach out.

Best,
Tobias

Hey jan,

thanks again for this contribution, it was a pleasure reviewing this.
We also added some documentation to the Update Guide. This will soon be available publicly.
Plus, we will feature this in our upcoming 7.18.0-alpha2 release blog post.

Best,
Tobias