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

Exception thrown for incorrectly annotated record changed from InvalidDefinitionException to MismatchedInputException #3988

Closed
soc opened this issue Jun 16, 2023 · 23 comments
Labels
to-evaluate Issue that has been received but not yet evaluated

Comments

@soc
Copy link

soc commented Jun 16, 2023

Describe the bug

The exception thrown for an incorrectly annotated record has changed.

Version information
2.15.2

To Reproduce

Consider these two examples:

public record RecordWithSingleValueValue(@JsonValue String value) {}

public record RecordWithSingleValueCreatorDelegating(String value) {

  @JsonCreator(mode = JsonCreator.Mode.DELEGATING)
  public RecordWithSingleValueCreatorDelegating {}

}

public class JacksonSerializationTest {

  private static final ObjectMapper mapper = new ObjectMapper();

  @Test
  public void recordWithSingleValueValue() throws JsonProcessingException {
    var instance = new RecordWithSingleValueValue("foo");
    var result = mapper.writeValueAsString(instance);
    assertEquals("\"foo\"", result);
    // missing setter
    assertThrows(InvalidDefinitionException.class,
        () -> mapper.readValue(result, RecordWithSingleValue.class));
  }

  @Test
  public void recordWithSingleValueCreatorDelegating() throws JsonProcessingException {
    var instance = new RecordWithSingleValueCreatorDelegating("foo");
    var result = mapper.writeValueAsString(instance);
    assertEquals("{\"value\":\"foo\"}", result);
    // missing setter
    assertThrows(InvalidDefinitionException.class,
        () -> mapper.readValue(result, RecordWithSingleValueCreatorDelegating.class));
  }
}

Expected behavior
In 2.14.2 InvalidDefinitionException was thrown as shown above, with 2.15.3 this changed to MismatchedInputException.
From my understanding InvalidDefinitionException was perhaps more "correct"?

Additional context
I tried to figure out whether this change was intentional or not, but couldn't find anything in the changelog or issues.
I just wanted to let you know of this change – apologies if this was intentional and I missed it.

(I also found some other behavioral changes in 2.15, but I assume they are intentional – things that weren't deserializing before are now deserializing.)

@soc soc added the to-evaluate Issue that has been received but not yet evaluated label Jun 16, 2023
@yihtserns
Copy link
Contributor

Looking at the javadoc:

  • InvalidDefinitionException: Something is wrong with the class to deserialize into.
  • MismatchedInputException: The wrong input has been provided.

The scenario is providing JSON String to a class that expects JSON Object, and providing JSON Object to a class that expects a JSON String. Isn't that "the wrong input has been provided"?

@yihtserns
Copy link
Contributor

...things that weren't deserializing before are now deserializing.

What's the scenarios for those?

@soc
Copy link
Author

soc commented Jun 16, 2023

Isn't that "the wrong input has been provided"?

Good point! Perhaps my confusion comes from things changing for record, but not for class. 🤔
(I basically have a small test suite that tests all possible combinations of Jackson annotations.)

@soc
Copy link
Author

soc commented Jun 16, 2023

Ooops, wait a minute, I copied the wrong record definitions above.

Edit: Fixed, apologies. Both records in the original post were the wrong ones for the tests I showed.

@soc
Copy link
Author

soc commented Jun 16, 2023

What's the scenarios for those?

public record RecordWithSingleValue(String value) {}

public record RecordWithSingleValueCreatorProperties(String value) {

  @JsonCreator(mode = JsonCreator.Mode.PROPERTIES)
  public RecordWithSingleValueCreatorProperties {}

}

now both round-trip, they failed with InvalidDefinitionException on 2.14.

@yihtserns
Copy link
Contributor

yihtserns commented Jun 16, 2023

I think you need to double check your scenarios, because when I tried using 2.14.0:

recordWithSingleValueValue recordWithSingleValueCreatorDelegating
Failed with:
com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Problem with definition of [AnnotedClass com.github.yihtserns.records.jackson.RecordWithSingleValueValue]: Multiple 'as-value' properties defined ([field com.github.yihtserns.records.jackson.RecordWithSingleValueValue#value] vs [method com.github.yihtserns.records.jackson.RecordWithSingleValueValue#value()])
at:
@Test
public void recordWithSingleValueValue() throws JsonProcessingException {
    var instance = new RecordWithSingleValueValue("foo");
    var result = mapper.writeValueAsString(instance); <---- failed here
    ...
}
org.opentest4j.AssertionFailedError: Unexpected exception type thrown, 
Expected :class com.fasterxml.jackson.databind.exc.InvalidDefinitionException
Actual   :class com.fasterxml.jackson.databind.exc.MismatchedInputException

public record RecordWithSingleValue(String value) {}

public record RecordWithSingleValueCreatorProperties(String value) {
  @JsonCreator(mode = JsonCreator.Mode.PROPERTIES)
  public RecordWithSingleValueCreatorProperties {}
}

now both round-trip, they failed with InvalidDefinitionException on 2.14.

Sorry I don't understand what that means, you'd have to explain more than that... 😞

@soc
Copy link
Author

soc commented Jun 16, 2023

I think you need to double check your scenarios, because when I tried using 2.14.0

@yihtserns Did you see my edit regarding the class definitions above?
Previously I ran on 2.14.2, so perhaps your 2.14.0 does things differently?

Sorry I don't understand what that means, you'd have to explain more than that... disappointed

Each of the examples can deserialize the value it serialized.


I'm reverting the whole thing, just got another failure about

Invalid type definition for type ...Dto: Argument #5 of constructor [constructor for ...Dto (6 args), annotations: [null] has no property name annotation; must have name when multiple-parameter constructor annotated as Creator

for a record that is not annotated with @JsonCreator, and also has no property name annotations for args #1-#4 or #6 either.

@yihtserns
Copy link
Contributor

yihtserns commented Jun 16, 2023

Version ⬇️ recordWithSingleValueValue recordWithSingleValueCreatorDelegating
2.14.2
org.opentest4j.AssertionFailedError: Unexpected exception type thrown, 
Expected :class com.fasterxml.jackson.databind.exc.InvalidDefinitionException
Actual   :class com.fasterxml.jackson.databind.exc.MismatchedInputException
org.opentest4j.AssertionFailedError: Unexpected exception type thrown, 
Expected :class com.fasterxml.jackson.databind.exc.InvalidDefinitionException
Actual   :class com.fasterxml.jackson.databind.exc.MismatchedInputException
2.15.3-SNAPSHOT Successfully deserializes because of new feature #3654 (+#3724).
org.opentest4j.AssertionFailedError: Unexpected exception type thrown, 
Expected :class com.fasterxml.jackson.databind.exc.InvalidDefinitionException
Actual   :class com.fasterxml.jackson.databind.exc.MismatchedInputException

I see there's no change for recordWithSingleValueCreatorDelegating between 2.14.2 & 2.15.3-SNAPSHOT (NOTE: 2.15.3 is not released yet): they're both throwing MismatchedInputException in my machine. 🤔

@yihtserns
Copy link
Contributor

I'm reverting the whole thing, just got another failure about

Invalid type definition for type ...Dto: Argument #5 of constructor [constructor for ...Dto (6 args), annotations: [null] has no property name annotation; must have name when multiple-parameter constructor annotated as Creator

for a record that is not annotated with @JsonCreator, and also has no property name annotations for args #1 or #6 either.

Without direct access to your machine, it's basically impossible to know what you're talking about. Maybe you can create a Github repository with all the (failing) test cases to describe the issue - that's the most effective way for us to be "in-sync" with each other.

@cowtowncoder
Copy link
Member

I would suggest actually addressing a small number -- possibly just one -- fail at a time and not bundling together all cases.

But aside from that, the general idea would be that:

  1. Things that are wrong with definition -- regardless of input; that is, occurring during construction/initialization of deserializer -- should results in InvalidDefinitionException
  2. Things that could work (value/deserializer annotations valid) but do not work for encountered input should result in exceptions like MismatchedInputException

Whether change in specific case is intentional, valid, or invalid depends on many things of course. Above is just the general guidance.
We try not to change exception types unless there was an incorrect one before; exceptions thrown are considered part of API definition. One exception to this (no pun intended) is that sometimes we may start using a more specific one (new subtype).

@soc
Copy link
Author

soc commented Jun 19, 2023

It appears as if Jackson behaves wildly different with and without a Java module-info file, even on 2.14.2.
I believe this should not be the case.

2.14.2 with module-info:
jackson-2 14 2-with-module-info

2.14.2 without module-info:
jackson-2 14 2-without-module-info

2.15.2 with module-info:
jackson-2 15 2-with-module-info

2.15.2 without module-info:
jackson-2 15 2-without-module-info

I'm not sure it makes sense to look at the behavioral changes between 2.14 and 2.15, if using the Java module system makes the results diverge that much.

Reproduction: https://github.com/soc/jackson-repro

@soc
Copy link
Author

soc commented Jun 19, 2023

I added the reproduction for

Invalid type definition for type ...Dto: Argument #5 of constructor [constructor for ...Dto (6 args), annotations: [null] has no property name annotation; must have name when multiple-parameter constructor annotated as Creator

in a second commit in the same repo.

@yihtserns
Copy link
Contributor

@soc can you also commit a Maven wrapper into your repo, please? Just want to ensure our environmental settings are close enough:

  • Running mvn test ignores all the test methods, I had to specifically specify a newer maven-surefire-plugin - this is likely because my personal Maven installation uses an older maven-surefire-plugin by default.
  • When I run JacksonSerializationTest directly in Intellij, the result WITH module-info is the same as your result for WITHOUT module-info:
    image

@yihtserns
Copy link
Contributor

yihtserns commented Jun 19, 2023

For these test cases in JacksonSerializationTest:

  • recordWithSingleValueCreatorProperties
  • recordWithSingleValue
  • recordWithSingleValueValue
  • recordWithSingleValueCreatorDelegating

...in 2.14.2 they failed with:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Invalid type definition for type `<...snip>`: Failed to call `setAccess()` on Field '<...snip>' (of class `<...snip>`) due to `java.lang.reflect.InaccessibleObjectException`, problem: Unable to make field private final java.lang.String <...snip> accessible: module repro does not "opens repro" to module com.fasterxml.jackson.databind

That InvalidDefinitionException was a bug that has been fixed by #3352. Which is why:

  • recordWithSingleValueCreatorProperties: can now deserialize the valid JSON
  • recordWithSingleValue: can now deserialize the valid JSON
  • recordWithSingleValueValue: can now fail with "mismatched input" error for the invalid JSON
  • recordWithSingleValueCreatorDelegating: can now fail with "mismatched input" error for the invalid JSON

As for JacksonSerialization2Test failing with:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Invalid type definition for type `repro2.RecordWithMultipleValuesAndOnePropertyAccessAnnotationDto`: Argument #​1 of constructor [constructor for `repro2.RecordWithMultipleValuesAndOnePropertyAccessAnnotationDto` (2 args), annotations: [null] has no property name annotation; must have name when multiple-parameter constructor annotated as Creator

...that is caused by #3724 not covering the scenario where deserialization uses a Record with a constructor property with access=READ_ONLY .

@soc
Copy link
Author

soc commented Jun 19, 2023

Ok, thanks! Then I'll wait for 2.15.3 and report back!

@cowtowncoder
Copy link
Member

This is too convoluted an issue, discussion, to be of much value (IMO), so I'll close it. Relevant, remaining pieces may be re-filed (with possible ref to this one), with clear explanation of intended correct outcome, and not so much on what has changed -- some changes are fine (fixes), others not: but it's perfectly fine to suggest that specific change is wrong (bug/regression). There is no need to ask "why was this changed" if change seems wrong: but there is clear need to clearly indicate both type of exception and message being reported -- latter likely explains how change came about (new validation, regression).

@Mochis
Copy link

Mochis commented Sep 15, 2023

I have some doubts, is the problem related to constructor property with access=READ_ONLY resolved in 2.15.3 or 2.16?

...that is caused by #3724 not covering the scenario where deserialization uses a Record with a constructor property with access=READ_ONLY .

@cowtowncoder
Copy link
Member

@Mochis Read my note above. If you have remaining problem case that is not covered by an open issue, please file a new, targeted issue for specific problem.

@Mochis
Copy link

Mochis commented Sep 19, 2023

Thank you @cowtowncoder. Reported here #4119

@soc
Copy link
Author

soc commented Sep 28, 2023

Short update: After I reported this issue, I adapted the test to the behavior on 2.15.2.

Now, with the same Jackson version (2.15.2), but updated Java version (to 21) I get similar failures for classes as previously for records:

Failures: 
  JacksonSerializationTest.classWithSingleValueCreatorDelegating:31 Unexpected exception type thrown, expected: <com.fasterxml.jackson.databind.exc.InvalidDefinitionException> but was: <com.fasterxml.jackson.databind.exc.MismatchedInputException>
  JacksonSerializationTest.classWithSingleValueCreatorDelegatingProperty:41 Unexpected exception type thrown, expected: <com.fasterxml.jackson.databind.exc.InvalidDefinitionException> but was: <com.fasterxml.jackson.databind.exc.MismatchedInputException>
  JacksonSerializationTest.classWithSingleValueCreatorPropertiesProperty:61 Expected com.fasterxml.jackson.databind.exc.InvalidDefinitionException to be thrown, but nothing was thrown.
  JacksonSerializationTest.classWithSingleValueGetConstructor:71 Unexpected exception type thrown, expected: <com.fasterxml.jackson.databind.exc.InvalidDefinitionException> but was: <com.fasterxml.jackson.databind.exc.MismatchedInputException>
  JacksonSerializationTest.classWithSingleValueProperty:90 Unexpected exception type thrown, expected: <com.fasterxml.jackson.databind.exc.InvalidDefinitionException> but was: <com.fasterxml.jackson.databind.exc.MismatchedInputException>

@soc
Copy link
Author

soc commented Nov 27, 2023

Just checked with 2.16.0 and Java 21:

Example JacksonSerializationTest.classWithSingleValueProperty:

  @Test
  public void classWithSingleValueProperty() throws JsonProcessingException {
    var instance = new ClassWithSingleValueProperty("foo");
    var result = mapper.writeValueAsString(instance);
    assertEquals("{\"value\":\"foo\"}", result);
    // missing setter
    assertThrows(InvalidDefinitionException.class,
        () -> mapper.readValue(result, ClassWithSingleValueProperty.class));
  }
  • Test green when running the test from the IDE.
  • Error message when running mvn from the CLI:
com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `ClassWithSingleValueProperty` (although at least one Creator exists): cannot deserialize from Object value (no delegate- or property-based Creator)
 at [Source: (String)"{"value":"foo"}"; line: 1, column: 2]

If I adjust the example to

  @Test
  public void classWithSingleValueProperty() throws JsonProcessingException {
    var instance = new ClassWithSingleValueProperty("foo");
    var result = mapper.writeValueAsString(instance);
    assertEquals("{\"value\":\"foo\"}", result);
    // missing setter
    assertThrows(InvalidDefinitionException.class,
        () -> mapper.readValue("\"foo\"", ClassWithSingleValueProperty.class));
  }
  • Test green when running the test from the IDE.
  • Error message when running mvn from the CLI (this is expected):
Expected com.fasterxml.jackson.databind.exc.InvalidDefinitionException to be thrown, but nothing was thrown

@JooHyukKim
Copy link
Member

JooHyukKim commented Nov 30, 2023

Test green when running the test from the IDE.
Error message when running mvn from the CLI:

What is version IDE is configured with? @soc
Also, could you check mvn version? (stdout will have jdk version)

Asking this because IDE, mvn usually not the problem but the JDK version they load.

@cowtowncoder
Copy link
Member

This is closed issue, fwtw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to-evaluate Issue that has been received but not yet evaluated
Projects
None yet
Development

No branches or pull requests

5 participants