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

Unexpected deserialization behavior with @JsonCreator, @JsonProperty and javac -parameters #4545

Closed
1 task done
ajacob opened this issue May 28, 2024 · 6 comments
Closed
1 task done

Comments

@ajacob
Copy link

ajacob commented May 28, 2024

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

I found a behavior change when enabling the javac -parameters option on an existing java project.
Not sure if this is a bug or intended, just sharing in case it can help others.

  • In a @JsonCreator constructor, the value from a @JsonProperty appears to be ignored for a parameter if a match exists between a json key name and the parameter name
  • This behavior is not true anymore if the code is compiled with -parameters (javac) and ParameterNamesModule module is registered

Version Information

2.17.1

Reproduction

Have jackson-module-parameter-names as a dependency:

<dependency>
	<groupId>com.fasterxml.jackson.module</groupId>
	<artifactId>jackson-module-parameter-names</artifactId>
</dependency>

Object used for deserialization:

public class Payload {

    private final String key1;
    private final String key2;

    @JsonCreator
    public Payload(
            @JsonProperty("key") String key1, // NOTE: the mismatch `key` / `key1` is important
            @JsonProperty("key2") String key2
    ) {
        this.key1 = key1;
        this.key2 = key2;
    }

    public String getKey1() {
        return key1;
    }

    public String getKey2() {
        return key2;
    }
}

Test:

@Test
void test() throws JsonProcessingException {
	var jsonPayload = """
			{
				"key1": "val1",
				"key2": "val2"
			}
			""";

	Payload payload = new ObjectMapper()
			.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
			.findAndRegisterModules()
			.readValue(jsonPayload, Payload.class);

	Assertions.assertEquals(payload.getKey1(), "val1");
	Assertions.assertEquals(payload.getKey2(), "val2");
}

This test pass, I find it a bit weird that the @JsonProperty value can be by-passed if the parameter name matches, but I guess this is a wanted/documented behavior.

Now, compile the code with -parameters javac option:

<build>
	<plugins>
		<plugin>
			<groupId>org.apache.maven.plugins</groupId>
			<artifactId>maven-compiler-plugin</artifactId>
			<version>3.12.1</version>
			<configuration>
				<parameters>false</parameters>
			</configuration>
		</plugin>
	</plugins>
</build>

Without any code change, the test does not pass anymore because key1 is null after deserialization.

I have created a small project to reproduce this case: https://github.com/ajacob/jackson-javac-parameters

Expected behavior

We expected key1 to be null after deserialization when -parameters is not enabled but may be this is a wanted behavior to have a fallback on the parameter name.

We didn't expected a behavior change when -parameters option is enabled on javac with ParameterNamesModule enabled.

In our case, we simply fixed the wrongly annotated parameters from the constructor to fix our issue

Additional context

No response

@ajacob ajacob added the to-evaluate Issue that has been received but not yet evaluated label May 28, 2024
@ajacob
Copy link
Author

ajacob commented May 30, 2024

We also noticed that serialization behavior was different, without -parameters the resulting json is:

{
    "key2" : "val2",
    "key1" : "val1"
}

with -parameters the resulting json becomes:

{
    "key" : "val1",
    "key2" : "val2"
}

This can be reproduced with the following test:

@Test
void test_serialization() throws JsonProcessingException {
	Payload payload = new Payload("val1", "val2");

	String jsonPayload = new ObjectMapper()
			.enable(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS) // deterministic output
			.enable(SerializationFeature.INDENT_OUTPUT)
			.findAndRegisterModules()
			.writeValueAsString(payload);

	Assertions.assertEquals("""
			{
			  "key2" : "val2",
			  "key1" : "val1"
			}""", jsonPayload);
}

I have updated the following repository: https://github.com/ajacob/jackson-javac-parameters

@cowtowncoder
Copy link
Member

Quick note: change in serialization order is most likely due to MapperFeature.SORT_CREATOR_PROPERTIES_FIRST defaulting to true (for historical reasons): basically, properties bound to a Creator (constructor or factory method) gets sorted before any other properties, regardless of whether alphabetic ordering is enabled.
Note, though, that without specific definition of property sorting, order is not really guaranteed, because JDK does not guarantee order in which Field and Method declarations are exposed via Reflection

Anyway: you may want to disable that setting.

@JooHyukKim
Copy link
Member

Should we start this issue over at parameter-names-module repo?

@cowtowncoder
Copy link
Member

I was hoping to create a reproduction on jackson-databind side, with implicit name introspector.
That way we could cover 2.x and 3.0 both (3.0 won't have separate parameter-names module).

cowtowncoder added a commit that referenced this issue May 31, 2024
@cowtowncoder
Copy link
Member

Ok, with #4515 work, this actually works as expected now: deserialization fails as expected.

However: I noticed that there can be cases where it unexpectedly passes (withOUT parameter names module...). This is because of combination of public String getKey1(); and field String key1.
With default settings this field is actually considered mutator for key1 -- despite being final.
To disable it, MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS needs to disabled (enabled by default).

cowtowncoder added a commit that referenced this issue May 31, 2024
@ajacob
Copy link
Author

ajacob commented Jun 4, 2024

Ok, with #4515 work, this actually works as expected now: deserialization fails as expected.

However: I noticed that there can be cases where it unexpectedly passes (withOUT parameter names module...). This is because of combination of public String getKey1(); and field String key1. With default settings this field is actually considered mutator for key1 -- despite being final. To disable it, MapperFeature.ALLOW_FINAL_FIELDS_AS_MUTATORS needs to disabled (enabled by default).

Thank you for having a look and for detailed explanation

@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants