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

Inconsistent BeanPropertyDefinition#couldDeserialize and actual deserialization behavior #316

Closed
1 task done
kaqqao opened this issue Jun 22, 2024 · 4 comments
Closed
1 task done

Comments

@kaqqao
Copy link

kaqqao commented Jun 22, 2024

Search before asking

  • I searched in the issues and found nothing similar.

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
Copy link
Member

cowtowncoder commented Jun 23, 2024

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 (FasterXML/jackson-databind#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
Copy link
Member

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
Copy link
Author

kaqqao commented Aug 18, 2024

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants