Skip to content

Fix #571: Unable to deserialize a pojo with IonStruct #573

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 11, 2025
Merged

Fix #571: Unable to deserialize a pojo with IonStruct #573

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0510423
Fix #571: Unable to deserialize a pojo with IonStruct
seadbrane Apr 9, 2025
c92fce8
Fix #571: Address PR comments
seadbrane Apr 11, 2025
b606b31
Minor tweaks
cowtowncoder Apr 11, 2025
cebe606
Update release notes
cowtowncoder Apr 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions ion/src/main/java/com/fasterxml/jackson/dataformat/ion/IonParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ public enum Feature implements FormatFeature // in 2.12
* @since 2.12.3
*/
USE_NATIVE_TYPE_ID(true),
/**
* Whether to convert "null" to an IonValueNull (true);
* or leave as a java null (false) when deserializing.
*<p>
* Enabled by default for backwards compatibility as that has been the behavior
* of `jackson-dataformat-ion` since 2.13.
*
* @see <a href="https://amzn.github.io/ion-docs/docs/spec.html#annot">The Ion Specification</a>
*
* @since 2.19.0
*/
READ_NULL_AS_IONVALUE(true),
;

final boolean _defaultState;
Expand Down Expand Up @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of tests fail if this _updateToken(JsonToken.VALUE_EMBEDDED_OBJECT); is removed. Perhaps a 3.0 cleanup activity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Makes sense there is a reason -- should have caught when these were added (or added comments to explain why/how)

if (_valueToken == JsonToken.VALUE_NULL && !IonType.isContainer(v.getType())) {
return null;
}
}
return v;
}

Expand Down
57 changes: 52 additions & 5 deletions ion/src/main/java/com/fasterxml/jackson/dataformat/ion/ionvalue/IonValueDeserializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,51 @@

import java.io.IOException;

import com.amazon.ion.*;

import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonToken;
import com.fasterxml.jackson.databind.*;
import com.fasterxml.jackson.databind.BeanProperty;
import com.fasterxml.jackson.databind.DeserializationContext;
import com.fasterxml.jackson.databind.JavaType;
import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.deser.ContextualDeserializer;
import com.fasterxml.jackson.databind.JsonDeserializer;
import com.fasterxml.jackson.databind.util.AccessPattern;
import com.fasterxml.jackson.dataformat.ion.IonParser;
import com.amazon.ion.IonContainer;
import com.amazon.ion.IonList;
import com.amazon.ion.IonSexp;
import com.amazon.ion.IonStruct;
import com.amazon.ion.IonSystem;
import com.amazon.ion.IonType;
import com.amazon.ion.IonValue;
import com.amazon.ion.Timestamp;

/**
* Deserializer that knows how to deserialize an IonValue.
*/
class IonValueDeserializer extends JsonDeserializer<IonValue> {
class IonValueDeserializer extends JsonDeserializer<IonValue> implements ContextualDeserializer {

private final JavaType targetType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Member variables with leading underscore -> _targetType


public IonValueDeserializer() {
this.targetType = null;
}

public IonValueDeserializer(JavaType targetType) {
this.targetType = targetType;
}

@Override
public JsonDeserializer<?> createContextual(DeserializationContext ctxt, BeanProperty property) {
JavaType contextualType = (property != null)
? property.getType()
: ctxt.getContextualType(); // fallback
return new IonValueDeserializer(contextualType);
}

@Override
public IonValue deserialize(JsonParser jp, DeserializationContext ctxt) throws IOException {

Object embeddedObject = jp.getEmbeddedObject();
if (embeddedObject instanceof IonValue) {
return (IonValue) embeddedObject;
Expand Down Expand Up @@ -62,17 +92,34 @@ public IonValue getNullValue(DeserializationContext ctxt) throws JsonMappingExce
if (embeddedObj instanceof IonValue) {
IonValue iv = (IonValue) embeddedObj;
if (iv.isNullValue()) {
if (IonType.isContainer(iv.getType())) {
return iv;
}
IonType containerType = getIonContainerType();
if (containerType != null) {
IonSystem ionSystem = ((IonParser) parser).getIonSystem();
return ionSystem.newNull(containerType);
}
return iv;
}
}
}

return super.getNullValue(ctxt);
} catch (IOException e) {
throw JsonMappingException.from(ctxt, e.toString());
}
}

private IonType getIonContainerType() {
if (targetType != null) {
Class<?> clazz = targetType.getRawClass();
if (IonStruct.class.isAssignableFrom(clazz)) return IonType.STRUCT;
if (IonList.class.isAssignableFrom(clazz)) return IonType.LIST;
if (IonSexp.class.isAssignableFrom(clazz)) return IonType.SEXP;
}
return null;
}

@Override
public AccessPattern getNullAccessPattern() {
return AccessPattern.DYNAMIC;
Expand Down
78 changes: 61 additions & 17 deletions ...src/test/java/com/fasterxml/jackson/dataformat/ion/ionvalue/IonValueDeserializerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
package com.fasterxml.jackson.dataformat.ion.ionvalue;

import java.io.IOException;
import java.util.*;

import com.amazon.ion.*;
import com.amazon.ion.IonSystem;
import com.amazon.ion.IonValue;
import com.amazon.ion.IonStruct;
import com.amazon.ion.system.IonSystemBuilder;
import org.junit.jupiter.api.Test;

import com.fasterxml.jackson.annotation.*;
import com.fasterxml.jackson.annotation.JsonAnyGetter;
import com.fasterxml.jackson.annotation.JsonAnySetter;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.util.AccessPattern;
import com.fasterxml.jackson.dataformat.ion.IonObjectMapper;
import com.fasterxml.jackson.dataformat.ion.IonParser;


import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;

import org.junit.jupiter.api.Test;

import static com.fasterxml.jackson.databind.PropertyNamingStrategies.SNAKE_CASE;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change from static to per-instance? Did it fail otherwise? (possibly due to null-value caching)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@Test
public void shouldBeAbleToDeserialize() throws Exception {
Expand All @@ -92,23 +100,49 @@ public void shouldBeAbleToDeserializeIncludingNullList() throws Exception {
}

@Test
public void shouldBeAbleToDeserializeNullList() throws Exception {
IonValue ion = ion("{c:null.list}");

IonValueData data = ION_VALUE_MAPPER.readValue(ion, IonValueData.class);
public void shouldBeAbleToDeserializeNullToIonNull() throws Exception {
String ion = "{c:null}";
verifyNullDeserialization(ion, SYSTEM.newNull());
ION_VALUE_MAPPER.disable(IonParser.Feature.READ_NULL_AS_IONVALUE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

verifyNullDeserialization(ion, null);
}

assertEquals(1, data.getAllData().size());
assertEquals(SYSTEM.newNullList(), data.getAllData().get("c"));
@Test
public void shouldBeAbleToDeserializeNullList() throws Exception {
String ion = "{c:null.list}";
verifyNullDeserialization(ion, SYSTEM.newNullList());
ION_VALUE_MAPPER.disable(IonParser.Feature.READ_NULL_AS_IONVALUE);
verifyNullDeserialization(ion, SYSTEM.newNullList());
}

@Test
public void shouldBeAbleToDeserializeNullStruct() throws Exception {
IonValue ion = ion("{c:null.struct}");
String ion = "{c:null.struct}";
verifyNullDeserialization(ion, SYSTEM.newNullStruct());
ION_VALUE_MAPPER.disable(IonParser.Feature.READ_NULL_AS_IONVALUE);
verifyNullDeserialization(ion, SYSTEM.newNullStruct());
}

IonValueData data = ION_VALUE_MAPPER.readValue(ion, IonValueData.class);
@Test
public void shouldBeAbleToDeserializeNullSexp() throws Exception {
String ion = "{c:null.sexp}";
verifyNullDeserialization(ion, SYSTEM.newNullSexp());
ION_VALUE_MAPPER.disable(IonParser.Feature.READ_NULL_AS_IONVALUE);
verifyNullDeserialization(ion, SYSTEM.newNullSexp());
}

private void verifyNullDeserialization(String ionString, IonValue expected) throws Exception {

IonValueData data = ION_VALUE_MAPPER.readValue(ionString, IonValueData.class);

assertEquals(1, data.getAllData().size());
assertEquals(expected, data.getAllData().get("c"));

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

assertEquals(1, data.getAllData().size());
assertEquals(SYSTEM.newNullStruct(), data.getAllData().get("c"));
assertEquals(expected, data.getAllData().get("c"));
}

@Test
Expand Down Expand Up @@ -162,7 +196,17 @@ public void shouldBeAbleToSerializeAndDeserializeStringData() throws Exception {

IonValue data = ION_VALUE_MAPPER.writeValueAsIonValue(source);
StringData result = ION_VALUE_MAPPER.parse(data, StringData.class);
assertEquals(source, result);
}

@Test
public void shouldBeAbleToSerializeAndDeserializeStringDataAsString() throws Exception {
StringData source = new StringData();
source.put("a", "1");
source.put("b", null);

String data = ION_VALUE_MAPPER.writeValueAsString(source);
StringData result = ION_VALUE_MAPPER.readValue(data, StringData.class);
assertEquals(source, result);
}

Expand Down