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

Added a new Engine constructor with a varBindings argument #9

Closed
wants to merge 5 commits into from

Conversation

scranefield
Copy link

ch.maxant.rules.Engine.getMatchingRules calls MVEL.executeExpression(Object compiledExpression, Map vars) and uses the vars argument to pass in the input object (associated with the chosen input variable name).

It would be useful for users to be able to pass in other variable bindings to MVEL. This modification adds a new constructor for Engine that provides an extra varBindings argument (a Map), which the user can use to define other variables that are useful in rule conditions. For example, MVEL allows static methods (represented as java.lang.reflect.Method objects) to be bound to variables and then called in scripts as methodName(arg1,...,argn).

Copy link
Owner

@maxant maxant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've was in the middle of adding this PR but I'm not sure it makes that much sense.
I started to wonder why we don't make the getMatchingRules method take a map, rather just an object. The way you've implemented this PR, all other inputs are added when the engine is instantiated, rather than at the time you want to execute the rules, which seems a little strange.

I think the point is that you can already have a number of inputs today, say a date and a person. There is no need to change the API.

If a rule needs to reference two input parameters, you can solve that problem today by making the input a composite, containing all the input objects which the rules reference, e.g.:

class MyInput { //this is a composite
    LocalDate date;
    Person person;
} 

class Person {
    int age;
    String name;
}

String rule = "input.date > LocalDate.now() && input.person.age > 30";

Sure, you could insist on writing the rule like this, ie referencing each input separately:

String rule = "date > LocalDate.now() && person.age > 30";

But if you're willing to write the rule like I have further above, there is no need to make any changes to the library.

@scranefield Maybe you could tell me more about your use case?

@@ -395,7 +395,6 @@ private void addCompiledRule(boolean throwExceptionIfCompilationFails, Rule r) t

Map<String, Object> vars = new HashMap<>(varBindings);
vars.put(inputName, input);
System.out.println("*** MVEL variable bindings: " + vars);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could leave this in as a debug log statement.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I had meant to delete that. I don't think it needs to be logged.

@@ -103,7 +103,8 @@

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scranefield please update the unit tests too, then i'll happily merge your PR and build a new version and make it available at maven central. Thanks for the PR!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Would that just involve adding a new test to DefaultEngineTest.java?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please

@scranefield
Copy link
Author

scranefield commented Sep 4, 2018

Thanks for your feedback.

You suggested that "There is no need to change the API". However, I am proposing to extend the API, not change it. Yes, it is possible to provide a structured object that can be decomposed within rules using named properties or method calls, but my concern is with the readability of rules and separation of concerns. It could be the case the rules are written by different people from those creating the input objects. The more property accesses that need to be used in the rules, the more documentation is required to ensure that the rule authors get it right and readers understand what the rule is doing. The less that rule authors need to know about the application's class structure, the better.

I find it particularly useful to bind static methods of a class to MVEL variables, and these can then be used as functions in rule conditions. For example, my use case is an agent-based simulation in which agents have their internal action-selection logic coded as maxant rules - these rules receive an input object representing the agent that is running the rules. The rule conditions need to refer to agent properties, but there is also an external environment that the agents interact in, and rules need to be able to query this. Part of the environment is stored inside a Prolog interpreter that uses an implementation of a logical theory called the event calculus to determine the effects of actions. I need rule conditions to be able to query these effects. Rather than having expressions such as "input.env.prolog.hasSolution(Query)" I can just write "prolog(Query)" , provided that I can bind the variable "prolog" to the method org.jpl7.Query.hasSolution (this is a method in the JPL Java/Prolog bridge). It would be cumbersome to have to provide the methods as part of the input object and then write, e.g. input.prolog(Query). It would add complexity, and would require me to add the following clarification when explaining the rule: "Just ignore the 'input.' - the maxant library requires it needs to be there, but really the rule is just querying the Prolog environment".

It also just seems a shame that the MVEL interpreter allows multiple variable bindings to be passed in to the interpreter, but maxant rules restricts this to just one variable. It's hard to see a good reason to prevent the use of this useful feature.

@maxant
Copy link
Owner

maxant commented Sep 4, 2018

Ok I think I understand now and it makes more sense. I didn't realise that you can bind static methods, that's quite cool. Please write a simple test and I'll merge it, unfortunately not til later in the week tho.

scranefield and others added 2 commits September 6, 2018 20:27
…avascriptEngine (already available for the Engine class).

Added test cases (in DefaultEngineTest.java and JavascriptEngineTest) testing this feature for Engine and JavascriptEngine.
@scranefield
Copy link
Author

I added a test case for the use of additional initial variable bindings, as requested. I also added that functionality to the Javascript engine, and added a test case for that. I have not changed the Java 8 and Scala engine classes. They don't currently have the same constructors as the main Java and Javascript engines. In particular, they don't support changing the name of the input variable.

I also noticed that the Javadoc comments for two of the JavascriptEngine constructors refer to the non-existent parameter jsonifyInput, but I didn't think it was my business to remove those.

@maxant
Copy link
Owner

maxant commented Sep 9, 2018

@scranefield I've released version 2.3.1-SNAPSHOT - see if it fulfils your requirements. I'll release it fully once I figure out how sonatype now works, since something has changed :-(

@scranefield
Copy link
Author

@maxant Thank you! Version 2.3.1-SNAPSHOT works for my application.

@maxant
Copy link
Owner

maxant commented Sep 12, 2018

@scranefield : I've released version 2.3.1 - it's already available here: https://oss.sonatype.org/#nexus-search;quick~maxant and should appear in maven central in the next 24h: https://mvnrepository.com/artifact/ch.maxant/rules

@scranefield
Copy link
Author

@maxant Thank you, again!

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

Successfully merging this pull request may close these issues.

None yet

2 participants