Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

javax scripting added #237

Closed
wants to merge 3 commits into from
Closed

javax scripting added #237

wants to merge 3 commits into from

Conversation

nmondal
Copy link

@nmondal nmondal commented Nov 1, 2019

Hi,
I have added javax.scripting support, which now ensures all jsr-223 languages can be supported by easy-rules, something drools does not have!
Default is set to be Nashorn, JavaScript engine.

@coveralls
Copy link

coveralls commented Nov 3, 2019

Coverage Status

Coverage increased (+1.6%) to 92.44% when pulling e8152b8 on nmondal:master into f700ee3 on j-easy:master.

@nmondal
Copy link
Author

nmondal commented Nov 3, 2019

Hi Guys, this should fix everything including broken travis. Can we please merge it?

@fmbenhassine
Copy link
Member

Thank you for opening this PR, this is really awesome! However, I'm not sure I will be able to maintain it in the long term 😞

Can you create a repo with this new module and I will add a link to it in the home page here ? This way, all the credit goes to you for creating and maintaining the module.

@nmondal
Copy link
Author

nmondal commented Nov 6, 2019

Hi Ben,
As discussed in mail -- you can make me a maintainer! You started this journey, I do not want to fork off now. It won't make sense.
But if you insist, I certainly can.
I would still love you to merge this, and better love the fact you can make me a maintainer!

@fmbenhassine
Copy link
Member

Hi Nabarun,

I reviewed your PR and had a couple of thoughts:

  • I see that conditions/actions use a JScriptingHandler which in turn creates a ScriptEngine. This means we will have a new ScriptEngine instance for each condition/action of every rules. I did not do any performance benchmark (I never optimize prematurely), but I have a feeling this is not optimal.
  • Bindings (the context) are at the condition/action level. This is not correct, bindings should be part of the facts. Look at this example:
    @Test
    public void testJSConditionWithExpressionAndParserContext() {
        // given
        Bindings context = new SimpleBindings();
        context.put("x", 42);
        context.put("y", 42);

        Condition condition = new JScriptingCondition(DEFAULT_JVM_LINGO,"x == y ", context);
        Facts facts = new Facts();
        // when
        boolean evaluationResult = condition.evaluate(facts);
        // then
        assertThat(evaluationResult).isTrue();
    }

In this example, the context is bound to the condition itself, and the condition is evaluated against its own bindings, not against the set of facts. The example should be rather:

    @Test
    public void testJSConditionWithExpressionAndParserContext() {
        // given
        Facts facts = new Facts();
        facts.put("x", 42);
        facts.put("y", 42);
        Condition condition = new JScriptingCondition(DEFAULT_JVM_LINGO,"x == y ");

        // when
        boolean evaluationResult = condition.evaluate(facts);
        // then
        assertThat(evaluationResult).isTrue();
    }

This is the same for actions. This approach makes rules stateful, which was a drawback of Easy Rules v2 (and fixed in v3, see #37). So there is a design issue with the current approach.

  • When I run the tests, I have the following warning: Warning: Nashorn engine is planned to be removed from a future JDK release. Looking at the origin of this deprecation in JEP 335, I read in the "Risks and Assumptions" section: The risk of removing Nashorn is that certain applications will no longer run because of the expectation that JavaScript is present.. To be honest, this makes me nervous. I'm not ready to be surprised that this new module easy-rules-javax-scripting will stop working at some point.. We can document this clearly, but unfortunately users will not read the docs and just complain about the broken module.. Which brings me to the next point.

As you know, maintenance is not only about the code. Please don't get me wrong, if you want to join the org, you are welcome! My point is that the burden of support (I get dozens of emails, DMs on twitter, gitter, etc) will still land in my inbox. The effort on releases will still be on my side, etc.

To be honest, I believe the best way to use Easy Rules in javascript/typescript/whateverscript is to port it to javascript/typescript/whateverscript, and not trough a bridge. Personally, I would never use Java to call a python script, I would rather use python directly (using Java to call python via jython is just wrong to me, but I could be wrong).

That's why I believe the best option for now is that you maintain the module on your side as an extension (or a plugin) which will be very welcome to get a reference on Easy Rules in other languages section of the home page. You are not forking off, you are contributing an extension that you own. I really appreciate your effort and I hope you understand my position.

Best regards,
Mahmoud

@nmondal
Copy link
Author

nmondal commented Dec 2, 2019

Hi Ben, this code runs :

@Test
    public void testJSConditionWithExpressionAndParserContext() {
        // given
        Facts facts = new Facts();
        facts.put("x", 42);
        facts.put("y", 42);
        Condition condition = new JScriptingCondition(DEFAULT_JVM_LINGO,"x == y ");

        // when
        boolean evaluationResult = condition.evaluate(facts);
        // then
        assertThat(evaluationResult).isTrue();
    }

As is, even now. I probably did not understand what it meant when I was writing the test case but it does not matter, no code change is required to run it - because things are taken care of already.

See here:
Screenshot 2019-12-02 at 8 59 10 PM

I can fix that test code immediately.

Now coming back the NashHorn problem, it is not a problem at all, because in a real app scenario people would drop appropriate expression engine, and will use them instead of JS. Why?
Because for once JS is a poor choice. But why did I put JS? Because we wanted to test it, and JS engine comes free as of now with JDK.

Implementation is NOT tied with JS at all, only the test cases are, really.
Look at this:

private JScriptingRuleFactory  factory = new JScriptingRuleFactory(new YamlRuleDefinitionReader(),DEFAULT_JVM_LINGO);

We are supposed to pass it through.
If you do have reservations not allowing JSR languages to integrating with easy rule, I am afraid you are crippling the power of the system you built.
All I want to unleash the power - and have something that industry loves using because of the same.

Imagine the case of MS-Excel with VBScript macro. That is what makes excel truly powerful. In case of Windows itself -- it is PowerShell that makes it programmable.

However it is your call, entirely.
I can only request, and wait.
Thanks,
--Noga

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants