FIX: coercion on json pointer values #1178
Conversation
Signed-off-by: Chen <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1178 +/- ##
============================================
+ Coverage 93.81% 93.88% +0.07%
- Complexity 1027 1044 +17
============================================
Files 136 139 +3
Lines 2925 2962 +37
Branches 262 265 +3
============================================
+ Hits 2744 2781 +37
Misses 130 130
Partials 51 51
Continue to review full report at Codecov.
|
| private Object resolveJsonPointerValue(final String jsonPointer, final Event event) { | ||
| final Object value = event.get(jsonPointer, Object.class); | ||
| if (value == null) { | ||
| throw new ExpressionCoercionException("Unsupported type for value " + value); |
There was a problem hiding this comment.
Why does a null value throw?
I see that there is no clear default value to use in this case. Maybe just return a new Object or have a NullValue class which represents any null values.
There was a problem hiding this comment.
Right now there is no null value support in the grammar literal types so it is impossible for cx to check jsonpointer value is null in conditional expression. We have two options for null appearing in jsonPointer value:
- throw Exception
- return null and let each operator complain about the arg type.
I think both should work and I do not favor one over the other. @sbayer55 thoughts?
There was a problem hiding this comment.
So in the current design /message == 200 would have to fail if /message is null? I think this is clearly a false expression. I think if we returned a special NullValue class the rest of the code would work as expected. Right?
There was a problem hiding this comment.
Looking at this code (currently in PR), I think the equals operator would work correctly if the ParseTreeCoercsionService provided a new Object().
We could even do something like this:
class NullValue {
private static NULL_VALUE = new NullValue();
private NullValue() { }
public static nullValue() {
return NULL_VALUE;
}
}
And return:
return NullValue.nullValue()
|
|
||
| @Named | ||
| class ParseTreeCoercionService { | ||
| private final List<? extends Class<? extends Serializable>> literalTypes = Arrays.asList( |
| throw new ExpressionCoercionException("Unsupported type for value " + value); | ||
| } else if (literalTypes.contains(value.getClass())) { | ||
| return value; | ||
| } else if (value instanceof Double) { |
There was a problem hiding this comment.
This approach would be easier to extend if we have other possible types.
private static final Map<Class, Function<Object, Object>> literalTypes;
static {
literalTypes = new HashMap<>();
literalTypes.put(String.class, Function.identity());
literalTypes.put(Boolean.class, Function.identity());
literalTypes.put(Integer.class, Function.identity());
literalTypes.put(Float.class, Function.identity());
literalTypes.put(Double.class, v -> ((Double) value).floatValue());
}
And here:
else if (literalTypes.containsKey(value.getClass())) {
return literalTypes.get(value.getClass()).apply(value);
But, this is all internal and later refactoring is light. So I don't have a strong opinion on it.
Signed-off-by: Chen <[email protected]>
Signed-off-by: Chen <[email protected]>
|
|
||
| @Named | ||
| class ParseTreeCoercionService { | ||
| private static final Map<Class<? extends Serializable>, Function<Object, Object>> literalTypes; | ||
|
|
||
| static { |
There was a problem hiding this comment.
Could this be moved to a constructor argument and generated in a bean configuration function? Static variable can make classes difficult to unit test.
|
|
||
| static { | ||
| literalTypes = new HashMap<>(); | ||
| literalTypes.put(String.class, Function.identity()); |
There was a problem hiding this comment.
Are all of these keys needed? Could we use something like
if (literalTypes.hasKey(value.getClass()))
return literalTypes..get(value.getClass()).apply(value);
else
return value;
There was a problem hiding this comment.
I think that will not work for Long type which we want to throw exception. Also, for other custom types, it will just return literal objects which I am not sure if we want to support. e.g.
/code1 == /code2 | {"code1": StatusCode, "code2": StatusCode}
| final Event testEvent = createTestEvent(new HashMap<>()); | ||
| @ParameterizedTest | ||
| @MethodSource("provideUnSupportedJsonPointerValues") | ||
| void testCoerceTerminalNodeJsonPointerTypeUnSupportedValues(final Object testValue) { |
There was a problem hiding this comment.
Could testValue type be changed to Event and create the events in the provideUnSupportedJsonPointerValues method? I think it would make the test more readable.
There was a problem hiding this comment.
Actually the same method is used for both jsonpointer and escape jsonpointer test case in which keys are separate. So providing the value only will probably make it more compact and focused on the difference (testValue). Thought?
Signed-off-by: Chen <[email protected]>
Signed-off-by: Chen <[email protected]>
chenqi0805
deleted the
fix/1003-resolve-json-pointer-value-type-in-coercion-service
branch
Signed-off-by: Chen [email protected]
Description
This PR fixes coercion on corner cases in jsonpointer value, especially
Issues Resolved
Resolves #1003
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.