Skip to content
This repository has been archived by the owner on Jul 9, 2019. It is now read-only.

Custom FEEL Function Extensions incl. "starts with" support #8

Closed
wants to merge 16 commits into from

Conversation

stefanLeo
Copy link

This Pull Requests extends the DMN Engine's FEEL Engine in a way to easily support the addition of custom
FEEL extsion methods for i.e. starts with, ends with or whatever.

The current way to inject custom methods is very cumbersome IMHO (I succeeded but it really needed a lot of additional classes/work).
This is just a proposal - if you have any better approaches/alternatives feel free.

This resulted out of the discussion at:
https://forum.camunda.org/t/camunda-feel-supporting-regex-startswith-and-the-like/2131/2

@saig0 saig0 self-assigned this Jan 9, 2017
@saig0
Copy link
Member

saig0 commented Jan 10, 2017

Hi @stefanLeo ,

I like the idea of making it easier to extend / customize the feel engine. For a better understanding, can you please describe / showing code what is cumbersome in the current way and how your changes can improve this?

Regarding the "starts with" function: I think it can be usefull if you use it together with the transformation, so that you can write "starts with('foo')" without passing the input. But this doesn't fit to the DMN specification. So I'm currently not sure if it's good to include this extension.

@stefanLeo
Copy link
Author

Hi @saig0

Yes of course - thx. for having a look!

Regarding the cumbersomeness:
With the extensions of this pull request as a client I can add new functions by "simply" doing the following:

FeelElContextFactory.setFeelElContextClass(CustomFeelElContext.class.getName()); FeelFunctionMapper.addMethod("endsWith", CustomFeelFunctionMapper.class.getMethod("endsWith", String.class, String.class)); FeelToJuelTransformImpl.addFunctionTransfomer(new EndsWithFunctionTransformer());

Without it I have to create around 6 classes in order to inject my custom FunctionTransformer and my custom JUEL Method extensions. Moreover I had to use reflection in order to "reinit" the DMN engine again after I have exchanged the DmnEngineConfiguration with a custom one which injects my custom FeelEngineFactoryImpl and custom FeelELContextFactory and so on....
FYI: I did that within the Spring Boot Application - it may be a bit simple when only interacting with the DMN Engine not using the complete Spring stuff around it.
If you are interested in more detail I could upload a version of my extensions code to Github as well. I just need to switch back to the old version (as I am currently working with the PULL request version) and strip a bit of our internal code.

Regarding "starts with" - Totally agree.
We can easily get rid of it for the default engine - I just added it in order to show how to integrate a new function.
The DMN Standard says "starts with(string, match)", but IMHO it doesn't really make sense to add the string here again as it is anyhow available as a cell-input. I wanted to make it simpler :-)

Thank you & BR
Stefan

BTW: If I didn't mention it so far - I really like your engine! The open source approach is awesome and what I like most about it, is that is really lightweight!

@saig0
Copy link
Member

saig0 commented Jan 11, 2017

Hi Stefan,

thank you for your description. Now, I've a better feeling about it. I think you're on the right way but I want to propose some changes to make it more flexible and stable.

  • FeelElContextFactory: instead of change the FeelElContext instantiation - just add a getter for the function mapper and add the custom function mappers
  • FeelToJuelTransformImpl: invoke the custom function transformers in method transformSimplePositiveUnaryTest (same level as the operators) and make the add method non-static
  • also it may be useful to split between operators and functions - "starts with" is more like an operator since it don't pass the input - a function could be more like "max(list)" function which is used as end point and transformed to "${cellInput == max(list)}"
  • and provide more test cases that shows how to use these extension points to add a custom function and transformer

Following the hints, you may end up on one additional class for the custom FeelEngineFactory + classes for function mapper and transformer:

DefaultDmnEngineConfiguration config = new DefaultDmnEngineConfiguration();

CustomFeelEngineFactory factory = new CustomFeelEngineFactory();
config.setFeelEngineFactory(factory);

config.buildEngine();
class CustomFeelEngineFactory extends FeelEngineFactoryImpl {

    @Override
    protected ElContextFactory createElContextFactory()
    {
         FeelElContextFactory factory = new FeelElContextFactory();
         // provide a get for the function mapper
         CompositeFunctionMapper functionMapper = factory.getFunctionMapper();

         // this mapper just holds a map of methods and delegate to them - similar to FeelFunctionMapper
         CustomFunctionMapper customFunctionMapper = new CustomFunctionMapper();
         customFunctionMapper.addMethod("startsWith", CustomFeelFunctionMapper.class.getMethod("startsWith", String.class, String.class));
         customFunctionMapper.addMethod("endsWith", CustomFeelFunctionMapper.class.getMethod("endsWith", String.class, String.class));

         functionMapper.add(customFunctionMapper);

         return factory;
    }

    @Override
    protected FeelToJuelTransform createFeelToJuelTransform()
    {
        FeelToJuelTransformImpl transform = new FeelToJuelTransformImpl();

        // add custom function transformer > same level as transformSimplePositiveUnaryTest
        // maybe call it 'operators' instead of functions
        transform.addCustomFunctionTransformer(new StartsWithFunctionTransformer());
        transform.addCustomFunctionTransformer(new EndsWithFunctionTransformer());

        return transform;
    }
  }

What do you think?

- Easier add of methods
- Added Annotation for custom methods in order to further ease usage
- Added additional Unit Test showing new approach
- Removed startsWith example from base code
@stefanLeo
Copy link
Author

Hi

I incorporated your comments.
I think it is much simpler now to add custom functions -> see the new JUnit Test.

I did also include an Annotation for simpler adding/injecting of FunctionTransformers.
Pls. have a look and let me know about your thoughts.

Thx & BR
Steafn

@saig0
Copy link
Member

saig0 commented Jan 18, 2017

Hi Stefan,

I like your idea with the annotations. But I think you should not include this because annotations are not used in the Camunda platform (except for Spring or JDI). I see that it would reduce the code a bit but I prefer consistency.

Beside that, I would not recommend to allow adding custom functions to the FeelEngine at runtime. All other properties are initialized and set on creation via FeelEngineFactory. Do you see something what makes it necessary to dynamically change the FEEL engine?

Best regards,
Philipp

# Conflicts:
#	feel-juel/src/main/java/org/camunda/bpm/dmn/feel/impl/juel/el/FeelElContext.java
#	feel-juel/src/main/java/org/camunda/bpm/dmn/feel/impl/juel/el/FeelElContextFactory.java
#	feel-juel/src/main/java/org/camunda/bpm/dmn/feel/impl/juel/el/FeelFunctionMapper.java
#	feel-juel/src/main/java/org/camunda/bpm/dmn/feel/impl/juel/transform/FeelToJuelTransformImpl.java
@stefanLeo
Copy link
Author

Hi Philipp,

Sorry for the late reply.
I removed the Annotations again and integrated it now via the FeelEngineFactory.
I did add some abstractions in order to make it simpler to add custom methods:

`public class TestFeelEngineFactory extends FeelEngineFactoryImpl {

public FeelEngine createInstance() {
  //Only needed for making this unit test easier...
  FeelEngineCustomFunctionTest.feelEngine = super.createInstance();
  return FeelEngineCustomFunctionTest.feelEngine;
}

@Override
protected List<FeelToJuelFunctionTransformer> getFunctionTransformers() {
  //Adding the custom extensions here -> The details how to add where and when is hidden in the Factory itself.
  List<FeelToJuelFunctionTransformer> functionTransformers = new ArrayList<FeelToJuelFunctionTransformer>(2);
  functionTransformers.add(new StartsWithFunctionTransformer());
  functionTransformers.add(new EndsWithFunctionTransformer());
  return functionTransformers;
}

}`

This is now needed in addition to the 2 function classes (starts/endswith).
But I think this is reasonable - You think it better fits into the engine now?

Thank you & Best regards,
Stefan

@saig0
Copy link
Member

saig0 commented Feb 7, 2017

Hi Stefan,

did you miss something to commit?
The implementation still include some annotation stuff.

What about:

Beside that, I would not recommend to allow adding custom functions to the FeelEngine at runtime. All other properties are initialized and set on creation via FeelEngineFactory. Do you see something what makes it necessary to dynamically change the FEEL engine?

Best regards,
Philipp

@stefanLeo
Copy link
Author

Hi Philipp

Sorry for that - I think my commit didn't work out at all.
I'll fix it.

Sorry again & BR
Stefan

# Conflicts:
#	feel-juel/src/main/java/org/camunda/bpm/dmn/feel/impl/juel/el/FeelElContext.java
#	feel-juel/src/main/java/org/camunda/bpm/dmn/feel/impl/juel/el/FeelElContextFactory.java
#	feel-juel/src/main/java/org/camunda/bpm/dmn/feel/impl/juel/el/FeelFunctionMapper.java
#	feel-juel/src/main/java/org/camunda/bpm/dmn/feel/impl/juel/transform/FeelToJuelTransformImpl.java
@stefanLeo
Copy link
Author

Should work now - I had some issue with merge conflicts...

@saig0
Copy link
Member

saig0 commented Feb 14, 2017

Hi Stefan,

I merged the changes to master (317fdaf). I adjusted the code a bit so that it fit more to the existing code. Please have a look it. I'm waiting for your feedback if it's ok.

Best regards,
Philipp

@stefanLeo
Copy link
Author

Hi Philipp

Yes looks very clean! Thank you for merging.

One minor thing I noticed which made the code hard to read for me (I just realized it now as I saw how you are adding the custom functions now via the custom function mapper - which obviously is much more elegant):
Pls. add the @OverRide annotations on the FeelElContext class for getELResolver, getFunctionMapper and getVariableMapper methods.

Will this "extension to DMN be part of Camunda Rel. 7.7?

THX & BR
Stefan

@saig0
Copy link
Member

saig0 commented Feb 15, 2017

Hi Stefan,

we have the guideline not to use @Override if the class only implement the method - instead of override the behavior of the super class.

However, the changes are on master now, so they are a part of the next 7.7 alpha release ;)
I thank you very much for your contribution and endurance.

Best regards,
Philipp

@saig0 saig0 closed this Feb 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants