Fix #571: Unable to deserialize a pojo with IonStruct #573
Conversation
cowtowncoder
added
2.19
cla-received
| @@ -563,6 +575,12 @@ private IonValue getIonValue() throws IOException { | |||
| writer.writeValue(_reader); | |||
| IonValue v = l.get(0); | |||
| v.removeFromContainer(); | |||
|
|
|||
| if (v.isNullValue() && !Feature.READ_NULL_AS_IONVALUE.enabledIn(_formatFeatures)) { | |||
There was a problem hiding this comment.
I know existing is the way it is, but call above to:
_updateToken(JsonToken.VALUE_EMBEDDED_OBJECT);
looks wrong, given this is called (only) from getEmbeddedObject()... which should only be called when we do have JsonToken.VALUE_EMBEDDED_OBJECT (and if not, fail).
Would probably make sense to add a comment here to explain logic wrt !IonType.isContainer(v.getType()) -- I am not sure I understand the logic myself.
There was a problem hiding this comment.
A couple of tests fail if this _updateToken(JsonToken.VALUE_EMBEDDED_OBJECT); is removed. Perhaps a 3.0 cleanup activity.
There was a problem hiding this comment.
Ah. Makes sense there is a reason -- should have caught when these were added (or added comments to explain why/how)
| class IonValueDeserializer extends JsonDeserializer<IonValue> { | ||
| class IonValueDeserializer extends JsonDeserializer<IonValue> implements ContextualDeserializer { | ||
|
|
||
| private final JavaType targetType; |
There was a problem hiding this comment.
Member variables with leading underscore -> _targetType
ion/src/main/java/com/fasterxml/jackson/dataformat/ion/ionvalue/IonValueDeserializer.java
Outdated
Show resolved
Hide resolved
ion/src/main/java/com/fasterxml/jackson/dataformat/ion/ionvalue/IonValueDeserializer.java
Show resolved
Hide resolved
| @@ -65,7 +73,7 @@ static class IonValueData extends Data<IonValue> { | |||
| } | |||
|
|
|||
| private static final IonSystem SYSTEM = IonSystemBuilder.standard().build(); | |||
| private static final IonValueMapper ION_VALUE_MAPPER = new IonValueMapper(SYSTEM, SNAKE_CASE); | |||
| private final IonValueMapper ION_VALUE_MAPPER = new IonValueMapper(SYSTEM, SNAKE_CASE); | |||
There was a problem hiding this comment.
Why change from static to per-instance? Did it fail otherwise? (possibly due to null-value caching)
There was a problem hiding this comment.
This was changed so that it can be configured in each test, but not needed if I create a new IonValueMapper for the READ_NULL_AS_IONVALUE cases.
| public void shouldBeAbleToDeserializeNullToIonNull() throws Exception { | ||
| String ion = "{c:null}"; | ||
| verifyNullDeserialization(ion, SYSTEM.newNull()); | ||
| ION_VALUE_MAPPER.disable(IonParser.Feature.READ_NULL_AS_IONVALUE); |
There was a problem hiding this comment.
Should not try to change configuration of ION_VALUE_MAPPER after use, across methods; not guaranteed to take effect. Instead can create ObjectReader, call ObjectReader.with() and .without() to enable/disable features.
(also makes it easier to merge tests to 3.x as 3.x does not have methods to directly configure ObjectMapper)
There was a problem hiding this comment.
Should not try to change configuration of ION_VALUE_MAPPER after use,across methods; not guaranteed to take effect.
Why is that - caching?
Using ObjectReader is not a great option here, since it doesn't support Ion natively. Will create another IonValueMapper.
There was a problem hiding this comment.
Yes, indirectly caching of (de)serializers. Some of configuration settings affect construction and so effects remain if settings are changed.
Settings changed here are probably ok, although code won't work in 3.0 as mappers are truly immutable (builders only)
(fixes #571)
Make IonValueDeserializer a ContextualDeserializer in order to create the correct Ion null type.
Also, add new IonParser feature READ_NULL_AS_IONVALUE in order to be able to serialize
nullas either an IonNull or javanull.