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

Record deserialization bug/breaking change in 2.15 #4105

Closed
1 task done
aelgn opened this issue Sep 5, 2023 · 6 comments
Closed
1 task done

Record deserialization bug/breaking change in 2.15 #4105

aelgn opened this issue Sep 5, 2023 · 6 comments

Comments

@aelgn
Copy link

aelgn commented Sep 5, 2023

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

In 2.14.x a record could be deserialized without any annotations and dummy default constructor with permissive Field visibility.
Release 2.15.x changed this behavior with I assume #3736

It was very handy to be able to deserialize records without any further ceremony.

As it stands we cannot upgrade to 2.15.x without introducing a lot of boilerplate to our records - or is there a configuration option I have missed?

Version Information

2.15.x

Reproduction

ObjectMapper mapper = new ObjectMapper().setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY);
record Qwop(String q) {}
final Qwop qwop = new Qwop("");
final String s = mapper.writer().writeValueAsString(qwop);
final Qwop o = mapper.reader().forType(Qwop.class).readValue(s);

Expected behavior

Expected record deserialization to work with field visibility as in 2.14.x

@aelgn aelgn added the to-evaluate Issue that has been received but not yet evaluated label Sep 5, 2023
@yihtserns
Copy link
Contributor

Tested with:

public class JacksonDatabind4105Test {

    @Test
    public void test() throws Exception {
        ObjectMapper mapper = new ObjectMapper().setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY);
        System.out.println("Jackson: " + mapper.version());

        record Qwop(String q) {}
        final Qwop qwop = new Qwop("");

        final String s = mapper.writer().writeValueAsString(qwop);
        System.out.println("JSON: " + s);

        final Qwop o = mapper.reader().forType(Qwop.class).readValue(s);
        System.out.println("Record: " + o);
    }
}

System.out for jackson-databind:2.14.2:

Jackson: 2.14.2
JSON: {"q":""}
Record: Qwop[q=]

System.out for jackson-databind:2.15.0:

Jackson: 2.15.0
JSON: {"q":""}
Record: Qwop[q=]

Can you provide a more detailed example to demonstrate the issue, please?

@aelgn
Copy link
Author

aelgn commented Sep 5, 2023

I'm sorry, when I simplified my code for the github issue I inadvertently removed an ObjectMapper configuration that seems to be the culprit:

final ObjectMapper mapper = new ObjectMapper()
                .setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE)
                .setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY);

Setting ALL visibility to NONE and then overriding FIELD to ANY the classic InvalidDefinitionException is thrown:
com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of a.b.c$1Qwop (no Creators, like default constructor, exist): cannot deserialize from Object value (no delegate- or property-based Creator)

I have now tested the various visibilities and can conclude that in 2.15.x non-annotated records without a default constructor can be deserialized with the configuration:

final ObjectMapper mapper = new ObjectMapper()
                .setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE)
                .setVisibility(PropertyAccessor.GETTER, JsonAutoDetect.Visibility.ANY)
                .setVisibility(PropertyAccessor.CREATOR, JsonAutoDetect.Visibility.ANY);

So it seems like the "breaking" change in 2.15.x is that records are not deserialized with FIELD but with CREATOR + GETTER instead. This is great news, since this is easily configured - but might need documentation to avoid pesky runtime surprises.

Leaving this issue open for someone to evaluate if this is the preferred behavior and/or needs documentation.

@aelgn
Copy link
Author

aelgn commented Sep 5, 2023

I am a bit confounded over the need to make GETTERs visible to be able to deserialize records. This change breaks our class based dto:s.

To minimize ceremony we are using the ParameterNamesModule for class deserialization, with a @JsonCreator constructor matching the fields. Turning on GETTER visibility introduces a lot of unserializable json given DTO:s with computed properties.

static class Plopp {
    final String p;
    @JsonCreator
    Plopp(final String p) {
        this.p = p;
    }
    int getL() {
        return p.length();
    }
}
public static void main(final String[] args) {
    try {
        final ObjectMapper mapper = new ObjectMapper()
            .setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE)
            .setVisibility(PropertyAccessor.GETTER, JsonAutoDetect.Visibility.ANY)
            .setVisibility(PropertyAccessor.CREATOR, JsonAutoDetect.Visibility.ANY)
            .setVisibility(PropertyAccessor.FIELD, JsonAutoDetect.Visibility.ANY)
            .registerModule(new ParameterNamesModule());
        System.out.println("Jackson: " + mapper.version());
        record Qwop(String q) {}
        final Qwop qwop = new Qwop("q");
        final Plopp plopp = new Plopp("p");

        {
            final String s = mapper.writer().writeValueAsString(qwop);
            System.out.println("ser: " + s);
            final Object o = mapper.reader().forType(Qwop.class).readValue(s);
            System.out.println("des: " + o);
        }
        {
            final String s = mapper.writer().writeValueAsString(plopp);
            System.out.println("ser: " + s);
            final Object o = mapper.reader().forType(Plopp.class).readValue(s);
            System.out.println("des: " + o);
        }
    } catch (final JsonProcessingException e) {
        throw new RuntimeException(e);
    }
}

It seems like the types Qwop and Plopp cannot be deserialized with the same ObjectMapper anymore (since 2.15.x).
Qwop deserialization now requires GETTER visibility, but this visibility breaks Plopp deserialization.

.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
One workaround to get the above code to run is to ignore all unknown properties - but this is not preferable. And since the GETTER is still visible in serialization the produced json will in best case include properties that are not meant for serialization and in worst case throw exception when the returned type is unserializable.

@yihtserns
Copy link
Contributor

yihtserns commented Sep 5, 2023

About the visibility config problem above, there are 2 issues here:

  1. Serialization produces empty JSON (i.e. {}):
  2. Deserialization failed because "no creator" found:

@aelgn
Copy link
Author

aelgn commented Sep 5, 2023

Thank you, that is great.
I can confirm that from 2.15.1 GETTER visibility is no longer needed for deserializing "simple" records. CREATOR visibility is however still needed - which is absolutely fine by me since it does not alter our other serialization (that I know of).

@aelgn aelgn closed this as completed Sep 5, 2023
@cowtowncoder
Copy link
Member

Thank you @aelgn, @yihtserns for trouble-shooting this. I know Record-handling changes have been frustrating wrt 2.15, but I hope we are in net-positive situation wrt improvements and resolving some (if not all) regressions.

@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Dec 25, 2023
@Mariamalmesfer Mariamalmesfer mentioned this issue Nov 5, 2024
Closed
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

3 participants