Skip to content
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

BeanDeserializer updates currentValue incorrectly when deserialising empty Object #4184

Closed
nocny-x opened this issue Nov 2, 2023 · 8 comments
Labels
has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue

Comments

@nocny-x
Copy link

nocny-x commented Nov 2, 2023

          Would need a reproduction; may be re-opened/re-filed with one.

Originally posted by @cowtowncoder in #1834 (comment)

{
    "field1": {},
    "field2": {
	  "value": "A"
    }
}

field2 has a deserializer and get the the context's current value in deserialize method, the context's current value is the value of field1.

Field2 Deserializer code snippet:

@Override
public TypeOfFeild2 deserialize(JsonParser jp, DeserializationContext ctxt) throws IOException {
    Object currentValue = jp.getCurrentValue(); // currentValue is the value of field1 here, not parent's value.
    // ...
}
@JooHyukKim
Copy link
Member

JooHyukKim commented Nov 2, 2023

There is still no reproduction yet, makes no difference from original #1834 labeld as "need-test-case"🤔.

You may refer to issues that are labeled as "has-failing-test" for reproductions are provided --the reproduction would be complete, but minimal.

@cowtowncoder
Copy link
Member

cowtowncoder commented Nov 2, 2023

Still need reproduction, no work will be done without one. As @JooHyukKim suggested, verbal description is not reproduction.

But one thing to note is that deserializers do not need to reset anything: current value should be bound at correct level, so as long as they do not overwrite current values it should all work due to nesting (different read/write context levels have different current values).

@cowtowncoder cowtowncoder added the need-test-case To work on issue, a reproduction (ideally unit test) needed label Nov 2, 2023
@nocny-x
Copy link
Author

nocny-x commented Nov 3, 2023

@JooHyukKim @cowtowncoder

Reproduction:

public class JsonTest {

    final static ObjectMapper mapper = new ObjectMapper();

    @Test
    void parsed() throws JsonProcessingException {
        String json = "{\"role\": {\"name\": \"Manager\"}, \"type\": {\"value\":1}}";

        User user = mapper.readValue(json, User.class);
        assertNotNull(user);
        assertEquals(UserType.ADMIN, user.getType());
    }

    @Test
    void parseFailed() {
        String json = "{\"role\": {}, \"type\": {\"value\":1}}";

        assertThrowsExactly(JsonMappingException.class, () -> {
            mapper.readValue(json, User.class);
        });
    }
}

@JsonDeserialize(using = EnumBaseDeserializer.class)
interface EnumBase<V> {
    V getValue();

    String getName();

    static <E extends Enum<E> & EnumBase> E valueOf(Object value, Class<?> clazz) {
        E em;
        if (!clazz.isEnum()) {
            throw new RuntimeException(clazz.getName() + " is not a enum");
        }
        Class<E> enumClass = (Class<E>) clazz;
        E[] enums = enumClass.getEnumConstants();
        String enumName = null;
        for (EnumBase e : enums) {
            if (e.getValue().equals(value)) {
                enumName = e.getName();
            }
        }
        if (null != enumName) {
            em = Enum.valueOf(enumClass, enumName);
        } else {
            throw new RuntimeException(value + "not found");
        }
        return em;
    }

    static <E extends Enum<E> & EnumBase> E nameOf(String name, Class<E> clazz) {
        return Enum.valueOf(clazz, name);
    }
}

enum UserType implements EnumBase<Integer> {
    ADMIN(1),
    USER(2);

    final int value;

    UserType(int value) {
        this.value = value;
    }

    @Override
    public Integer getValue() {
        return this.value;
    }

    @Override
    public String getName() {
        return this.name();
    }
}

class Role {
    String name;

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }
}

class User {
    Role role;

    UserType type;

    public Role getRole() {
        return role;
    }

    public void setRole(Role role) {
        this.role = role;
    }

    public UserType getType() {
        return type;
    }

    public void setType(UserType type) {
        this.type = type;
    }
}

class EnumBaseDeserializer extends JsonDeserializer<EnumBase<?>> {
    @Override
    public EnumBase<?> deserialize(JsonParser jp, DeserializationContext cxt) throws IOException {
        JsonNode node = jp.getCodec().readTree(jp);
        if (StringUtils.isBlank(node.get("value").asText())) {
            return null;
        }
        Object currentValue = jp.getCurrentValue(); // currentValue is role in parseFailed() test case, not the user
        if(null == currentValue) {
            return null;
        }
        String currentName = jp.currentName();
        Class<?> clazz = BeanUtils.findPropertyType(currentName, currentValue.getClass());
        EnumBase em = EnumBase.valueOf(node.get("value").intValue(), clazz);
        return em;
    }
}

@cowtowncoder cowtowncoder added has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue and removed need-test-case To work on issue, a reproduction (ideally unit test) needed labels Nov 3, 2023
@cowtowncoder
Copy link
Member

One quick note:

JsonNode node = jp.getCodec().readTree(jp);

will definitely parse content and likely change currentValue for jp. So jp.getCurrentValue(); would need to be called before advancing parser.

@nocny-x
Copy link
Author

nocny-x commented Nov 3, 2023

c99f5c6ce9ddb7ed74907407c15fe9f

a3e4ca158a818db56df6fcd1f6b8c75

8ee7db8ee3c49a577443b7e383ece7a

@cowtowncoder
Copy link
Member

Ok. I forgot one aspect here: "current value" is specified for each nesting level, and so when deserializing and opening START_OBJECT (that is, { token) has been received, current level will not have current value.
So access at the beginning deserialize needs to get value from parent context -- assuming we want User -- is:

Object currentValue = p.getParsingContext().getParent().getCurrentValue();

but with that, I can reproduce discrepancy between Empty Object and non-empty.

I'll see if this can be fixed.

@cowtowncoder cowtowncoder changed the title BeanDeserializer does not reset jsonParser currentValue after deserialising nested empty Node BeanDeserializer updates currentValue incorrectly when deserialising empty Object Nov 4, 2023
@cowtowncoder
Copy link
Member

Found a way to resolve the issue, have simplified reproduction; fix will go in 2.16.0 -- no plans to backport in 2.15 or earlier.
2.16.0 should be released within 1-2 weeks now.

@cowtowncoder
Copy link
Member

Thank you @nocny-x for reporting this and providing the reproduction!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Projects
None yet
Development

No branches or pull requests

3 participants