Adding easy-rules-jexl to support Java Expression Language (JEXL) #271
Conversation
|
This is awesome, thank you! I will take a look in details and get back to you asap. |
|
|
||
| // then | ||
| assertThat(adultRule.getName()).isEqualTo("adult rule"); | ||
| assertThat(adultRule.getDescription()).isEqualTo("when age is greater then 18, then mark as adult"); |
There was a problem hiding this comment.
There is a typo here: then should be than. It was fixed in 94d29cc. Please fix it here as well. (there are a couple of other occurrences in this PR).
| facts.put("System", System.class); | ||
| facts.put("UUID", UUID.class); |
There was a problem hiding this comment.
Facts are designed to store business data. I'm not a fan of putting technical classes as facts like in here. MVEL and SpEL provide a ParserContext API that allows to register/import packages and functions (See example here). Is there something similar in JEXL?
My big concern here is that those tests serve as documentation as well, and in my experience, people will copy/paste and misuse facts to store any kind of non business facts.
There was a problem hiding this comment.
Is there something similar in JEXL?
It looks like Jexl namespaces are the closest concept to ParserContext in Mvel/SpEL. Here is a quick example:
@Test
public void testJexlNamespaces() {
Map<String, Object> namspaces = new HashMap<>();
namspaces.put("greeter", new Greeter());
JexlEngine jexlEngine = new JexlBuilder()
.namespaces(namspaces)
.create();
MapContext mapContext = new MapContext();
mapContext.set("name", "foo");
JexlScript jexlScript = jexlEngine.createScript("greeter:sayHello(name)");
jexlScript.execute(mapContext);// prints "hello foo"
}
public static class Greeter {
public void sayHello(String name) {
System.out.println("hello " + name);
}
}| .create(); | ||
|
|
||
| Facts facts = new Facts(); | ||
| facts.put("result", null); |
There was a problem hiding this comment.
This is not type safe. Even though it is possible, I would not use it in real case scenario. While defining a method in an EL expression and call it later on is ok, assigning a variable inside the expression and get the result outside is not a good practice IMO. This comment is about the practice itself rather than the code (again, as mentioned in my other comment in this PR, my concern is that people will just do the same). Each time I write something like this, there is a little voice in my head reminding me that the end goal is not to put code in Strings 😄
|
Hi @laurikimmel , Thank you for your PR! I really like this new module, but we need some updates before merging it (I can take care of some of those if needed). I added a couple of minor inline comments. There are also some changes that were introduced since you opened the PR, so I pushed a few commits here: https://github.com/j-easy/easy-rules/commits/pull/271 to explain them (see the messages of the last 3 commits). Please take a look and let me know about your thoughts. Do not hesitate to add javadocs to public APIs and add your name in the Thank you upfront! |
| @Rule | ||
| public final SystemOutRule systemOutRule = new SystemOutRule().enableLog(); | ||
|
|
||
| @SuppressWarnings("deprecation") |
There was a problem hiding this comment.
Instead of adding @SuppressWarnings, I would update the code to not use the deprecated API (See example in 90ecf58).
|
Hi @laurikimmel , I just noticed that JEXL has not been released since I suggest that you create a repo that you own/maintain with the JEXL module in this PR and I will add a reference to it in the readme. wdyt? |
|
Hi @benas! My apologies for not answering sooner. I have been busy with other tasks lately. I hope to have some time to deal with the concerns you raised. I agree, latest version of JEXL is quite old. At the same time, development is still ongoing and judging by the issue list they are getting close to releasing new version. |
|
If JEXL is still maintained then it's ok for me to merge this module once v3.2 is released. In the meantime, please update the PR and let me know if you need help. I still want to have your opinion on you owning this module and me adding a reference to it in the readme. |
|
Rebased and merged as eab2e3b. Revised in 1a06601 to address the comments above (see commit message for more details). I believe this module is consistent with other EL modules now. This is a great addition, thank you very much @laurikimmel for your contribution! This will be part of the upcoming v4.1. As a side note, since JEXL 3.2 is not released yet, I will release ER 4.1 based on JEXL 3.1 (as in this PR) and update JEXL to 3.2 in ER 4.2. |
Adding a module to support JEXL.