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

Deserialization behavior change with Records, @JsonCreator and @JsonValue between 2.17 and 2.18 #4724

Closed
1 task done
arlampin opened this issue Sep 30, 2024 · 10 comments
Closed
1 task done
Labels
2.18 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue Record Issue related to JDK17 java.lang.Record support
Milestone

Comments

@arlampin
Copy link

arlampin commented Sep 30, 2024

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

Using both JsonCreator and JsonValue annotations with Java Records makes ObjectMapper seemingly skip single-arg static JsonCreator method and try to contract instance directly. I couldn't find any hint that this was a planned change.

Version Information

2.18.0

Reproduction

The following code works with 2.17.2, but fails with 2.18.0

    @Test
    void deserialization() throws Exception {
        new ObjectMapper().readValue("\"\"", Something.class);
    }

    public record Something(String value) {

        public Something {
            if (value == null || value.isEmpty()) {
                throw new IllegalArgumentException("Value cannot be null or empty");
            }
        }

        @JsonCreator
        public static Something of(String value) {
            if (value.isEmpty()) {
                return null;
            }
            return new Something(value);
        }

        @Override
        @JsonValue
        public String toString() {
            return value;
        }

    }

Removing JsonValue annotation makes deserialization to work again.

Expected behavior

No response

Additional context

No response

@arlampin arlampin added the to-evaluate Issue that has been received but not yet evaluated label Sep 30, 2024
@ianfp
Copy link

ianfp commented Sep 30, 2024

This also happens in Kotlin. It seems to have something to do with data classes that have secondary constructors. This test reproduces it:

import com.fasterxml.jackson.databind.DeserializationFeature
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper
import com.fasterxml.jackson.module.kotlin.readValue
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.assertDoesNotThrow
import java.util.UUID
import java.util.UUID.randomUUID

data class MyThing(val id: UUID, val status: ThingStatus, val child: ChildThing) {
    constructor(status: ThingStatus, child: ChildThing) : this(randomUUID(), status, child)
}

enum class ThingStatus {
    COOL,
    NOT_COOL,
}

data class ChildThing(val id: UUID)

class MyThingTest {
    private val json = jacksonObjectMapper()
        .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)

    @Test
    fun fromJson() {
        assertDoesNotThrow {
            json.readValue<MyThing>(
                """
            {
                "id": "${randomUUID()}",
                "status": "NOT_COOL",
                "child": {
                    "id": "${randomUUID()}"
                }
            }
                """.trimIndent(),
            )
        }
    }
}

That test passes on 2.17; fails on 2.18.

@cowtowncoder cowtowncoder added Record Issue related to JDK17 java.lang.Record support 2.18 labels Sep 30, 2024
@cowtowncoder
Copy link
Member

That looks like a bug for sure: I would expect:

  1. Deserialization to use @JsonCreator annotated factory method
  2. Serialization to use value returned by @JsonValue annotated method.

One to note tho: it may (altho should not!) be necessary to use

@JsonCreator(mode = JsonCreator.Mode.DELEGATING)

if the idea is to deserialize from JSON String (instead of single-property JSON Object).

@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Sep 30, 2024
@cowtowncoder
Copy link
Member

One quick thing to check: would behavior be different if @JsonValue annotated was anything other than toString()? Just trying to rule out possibilities of what may be going on.

I also assume this same case would work for regular POJOs.

@cowtowncoder
Copy link
Member

cowtowncoder commented Sep 30, 2024

@ianfp Let's not assume it's same thing, your case looks very different from OP's.
In fact not 100% sure why you think it is the same thing at all? (perhaps it also appears as regression b/w 2.17 and 2.18... or?)

You may want to file separate issue against jackson-module-kotlin wrt your issue.

@arlampin
Copy link
Author

arlampin commented Oct 1, 2024

  1. mode = JsonCreator.Mode.DELEGATING doesn't seem to have any effect
  2. The name of the method doesn't seem to have any effect either
  3. Regular POJO does work

@cowtowncoder cowtowncoder added the has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue label Oct 1, 2024
@cowtowncoder
Copy link
Member

Thank you for checking @arlampin! These are about as I expected -- but wanted to rule out some possibilities.

Failing test added via #4726, to hopefully help fix the issue.

@yihtserns
Copy link
Contributor

What we know:

  1. When the constructor has only 1-arg + there is @JsonValue annotation, currently that constructor becomes a delegating creator:
    /**
    * [databind#3180]
    * This test-case is just for documentation purpose:
    * Unlike {@link #testDeserializeUsingImplicitSingleValueConstructor()}, annotating {@code @JsonValue}
    * to a Record's header results in a delegating constructor.
    */
    @Test
    public void testDeserializeUsingImplicitSingleValueConstructor_WithJsonValue() throws Exception {
    // Can use delegating creator
    RecordWithSingleValueConstructorWithJsonValue value = MAPPER.readValue(
    "123",
    RecordWithSingleValueConstructorWithJsonValue.class);
    assertEquals(new RecordWithSingleValueConstructorWithJsonValue(123), value);
    try {
    // Can no longer use properties-based creator
    MAPPER.readValue("{\"id\":123}", RecordWithSingleValueConstructorWithJsonValue.class);
    fail("should not pass");
    } catch (MismatchedInputException e) {
    verifyException(e, "Cannot construct instance");
    verifyException(e, "RecordWithSingleValueConstructorWithJsonValue");
    verifyException(e, "although at least one Creator exists");
    verifyException(e, "cannot deserialize from Object value");
    }
    }
  2. In previous versions, a constructor/method annotated with @JsonCreator will have precedence, which is why Something.of is used as the delegating creator - seems like this precedence is likely swapped in 2.18.0.

While #​2 is out of their control, a (desperate) user can do something with #​1 to work around the regression:

public record Something(...) {

    @JsonCreator(mode = Mode.DISABLED)
    public Something {
        ...
    }
    ...
}

@JooHyukKim
Copy link
Member

In previous versions, a constructor/method annotated with @JsonCreator will have precedence, which is why Something.of is used as the delegating creator - seems like this precedence is likely swapped in 2.18.0.

Do you think there is chance that solution is as simple as swapping back the precedence, @yihtserns?
I am afraid not, but just in case 😀.

@cowtowncoder
Copy link
Member

Changing precedence not intentional; but this wasn't a case covered by unit tests it seems.

Records have "default" or "preferred" constructor, but that should have lower precedence than explicitly annotated constructor or factory method.

@cowtowncoder
Copy link
Member

Yes, unless drastic changes were needed, fix for 2.18.1 it all possible.

@cowtowncoder cowtowncoder changed the title Deserialization behavior change with Java Records, JsonCreator and JsonValue between 2.17.2 => 2.18.0 Deserialization behavior change with Records, @JsonCreator and @JsonValue between 2.17 and 2.18 Oct 9, 2024
@cowtowncoder cowtowncoder modified the milestones: 2.18.0, 2.18.1 Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.18 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue Record Issue related to JDK17 java.lang.Record support
Projects
None yet
Development

No branches or pull requests

5 participants