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

ParameterNamesModule does not deserialize with a single parameter constructor when using SnakeCase PropertyNamingStrategy #67

Closed
sonnygill opened this issue Apr 4, 2018 · 16 comments
Milestone

Comments

@sonnygill
Copy link

sonnygill commented Apr 4, 2018

Since release 2.9.2, the following test that passes with previous versions is broken.

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.PropertyNamingStrategy;

import org.junit.Test;

import static org.assertj.core.api.BDDAssertions.*;

public class JsonCreatorSnakeCaseNamingTest
{
	@Test
	public void shouldDeserializeClassWithJsonCreatorWithSnakeCaseNaming() throws Exception {

		// given
		ObjectMapper objectMapper = new ObjectMapper();
		objectMapper.registerModule(new ParameterNamesModule());
		objectMapper.setPropertyNamingStrategy(PropertyNamingStrategy.SNAKE_CASE);

		// when
		String json = "{\"first_property\":\"1st\"}";
		ClassWithOneProperty actual = objectMapper.readValue(json, ClassWithOneProperty.class);

		then(actual).isEqualToComparingFieldByField(new ClassWithOneProperty("1st"));
	}

	static class ClassWithOneProperty {
		public final String firstProperty;

		@JsonCreator
		public ClassWithOneProperty(String firstProperty) {
			this.firstProperty = firstProperty;
		}
	}
}

The test passes with if I create the ParameterNamesModule with JsonCreator.Mode.PROPERTIES as the argument.

Is this intended? If so, this should probably be documented as a breaking change. Otherwise existing code bases break without warning.

It appears that the code responsible for this is BasicDeserializerFactory._addExplicitAnyCreator which was newly added in 2.9.2, but I am not certain.

Should I wait for a fix, or switch to using new ParameterNamesModule(JsonCreator.Mode.PROPERTIES)? Can that have any side effects that I should watch out for?

Thanks.

  • Sonny
@cowtowncoder
Copy link
Member

Is this something that passed in 2.9.1 but not in 2.9.2? Or between 2.8 and 2.9?

Changes in general are not intentional, but due to complexity of auto-detection of mode between properties/delegating model, with possibly detection of implicit names for constructor argument, they may occur. Most likely change in behavior is due to a fix to another problem, but to know which issue, it's necessary to know version in which change occurred.

As to work-around, I would recommend addition of explicit mode declaration: single-argument case is very complex, and I think existing heuristics make for fragile behavior. In a way it would be better if there was never an attempt by Jackson to guess mode, and instead require it being specified (either by annotation, or explicit mode)
But the main reason I suggest adding mode, aside from stability (explicit mode will be used and heuristics avoided) is that 2.9.5 was just released, and even if there is a flaw to fix, it will take a while for that to make it into official release.

@sonnygill
Copy link
Author

Yes. It passes in 2.9.1 but not in 2.9.2.
I just checked, and it also passes on the 2.8 branch.

I understand the single argument is complex, and adding the explicit mode declaration is an easy fix.

I would recommend documenting this in 2.9.2 release notes though. If you update to 2.9.2, it is not immediately clear that the problem is with ParameterNamesModule. All I saw was that PropertyNamingStrategy.SNAKE_CASE was no longer working.
Somewhat surprisingly, the test passes if we are not using SNAKE_CASE naming strategy and the input json uses camel case for the property name, so "{\"firstProperty\":\"1st\"}" works for a single argument constructor.

Thanks for the quick response, and for all the work put into this excellent library!

@cowtowncoder
Copy link
Member

@sonnygill Just to make sure if it is not obvious: there is no intentional change that should break handling. If and when I figure out what is going on, a note would make sense.
Actually, I'll add one now -- maybe it'll help some other users.

@sonnygill
Copy link
Author

Exactly. We postponed upgrading from 2.9.1 for quite a while because it wasn't obvious what was breaking. This might help someone else in the same situation.

To summarize, the issue happens when the following conditions are met:

  • Using ParameterNamesModule
  • Single argument constructor with JsonCreator annotation
  • custom naming strategy

The issue does not happen if any of the following conditions is true:

  • Using JsonProperty annotation
  • no custom naming strategy
  • constructor with two or more arguments

The workaround is of course to specify an explicit creatorBinding for ParameterNamesModule, for example,

new ParameterNamesModule(JsonCreator.Mode.PROPERTIES)

@sonnygill
Copy link
Author

Updated test.

package com.fasterxml.jackson.module.paramnames;

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.PropertyNamingStrategy;

import org.junit.Test;

import static org.assertj.core.api.BDDAssertions.*;

public class JsonCreatorWithPropertyNamingStrategyTest
{
	@Test
	public void testSnakeCaseNaming() throws Exception {
		verifyObjectDeserializationWithNamingStrategy(PropertyNamingStrategy.SNAKE_CASE, "{\"first_property\":\"1st\"}");
	}

	@Test
	public void testUpperCamelCaseNaming() throws Exception {
		verifyObjectDeserializationWithNamingStrategy(PropertyNamingStrategy.UPPER_CAMEL_CASE, "{\"FirstProperty\":\"1st\"}");
	}

	@Test
	public void testLowerCamelCaseNaming() throws Exception {
		verifyObjectDeserializationWithNamingStrategy(PropertyNamingStrategy.LOWER_CAMEL_CASE, "{\"firstProperty\":\"1st\"}");
	}

	@Test
	public void testLowerCaseNaming() throws Exception {
		verifyObjectDeserializationWithNamingStrategy(PropertyNamingStrategy.LOWER_CASE, "{\"firstproperty\":\"1st\"}");
	}
	
	@Test
	public void testKebabCaseNaming() throws Exception {
		verifyObjectDeserializationWithNamingStrategy(PropertyNamingStrategy.KEBAB_CASE, "{\"first-property\":\"1st\"}");
	}

	private void verifyObjectDeserializationWithNamingStrategy(final PropertyNamingStrategy propertyNamingStrategy, final String json) throws Exception {
		// given
		ObjectMapper objectMapper = new ObjectMapper();
		objectMapper.registerModule(new ParameterNamesModule());
		objectMapper.setPropertyNamingStrategy(propertyNamingStrategy);

		// when
		ClassWithOneProperty actual = objectMapper.readValue(json, ClassWithOneProperty.class);

		then(actual).isEqualToComparingFieldByField(new ClassWithOneProperty("1st"));
	}

	static class ClassWithOneProperty {
		public final String firstProperty;

		@JsonCreator
		public ClassWithOneProperty(String firstProperty) {
			this.firstProperty = firstProperty;
		}
	}
}

@scranen
Copy link

scranen commented May 24, 2018

I think this issue may have been reported before here, but wasn't recognised as a breaking change (that ticket was closed).

@scranen
Copy link

scranen commented May 24, 2018

I found another behavioural change from 3.9.1 to 3.9.2, so probably related to this. Add to the above test:

private static class ClassWithTwoProperties {
    public final int a;
    public final int b;

    private ClassWithTwoProperties(@JsonProperty("a") int a, @JsonProperty("b") int b) {
        this.a = a;
        this.b = b;
    }
}

@Test
public void testPrivateConstructorWithPropertyAnnotations() throws Exception {
    verifyObjectDeserializationWithNamingStrategy(PropertyNamingStrategy.SNAKE_CASE, "{\"a\":1, \"b\": 2}");
}

This test passes in 3.9.1, but fails in 3.9.2 (to get it to pass in 3.9.2, I need to add an @JsonCreator annotation to the constructor).

@cowtowncoder
Copy link
Member

@scranen you probably mean 2.9.1 and 2.9.2.

@scranen
Copy link

scranen commented May 24, 2018

Apologies, I do. Too many version upgrades today.

@cowtowncoder
Copy link
Member

@scranen I can not reproduce your issue; passes for me, for pre-2.9.6. Case is different as it has 2 parameters, @JsonProperty, so if you can get it to reproduce on 2.9.5 could you please file a separate issue?

@cowtowncoder
Copy link
Member

@sonnygill I can reproduce this issue, and as you say, it will only occur if:

  • @JsonCreator type (delegating vs properties) is inferred by existence if similarly named field (without linkage, it would be inferred as delegating)

While I don't know exact details of why this causes problem, I think this is due to the fact that implicitly discovered creators must be handled at a later point, when more of discovery has occurred. Unfortunately it appears that naming-strategy induced renaming of properties happens before this point (but after explicitly found Creators).

I suspect that the change itself is due to reordering of parts of processing, needed to fix some other issue. Usually one of fixed bugs in release notes would correlate to likely change but I do not see one here. It is possible this could be for a bug somewhere else (like Afterburner module), or, even due to unrelated refactoring (although less likely).

Big problem, now, is that assuming I can locate the place of change, it is likely I can not just refer the change (if it fixed a problem). It is also possible that attempts at reordering processing cause other regressions.
So although I hope to resolve this, I would suggest addition of explicit model as work-around, as it has other benefits to stability as well (heuristics for mode auto-detection really are not very robust -- if you renamed the field, detection would change to delegating).

@cowtowncoder
Copy link
Member

Also note related: FasterXML/jackson-databind#806 (in theory, even @JsonCreator should be unnecessary).

Plan is to rewrite Creator discovery process for 3.0 due to a few known issues.

@scranen
Copy link

scranen commented May 24, 2018

Sorry, forgot my project also uses different visibility settings. Added a full test script in this new ticket.

@cowtowncoder
Copy link
Member

@scranen thanks!

@cowtowncoder
Copy link
Member

For original issue, added test in jackson-databind, wrt:

FasterXML/jackson-databind#2008

which is probably somehow related.

@cowtowncoder
Copy link
Member

Ok. Found the root cause (or at least something to fix):

FasterXML/jackson-databind#2051

and that seems to resolve the problem.

@cowtowncoder cowtowncoder changed the title Regression: ParameterNamesModule does not deserialize with a single parameter constructor when using SnakeCase PropertyNamingStrategy ParameterNamesModule does not deserialize with a single parameter constructor when using SnakeCase PropertyNamingStrategy May 25, 2018
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