FasterXML / jackson-databind

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

`@JsonProperty` not serializing field names properly on `@JsonCreator` in Record #4452

Closed Incara closed 2 weeks ago

Incara commented 2 months ago

Search before asking

Describe the bug

Serialization of java records do not appear to be honoring @JsonProperty names in @JsonCreator constructors.

Version Information

2.15.x+

Reproduction

A test below fails for me and I expect it to pass.

import static org.junit.Assert.assertTrue;

import org.junit.Test;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;

public class RecordTest {

    public static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();

    public record TestObject(String testFieldName, Integer testOtherField) {

        @JsonCreator
        public TestObject(@JsonProperty ("strField") String testFieldName,
                          @JsonProperty ("intField") Integer testOtherField) {
            this.testFieldName = testFieldName;
            this.testOtherField = testOtherField;
        }
    }

    @Test
    public void test() throws JsonProcessingException {
        final String result = OBJECT_MAPPER.writeValueAsString(new TestObject("test", 1));
        assertTrue(result.contains("strField"));
    }
}

Expected behavior

The field is renamed to what is specified in the @JsonProperty annotation in the record's @JsonCreator

Additional context

We encountered this upgrading from 2.14.x up to 2.16.x, so this functionality was working at that earlier version (perhaps before records were really addressed).

Perhaps we have versions incorrect between jackson modules and libraries, but we haven't found the exact mismatch yet.

yihtserns commented 2 months ago

Tested using 2.14.2 - not working. But it's amazing that it ever worked at all because annotating on constructor arguments will only produce this compiled code:

public static record TestObject(String testFieldName, Integer testOtherField) {
    @JsonCreator
    public TestObject(@JsonProperty("strField") String testFieldName, @JsonProperty("intField") Integer testOtherField) {
        this.testFieldName = testFieldName;
        this.testOtherField = testOtherField;
    }

    public String testFieldName() {
        return this.testFieldName;
    }

    public Integer testOtherField() {
        return this.testOtherField;
    }
}

...the @JsonProperty annotation won't be propagated to the field & accessor method since Java doesn't know which constructor argument is associated with which field/accessor.

Only by annotating in the Record's header i.e.:

public static record TestObject(@JsonProperty("strField") String testFieldName, @JsonProperty("intField") Integer testOtherField) {
    @JsonCreator
    public TestObject(String testFieldName, Integer testOtherField) {
        this.testFieldName = testFieldName;
        this.testOtherField = testOtherField;
    }

    public String testFieldName() {
        return this.testFieldName;
    }

    public Integer testOtherField() {
        return this.testOtherField;
    }
}

...will a usable compiled code be produced:

public static record TestObject(String testFieldName, Integer testOtherField) {
    @JsonCreator
    public TestObject(String testFieldName, Integer testOtherField) {
        this.testFieldName = testFieldName;
        this.testOtherField = testOtherField;
    }

    @JsonProperty("strField")
    public String testFieldName() {
        return this.testFieldName;
    }

    @JsonProperty("intField")
    public Integer testOtherField() {
        return this.testOtherField;
    }
}
Incara commented 2 months ago

Yeah, I think we've figured out this test specifically doesn't work on any version. We're not sure why it works for us with our application's old dependency combination. I don't know if it's something about the parameter names module or properties on the spring ObjectMapper injected at runtime that allows it to be possible.

Is this something that is intended to be supported or is it a requirement that @JsonProperty annotations must appear in the record declaration?

yihtserns commented 2 months ago

If the ObjectMapper is created with:

...then it'll work but I think that's more of a happy coincidence that worked because the constructor argument names happen to match the property (field/accessor) names. And that's now broken in 2.16.0 (works OK in 2.15.4).

I'm not entirely sure, but I vaguely remember seeing an issue talking about names... But I'm not gonna dig further since I'm just here to make sure it wasn't my contribution from 1 year ago that caused this - imma leave this to the core maintainers.

🏃‍♀️💨

Incara commented 2 months ago

Much appreciated, this helps with analysis for sure. I think that narrows this down to something changed between 2.15.4 and 2.16.0 and now it doesn't seem like @JsonProperty renames are honored on @JsonCreator in records. The below works in 2.15.4 but fails in 2.16.0:

import static org.junit.Assert.assertTrue;

import org.junit.Test;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.module.paramnames.ParameterNamesModule;

public class RecordTest {

    public static final ObjectMapper OBJECT_MAPPER = new ObjectMapper()
            .registerModule(new ParameterNamesModule()).findAndRegisterModules();

    public record TestObject(String testFieldName, Integer testOtherField) {

        @JsonCreator
        public TestObject(@JsonProperty ("strField") String testFieldName,
                          @JsonProperty ("intField") Integer testOtherField) {
            this.testFieldName = testFieldName;
            this.testOtherField = testOtherField;
        }
    }

    @Test
    public void test() throws JsonProcessingException {
        final String result = OBJECT_MAPPER.writeValueAsString(new TestObject("test", 1));
        assertTrue(result.contains("strField"));
    }
}

For now, our solution is to eliminate our @JsonCreators on records in favor using @JsonProperty directly on the record definition attributes since there is larger support in jackson for records since we last updated.

I don't know if there is supposed to be support for the above test case from the repo maintainers, but I will also stop and let them comment.

JooHyukKim commented 2 months ago

So as per release note there seem to be these three PRs --#3906, #3992, #4175 -- that are titled with the word record in it. Other than that, there is a long list of updates that got included in 2.16.0 release.

yihtserns commented 2 months ago

...titled with the word record in it.

I don't think this thing is Record-specific.

JooHyukKim commented 2 months ago

Because of project's current on-going milestone aka Property Introspection Rewrite (also mentioned in 2.17), we neither can have have fix any time soon nor confidently specify how record should behave.

Incara commented 2 months ago

Because of project's current on-going milestone aka Property Introspection Rewrite (also mentioned in 2.17), we neither can have have fix any time soon nor confidently specify how record should behave.

I think that's fine. The below test does work on 2.16.x and that suits our use cases for now.

import static org.junit.Assert.assertTrue;

import org.junit.Test;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.module.paramnames.ParameterNamesModule;

public class RecordTest {

    public static final ObjectMapper OBJECT_MAPPER = new ObjectMapper()
            .registerModule(new ParameterNamesModule()).findAndRegisterModules();

    public record TestObject(@JsonProperty ("strField") String testFieldName,
                             @JsonProperty ("intField") Integer testOtherField) {
    }

    @Test
    public void test() throws JsonProcessingException {
        final String result = OBJECT_MAPPER.writeValueAsString(new TestObject("test", 1));
        assertTrue(result.contains("strField"));
    }
}

It's possible that this is the correct and only supported future usage, but there probably needs to be some consideration (an error, documentation, warning, etc) about the presence of a @JsonCreator in a record that modifies field names or changes the input of the record. Another example below:

import static org.junit.Assert.assertTrue;

import org.junit.Test;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.module.paramnames.ParameterNamesModule;

public class RecordTest {

    public static final ObjectMapper OBJECT_MAPPER = new ObjectMapper()
            .registerModule(new ParameterNamesModule()).findAndRegisterModules();

    public record TestObject(String testFieldName, Integer testOtherField) {

        @JsonCreator
        public TestObject(@JsonProperty ("strField") String testFieldName,
                          @JsonProperty ("someOtherIntField") Integer testOtherIntField,
                          @JsonProperty ("intField") Integer testOtherField) {
            this(testFieldName, testOtherField + testOtherIntField);
        }
    }

    @Test
    public void test() throws JsonProcessingException {
        final String result = OBJECT_MAPPER.writeValueAsString(new TestObject("test", 2, 1));
        assertTrue(result.contains("intField"));
        assertTrue(result.contains("strField"));
    }
}

I'm not even sure how the above could work, so it's most likely not valid or going to be supported, but I don't know that as a consumer.

cowtowncoder commented 2 months ago

FWTW, if not Record-specific, reproduction without Record would be great (since it's likely in shared code b/w POJOs, Records).

cowtowncoder commented 1 month ago

Looking at the original case, it does seem like a flaw: @JsonCreator-annotated constructor should be used as the primary one, and any property name changes should be used from that.

And while fix may or may not be easy, a PR for reproduction, would be great. It seems to be this would be Record-specific thing, fwtw, as fully annotated case

...titled with the word record in it.

I don't think this thing is Record-specific.

Looking at the original reproduction, it would seem it is? Or why do you think it isn't?

cowtowncoder commented 1 month ago

Ok, at any rate: a PR for failing test case (under src/test-jdk17/java/..../failing) would be great, either against 2.17 or 2.18 branch.

JooHyukKim commented 1 month ago

Ok, at any rate: a PR for failing test case (under src/test-jdk17/java/..../failing) would be great, either against 2.17 or 2.18 branch.

Most test cases here make use of ParameterNamesModule, so it seems not possible?

yihtserns commented 1 month ago

@JooHyukKim I think this can help: https://github.com/FasterXML/jackson-databind/blob/2.18/src/test-jdk17/java/com/fasterxml/jackson/databind/records/Jdk8ConstructorParameterNameAnnotationIntrospector.java

cowtowncoder commented 1 month ago

Ugh. I missed ParameterNamesModule reference since that wasn't in the original description.

Correct, tests here cannot use it.

There are a few tests tho that implement replica, AnnotationIntrospector that provides bogus names for testing. So it is possible, if tedious, to do that (beside one that @yihtserns referenced).

JooHyukKim commented 1 month ago

Filed PR #4477. It does fail from 2.16 version and on.

cowtowncoder commented 2 weeks ago

Ok, I know what is causing this: before #4515, Constructor properties are not resolved for Record serialization (for some reason). It is likely this can be fixed ones #4515 fix is complete enough.