Introspection includes delegating ctor's only parameter as a property in BeanDescription

[nikita2206]

If I have ParameterNamesModule and this data class:

public class Data {
  private final String foo;
  private final Integer bar;

  @JsonCreator(mode = JsonCreator.Mode.DELEGATING)
  static Data fromBuilder(Builder builder) {
    return new Data(builder.foo, builder.bar);
  }

  private Data(String foo, Integer bar) {
    this.foo = foo;
    this.bar = bar;
  }

  public String getFoo() {
    return foo;
  }

  public Integer getBar() {
    return bar;
  }

  public static class Builder {
    private String foo;
    private Integer bar;

    @JsonProperty("foo")
    public Builder foo(String foo) {
      this.foo = foo;
      return this;
    }

    @JsonProperty("bar")
    public Builder bar(Integer bar) {
      this.bar = bar;
      return this;
    }

    public Data build() {
      return Data.fromBuilder(this);
    }
  }
}

Then running objectMapper.getSerializationConfig().introspect(/* Data type */); will return a BeanDescription that includes builder as a property.

This happens because with ParameterNamesModule we are able to infer the name of the JsonCreator parameter here and when we are, we include this parameter in the properties.

I think here we should be checking if the creator factory is a delegating kind that takes a complex value as an input. If maintainers of this repo agree, I will file a PR with the fix.

[cowtowncoder]

Hmmmh. If I understand description correctly, yes, creator method should be detected for deserialization purposes. But handling of Creator methods is not done by POJOPropertiesCollector (for historical reasons), so it does not have (nor, I think, should have) logic to deal with Creators. At least for Jackson 2.x: for 3.0 I hope we can and will fully rewrite the logic as this division is problematic.

Given this, I think that:

1. From above definition, ObjectMapper should be able to deserialize things using Builder-style pattern here. If not, a test showing failure would be useful
2. I am not sure that it is possible o make BeanDescription understand that builder is not a property -- and while it would be nice, it is not a requirement unless needed to make (1) work

Does this make sense? So, basically, there may be a problem, but that would need to be shown end-to-end, and not at level of BeanDescription.

[cowtowncoder]

One other note: also keep in mind that properties may be needed for serialization, so even when no properties are used for deserialization (where Creators matter), it may be necessary to retain property information for serialization.

I am not against pruning information to keep information more sensible for other uses, if possible: a reasonable test is to make sure none of tests fail with proposed changes (or if they fail there is a problem with tests to be fixed, uncovered by changes).
So I would be open to PR. My notes above are just suggesting it may not be trivial fix.

[kirmerzlikin]

I recently encountered the same behaviour and it lead to the deserialization failure.

You can find a reproducing test case here - https://github.com/kirmerzlikin/jackson-delegating-creator-property-bug/blob/master/src/test/java/com/example/NamesTest.java.

Test case summary

public class NamesTest {

    private final ObjectMapper objectMapper = new ObjectMapper()
  		  .registerModule(new ParameterNamesModule());

    @Test
    void testDeserialization() throws JsonProcessingException{
  	  /*
  	  * Deserialization fails with
  	  * com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Invalid definition for property 'fullName'
  	  * (of type `com.example.NamesTest$Names`): Could not find creator property with name 'fullName'
  	  * (known Creator properties: [firstName, lastName])
  	  */
  	  Names names = objectMapper.readValue("{\"firstName\":\"John\",\"lastName\":\"Doe\"}", Names.class);

  	  assertThat(names.getFirstName()).isEqualTo("John");
  	  assertThat(names.getLastName()).isEqualTo("Doe");
    }

    /*
    * Tests pass when following annotation is added
    * @JsonIgnoreProperties("fullName")
    */
    public static class Names {

  	  private final String firstName;

  	  private final String lastName;

  	  @JsonCreator(mode = JsonCreator.Mode.PROPERTIES)
  	  public Names(@JsonProperty("firstName") String firstName, @JsonProperty("lastName") String lastName) {
  		  this.firstName = firstName;
  		  this.lastName = lastName;
  	  }

  	  @JsonCreator(mode = JsonCreator.Mode.DELEGATING)
  	  public static Names fromFullName(String fullName) {
  		  String[] names = fullName.split("\\s+", 2);
  		  return new Names(names[0], names[1]);
  	  }

  	  public String getFirstName() {
  		  return firstName;
  	  }

  	  public String getLastName() {
  		  return lastName;
  	  }
    }
}

As far as I can understand, the reason for this failure is that Jackson considers a named argument of a delegating @JsonCreator a property and then attempts to use this "property" during deserialization but can not find a corresponding "setter".

IMHO if it's a delegating creator, then its argument shouldn't be considered a property since it's supposed to be a single piece of data representing the whole object, from which the said object can be created.

To achieve this, it would be enough to make POJOPropertiesCollector to skip delegating creators when collecting properties from them. Something like this - kirmerzlikin@fd76078. This change doesn't break any existing tests and fixes this new one (but I had some trouble trying to decide where to put the test case, since it looks like in jackson-databind we cannot depend on ParameterNamesModule, and putting it in jackson-modules-java8 doesn't feel right, since it's fixed in jackson-databind).

[cowtowncoder]

@kirmerzlikin Yes, no named parameter should be detected for a constructor considered delegating one. Also: good detective work; your understanding is solid here about everything you mention.

As to test: there are some tests in jackson-databind that emulate behavior of ParameterNamesModule, to allow reproducing of problems like this one; for example ImplicitParamsForCreator806Test.
So a reproduction unit test would be a nice contribution, if possible.

In the meantime I'll have a look at your fix: although properly Bean Property Introspection rewrite is needed (there's #3719 as sort of pre-req), to make Creator discovery occur before POJOPropertiesCollector activity, perhaps there could be a smaller fix in the meantime.

[cowtowncoder]

@kirmerzlikin I think your fix just might work; I can't think of an immediate problem.

If you could come up with a reproduction using simple bogus AnnotationIntrospector similar to, say, src/test/java/com/fasterxml/jackson/failing/ImplicitParamsForCreator806Test.java (or src/test/java/com/fasterxml/jackson/databind/deser/creators/DelegatingCreatorImplicitNames1001Test.java) that'd be absolutely great.
(if not, it's fine, I should be able to create a repro of my own -- just may take bit time)

[kirmerzlikin]

Thanks for taking a look, @cowtowncoder.
I'll add a test and create a PR.

[cowtowncoder]

Thank you in advance @kirmerzlikin !

Along with PR, we'd need CLA (unless we already have one from you; it's one-time-only thing), from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and that is usually easiest do by printing, filling & signing, scanning/photo, sending to cla at fasterxml dot com.
This is needed before merging the first PR (I can code review etc without one, just can't merge).

Looking forward to getting this all in, thank you once again!

[cowtowncoder]

Thank you @kirmerzlikin ! This went in and to be included in 2.17.0, release of which just started...