IonValueDeserializer's getNullValue doesn't deserialize java nulls correctly

[atokuzAmzn]

Problem definition

This simple test case that I have added to IonValueDeserializerTest class fails:

    @Test
    public void shouldBeAbleToDeserializeJavaNullValue() throws Exception {
        IonValueData source = new IonValueData();
        source.put("c", null);

        IonValue data = ION_VALUE_MAPPER.writeValueAsIonValue(source);
        IonValueData result = ION_VALUE_MAPPER.readValue(data, IonValueData.class);

        assertEquals(source, result);
    }

All the Ion core types have different null values (null.struct, null.list etc) and the fix that comes with #295 provides proper deserialization of these different null types. But it doesn't handle java nulls correctly.

Apart from these different null values, Ion also has a raw null type which implements IonNull interface. Only this raw null type implements IonNull and the other null values (null.struct, null.list etc) are valid IonValues and they are NOT implementing IonNull interface.

public interface IonNull extends IonValue

When serialized, Java nulls are translated into Ion’s raw null type and they become IonNull. And naturally when deserialized it returns this IonValue (aka IonNull) which doesn’t match with Java null and causes the test listed above to fail.

Solution Proposal

We can fix this issue by adding an extra check when deserializing null values. If the embedded object is actually an IonNull then it returns java null as deserialization response. Of course this will NOT affect the other type specific null values like null.struct or null.list etc. because they are not an instance of IonNull so they will continue to be deserialized into their respective type.

Here is the proposal for getNullValue method with this extra IonNull check. (The code below also includes the fix proposal for #317)

    @Override
    public IonValue getNullValue(final DeserializationContext ctxt) throws JsonMappingException {
        try {
            final JsonParser parser = ctxt.getParser();
            if (parser.getCurrentToken() == JsonToken.VALUE_NULL) {
                final Object embeddedObj = parser.getEmbeddedObject();
                if ((embeddedObj instanceof IonValue) && !(embeddedObj instanceof IonNull)) {
                    final IonValue iv = (IonValue) embeddedObj;
                    if (iv.isNullValue()) {
                        return iv;
                    }
                }
            }
            return super.getNullValue(ctxt);
        } catch (IOException e) {
            throw JsonMappingException.from(ctxt, e.toString());
        }
    }

Remaining issue

The only problem left is when we specifically try to serialize IonNull type. Then there will be no way to discriminate these from java nulls because both of them will be converted into same Ion definition which causes deserialization to fail.

As per my understanding the test case below can never pass.

@Test
public void impossibleToDeserialize() throws Exception {
    IonValueData source = new IonValueData();
    source.put("a", null);
    source.put("b", SYSTEM.newNull()); //Creates an IonNull

    IonValue data = ION_VALUE_MAPPER.writeValueAsIonValue(source);
    System.out.println("Serialized IonValue:"+data);
    IonValueData result = ION_VALUE_MAPPER.readValue(data, IonValueData.class);

    assertEquals(source, result);
}

The reason that this test case is impossible can easily be understood by looking at the output of the debug line:

[junit] Serialized IonValue:{a:null,b:null}

Both Java null and Ion null is converted into IonNull so whatever we decide to return from deserialization either IonNull (current status) or java null (proposal) both of them will always receive the same value so we can not satisfy this test case.

As far as I know, we don’t usually serialize raw IonNulls but we definitely want to serialize Java nulls.
Also as it is mentioned in Ion Specification the raw null type in Ion exists primarily for compatibility reasons. Since they are used to represent java nulls in ion world and when deserialized they need to be converted back to java nulls.

If we move forward with the proposal we need to remove this test case from IonValueDeserializerTest suite since it will fail.

 @Test
 public void shouldBeAbleToDeserializeNullValue() throws Exception {
     IonValue ion = SYSTEM.newNull();

     IonValue data = ION_VALUE_MAPPER.readValue(ion, IonValue.class);

     assertEquals(ion, data);
 }

[atokuzAmzn]

I pushed a PR with proposed changes for you to review:#319

[cowtowncoder]

Also related, merged: #317.

[cowtowncoder]

I don't think there are plans to do this, looking at closed PR #317. Closing.