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

`FactoryBasedEnumDeserializer` unable to deserialize enum object with Polymorphic Type Id ("As.WRAPPER_ARRAY") - fails on START_ARRAY token

Open nitinsatapara opened this issue 1 year ago • 7 comments

Describe the bug FactoryBasedEnumDeserializer is unable to deserialize enum value which is wrapped in Array.

Version information This is for Jackson 2.13.1 - It worked fine for release 2.10.1

To Reproduce If you have a way to reproduce this with:

public class EnumDeserializeTest {

    public static void main(String[] args) throws IOException {
        ObjectMapper mapper = new ObjectMapper();
        GenericJackson2JsonRedisSerializer serializer = new GenericJackson2JsonRedisSerializer();
        Frequency frequency = Frequency.DAILY;
        byte[] frequencyAsBytes = serializer.serialize(frequency);
        Frequency frequencyDeserialized = mapper.readValue(frequencyAsBytes, Frequency.class);
    }
}

Value is serialized as : ["Frequency","DAILY"]

This results in exception:

Exception in thread "main" com.fasterxml.jackson.databind.exc.ValueInstantiationException: Cannot construct instance of Frequency, problem: Unexpected value '' at [Source: (byte[])"["Frequency","DAILY"]"; line: 1, column: 21] at com.fasterxml.jackson.databind.exc.ValueInstantiationException.from(ValueInstantiationException.java:47) at com.fasterxml.jackson.databind.DeserializationContext.instantiationException(DeserializationContext.java:2047) at com.fasterxml.jackson.databind.DeserializationContext.handleInstantiationProblem(DeserializationContext.java:1400) at com.fasterxml.jackson.databind.deser.std.FactoryBasedEnumDeserializer.deserialize(FactoryBasedEnumDeserializer.java:182) at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:323) at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4674) at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3690) at EnumDeserializeTest.main(EnumDeserializeTest.java:26)

Expected behavior Deserialization should work fine with FactoryBasedEnumDeserializer but fails when it encounters START_ARRAY token. EnumDeserializer works just fine and it is able to parse the array tokens and retrieves the enum value. Similarly, FactoryBasedEnumDeserializer should also work.

Additional context This issue is faced when using GenericJackson2JsonRedisSerializer. A change was made to this serialiser in Spring-data-redis 2.7.2 which uses JsonTypeInfo.Id.CLASS annotation as default for all types. Prior to this release, enum types were serialised as simple/plain values but with this change they are wrapped in an array where 1st element is denoted for class and 2nd element holds the enum value.

nitinsatapara avatar Aug 09 '22 11:08 nitinsatapara

Thank you for reporting the issue, as well as including a reproduction. Unfortunately I would need bit more information (ideally a stand-alone test case):

  • Definition of Frequency (I assume it's a simple Enum but still)
  • Configuration of ObjectMapper (wrt Default Typing, I think)
  • Whatever write code GenericJackson2JsonRedisSerializer uses.

Wrapping in an array is likely for AS_ARRAY inclusion type of Default Typing. It really wouldn't be needed for Enums but I assume this is just generic interface for any and all types.

cowtowncoder avatar Aug 09 '22 22:08 cowtowncoder

@cowtowncoder Thanks for your prompt response.

Here's the Frequency Class:

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonValue;

public enum Frequency {
    DAILY("DAILY"),
    WEEKLY("WEEKLy");
    private String value;

    Frequency(String value) {
        this.value = value;
    }

    @JsonValue
    public String getValue() {
        return value;
    }

    @Override
    public String toString() {
        return String.valueOf(value);
    }

    @JsonCreator
    public static Frequency fromValue(String value) {
        for (Frequency frequency : Frequency.values()) {
            if (frequency.value.equals(value)) {
                return frequency;
            }
        }
        throw new IllegalArgumentException("Unexpected value '" + value + "'");
    }
}

Here's how ObjectMapper is used inside GenericJackson2JsonRedisSerializer [FYI - the below snippet is from Spring-data-redis 2.7.2 version]

public GenericJackson2JsonRedisSerializer(@Nullable String classPropertyTypeName) {

		this(new ObjectMapper());

		// simply setting {@code mapper.disable(SerializationFeature.FAIL_ON_EMPTY_BEANS)} does not help here since we need
		// the type hint embedded for deserialization using the default typing feature.
		registerNullValueSerializer(mapper, classPropertyTypeName);

		StdTypeResolverBuilder typer = new TypeResolverBuilder(DefaultTyping.EVERYTHING,
				mapper.getPolymorphicTypeValidator());
		typer = typer.init(JsonTypeInfo.Id.CLASS, null);
		typer = typer.inclusion(JsonTypeInfo.As.PROPERTY);

		if (StringUtils.hasText(classPropertyTypeName)) {
			typer = typer.typeProperty(classPropertyTypeName);
		}
		mapper.setDefaultTyping(typer);
	}

Please do let me know if you need more info.

In my perspective, wrapping enum values during serialisation is fine - having 1st element as class and 2nd element as value. This can be deserialised properly by EnumDeserializer but not by FactoryBasedEnumDeserializer. There is an exception if such kind of enum value is deserialised by FactoryBasedEnumDeserializer.

nitinsatapara avatar Aug 10 '22 21:08 nitinsatapara

Ok, this is quite close, but I think the last part is just to tie it all together in full reproduction.

One problem I do see is use of DefaultTyping.EVERYTHING -- this should almost NEVER be used for anything, anywhere, by anyone. I think my adding of that setting due to user request was a mistake; and I have yet to see a case where it makes sense and works well.

But having said that, I also think code also should rather use JsonTypeInfo.As.WRAPPER_ARRAY since PROPERTY cannot really work for Enums (since they are not serialized as JSON Objects -- and only Objects have properties). As a result Jackson will actually simply use WRAPPER_ARRAY anyway. So that setting may not make much difference for the issue. But if this was my code I'd definitely change that.

So the wrapping into 2-element array is done by Polymorphic type handler, not as part of Enum (value) serializer or deserializer -- type handlers are TypeDeserializer and TypeSerializer. Actual value deserializer like FactoryBasedEnumDeserializer does delegate calls to TypeDeserializer.

What the test should be able to show, I think, is where does the wrapping/unwrapping fail.

But I am also bit suspicious of custom StdTypeResolverBuilder in code above: that is advanced functionality and easy to get wrong. And in fact I am not sure why it is used here, actually; ObjectMapper has activateDefaultTyping methods that should work fine (even include EVERYTHING option that I dislike). I guess the only reason it is being used is the insistence of using As.PROPERTY with specific classPropertyTypeName -- if As.WRAPPER_ARRAY was used instead (which makes much more sense) -- this wouldn't be needed.

cowtowncoder avatar Aug 11 '22 03:08 cowtowncoder

One last thing: by "full" reproduction all I mean is that from the original examples write + read things would go through without references to any external code outside of jackson-databind.

cowtowncoder avatar Aug 11 '22 03:08 cowtowncoder

@cowtowncoder As you quoted: Actual value deserializer like FactoryBasedEnumDeserializer does delegate calls to TypeDeserializer. Here, inside FactoryBasedEnumDeserializer, it doesn't anticipate START_ARRAY token. When it encounters it, it simply sets value as an empty string and skips the children. It doesn't delegate calls to Deserialiser.

I agree with your thought process. Having WRAPPER_ARRAY makes more sense. However, I am bit surprised at the behaviour where EnumDeserializer works fine to deserialize enum values but FactoryBasedEnumDeserializer fails.

nitinsatapara avatar Aug 11 '22 10:08 nitinsatapara

@cowtowncoder Do you need any details from my side?

nitinsatapara avatar Aug 11 '22 10:08 nitinsatapara

I think this makes sense. I do need a unit test however, ideally stripped to minimal size. I can make one eventually, but will likely not have time in near future to work on this issue (huge backlog of things). But eventually I'll get here, and information here should be enough for me or whoever else has time to dig into it.

The main open question for me is whether this would be an issue with FactoryBasedEnumDeserializer with standard default typing configuration (bigger problem); or if this is due to custom set up with StdTypeResolverBuilder (less likely to affect users, so lesser problem).

cowtowncoder avatar Aug 11 '22 22:08 cowtowncoder

@cowtowncoder In response to your latest comment: In my perspective, this would be an issue with FactoryBasedEnumDeserializer with standard default typing configuration (bigger problem).

I have shared the unit test above. Would you require a trimmed/minimal version of the same? I think the one I mention above is the most basic one and doesn't have any extra details. If you think I can get rid of anything, let me know. I am happy to provide any details/code which you may require to get things going. Happy to contribute !

nitinsatapara avatar Nov 29 '22 16:11 nitinsatapara

@nitinsatapara yes, reproduction needs to have no external dependencies (no Spring), that's the main thing.

I am bit concerned by use of custom setup of default typing, including EVERYTHING inclusion criteria but I guess first things first; isolated reproduction would be the right next step (ideally only activateDefaultTyping() on ObjectMapper was used, if that is enough to trigger the issue, but that's more a follow-up).

cowtowncoder avatar Nov 29 '22 19:11 cowtowncoder

@cowtowncoder I have an example for you. This is impacting me as well, even with DefaultType.NON_FINAL and JsonTypeInfo.As.WRAPPER_ARRAY

    @Test
    public void iBlowUpForFun()
            throws JsonProcessingException {
        ObjectMapper objectMapper = new ObjectMapper();
        // enables Default Typing
        objectMapper.enableDefaultTyping(DefaultTyping.NON_FINAL
                , As.WRAPPER_ARRAY);
        Foo<Bar> expected = new Foo<>();
        expected.setItem(Bar.ENABLED);

        String serialized = objectMapper.writeValueAsString(expected);
        JavaType javaType =
                objectMapper.getTypeFactory().constructParametricType(Foo.class
                        , new Class[]{Bar.class});
        Set<Bar> deserialized =
                objectMapper.readValue(serialized, javaType);
        assertEquals(deserialized,
                expected, "Bar does not match");
    }

with Foo and Bar defined as

    public class Foo<T> {
        private T item;
        public T getItem() {
            return item;
        }
        public void setItem(T item) {
            this.item = item;
        }

        @Override
        public boolean equals(Object compare) {
            if(this.item == null) {
                return compare == null;
            }
            return this.item.equals(compare);
        }

        @Override
        public int hashCode() {
            if(this.item == null) {
                return 0;
            }
            return this.item.hashCode();
        }
    }
    public enum Bar {
        ENABLED,
        DISABLED,
        HIDDEN;
        @JsonCreator
        public static Bar fromValue(String value) {
            String upperVal = value.toUpperCase();

            for (Bar enumValue : Bar.values()) {
                if (enumValue.name().equals(upperVal)) {
                    return enumValue;
                }
            }
            throw new IllegalArgumentException("Bad input [" + value + "]");
        }
    }

I get an error deserializing the string back. Note that the input shows as an empty string.

com.fasterxml.jackson.databind.exc.ValueInstantiationException: Cannot construct instance of ...Bar, problem: Bad input [] at [Source: (String)"["com.kareo.services.auxiliary.featuretoggle.implementation.auditing.AuditTest$Foo",{"item":["com.kareo.services.auxiliary.featuretoggle.implementation.auditing.AuditTest$Bar","ENABLED"]}]"; line: 1, column: 186] (through reference chain: com.kareo.services.auxiliary.featuretoggle.implementation.auditing.AuditTest$Foo["item"])

As far a I can tell, the issue is with FactoryBasedEnumDeserializer#deserialize:

JsonToken t = p.currentToken();
        if (t != null && !t.isScalarValue()) {
            value = "";
            p.skipChildren();
        } 

Is the wrong deserializer being used somehow?

rmekhail-tebra avatar Oct 11 '23 16:10 rmekhail-tebra

First of, thank you @rmekhail-tebra for the reproduction! This is super helpful.

As to the issue, it is difficult to say without digging deeper: this is pretty complex case that combines 2 things that do not play nicely together -- polymorphic deserialization and generic Java types.

cowtowncoder avatar Oct 11 '23 22:10 cowtowncoder

thanks @rmekhail-tebra for sharing the test case. I faced exactly the same error. @cowtowncoder From the debugging that I have done, it seems the issue lies in FactoryBasedEnumDeserializer - at the point where it encounters 'Array token'. It simply abrupts from there - It's like it doesn't want to process any sort of array token at all.

nitinsatapara avatar Oct 13 '23 09:10 nitinsatapara

I think this issue was resolved in later versions, judging by the implementation changes in 2.15 per GitHub. For now I get around this issue by converting the enum to a string since upgrading Jackson isn't an option ATM

rmekhail-tebra avatar Oct 13 '23 15:10 rmekhail-tebra

@rmekhail-tebra If there is any chance you could see if your test case did pass on 2.15.2 (or, 2.15.3 released yesterday), that'd be very useful information to add.

cowtowncoder avatar Oct 13 '23 18:10 cowtowncoder

Again, thank you for super helpful reproduction! I tried testing in latest 2.15 branch, but still no luck 🤔. So I filed #4159 that may resolve the issue. Do you mind testing on your side also, @rmekhail-tebra?

JooHyukKim avatar Oct 15 '23 12:10 JooHyukKim

One problem I do see is use of DefaultTyping.EVERYTHING -- this should almost NEVER be used for anything, anywhere, by anyone. I think my adding of that setting due to user request was a mistake; and I have yet to see a case where it makes sense and works well.

Filed an issue for slowly fading out DefaultTyping.EVERYTHING https://github.com/FasterXML/jackson-databind/issues/4160

JooHyukKim avatar Oct 15 '23 12:10 JooHyukKim

Here is working example in 2.15. Works as expected, as mentioned above.

public class TestDefaultForEnums
{
    static class Foo3569<T> {
        public T item;
    }

    enum Bar3569 {
        ENABLED, DISABLED, HIDDEN;

        @JsonCreator
        public static Bar3569 fromValue(String value) {
            String upperVal = value.toUpperCase();
            for (Bar3569 enumValue : Bar3569.values()) {
                if (enumValue.name().equals(upperVal)) {
                    return enumValue;
                }
            }
            throw new IllegalArgumentException("Bad input [" + value + "]");
        }
    }

    public void testEnumAsWrapperArrayWithCreator() throws JsonProcessingException
    {
        ObjectMapper objectMapper = JsonMapper.builder()
                .activateDefaultTyping(
                        new DefaultBaseTypeLimitingValidator(),
                        ObjectMapper.DefaultTyping.EVERYTHING,
                        JsonTypeInfo.As.WRAPPER_ARRAY)
                .build();

        Foo3569<Bar3569> expected = new Foo3569<>();
        expected.item = Bar3569.ENABLED;

        // First, serialize
        String serialized = objectMapper.writeValueAsString(expected);

        // Then, deserialize with TypeReference
        assertNotNull(objectMapper.readValue(serialized, new TypeReference<Foo3569<Bar3569>>() {}));
        // And, also try as described in [databind#3569]
        JavaType javaType = objectMapper.getTypeFactory().constructParametricType(Foo3569.class, new Class[]{Bar3569.class});
        assertNotNull(objectMapper.readValue(serialized, javaType));
    }
}

JooHyukKim avatar Oct 16 '23 23:10 JooHyukKim

Considered fixed or work-around-able, via #4159.

cowtowncoder avatar Oct 18 '23 03:10 cowtowncoder