Deserializing polymorphic type with defaultImpl sets type ID to null

[joschi]

Describe the bug

Starting with Jackson Databind 2.13.0, deserializing a polymorphic type with a default implementation (JsonTypeInfo.defaultImpl) and an explicit null for the type ID will produce an instance of the concrete type in which the type ID is null instead of a value provided during construction (in its constructor/@JsonCreator method).

In Jackson Databind 2.12.5 and earlier, this produced an instance of the concrete type in which the type ID field had the value assigned in the constructor/@JsonCreator method.

We're using Jackson Databind to deserialize user-provided input, so unfortunately we cannot prevent them from sending JSON payloads with null in the type ID field, even if it isn't valid in the first place.

Version information

Working on Jackson Databind 2.12.5.
Failing on Jackson Databind 2.13.0.

To Reproduce

class PolymorphicTest {
    @Test
    void deserializingPolymorphicTypeFillsTypeInfoField() throws JsonProcessingException {
        String s = "{\"@type\": null, \"custom\": \"value\"}";

        final BaseClass object = new ObjectMapper().readValue(s, BaseClass.class);

        assertTrue(object instanceof TypeA);
        assertEquals(TypeA.TYPE, object.type);
        assertEquals("value", ((TypeA) object).customA);
    }

    @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, defaultImpl = TypeA.class)
    @JsonSubTypes({
            @JsonSubTypes.Type(value = TypeA.class, name = TypeA.TYPE),
            @JsonSubTypes.Type(value = TypeB.class, name = TypeB.TYPE)
    })
    static abstract class BaseClass {
        @JsonTypeId
        @JsonProperty("@type")
        final String type;

        @JsonCreator
        BaseClass(@JsonProperty("@type") String type) {
            this.type = type;
        }
    }

    static class TypeA extends BaseClass {
        static final String TYPE = "A";
        String customA;

        @JsonCreator
        TypeA(@JsonProperty("custom") String custom) {
            super(TYPE);
            this.customA = custom;
        }
    }

    static class TypeB extends BaseClass {
        static final String TYPE = "B";
        String customB;

        @JsonCreator
        TypeB(@JsonProperty("custom") String custom) {
            super(TYPE);
            this.customB = custom;
        }
    }
}

Expected behavior

With Jackson Databind 2.12.5, the unit test succeeds while with Jackson Databind 2.13.0 it fails because object.type is null instead of using the value of the default implementation TypeA ("A"):

org.opentest4j.AssertionFailedError: expected: <A> but was: <null>
	at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
	at app//org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
	at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
	at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
	at app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1141)
	at app//jackson.polymorphic.bug.PolymorphicTest.deserializingPolymorphicTypeFillsTypeInfoField(PolymorphicTest.java:24)

Additional context

I suspect this behavior was changed in the context of #3271 and 32eee1b.

If the new behavior is intentional, it should be mentioned in the release notes for Jackson 2.13.0 as a breaking change.

[cowtowncoder]

No, I don't think this was intentional change.

However, I am not sure about the test -- does it default to inclusion of As.PROPERTY? Whereas to actually pass type id as regular property it should be As.EXISTING_PROPERTY. So I
I don't know how that'd have worked before however.

Note that @JsonTypeId annotation only affects serialization; during deserialization it has no effect.

[joschi]

However, I am not sure about the test -- does it default to inclusion of As.PROPERTY?

Yes, it's using the default of JsonTypeInfo.As.PROPERTY.

Whereas to actually pass type id as regular property it should be As.EXISTING_PROPERTY.

The issue is reproducible with both, JsonTypeInfo.As.EXISTING_PROPERTY and JsonTypeInfo.As.PROPERTY.

[cowtowncoder]

Ah.

After quickly reading this again, I am not sure this is a bug -- if input has null, then Type Id is null. It should not magically default to something else. If this happened in the past, that would seem like a bug.
That there is a difference in behavior is of course unfortunate.

[joschi]

@cowtowncoder I'm fine with either outcome, either reverting to the old behavior with regards to the null-handling for the type identifier property, or adding it as a breaking change to the Jackson 2.13.0 release notes. 😉

[benneq]

I think I stumbled across the same (or at least similar) issue.

With Jackson 2.12.5 this was working fine with { type: null }:

@JsonTypeInfo(
		use = JsonTypeInfo.Id.NAME,
		include = JsonTypeInfo.As.PROPERTY,
		property = "type"
)
@JsonTypeIdResolver(MyClassType.Resolver.class)
public abstract class MyClass<T> {
	protected MyClass(String type) {
		this.type = type;
	}
	
	private String type;
	public String getType() { return type; }
	public void setType(String type) { this.type = type; }
}

public class MyClassType {
	private static final Map<String, Class<? extends MyClass<?>>> typeMap = ...;

	private static final Class<? extends MyClass<?>> defaultValue = ...;

	public static class Resolver extends TypeIdResolverBase {
		@Override
		public String idFromValue(Object value) {
			return idFromValueAndType(value, null);
		}
		@Override
		public String idFromValueAndType(Object value, Class<?> suggestedType) {
			return ((MyClass<?>) value).getType();
		}
		@Override
		public JavaType typeFromId(DatabindContext context, String id) {
			return TypeFactory.defaultInstance().constructType(typeMap.getOrDefault(id, defaultValue));
		}
		@Override
		public Id getMechanism() {
			return Id.NAME;
		}
	}
}

And with Jackson 2.13.0 the overridden Resolver methods won't get called, but instead I get:

com.fasterxml.jackson.databind.exc.InvalidTypeIdException: Could not resolve subtype of [simple type, class MyClass<java.lang.Object>]: missing type id property 'type'

@joschi Do you have a workaround to convince Jackson 2.13.0 to allow null values?

[joschi]

@benneq We downgraded to Jackson 2.12.5 until we have time to work around the issue in our own code or the old behavior has been restored in a Jackson 2.13.x patch release.

[cowtowncoder]

Quick note: my definite leaning is to document this (contributions for verbiage would be appreciated but I'll try to add something), and not change behavior.

I updated 2.13 release notes with an entry wrt jackson-databind behavior change:

https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.13

[b3nk4n]

Hi @cowtowncoder,

After quickly reading this again, I am not sure this is a bug -- if input has null, then Type Id is null. It should not magically default to something else. If this happened in the past, that would seem like a bug.
That there is a difference in behavior is of course unfortunate.

Are you sure this was a wise decision to accept this as the new expected behavior? Because for me, this is very counter intuitive. Especially by taking @joschi 's provided example:

static class TypeA extends BaseClass {
    static final String TYPE = "A";  // <<< this is FINAL
    String customA;

    @JsonCreator
    TypeA(@JsonProperty("custom") String custom) {
        super(TYPE);                 // <<< explicitly setting the type for a FINAL field in constructor
        this.customA = custom;
    }
}

if the use of e.g. the following in the code, which was completely uncritical before, but now causes an NPE or unexpected behaviour out of a sudden:

if (obj.getType().equals(TypeA.TYPE)) { ... } // NPE with 2.13.0+

if (TypeA.TYPE.equals(obj.getType())) { ... }  // evaluates to false in 2.13.0+

Just by reading the code, never on earth would I expect that type could ever be null and now suddenly can cause an NPE in my code after upgrading to 2.13.0. Because how should you know that a final variable is now suddenly assigned twice, which is kind of against the rules of final in Java or constants in general:

In the Java programming language, the final keyword is used in several contexts to define an entity that can only be assigned once.
(Wikipedia)

In my point of view, this change is a quite significant breaking change.