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

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

Open
k163377 opened this issue May 4, 2024 · 9 comments
Labels
2.18 property-discovery Problem with property discovery (introspection) Record Issue related to JDK17 java.lang.Record support

Comments

@k163377
Copy link
Contributor

k163377 commented May 4, 2024

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
FasterXML/jackson-module-kotlin#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
Copy link
Member

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

@k163377
Copy link
Contributor Author

k163377 commented May 4, 2024

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

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

k163377 commented May 5, 2024

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

@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.

@cowtowncoder cowtowncoder added property-discovery Problem with property discovery (introspection) 2.18 and removed to-evaluate Issue that has been received but not yet evaluated labels May 5, 2024
@yihtserns
Copy link
Contributor

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

yihtserns commented May 5, 2024

Likely caused by #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
Copy link
Contributor Author

k163377 commented May 5, 2024

@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 cowtowncoder added the Record Issue related to JDK17 java.lang.Record support label May 20, 2024
@cowtowncoder
Copy link
Member

cowtowncoder commented May 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.18 property-discovery Problem with property discovery (introspection) Record Issue related to JDK17 java.lang.Record support
Projects
None yet
Development

No branches or pull requests

3 participants