-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[DROOLS-4698] Managing user defined expressions for Collections #2691
Changes from 37 commits
89ca53f
0d20a87
0b32695
897685a
16e1f4b
f2fca62
1200e94
325bf32
852faf2
7af6053
2720596
7adebb2
8ffcecd
34a078e
668e74f
88bd104
bbd4453
df4930e
799fa6c
424cfe8
0f24b1d
de0e0bd
91dc107
3e581ef
8d94b6b
0facfa2
a326460
a3fa40f
c901f36
cf71bf5
44cf04f
3fe828e
6859b51
8701835
026c7b3
7c4e797
0b58b46
730a2b4
59369bd
f499aed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,12 @@ | |
|
||
package org.drools.scenariosimulation.backend.expression; | ||
|
||
import java.util.Optional; | ||
|
||
import com.fasterxml.jackson.databind.JsonNode; | ||
import org.drools.scenariosimulation.api.model.FactMappingValue; | ||
import org.drools.scenariosimulation.api.model.ScenarioSimulationModel.Type; | ||
import org.drools.scenariosimulation.backend.util.JsonUtils; | ||
|
||
import static org.drools.scenariosimulation.api.utils.ConstantsHolder.MVEL_ESCAPE_SYMBOL; | ||
|
||
|
@@ -54,15 +58,34 @@ public ExpressionEvaluator getOrCreate(FactMappingValue factMappingValue) { | |
return getOrCreateDMNExpressionEvaluator(); | ||
} | ||
|
||
Object rawValue = factMappingValue.getRawValue(); | ||
String rawValue = (String) factMappingValue.getRawValue(); | ||
|
||
if (rawValue instanceof String && ((String) rawValue).trim().startsWith(MVEL_ESCAPE_SYMBOL)) { | ||
if (isAnMVELExpression(rawValue)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please notice we changed the logic, we are now not checking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jomarko You're right BUT: rawValue is always a |
||
return getOrCreateMVELExpressionEvaluator(); | ||
} else { | ||
return getOrCreateBaseExpressionEvaluator(); | ||
} | ||
} | ||
|
||
/** | ||
* A rawValue is an MVEL expression if: | ||
* - NOT COLLECTIONS CASE: It's a <code>String</code> which starts with MVEL_ESCAPE_SYMBOL ('#') | ||
* - COLLECTION CASE: It's a JSON String node, which is used only when an expression is set | ||
* (in other cases it's a JSON Object (Map) or a JSON Array (List)) and it's value starts with MVEL_ESCAPE_SYMBOL ('#') | ||
* @param rawValue | ||
* @return | ||
*/ | ||
protected boolean isAnMVELExpression(String rawValue) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is standard public static final helper method, so just wondering if the placement is the best choice we have. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 |
||
/* NOT COLLECTIONS CASE */ | ||
if (rawValue.trim().startsWith(MVEL_ESCAPE_SYMBOL)) { | ||
return true; | ||
} | ||
/* COLLECTION CASE */ | ||
Optional<JsonNode> optionalNode = JsonUtils.convertFromStringToJSONNode(rawValue); | ||
return optionalNode.filter( | ||
jsonNode -> jsonNode.isTextual() && jsonNode.asText().trim().startsWith(MVEL_ESCAPE_SYMBOL)).isPresent(); | ||
} | ||
|
||
private ExpressionEvaluator getOrCreateBaseExpressionEvaluator() { | ||
if (baseExpressionEvaluator == null) { | ||
baseExpressionEvaluator = new BaseExpressionEvaluator(classLoader); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
/* | ||
* Copyright 2019 Red Hat, Inc. and/or its affiliates. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.drools.scenariosimulation.backend.util; | ||
|
||
import java.io.IOException; | ||
import java.util.Optional; | ||
|
||
import com.fasterxml.jackson.core.JsonParseException; | ||
import com.fasterxml.jackson.databind.JsonNode; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
|
||
/** | ||
* Class used to provide JSON common utils | ||
*/ | ||
public class JsonUtils { | ||
|
||
private JsonUtils() { | ||
// Not instantiable | ||
} | ||
|
||
/** | ||
* This method aim is to to evaluate if any possible String is a valid json or not. | ||
* Given a json in String format, it try to convert it in a <code>JsonNode</code>. In case of success, i.e. | ||
* the given string is a valid json, it put the <code>JsonNode</code> in a <code>Optional</code>. An empty | ||
* <code>Optional</code> is passed otherwise. | ||
* @param json | ||
* @return | ||
*/ | ||
public static Optional<JsonNode> convertFromStringToJSONNode(String json) { | ||
if (json == null || json.isEmpty()) { | ||
return Optional.empty(); | ||
} | ||
try { | ||
ObjectMapper objectMapper = new ObjectMapper(); | ||
JsonNode jsonNode = objectMapper.readTree(json); | ||
return Optional.of(jsonNode); | ||
} catch (JsonParseException e) { | ||
return Optional.empty(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yesamer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gitgabrio This is a good point, I had the thought, but I didn't find a different way to validate a json. Then, I can improve it managing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yesamer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gitgabrio no, a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yesamer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is the correct case, I updated the javadoc to clarify it. |
||
} catch (IOException e) { | ||
throw new IllegalArgumentException("Generic error during json parsing: " + json, e); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yesamer
Not related to this PR, but we have to get rid of this
Object
rawValue (is cast to String everywhere)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gitgabrio @danielezonca already planned a future refactor to set rawValue to
String
everywhere.