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

Serializing a Java 16 Record includes transient data from getters from implemented interfaces #3628

Open
hjohn opened this issue Oct 16, 2022 · 10 comments
Labels
to-evaluate Issue that has been received but not yet evaluated

Comments

@hjohn
Copy link

hjohn commented Oct 16, 2022

This may be by design, but I think it may be a bug...

Describe the bug
When serializing a record that implements an interface, Jackson also serializes any values resulting from getters in an implemented interface. Such additional "getters" should however always be perceived as transient, as they cannot influence the actual data stored in the record.

For example:

public sealed interface StreamMetaDataEvent extends AggregateEvent {
  public record Updated(StreamMetaData streamMetaData) implements StreamMetaDataEvent {
    @Override
    public Type getType() {
      return Type.FULL;
    }

    @Override
    public String getAggregateId() {
      return "" + streamMetaData.contentId().asInt();
    }
  }

  public record Removed(ContentID id) implements StreamMetaDataEvent {
    @Override
    public Type getType() {
      return Type.DELETE;
    }

    @Override
    public String getAggregateId() {
      return "" + id.asInt();
    }
  }
}

The getters here are transient (they contain no new information) and are coming from the AggregateEvent interface:

public interface AggregateEvent {
  enum Type { FULL, DELETE }

  Type getType();
  String getAggregateId();
}

Work around

Configuring setVisibility(PropertyAccessor.GETTER, Visibility.NONE) will also ignore the records fields (not sure if they should be considered "getters", but I suppose they might). Configuring in addition setVisibility(PropertyAccessor.FIELD, Visibility.ANY) resolves the issue.

Version information
Which Jackson version(s) was this for? 2.13.4

Expected behavior
I would expect Jackson to recognize records, and recognize that it is fully defined by the records fields; any additional methods, getters or otherwise, should be considered irrelevant for reconstructing the record.

@hjohn hjohn added the to-evaluate Issue that has been received but not yet evaluated label Oct 16, 2022
@cowtowncoder
Copy link
Member

My first thought is that I would consider this a feature: a getter is a getter, even for Records. If they are not to be serialized, they should be marked to be ignored; either by annotations (@JsonIgnore, @JsonIgnoreProperties), or you may consider enabling MapperFeature.REQUIRE_SETTERS_FOR_GETTERS which would prevent serialization of any potential properties that cannot be deserialized (there is no matching "setter"; in case of Record meaning constructor parameter).

In theory I guess it would be possible to change default visibility level of Records such that no getters would be auto-detected, if users feel this is the way to go. Right now you can change visibility levels (see @JsonAutoDetect), but there's no way to easily apply it to all Records.
... actually, come to think of it, it is quite straight-forward to achieve that by user code: override method findAutoDetectVisibility() of AnnotationIntrospector (or implementation JacksonAnnotationIntrospector), and if class is a record type, return different settings: despite name, AnnotationIntrospector need not use annotations at all -- it is just the extension/API Jackson uses to make annotation-access modular. So one can use it to "emulate" existence of all Jackson annotations.

@yihtserns
Copy link
Contributor

yihtserns commented Apr 7, 2023

Reading the last comment somehow reminds me of 50dd52f, where you created a custom default VisibilityChecker for Records, for v3.

It is a possible option to change that VisibilityChecker (albeit for v3) to match the behaviour expected by this issue? 🤔 But that will cause the behaviour to drift further away again from POJO de/serialization behaviour...

@yihtserns
Copy link
Contributor

yihtserns commented Apr 7, 2023

@cowtowncoder Eh while trying a different mecha than AnnotationIntrospector (i.e. via ConfigOverride), it suddenly made me wonder if #3737 specifically 0a4cfc4 has broken the workaround used in his issue - specifically usage of setVisibility(PropertyAccessor.FIELD, Visibility.ANY) - by avoiding to detect fields for Record types?

@cowtowncoder
Copy link
Member

Yeah I don't know. Perhaps. That's the challenge of missing test coverage -- functionality not covered by tests can easily break.

@yihtserns
Copy link
Contributor

yihtserns commented Apr 9, 2023

@hjohn can you help provide a code snippet so we can know exactly how your workaround looks like, so we can properly test whether it's broken in 2.15.0?

@hjohn
Copy link
Author

hjohn commented Apr 9, 2023

I just configured a custom ObjectMapper for serializing records:

private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper()
  .setVisibility(PropertyAccessor.GETTER, Visibility.NONE)
  .setVisibility(PropertyAccessor.FIELD, Visibility.ANY)
  .registerModule(new JavaTimeModule())
  .registerModule(new Jdk8Module())
  .registerModule(new ParameterNamesModule(Mode.PROPERTIES));

With this setup it only detects the records fields, and since you can't add any fields to a record, that's all the data you would need to store.

@cowtowncoder
Copy link
Member

One thing to note is that Records do NOT require use of ParameterNamesModule as name information is accessed (via Reflection) using record-specific accessors, and anything module provides is basically overridden.
Module is needed for regular POJOs wrt Creators of course.

@yihtserns
Copy link
Contributor

That workaround + 2.15.0-rc2 = {}

😬

@hjohn
Copy link
Author

hjohn commented Apr 12, 2023

That workaround + 2.15.0-rc2 = {}

😬

Is that a cryptic way of saying that limiting visibility to fields only for records breaks in 2.15.0-rc2 ?

@yihtserns
Copy link
Contributor

yihtserns commented Apr 12, 2023

Yes. Only an empty JSON hash/object will be generated when that workaround is used with 2.15.0-rc2...

UPDATE: The workaround used above is working again in 2.15.1 courtesy of #3894.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to-evaluate Issue that has been received but not yet evaluated
Projects
None yet
Development

No branches or pull requests

3 participants