FasterXML / jackson-modules-java8

Set of support modules for Java 8 datatypes (Optionals, date/time) and features (parameter names)
Apache License 2.0
398 stars 116 forks source link

Inconsistent BeanPropertyDefinition#couldDeserialize and actual deserialization behavior #316

Closed kaqqao closed 2 weeks ago

kaqqao commented 2 months ago

Search before asking

Describe the bug

A bean property with an inaccessible (private) field and an implicit constructor returns true from BeanPropertyDefinition#couldDeserialize in 2.16.1, and false in 2.17.1. Yet both versions deserialize the bean and the field just fine. So the meta model and the deserializer behavior are inconsistent.

Specifically, this line was added to jackson-databind in 2.17 (despite the comment claiming "since 2.16") to address #736. And that bug was masking this one, as Jackson believed the field to be deserializable purely because the Java field existed, despite being inaccessible.

So maybe this is actually intentional behavior? But the meta-model/deserializer inconsistency is still surprising.

Version Information

2.17.1

Reproduction

public static class Book {

    private String title;
    private int serial;

    public Book(String title, int serial) {
        this.title = title;
        this.serial = serial;
    }

    public String getTitle() {
        return title;
    }

    public int getSerial() {
        return serial;
    }
}

public static void main(String[] args) throws JsonProcessingException {
    ObjectMapper mapper = new ObjectMapper().registerModule(new ParameterNamesModule());
    JavaType bookType = mapper.getTypeFactory().constructType(Book.class);
    BeanDescription desc = mapper.getDeserializationConfig().introspect(bookType);
    boolean deserializable = desc.findProperties().get(0).couldDeserialize();
    System.out.println(deserializable); //true in 2.16, false in 2.17
    Book book = mapper.readValue("{\"title\":\"boom\", \"serial\":123}", Book.class); //works equally in both
}

Expected behavior

I'd expect bean deserialization to fail if BeanPropertyDefinition#couldDeserialize == false for one of the implicated fields. Or, alternatively, BeanPropertyDefinition#couldDeserialize to return true if a usable bound constructor parameter is found.

Additional context

I am using Jackson's meta model to introspect Java types and create equivalent GraphQL types. For types used as inputs, it is important to know what fields are deserializable, as non-deserializable fields should not be mapped to GraphQL. For this reason, I have an extensive suite of tests for all sorts of Java types and deserialization configurations, and one of the tests caught this change in Jackson 2.17.

I originally opened an issue on jackson-databind but have since realized the problem is technically in this module. So I closed the original and opened this instead.

cowtowncoder commented 2 months ago

Ok, couple of comments.

First of all thank you for pointing out misleading comment. I fixed it in 2.17 branch. Also thank you for providing context information on your usage: it helps understand the problem.

Second: the main public API for wrt whether something is deserializable is ObjectMapper.canDeserialize(). BeanPropertyDefinition.couldDeserialize() is mostly meant for internal use and semantics are relevant for databind's own use (and different from ObjectMapper.canDeserialize()); in this case was needed to fix another issue (https://github.com/FasterXML/jackson-databind/issues/736 as comment says). So I would not recommend using that method.

Instead, I would simply base likely deserializability on existence of BeanPropertyDefinition -- introspection done for deserialization side (DeserializationConfig.introspect()) should not include any properties not used for deserialization: set of properties may well be different from that you'd get from SerializationConfig.introspect().

Finally: as to the original method in question: it is likely it will again return true for 2.18, due to rewriting of the POJO Property introspection. I think that reason return value changed here had to do with the problems of linking "Creator properties" (constructor/factory method parameters used to pass property values, instead of fields or setter methods), which is now finally fixed. But although things might once again work in 2.18 like they used to 2.16, I do not think you should try to use the method -- it is not designed for your case, nor supported to do that (meaning: you can use it, but its behavior is not guaranteed for external use). If relying on basic existence of property works (which I think it should), you are better off using that logic.

cowtowncoder commented 2 weeks ago

Ok: I consider BeanPropertyDefinition.couldDeserialize part of internal API, not meant to be used by non-Jackson code. As such while it'd be nice if its working didn't change, it's just nice-to-have.

Closing.

kaqqao commented 2 weeks ago

Thanks for the explanation! That was really helpful. I changed my code to rely on the mere presence of a property returned by DeserializationConfig.introspect() instead of also calling couldDeserialize on it. Makes sense to close this since I was apparently using it wrong.

cowtowncoder commented 2 weeks ago

@kaqqao Thank you for your understanding; glad you got it working.