[DROOLS-4698] Managing user defined expressions for Collections #2691

yesamer · 2019-12-16T22:51:22Z

@dupliaka @jomarko @danielezonca can you please review it and test it?

Collection editor currently use a JSON to send collections (Maps, Lists) data to the backend.
Lists are wrapped on JSONArray and Maps on JSONObject
The idea I had to manage used defined expressions are to wrap them to a simple JSONTextual node (i.e. a JSON which contains only a string). This is useful to easily separate the scope of data assigning it to a very specific data structure.
Then, the changes here consist basically to handle this new case ( JSONTextual ) to the already present cases of JSONArray and JSONObject, for all three Evaluators (MVEL, Base and FEEL).
JSONTextual format is: "expression", it wraps a String between quote " char.

https://issues.redhat.com/browse/DROOLS-4698

Related PR: kiegroup/drools-wb#1276

jomarko · 2020-01-03T12:36:35Z

I think review should be postponed until https://github.com/kiegroup/drools/pull/2690 is in, WDYT @yesamer ?

yesamer · 2020-01-03T13:26:46Z

I think review should be postponed until #2690 is in, WDYT @yesamer ?

@jomarko Yes, fine for me.

jomarko · 2020-01-08T10:00:01Z

jenkins please retest this

jomarko · 2020-01-08T10:00:27Z

jenkins execute compile downstream build

gitgabrio

@yesamer
Just some minor changes, pls

gitgabrio · 2020-01-13T13:37:45Z

.../main/java/org/drools/scenariosimulation/backend/expression/AbstractExpressionEvaluator.java

@@ -61,7 +62,7 @@ protected boolean isStructuredResult(Class<?> resultClass) {

    /**
     * Check if className represents a structured input
-     * @param className
+     * @param className Used to determine if a structured input is passed
     * @return
     */
    protected boolean isStructuredInput(String className) {


@yesamer
This method is functionally identical to the previous one, even if written differently: please remove one of them

@gitgabrio done! Thanks!

gitgabrio · 2020-01-13T13:45:28Z

...c/main/java/org/drools/scenariosimulation/backend/expression/ExpressionEvaluatorFactory.java

@@ -54,15 +58,34 @@ public ExpressionEvaluator getOrCreate(FactMappingValue factMappingValue) {
            return getOrCreateDMNExpressionEvaluator();
        }

-        Object rawValue = factMappingValue.getRawValue();
+        String rawValue = (String) factMappingValue.getRawValue();


@yesamer
Not related to this PR, but we have to get rid of this Object rawValue (is cast to String everywhere)

@gitgabrio @danielezonca already planned a future refactor to set rawValue to String everywhere.

gitgabrio · 2020-01-13T13:49:39Z

...o-simulation-backend/src/main/java/org/drools/scenariosimulation/backend/util/JsonUtils.java

+            JsonNode jsonNode = objectMapper.readTree(json);
+            return Optional.of(jsonNode);
+        } catch (IOException e) {
+            return Optional.empty();


@yesamer
Not sure about swallowing this exception that way: if we get here rawValue is expected to be a valid json, but if it throws exception, something bad has happened, IMO.
"Given a json in String format, it try to convert it in a JsonNode. In case of success, i.e.
the given string is a valid json"...: what would mean a "json in String format" but invalid json?

@gitgabrio This is a good point, I had the thought, but I didn't find a different way to validate a json.
According to Jackson docs:

@throws JsonParseException if underlying input contains invalid content

of type {@link JsonParser} supports (JSON for default case)
*/
public JsonNode readTree(String content) throws IOException {
return _readTreeAndClose(_jsonFactory.createParser(content));
}

Then, I can improve it managing JsonParseException and IOException in different ways: in the first case, it's a malformed content i.e. not a JSON, in the second case It could be any other IOException, I can re-throw it with an other exception (which one?)
Consider the JsonParseException is a sub class of IOException
WDYT?

@yesamer
If I've understood correctly, ObjectMapper.readTree(String content) may throw one or the other (it declares only the latter, being the ancestor).
So, I agree both of them should be catch and managed in different way.
In either case, a specific exception should be re-thrown: I would create twos, to help calling code differentiate the actual issue.

@gitgabrio no, a JsonParseException is thrown if the input is not a valid json. In this case, I return an empty Optional which means: the given string can't be parsed as json and then it's not a json. In all other cases, when a generic IOException is thrown, then I re-throw an IllegalArgumentException.

@yesamer
I thought that method would be invoked only to parse an (expected) json string. If this is true, then IMO JsonParseException should not be hided behind an Optional.empty(), because it means something unexpected is happening. If - instead - this method is invoked on any possible String to verify if it is a valid json, then the javadoc should somehow clarify that

@gitgabrio

If - instead - this method is invoked on any possible String to verify if it is a valid json

This is the correct case, I updated the javadoc to clarify it.

gitgabrio · 2020-01-13T13:52:57Z

...o-simulation-backend/src/main/java/org/drools/scenariosimulation/backend/util/JsonUtils.java

+     * @param json
+     * @return
+     */
+    public static boolean isAJSONTextualNode(String json) {


@yesamer
I'm not sure where it is used, but I'm almost sure you would need to re-convert the string again and retrieve the .isTextual object: would not be better to directly return an Optional"" here, as in convertFromStringToJSONNode?

@gitgabrio To be honest it's not used, I removed it.

@yesamer
great!

.../main/java/org/drools/scenariosimulation/backend/expression/AbstractExpressionEvaluator.java

danielezonca · 2020-01-13T13:41:08Z

.../src/main/java/org/drools/scenariosimulation/backend/expression/MVELExpressionEvaluator.java

@@ -59,7 +62,7 @@ public boolean evaluateUnaryExpression(String rawExpression, Object resultValue,

    @Override
    public Object evaluateLiteralExpression(String rawExpression, String className, List<String> genericClasses) {
-        Object expressionResult = compileAndExecute(rawExpression, Collections.emptyMap());
+        Object expressionResult = compileAndExecute(rawExpression, new HashMap<>());


This changed has been moved to this PR so we can remove it from here (with the tests of course)

@danielezonca I just merged master which now contains that PR.

.../src/main/java/org/drools/scenariosimulation/backend/expression/MVELExpressionEvaluator.java

danielezonca · 2020-01-13T14:39:48Z

.../test/java/org/drools/scenariosimulation/backend/expression/BaseExpressionEvaluatorTest.java

@@ -162,9 +163,9 @@ public void mapOfComplexTypeTest() {
        element.setPhones(phones);
        toCheck.put("first", element);

-        assertTrue(expressionEvaluator.verifyResult(mapJsonString, toCheck));
+        assertTrue(expressionEvaluator.verifyResult(mapJsonString, toCheck, null));


Can you please add a test where the last parameter of verifyResult is used and it has to be different from null?

@danielezonca added :)

jomarko

Code review

See inline comments

jomarko · 2020-01-17T09:16:47Z

...c/main/java/org/drools/scenariosimulation/backend/expression/ExpressionEvaluatorFactory.java

+     * @param rawValue
+     * @return
+     */
+    protected boolean isAnMVELExpression(String rawValue) {


This is standard public static final helper method, so just wondering if the placement is the best choice we have.

@jomarko It could be moved to a Util class. On the other side, this method is used only in this class and I don't see another place where it could be used. In my opinon, an Util class should be created if it cans hold static methods which can be shared in multiple places. This is not the case. I vote to don't change it, if you have an alternative pls share.

jomarko · 2020-01-17T09:19:19Z

...c/main/java/org/drools/scenariosimulation/backend/expression/ExpressionEvaluatorFactory.java


-        if (rawValue instanceof String && ((String) rawValue).trim().startsWith(MVEL_ESCAPE_SYMBOL)) {
+        if (isAnMVELExpression(rawValue)) {


Please notice we changed the logic, we are now not checking instanceof in the isAnMVELExpression. We automatically cast to String what theoretically can cause cast exception.

@jomarko You're right BUT: rawValue is always a String, even if it's declared as Object in FactMappingValue DTO. If is not a String, it's an error, then it's correct to receive an exception. Consider that we planned with @danielezonca a refactor where we'll change that Object type in String --> https://issues.redhat.com/browse/DROOLS-4948

jomarko · 2020-01-17T09:26:11Z

.../test/java/org/drools/scenariosimulation/backend/expression/BaseExpressionEvaluatorTest.java

+    @Test(expected = IllegalArgumentException.class)
+    public void expressionListVerifyResultTest() {
+        String expressionCollectionJsonString = new TextNode("10").toString();
+        List<BigDecimal> contextValue = Collections.singletonList(BigDecimal.valueOf(10));
+        assertTrue(expressionEvaluator.verifyResult(expressionCollectionJsonString, contextValue, List.class));
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void expressionMapVerifyResultTest() {
+        String expressionCollectionJsonString = new TextNode("{key_a : 1}").toString();
+        Map<String, BigDecimal> contextValue = Collections.singletonMap("key_a", BigDecimal.valueOf(1));
+        assertTrue(expressionEvaluator.verifyResult(expressionCollectionJsonString, contextValue, Map.class));
    }


Could you please help me understand this, why IllegalArgumentException is expected in both cases? Is it because expressionCollectionJsonString needs to start with '[' and ends with ']' ?

@jomarko Yes, the expression collection feature doesn't work with BaseExpressionEvaluator, but only with MVELExpressionEvaluator and DMNExpressionEvaluator. Consider that the frontend, with the current status, can never sends an expression defined collection to be evaluated with BaseExpressionEvaluator. If happens, it's not expected.

jomarko · 2020-01-17T09:29:23Z

.../test/java/org/drools/scenariosimulation/backend/expression/MVELExpressionEvaluatorTest.java

+                                                                                                                 "test.add(\"Michael\");\n" +
+                                                                                                                 "test;"),
+                                                                                          ArrayList.class.getCanonicalName(),
+                                                                                          new ArrayList<>()


Could we please use Collections.emptyList()?

@jomarko Done!

jomarko · 2020-01-17T09:29:39Z

.../test/java/org/drools/scenariosimulation/backend/expression/MVELExpressionEvaluatorTest.java

+
+        assertEquals(expectedMap, evaluator.evaluateLiteralExpression(mvelExpression("[\"Jim\" : \"Person\"]"),
+                                                                      HashMap.class.getCanonicalName(),
+                                                                      new ArrayList<>()


Could we please use Collections.emptyList()?

@jomarko Done!

jomarko · 2020-01-17T09:29:53Z

.../test/java/org/drools/scenariosimulation/backend/expression/MVELExpressionEvaluatorTest.java

+                                                                                             "test.put(\"Jim\", a);\n" +
+                                                                                             "test;"),
+                                                                      HashMap.class.getCanonicalName(),
+                                                                      new ArrayList<>()


Could we please use Collections.emptyList()?

@jomarko Done!

jomarko

Approving this part, but before merge I would like to:

increase coverage DROOLS-4698: Increase coverage yesamer/drools#2 slightly
Review drools-wb- part of ensemble
Do manual check

Thank you @yesamer

sonarqubecloud · 2020-01-27T10:21:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

85.5% Coverage
0.0% Duplication

yesamer added 11 commits December 11, 2019 18:18

DROOLS-4698: Managing expressions for Collections

89ca53f

DROOLS-4698: Managing expressions for Collections

0d20a87

DROOLS-4698: Managing expressions for Collections + Tests

0b32695

DROOLS-4698: Managing expressions for Collections + Tests

897685a

Merge remote-tracking branch 'origin/master' into DROOLS-4698

16e1f4b

DROOLS-4698: Managing expressions for Collections + Tests

f2fca62

DROOLS-4698: Managing expressions for Collections + Tests

1200e94

DROOLS-4698: Managing expressions for Collections + Tests

325bf32

DROOLS-4698: Managing expressions for Collections + Tests

852faf2

Merge remote-tracking branch 'origin/master' into DROOLS-4698

7af6053

DROOLS-4698: Managing expressions for Collections + Tests

2720596

yesamer mentioned this pull request Dec 18, 2019

[DROOLS-4698] Managing user defined expressions for Collections kiegroup/drools-wb#1276

Merged

yesamer changed the title ~~[DROOLS-4698] Managing used defined expressions for Collections~~ [DROOLS-4698] Managing usedrdefined expressions for Collections Dec 18, 2019

yesamer changed the title ~~[DROOLS-4698] Managing usedrdefined expressions for Collections~~ [DROOLS-4698] Managing user defined expressions for Collections Dec 18, 2019

yesamer added 2 commits December 18, 2019 16:42

Merge remote-tracking branch 'origin/master' into DROOLS-4698

7adebb2

Merge remote-tracking branch 'origin/master' into DROOLS-4698

8ffcecd

yesamer marked this pull request as ready for review December 20, 2019 08:39

yesamer added 2 commits December 20, 2019 09:46

Merge remote-tracking branch 'origin/master' into DROOLS-4698

34a078e

Merge remote-tracking branch 'origin/master' into DROOLS-4698

668e74f

yesamer requested a review from dupliaka January 2, 2020 11:48

yesamer added 2 commits January 7, 2020 12:59

Merging from origin/master

88bd104

Merging from origin/master

bbd4453

yesamer requested review from danielezonca, gitgabrio and jomarko and removed request for dupliaka January 7, 2020 16:33

gitgabrio requested changes Jan 13, 2020

View reviewed changes

danielezonca reviewed Jan 13, 2020

View reviewed changes

yesamer added 7 commits January 14, 2020 09:32

DROOLS-4698: Changes required during Code Review

91dc107

Merge remote-tracking branch 'origin/master' into DROOLS-4698

3e581ef

Merge remote-tracking branch 'origin/master' into DROOLS-4698

8d94b6b

Merge remote-tracking branch 'origin/master' into DROOLS-4698

0facfa2

DROOLS-4698: Changes required during Code Review

a326460

DROOLS-4698: Changes required during Code Review

a3fa40f

Merge remote-tracking branch 'origin/master' into DROOLS-4698

c901f36

yesamer requested review from danielezonca, jomarko and gitgabrio January 15, 2020 10:44

yesamer added 2 commits January 15, 2020 13:30

Merge remote-tracking branch 'origin/master' into DROOLS-4698

cf71bf5

DROOLS-4698: Changes required during Code Review

44cf04f

gitgabrio approved these changes Jan 15, 2020

View reviewed changes

yesamer added 2 commits January 16, 2020 10:57

DROOLS-4698: Changes required during Code Review

3fe828e

Merge remote-tracking branch 'origin/master' into DROOLS-4698

6859b51

danielezonca approved these changes Jan 16, 2020

View reviewed changes

jomarko reviewed Jan 17, 2020

View reviewed changes

yesamer added 4 commits January 17, 2020 11:28

DROOLS-4698: Changes required during Code Review

8701835

Merge remote-tracking branch 'origin/master' into DROOLS-4698

026c7b3

DROOLS-4698: Additional tests.

7c4e797

Merge remote-tracking branch 'origin/master' into DROOLS-4698

0b58b46

yesamer requested a review from jomarko January 21, 2020 08:29

jomarko approved these changes Jan 24, 2020

View reviewed changes

Jozef Marko and others added 3 commits January 24, 2020 09:47

DROOLS-4698: Increase coverage (#2)

730a2b4

Merge remote-tracking branch 'origin/master' into DROOLS-4698

59369bd

Merge remote-tracking branch 'origin/master' into DROOLS-4698

f499aed

danielezonca merged commit 0c0202b into apache:master Jan 27, 2020


		if (rawValue instanceof String && ((String) rawValue).trim().startsWith(MVEL_ESCAPE_SYMBOL)) {
		if (isAnMVELExpression(rawValue)) {

[DROOLS-4698] Managing user defined expressions for Collections #2691

[DROOLS-4698] Managing user defined expressions for Collections #2691

Conversation

yesamer commented Dec 16, 2019 • edited Loading

jomarko commented Jan 3, 2020

yesamer commented Jan 3, 2020 • edited Loading

jomarko commented Jan 8, 2020

jomarko commented Jan 8, 2020

gitgabrio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yesamer Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

gitgabrio Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

yesamer Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yesamer Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jomarko left a comment

Choose a reason for hiding this comment

Code review

Choose a reason for hiding this comment

yesamer Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yesamer Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jomarko left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 27, 2020

yesamer commented Dec 16, 2019 •

edited

Loading

yesamer commented Jan 3, 2020 •

edited

Loading

yesamer Jan 13, 2020 •

edited

Loading

gitgabrio Jan 13, 2020 •

edited

Loading

yesamer Jan 14, 2020 •

edited

Loading

yesamer Jan 15, 2020 •

edited

Loading

yesamer Jan 17, 2020 •

edited

Loading

yesamer Jan 17, 2020 •

edited

Loading