jackson-dataformat-ion does not handle null.struct deserialization correctly

[MartinGian]

Hi there,

Jackson-dataformat-ion 2.12 is not handling null deserialization correctly.

Here is the test:

import com.amazon.ion.IonSystem;
import com.amazon.ion.IonValue;
import com.amazon.ion.system.IonSystemBuilder;

@Test
public void shouldBeAbleToSerializeAndDeserializeNullStructs() throws Exception {
    // given
    IonSystem ionSystem = IonSystemBuilder.standard().build();
    IonObjectMapper mapper = new IonValueMapper(ionSystem)
            .configure(IonGenerator.Feature.USE_NATIVE_TYPE_ID, false)
            .configure(IonParser.Feature.USE_NATIVE_TYPE_ID, false);

    // when
    IonValue nullStruct = ionSystem.newNullStruct();
    IonValue ionValue = mapper.writeValueAsIonValue(nullStruct);

    // then
    IonValue parsedValue = mapper.readValue(ionValue, IonValue.class);
    assertThat(parsedValue, equalTo(nullStruct));
}

java.lang.AssertionError: 
Expected: <null.struct>
     but: was null
Expected :<null.struct>
Actual   :null

As you can see, it is deserializing null.struct into a Java null instead of deserializing it to an IonValue containing a null struct. This leads to NullPointerException and furthermore, IMO is losing data as a null struct is not the same as a Java null. It also loses any annotation that the null.struct may have, example: IonValue nullStruct = ionSystem.singleValue("foo::null.struct");, this is read by the mapper as a Java null as well.

The serialization works correctly, it is the deserialization that is not parsing this null values correctly.

Playing a bit with it, if you change the implementation of IonValueDeserializer and add the following override, the test above passes.

@Override
public IonValue getNullValue(DeserializationContext ctxt) throws JsonMappingException {
    try {
        return deserialize(ctxt.getParser(), ctxt);
    } catch (IOException e) {
        throw new JsonMappingException(e.getMessage());
    }
}

It would be great if someone with more experience with Jackson can validate this. I'm happy to create a PR with the change if someone from your team confirms that this is the correct implementation.

Regards.

[mcliedtke]

This does seem like a defect for the IonValueDeserializer since Ion does have first class null values that should be respected when deserializing back to the Ion domain objects. I assume that the only relevant lines from the deserialize method that are fixing this for you are:

Object embeddedObject = jp.getEmbeddedObject();
if (embeddedObject instanceof IonValue) {
    return (IonValue) embeddedObject;
}

So I think it would make more sense to just implement that instead (delegating to super.getNullValue(ctxt) if the condition is false).

Since this is technically breaking, can you put the PR up against 2.13? The release timeline is actually pretty soon I think.

[MartinGian]

Thanks @mcliedtke. PR is ready for you to review 🙂

[wiwanek]

@mcliedtke

Since this is technically breaking, can you put the PR up against 2.13?

If there is 2.12.6 would it be possible to include this fix there as well, or it is hard no due to change in behavior?

Asking because as I agree that this is change in behavior, this fix actually prevents data loss.

[mcliedtke]

Thanks @mcliedtke. PR is ready for you to review 🙂

Added a few minor comments.

If there is 2.12.6 would it be possible to include this fix there as well, or it is hard no due to change in behavior?
Asking because as I agree that this is change in behavior, this fix actually prevents data loss.

Some "breaking" changes (that are more like fixes) have been pushed for the Ion format in the past with 2.12 I believe, so we could consider it. With 2.13 potentially around the corner, I am not sure it would make sense if we could merge there instead and have that released soon.

With that said, I don't know if/when 2.12.6 would be released.

@cowtowncoder, do you mind sharing potential timelines for 2.13.0 and 2.12.6? Also, do we need @MartinGian added to any CCLA (likely falls under the amazon ccla)

[cowtowncoder]

No 2.12.6 for foreseeable future. Could do 2.12.5.1 micro-patch, in theory, the main complication is that would not want it for all binary format codecs.

Have been hoping to finalize 2.13.0, however, for a while now. But my new job has left me very little time to work on Jackson, alas.
Still, the goal is to release 2.13.0 before September ends (go Green Day!).

[MartinGian]

Thanks @mcliedtke, I've updated the PR.

Regarding:

Also, do we need @MartinGian added to any CCLA (likely falls under the amazon ccla)

I don't know if that question was for @cowtowncoder or for me, but I do work at Amazon if that clarifies something 🙂

[cowtowncoder]

Ah sorry missed the CCLA part: yes, if this is work for/via Amazon, CCLA will cover this from my perspective.
However, may be worth asking someone from Amazon side if they wanted to augment a list we have (I do not remember off-hand if you were included already); I think there is sort of double-layering where general inclusion should be enough, but just in case individuals are also listed.
(but that's just my reading: worth checking with OSS czar of Amazon)

[MartinGian]

I submitted a ticket to the OSPO in Amazon regarding this change. Will update here once I have a response from them. Thanks.

[hyandell]

Hi Tatu :)

All is good here Amazon OSPO wise. I'll drop you a 'please add Martin to CCLA' email, but also wanted to sprinkle happiness-indicators here so the patches can flow into that next release.

[cowtowncoder]

Hi @hyandell! Yup, figured this is the case. Thank you for confirming! Will merge tomorrow once I get back to working on Jackson stuff.