FasterXML / jackson-future-ideas

Repository for SOLE PURPOSE of issue tracker and Wiki for NEW IDEAS. Please: NO BUG REPORTS.
18 stars 6 forks source link

Support for record types in JDK 14 #46

Closed ctash-dev closed 4 years ago

ctash-dev commented 4 years ago

Add support for record classes (data classes) from JDK 14 Feature Preview. https://cr.openjdk.java.net/~briangoetz/amber/datum.html

Right now the out-of-the-box experience while using records in jackson is the following: Serializing a simple record with no extra methods:

public record TestRecord(int x, int y, int z) {
}

results in a "no properties" exception: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: No serializer found for class com.company.TestRecord and no properties discovered to create BeanSerializer

A @JsonAutoDetect annotation can be use to solve the problem:

@JsonAutoDetect(fieldVisibility = JsonAutoDetect.Visibility.ANY)
public record TestRecord(int x, int y, int z) {
}
ObjectMapper mapper = new ObjectMapper();
System.out.println(mapper.writeValueAsString(new TestRecord(1, 2, 3)));

results in a correct {"x":1,"y":2,"z":3} output

So right now it works with a little bit of work, but it would be great if there would be a correct out-of-the-box experience with record classes.

cowtowncoder commented 4 years ago

Since core jackson-databind 2.x only requires Java 7, it is likely it can not support this natively (I could be wrong as it all depends, but seems likely). But it could perhaps be part of Java 14 support module. I transferred the issue to repo for "future ideas", as this is definitely something that would be good to support, but may not be yet actionable.

I would be interested in knowing bit more about actual implementation of Records, to know what markers there might be to indicate a Record type, and if those can only be detected with JDK 14, or perhaps earlier (similar to how Enums and Annotations, for example, use existing facilities of bytecode to some degree, but JDK only added support in 1.5).

krisztian-toth commented 4 years ago

Hi @cowtowncoder, Since you are interested, record classes inherit from java.lang.Record, so checking whether a class is record or not could be as simple as:

I would say it's safe to assume that record type can be detected pre-JDK14 with the given method.

youribonnaffe commented 4 years ago

I wanted to check if records could be deserialized out of the box using Jackson. It turns out it work using @JsonProperty annotations on the field like:

static record MyRecord(@JsonProperty("id") int id, @JsonProperty("name")String name) { }

This works because annotations are propagated in the generated constructor parameters and Jackson will use it for deserialization.

However putting the annotations feels kind of redundant so I looked for a way to not do it. I found that by changing a bit the annotation interceptor to find implicit properties it could be done like that: https://gist.github.com/youribonnaffe/03176be516c0ed06828ccc7d6c1724ce

Given my knowledge of Jackson this is probably not covering all cases and features of the library.

cowtowncoder commented 4 years ago

@youribonnaffe Thank you for sharing your findings! At least it is good to first know what workarounds are needed currently.

And yes, this is one case where inheritance of constructor property arguments comes in pretty handy. Glad I implemented it in the first place (mostly for mix-in annotation use case), despite it being quite a bit of extra work back in the day. :-)

robberphex commented 4 years ago

implicitRecordAI is great. There is a new problem:

@Data
public class Person {
    @JsonProperty("id")
    private int id;
    private String name;
    private List<String> alias;

    @JsonSetter("alias")
    public void setAlias(String aliasStr) {
        alias = Arrays.asList(aliasStr.split(","));
    }

    public static void main(String[] args) throws IOException {
        ObjectMapper objectMapper = new ObjectMapper();
        String jsonStr = """
                {
                  "id":1,
                  "name":"name1",
                  "alias": "alias1,alias2"
                }
                """;
        Person person = objectMapper.readValue(jsonStr, Person.class);
        System.out.println(person);
    }
}

alias in json is a string, so use custom setter to convert it to List.

But with record, it couldn't work, the filed alias is final. The workaround is:

@JsonIgnoreProperties(ignoreUnknown = true)
public record Person(
        Integer id,
        String name,
        // if this field have custom converter, JsonProperty is required
        @JsonProperty(value = "alias", access = JsonProperty.Access.WRITE_ONLY)
        @JsonDeserialize(converter = AliasConverter.class)
        List<String>alias,
        @JsonProperty(value = "password", access = JsonProperty.Access.WRITE_ONLY)
        String password
) {
    @JsonGetter("alias")
    public String jsonGetAlias() {
        return String.join(",", alias);
    }

    public static void main(String[] args) throws IOException {
        ObjectMapper objectMapper = new ObjectMapper();
        // get default propertyName from record
        // thanks youribonnaffe
        JacksonAnnotationIntrospector implicitRecordAI = new JacksonAnnotationIntrospector() {
            @Override
            public String findImplicitPropertyName(AnnotatedMember m) {
                if (m.getDeclaringClass().isRecord()) {
                    if (m instanceof AnnotatedParameter parameter) {
                        return m.getDeclaringClass().getRecordComponents()[parameter.getIndex()].getName();
                    }
                    if (m instanceof AnnotatedMember member) {
                        for (RecordComponent recordComponent : m.getDeclaringClass().getRecordComponents()) {
                            if (recordComponent.getName().equals(member.getName())) {
                                return member.getName();
                            }
                        }
                    }
                }
                return super.findImplicitPropertyName(m);
            }
        };
        objectMapper.setAnnotationIntrospector(implicitRecordAI);
        String jsonStr = """
                {
                  "id":1,
                  "name":"name1",
                  "alias": "alias1,alias2",
                  "password": "password1"
                }
                """;
        Person person = objectMapper.readValue(jsonStr, Person.class);
        System.out.println(person);
        // Person[id=1, name=name1, alias=[alias1, alias2], password=password1]
        String jsonOut = objectMapper.writeValueAsString(person);
        System.out.println(jsonOut);
        // {"alias":"alias1,alias2","id":1,"name":"name1"}
    }

    /**
     * convert String to List, just for field alias
     * <p>
     * this is class-bound logic, so keep it internal
     */
    static class AliasConverter extends StdConverter<String, List<String>> {
        @Override
        public List<String> convert(String value) {
            return Arrays.stream(value.split(",")).collect(Collectors.toList());
        }
    }
}
youribonnaffe commented 4 years ago

Thanks for the feedback.

I tried it and it seems that the change you did in the JacksonAnnotationIntrospector is not required for it work. Using @JsonProperty probably makes Jackson use the private fields instead of the record's constructor.

I debugged a little and found this approach : https://gist.github.com/youribonnaffe/03176be516c0ed06828ccc7d6c1724ce (same Gist, updated). When the implicit constructor is searched for, it was not working anymore because the alias parameter was not properly resolved (seems that using @JsonDeserialize changes the behavior a bit).

It feels more hackish than the first version.

ttaranov commented 4 years ago

also, just wanted to mention that this lack of support for new format of getter methods makes using record types with springboot fail by default during json serialization with an obscure error like below (which can be tracked down to fasterxml/jackson) -

org.springframework.web.servlet.mvc.method.annotation.AbstractMessageConverterMethodProcessor.writeWithMessageConverters(AbstractMessageConverterMethodProcessor.java:230)
    at org.springframework.web.servlet.mvc.method.annotation.RequestResponseBodyMethodProcessor.handleReturnValue(RequestResponseBodyMethodProcessor.java:181)

the error goes away after adding annotation mentioned in the description

@JsonAutoDetect(fieldVisibility = JsonAutoDetect.Visibility.ANY)
raj-saxena commented 4 years ago

Hi, I am working on a problem that uses Jackson's csv capabilities and wanted to use Records for deserialization. It's a SpringBoot app and I noticed that the deserialization happens successfully in the unit test but fails for during the integration test. I have replicated the problem in this minimal project. Here's the unit test that passes and the integration test that fails with Failed to load ApplicationContext with the following error:

Caused by: java.lang.RuntimeException: com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.suspendfun.CsvRecord` (although at least one Creator exists): no String-argument constructor/factory method to deserialize from String value ('name')
 at [Source: (com.fasterxml.jackson.dataformat.csv.impl.UTF8Reader); line: 1, column: 1]

  1. I am a bit confused by @youribonnaffe 's comment as my unit-tests passes without the @JsonProperty annotation. Does anyone know why?

  2. Why does the same code fail the Spring app startup of an integration test? What is Spring doing differently? I want to understand the behavior better and help in finding a possible solution for Spring.

  3. Now, that JDK 14 is out, is this issue/work of supporting Records being tracked in a different place?

youribonnaffe commented 4 years ago

Hi,

There is one difference between the CsvService and the unit test when the CsvMapper is created :

  .addColumn("name")
  .addColumn("status")

Adding those in CsvService fixes the context initialization exception, then adding @JsonAutoDetect(fieldVisibility = JsonAutoDetect.Visibility.ANY) on the record makes the test pass.

Defining the columns in the schema probably helps Jackson to use the constructor (to be checked).

raj-saxena commented 4 years ago

Thanks @youribonnaffe for the quick code fix, explanation of why it (possibly) works (and sorry about the missing code part that adds the columns). I can confirm that after the suggested changes, the tests pass successfully.

About the last part, are you aware if the support for Records is being tracked somewhere else?

cowtowncoder commented 4 years ago

I don't think there is an existing issue yet, so I will go ahead and create one for jackson-databind as that is where most of the work would likely be.

cowtowncoder commented 4 years ago

Created: https://github.com/FasterXML/jackson-databind/issues/2709

youribonnaffe commented 4 years ago

@cowtowncoder I'll be happy to help on that. Not sure how to get started though as the code base of Jackson is quite big :).

cowtowncoder commented 4 years ago

@youribonnaffe Yes, understandably it is not easy to know where to get started.

One of first things would probably just be that of figuring out if a class is Record, similar to how Enums are detected; and in particular if it was possible to do so with JDK 8 only accessors of Class. And then document/link to how introspection of Record fields should work (wrt member fields and methods), logically speaking. Enums, for example, are implemented in quite interesting way -- sounds like Records are less complicated. It could be that big part of initial work would be just information gathering from articles: I have not read anything about implementation details yet, for example.

cowwoc commented 4 years ago

@cowtowncoder I think you can assume that any class that subclasses java.lang.Record is a record.

In fact, I just checked the source-code for Class.isRecord() and that's exactly what they do.

public boolean isRecord() {
        return getSuperclass() == JAVA_LANG_RECORD_CLASS && isRecord0();
    }

I don't know what isRecord0() does but this is probably good enough. I suspect that Class.getRecordComponents() allows you to differentiate between record fields and new fields/getters added by the user but I'm not sure that Jackson needs to differentiate between the two.

cowtowncoder commented 4 years ago

@cowwoc That would be simply enough then, from JDK8: just try to load java.lang.Record dynamically via reflection, catch and swallow exception; use at a later point (only) if available; dynamically load handler. Further, initially maybe hide behind MapperFeature (defaulting to false) to avoid possible problems on platforms with security constraints.

If getRecordComponents() is needed would either do bit heavier lifting with reflection, or separate module. But sounds like maybe that is not needed.

youribonnaffe commented 4 years ago

In terms of features, what should be offered out of the box? IMHO record fields should be recognized and not needing explicit annotation to be serialized or deserialized. To instanciate the record, the canonical constructor should probably be used.

krzyk commented 4 years ago

@youribonnaffe For deserialization it pretty much works (when you have more than one field in record, if you do then I need to add @JsonProperty, pretty annoying but workable) when you use -parameters in javac and ParameterNamesModule

I use jackson with records like that and I'm pretty happy with it, it is basically just like using a immutable class with all fields initialized with a single constructor.

cowtowncoder commented 4 years ago

@youribonnaffe That sounds reasonable to me (although I haven't tried using Records so I don't know if there are some caveats).

Another thing this might be related to are various similar types like Scala case classes, Kotlin data classes, where it seems that definition of properties is driven directly by field definitions. And Jackson databind could perhaps offer little more core-support for modules, to help with some issues regarding property naming (basically, start by naming convention derived from field, match accessors, if any; this results in better mapping than Bean-style attempt to start from accessor).

dgray16 commented 4 years ago

Another workaround is to use com.fasterxml.jackson.annotation.JsonCreator

youribonnaffe commented 4 years ago

Another update on records: https://openjdk.java.net/jeps/384 They introduced the idea of local records and more documentations around annotations.

ChrisHegarty commented 4 years ago

Hi,

I work on the JDK, and implemented the Serializable record support in Java 14. I would love to help out with this effort, in any way that I can.

I can provide as much details as you would like, but essentially Java serialization can use a record's accessors to extract the values of its components for serializing. On the deserializing side, the record's canonical constructor is invoked to instantiate the record object - the constructor is passed the appropriate component values.

From a runtime reflection point of view, Java 14 added two specific methods to support records: 1) Class::isRecord 2) Class::getRecordComponents ( and its j.l.r.RecordComponent type )

It is with these two primitives that the whole Java Serializable record support is built upon.

It is worth noting that starting in Java 15, core reflection will not be able to mutate the component fields of a record object. Construction should proceed through the canonical constructor.

Again, I would like to help here in any capacity that I can, so that Jackson works out-of-the-box with records. Yes, even local records, maybe ;-)

ChrisHegarty commented 4 years ago

@cowtowncoder I think you can assume that any class that subclasses java.lang.Record is a record.

In fact, I just checked the source-code for Class.isRecord() and that's exactly what they do.

public boolean isRecord() {
        return getSuperclass() == JAVA_LANG_RECORD_CLASS && isRecord0();
    }

I don't know what isRecord0() does but this is probably good enough. I suspect that Class.getRecordComponents() allows you to differentiate between record fields and new fields/getters added by the user but I'm not sure that Jackson needs to differentiate between the two.

isRecord0 is a native method that is implemented in the JVM. It effectively returns true if, and only if, the class has a Record Attribute in its class file - it is this attribute that carries the record component information: name, type, order.

The safest corse of action would be to reflectively determine if the Class::isRecord method is present, and invoke it reflectively if it is. If the method is present and returns true then the class is a record, otherwise it is not - you cannot have records on JDK's where this method is not present.

cowtowncoder commented 4 years ago

@ChrisHegarty excellent, help would be much appreciated!

One thing that I would really like to do but don't have time to right now is to create simple test repo(s) to test handling of new JDK features, regardless of whether a separate module is needed or not. There are for example:

but in this case could have JDK 14 compatibility tests (I assume for the most part LTS would benefit).

Alternatively if it's easy enough to do, adding optional (only run for certain profile) Maven tests for existing repos could also work.

Now as to support: I suspect that since detection of java.lang.Record seems simple enough using reflection (although may want to hide it behind MapperFeature?), and from that could just use defined Constructor rules, knowing that there must be a single canonical constructor? (or at least rules for detecting one to use).

But what if.... what if there was new JavaType detected? That could then be used for new paths for serializer/deserializer too. I guess more minimal thing would be just to add isRecord() in JavaType; but actual subtype could be a possibility if some aspects were important (although then again there is no separate subtype for Enum).

ChrisHegarty commented 4 years ago

Alternatively if it's easy enough to do, adding optional (only run for certain profile) Maven tests for existing repos could also work.

If the compilation, execution, command line args, of such tests can be conditional on the JDK version or some such, then that would be easiest. The tests could then be merged into the regular non-optional profile later. But a separate repo would be ok too, just maybe a little more awkward to manage?

Now as to support: I suspect that since detection of java.lang.Record seems simple enough using reflection (although may want to hide it behind MapperFeature?), and from that could just use defined Constructor rules, knowing that there must be a single canonical constructor? (or at least rules for detecting one to use).

A record can have more than one constructor, but there is always a single canonical constructor, that all other overloaded constructors will eventually delegate to. To find the canonical constructor one can do:

  Class<?>[] paramTypes = Arrays.stream(cls.getRecordComponents())
                     .map(RecordComponent::getType)
                     .toArray(Class<?>[]::new);
  Constructor<?> ctr = cls.getDeclaredConstructor(paramTypes);

For example, in Java Serialization: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/io/ObjectStreamClass.java#L1592

But what if.... what if there was new JavaType detected? That could then be used for new paths for serializer/deserializer too. I guess more minimal thing would be just to add isRecord() in JavaType; but actual subtype could be a possibility if some aspects were important (although then again there is no separate subtype for Enum).

I'm not quite sure what would be best: 1) JavaType::isRecord, or 2) a subtype of JavaType. I guess it depends where is best to add the serializer and deserializer specific logic for records. Maybe start with the least invasive change and see how far we can get with that.

I think it might be most straight forward to just get things working on JDK 14 / 15, and then see how/where the Java 14+ specific code would need to live and how best to make it optional ( so as to not require JDK 14+ for compilation or runtime ).

youribonnaffe commented 4 years ago

In this PR https://github.com/FasterXML/jackson-databind/pull/2714 I used a Maven profile activated with Java 14 and a separate test source folder to use records. The changes in Jackson don't use any API from Java 14 and we can still write tests with records.

The code you suggested @ChrisHegarty is very similar to https://github.com/FasterXML/jackson-databind/blob/a1b9a76491ddba2994c65457ba4b6e244a146b40/src/main/java/com/fasterxml/jackson/databind/util/RecordUtil.java#L52

ChrisHegarty commented 4 years ago

Thanks @youribonnaffe, I'll take a look at that PR, and post comments there.

ChrisHegarty commented 4 years ago

But what if.... what if there was new JavaType detected? That could then be used for new paths for serializer/deserializer too. I guess more minimal thing would be just to add isRecord() in JavaType; but actual subtype could be a possibility if some aspects were important (although then again there is no separate subtype for Enum).

I think this comes down to the amount of customisation that you want to allow for records, and then how that surfaces in the API. For example, Java Serialization of records allows no customisation of the serial-form of the record, i.e. the serial-form ( the serializable fields ) is that of the record components, nothing more and nothing less. This could be one choice, or alternatively records could be viewed as just another shape of bean; getters without the get prefix, no setters, and a single findable canonical constructor, and allowing much (all?) of the customisations of any other ordinary class.

ChrisHegarty commented 4 years ago

Update to my previous comment. I see @youribonnaffe has a test that uses@JsonProperty to rename a record component, seems reasonable. Given this, I think that @youribonnaffe is probably good as is. It might be that the record-ness of a class is a useful property to expose in the Jackson API, but I don't see that yet myself.

cowtowncoder commented 4 years ago

At this point there is probably not too much need to expose Record-ness, I agree. And while isRecordType() could be added in ClassUtil, it probably makes sense to rather bundle that and other accessors in RecordUtil as proposed in PR.

I added some minor comments to PR, but my main suggestion is just to separate introspection of the canonical constructor into bit separate code path (I can help with this), so that while introspection of annotations user can use (for renaming, custom (de)serializers) can and should shared code, all the logic for figuring out set of Creators can be avoided as logic for finding one should be straight-forward -- find components, then find constructor that matches it.

I hope to find time to follow up on this with less delay, now that I think I understand the constraints.

youribonnaffe commented 4 years ago

This is part of 2.12 now! I guess the issue can be closed @cowtowncoder.

cowtowncoder commented 4 years ago

Thanks! Yes indeed!

Anyone seeing the update, feedback on implementation (as in, testing, verifying it works) would be super helpful before 2.12.0 release (first release candidate hopefully out during September 2020).