jackson-databind icon indicating copy to clipboard operation
jackson-databind copied to clipboard

JsonTypeInfo feature duplicates the already existing 'null'ed JsonTypeInfo#property property

Open me0x847206 opened this issue 1 year ago • 5 comments

Describe the bug Instances are serialized with duplicated JSON properties.

Version information 2.13.3 and not only

To Reproduce Reproduce it by combining the following 2 aspects:

  • objectMapper.setSerializationInclusion(JsonInclude.Include.ALWAYS);
  • com.fasterxml.jackson.databind.ObjectMapper#registerSubtypes(com.fasterxml.jackson.databind.jsontype.NamedType...)

Expected behavior The final JSON string value contains 1 JSON com.fasterxml.jackson.annotation.JsonTypeInfo#property field only as per the com.fasterxml.jackson.annotation.JsonTypeInfo's java doc:

 *<p>
 * On serialization side, Jackson will generate type id by itself,
 * except if there is a property with name that matches
 * {@link #property()}, in which case value of that property is
 * used instead.
 *<p>

Actual behavior The final JSON string value contains 2 JSON com.fasterxml.jackson.annotation.JsonTypeInfo#property fields. One with null and another with an auto generated value.

UPDATE

Test Case:

package com.something;

import static org.junit.jupiter.api.Assertions.assertEquals;

import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.jsontype.NamedType;
import lombok.Getter;
import lombok.Setter;
import lombok.SneakyThrows;
import org.junit.jupiter.api.Test;

/**
 * @author someone
 */
class JacksonDatabindIssues3589Test {

    @Test
    @SneakyThrows
    void demonstration() {
        final Bean bean = new Bean();
        bean.setNotIgnored("importantValue");

        final String expectedJsonValue = "{\"type\":\"beanType\",\"notIgnored\":\"importantValue\",\"ignored\":null}";
        final String actualJsonValue = createObjectMapper().writeValueAsString(bean);

        assertEquals(expectedJsonValue, actualJsonValue);
    }

    private static ObjectMapper createObjectMapper() {
        final ObjectMapper objectMapper = new ObjectMapper();

        objectMapper.setSerializationInclusion(JsonInclude.Include.ALWAYS);
        objectMapper.registerSubtypes(new NamedType(Bean.class, Bean.TYPE));

        return objectMapper;
    }

}

@Setter
@Getter
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", visible = true)
abstract class SuperBean {
    private String type;
}

@Setter
@Getter
class Bean extends SuperBean {
    static final String TYPE = "beanType";

    private String notIgnored;
    private String ignored;
}

Actual Result:

org.opentest4j.AssertionFailedError: 
Expected :{"type":"beanType","notIgnored":"importantValue","ignored":null}
Actual   :{"type":"beanType","type":null,"notIgnored":"importantValue","ignored":null}

Another Actual Result when adding the bean.setType("hello"); right before the existing bean.set... line:

org.opentest4j.AssertionFailedError: 
Expected :{"type":"hello","notIgnored":"importantValue","ignored":null}
Actual   :{"type":"beanType","type":"hello","notIgnored":"importantValue","ignored":null}

NOTE: The SuperBean class is not a must to be an abstract one and even the extend part is also not a must thing for issue to be valid. It persists anyway.

me0x847206 avatar Sep 04 '22 21:09 me0x847206

Instead of verbal description, an actual code (ideally unit test) would be needed. I don't doubt a problem exists but there is no details to actually reproduce the issue (need Java class definition at very least).

I think there may be an earlier report of this issue tho, will see if I can find it.

cowtowncoder avatar Sep 05 '22 15:09 cowtowncoder

Added an update section with requested details.

me0x847206 avatar Sep 07 '22 17:09 me0x847206

Ok, looks like you are not specifying include property for @JsonTypeInfo, so that defaults to As.PROPERTY. This implies (due to historical reasons) existence of type id only as metadata, and not as actual data ("PROPERTY" here means "include as Object Property" as opposed to "wrapper Object" or "wrapper Array" -- not referencing POJO property).

Instead, if you want type id to be associated with an actual data property (so JSON property is used BOTH as type metadata AND bound to a POJO property -- not assumed to be the case by default), you need to use As.EXISTING_PROPERTY so like:

@JsonTypeInfo(include = EXISTING_PROPERTY)

This would then output value of type as the Type Id instead of determining it from actual type.

I think that visible property might not be needed either when changing inclusion definition.

cowtowncoder avatar Sep 07 '22 23:09 cowtowncoder

Got it. Thanks for details.

I believe the visible property was set for deserialization and we can ignore it here.

I've checked test with the proposed include change and that fixed the case when I explicitly set a value for the type java field. But there appears a new inconsistency when I'm not explicitly setting it. In that case it is serialized as null instead of the expected beanType value.

According to the attached java-doc section, I believe there has to be a dynamic behavior for it and exactly:

  • in case I'm explicitly setting a non-null value then that value is serialized
  • in case the type java field has null value then the bean's json subtype type vaue is identified and serialized

Do I correctly interpret the attached java-doc?

me0x847206 avatar Sep 08 '22 00:09 me0x847206

Yes, visible is for deserialization, but I think change to inclusion may make it unnecessary.

As to null; no, there is no logic for using "either or" -- what EXISTING_PROPERTY does, essentially, is prevent TypeSerializer from writing anything. Instead a regular property is expected to be written by normal property serializer. That serializer is constrained by serialization inclusion settings. This behavior is an implementation detail, in a way, as EXISTING_PROPERTY was added long after basic @JsonTypeInfo logic (with TypeSerializer, TypeDeserializer) was implemented -- so it tried to make use of existing functionality. The big challenge was (and is) that the Type Id handling is not closely integrated with data (property) handling; rather it "wraps" property handling (so TypeSerializer will delegate to JsonSerializer on writing; and TypeDeserializer to JsonDeserializer on reading).

Above is just an explanation as background: the implementation is such that EXISTING_PROPERTY will disable use of type-derived type ids on serialization.

cowtowncoder avatar Sep 08 '22 00:09 cowtowncoder