FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.51k stars 1.37k forks source link

Handling of annotations assigned to constructor parameters of the `record` class #4513

Open k163377 opened 4 months ago

k163377 commented 4 months ago

Describe your Issue

In Kotlin, annotations are given to constructor parameters of the record class when written in a natural way.

@JvmRecord
data class JacksonTest(
    @JsonProperty("propertyOne")
    val one: String,
    @JsonProperty("propertyTwo")
    val two: String
)

On the other hand, Jackson seemed to ignore such annotations. For this reason, the following problems have been reported https://github.com/FasterXML/jackson-module-kotlin/issues/773

I am not familiar with the handling of the record class, but is it possible to handle annotations on parameters in the same way as the normal class?

cowtowncoder commented 4 months ago

Could the example be translated into plain Java? Records support annotations just fine for general case, so details matter a lot.

k163377 commented 4 months ago

Records support annotations just fine for general case, so details matter a lot.

You are right about the record class support in Java. This is a Kotlin specific issue.

For verification purposes, I have created the following code. Temp.JacksonTest and JacksonTestKt are record classes with almost similar definitions. Running main will display the constructor parameters for each class, as well as the annotations assigned to the fields.

import com.fasterxml.jackson.annotation.JsonProperty

@JvmRecord
data class JacksonTestKt(
    @JsonProperty("propertyOne") val one: String,
    @JsonProperty("propertyTwo") val two: String,
)
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonProcessingException;

import java.lang.annotation.Annotation;
import java.util.Arrays;

public class Temp {
    record JacksonTest(
            @JsonProperty("propertyOne") String one,
            @JsonProperty("propertyTwo") String two
    ) {}

    static void printCtorParamAnn(Class<?> c) {
        System.out.println("- " + c.getSimpleName());

        Arrays.stream(c.getDeclaredConstructors()[0].getParameters()).forEach(p -> {
            var a = p.getAnnotation(JsonProperty.class);

            if (a == null) { return; }

            System.out.println("-- " + p.getName() + " " + a.annotationType());
        });
    }

    static void printFieldParamAnn(Class<?> c) {
        System.out.println("- " + c.getSimpleName());

        Arrays.stream(c.getDeclaredFields())
                .forEach(f -> {
                    for (Annotation a : f.getDeclaredAnnotations()) {
                        System.out.println("-- " + a.annotationType());
                    }
                });
    }

    public static void main(String[] args) throws JsonProcessingException {
        System.out.println("param ann");
        printCtorParamAnn(JacksonTest.class);
        printCtorParamAnn(JacksonTestKt.class);

        System.out.println();

        System.out.println("field ann");
        printFieldParamAnn(JacksonTest.class);
        printFieldParamAnn(JacksonTestKt.class);
    }
}

The result is as follows: Kotlin does not annotate the field.

param ann
- JacksonTest
-- one interface com.fasterxml.jackson.annotation.JsonProperty
-- two interface com.fasterxml.jackson.annotation.JsonProperty
- JacksonTestKt
-- arg0 interface com.fasterxml.jackson.annotation.JsonProperty
-- arg1 interface com.fasterxml.jackson.annotation.JsonProperty

field ann
- JacksonTest
-- interface com.fasterxml.jackson.annotation.JsonProperty
-- interface com.fasterxml.jackson.annotation.JsonProperty
- JacksonTestKt

I believe that Jackson skips parsing parameter annotations only when parsing the record class (sorry if I am wrong). On the other hand, at least in Kotlin, this behavior causes problems. I also feel that it is a bit difficult to fix this problem in kotlin-module.

To reiterate, I wanted to ask if it is possible to parse the record class in jackson-databind the same as the regular class (or if there is no reasonable reason to do so).

cowtowncoder commented 4 months ago

Ah. Ok, I agree: it should be possible to find annotations from Constructor parameters; annotations in Fields should NOT be required.

But one challenging part is reproducing the issue on Java side, to be able to reproduce it, verify fix & guard against regression.

One more thing: this almost certainly would be something to handle as part of major Bean Property Introspection rewrite (for which I just created #4515).

k163377 commented 4 months ago

But one challenging part is reproducing the issue on Java side, to be able to reproduce it, verify fix & guard against regression.

You are absolutely right. It is not possible to reproduce this in the usual way, so it would need to be mocked up or something.

this almost certainly would be something to handle as part of major Bean Property Introspection rewrite

Thank you very much. However, I am sorry to suggest this, but I honestly believe it would be a low priority. There are workarounds in Kotlin, and it is reasonable to reduce the amount of processing to match the general mechanics of Java.

I don't even think it should be fixed unless there is a way to annotate only parameters in Java or similar problems occur in Scala modules.

cowtowncoder commented 4 months ago

@k163377 On its own maybe low priority, but I would definitely hope to be able to solve this case as part of general rework -- I do not think annotations on underlying Record fields should be required.

yihtserns commented 4 months ago

The decompiled Kotlin bytecode looks so bizarre:

<..snip>
public record JacksonTest() {
   @NotNull
   private final String one;
   @NotNull
   private final String two;

   @NotNull
   public final String one() {
      return this.one;
   }

   @NotNull
   public final String two() {
      return this.two;
   }

   public JacksonTest(@JsonProperty("propertyOne") @NotNull String one, @JsonProperty("propertyTwo") @NotNull String two) {
      Intrinsics.checkNotNullParameter(one, "one");
      Intrinsics.checkNotNullParameter(two, "two");
      super();
      this.one = one;
      this.two = two;
   }

   @NotNull
   public final String component1() {
      return this.one;
   }

   @NotNull
   public final String component2() {
      return this.two;
   }

   <..snip>
}
yihtserns commented 4 months ago

Likely caused by https://github.com/FasterXML/jackson-databind/pull/3929.

But I think the "root" cause is that weird "record class" generated by @JvmRecord - it basically violates the Record spec esp by not propagating the @JsonProperty annotation to the accessor methods. UPDATE: I see that OP has filed a bug against Kotlin.

k163377 commented 4 months ago

@yihtserns Let me thank you again. As I am not familiar with the Java record specification, I considered that ticket as just a feature request and did not even share it with this Issue, but it seems I was wrong.

cowtowncoder commented 3 months ago

With #4515 now merged, this might be solvable more easily: detection and merging of annotations between Fields and Constructor parameters now works, even for implicitly (no annotations) detected Constructors (like canonical constructors of Record).

But will need a Java unit test in some form.

... or add in Kotlin module I suppose.