Skip to content

Unable to deserialize a pojo with IonStruct #571

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.

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

Closed
seadbrane opened this issue Apr 7, 2025 · 6 comments · Fixed by #573
Closed

Unable to deserialize a pojo with IonStruct #571

seadbrane opened this issue Apr 7, 2025 · 6 comments · Fixed by #573
Milestone

Comments

@seadbrane
Copy link
Contributor

seadbrane commented Apr 7, 2025

I am not seeing a good option to serialize/deserialize a simple Pojo with a null IonStruct.

The issue is that by default a null IonStruct gets serialized to null but due to this change #296 that gets converted to a an IonNull, and fails with argument type mismatch.

Failing testing:

  public static class Pojo {    
        @JsonCreator
        public Pojo(@JsonProperty("name") String name, @JsonProperty("data") IonStruct data) {   
            this.name = name;
            this.data = data;
        }
        public String name;
        public IonStruct data;
        
        
        @Override
        public int hashCode() {
            return Objects.hash(data, name);
        }
        @Override
        public boolean equals(Object obj) {
            if (this == obj)
                return true;
            if (obj == null)
                return false;
            if (getClass() != obj.getClass())
                return false;
            Pojo other = (Pojo) obj;
            return Objects.equals(data, other.data) && Objects.equals(name, other.name);
        }
    }

    private final IonSystem ionSystem = IonSystemBuilder.standard().build();
    private final IonValueMapper mapper = new IonValueMapper(ionSystem);

    @Test
    public void testRoundTrip() throws JsonProcessingException {
        Pojo p = new Pojo("test", null);
        String value =  mapper.writeValueAsString(p);
        Assert.assertEquals("{name:\"test\",data:null}", value);
        Pojo p2 = mapper.readValue(value, Pojo.class);
        Assert.assertEquals(p, p2);
    }
@seadbrane
Copy link
Contributor Author

One potential partial fix is to make IonValueDeserializer into a ContextualDeserializer and then create the correct IonNull container type.

class IonValueDeserializer extends JsonDeserializer<IonValue> implements ContextualDeserializer {
+
+    private final JavaType 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;
@@ -65,17 +92,34 @@ class IonValueDeserializer extends JsonDeserializer<IonValue> {
                 if (embeddedObj instanceof IonValue) {
                     IonValue iv = (IonValue) embeddedObj;
                     if (iv.isNullValue()) {
+                        if (IonType.isContainer(iv.getType())) {
+                            return iv;
+                        }
+                        IonType containerType = getContainerType();
+                        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 getContainerType() {
+        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;
+    }
+

However, I do not see anyway good way to handle the roundtrip test above since there is no way to distinguish between a java null and an IonNull for non Ion container types, since they both get serialized to null. So maybe a new Parser feature?

@@ -52,6 +52,18 @@ public class IonParser
          * @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;
@@ -562,6 +574,12 @@ public class IonParser
         writer.writeValue(_reader);
         IonValue v = l.get(0);
         v.removeFromContainer();
+
+        if (v.isNullValue() && !Feature.READ_NULL_AS_IONVALUE.enabledIn(_formatFeatures)) {
+            if (_valueToken == JsonToken.VALUE_NULL && !IonType.isContainer(v.getType())) {
+                return null;
+            }
+        }
         return v;
     }

@cowtowncoder
Copy link
Member

I was about to suggest a new parser feature, yes. Sounds like the way to go -- otherwise trying to deduce which way to go seems complicated and fragile.

seadbrane added a commit to seadbrane/jackson-dataformats-binary that referenced this issue Apr 9, 2025
@seadbrane
Copy link
Contributor Author

PR: #573

@seadbrane
Copy link
Contributor Author

I have run into a few more cases of the original change breaking expectations. I am inclined to default the new parser feature to false. Thoughts?

@cowtowncoder
Copy link
Member

@seadbrane I'll trust your judgment here. Related question, if false for 2.x, would it make sense to switch for Jackson 3.0?

@seadbrane
Copy link
Contributor Author

seadbrane commented Apr 10, 2025

Let's keep the default true for 2.x. This is the cleaner option when JsonInclude.Include.NON_NULL is also set, since the Java nulls will be omitted in the serialized output, but not the properties set to IonNull.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants