FasterXML / jackson-modules-java8

Set of support modules for Java 8 datatypes (Optionals, date/time) and features (parameter names)
Apache License 2.0
401 stars 117 forks source link

ParameterNamesModule: Conflicting getter definitions when using @JsonProperty annotation which overrides constructor parameter name #123

Open davidboden opened 5 years ago

davidboden commented 5 years ago

The intention of the example below is that the bean's value is serialised as both ownerUid and accountOwnerUid and that deserialisation uses the ownerUid value. The example can be made to work by removing the ParameterNameModules and relying on the @JsonProperty annotation. It appears to be rather a contrived example (it reflects some real examples, promise); I wanted to report it because I feel that it's fairly clear what the code should do and the behaviour is unexpected.

The error when running the test using Jackson 2.9.9.1 is: com.fasterxml.jackson.databind.JsonMappingException: Conflicting getter definitions for property "ownerUid": com.example.rest.server.RestServerJacksonModuleTest$TestBeanWithAnnotationsAndDeprecation#getOwnerUid(0 params) vs com.example.rest.server.RestServerJacksonModuleTest$TestBeanWithAnnotationsAndDeprecation#getAccountOwnerUid(0 params)

@Test
void testDeserialisationWorksWithAnnotationsAndDeprecation() throws Exception {
  UUID ownerUid = UUID.randomUUID();
  TestBeanWithAnnotationsAndDeprecation testBeanIn = new TestBeanWithAnnotationsAndDeprecation("endTo", ownerUid);

  StringWriter sw = new StringWriter();
  ObjectMapper objectMapper = new ObjectMapper();
  objectMapper.registerModule(new ParameterNamesModule());
  objectMapper.writeValue(sw, testBeanIn);

  TestBeanWithAnnotationsAndDeprecation testBeanOut = objectMapper.readValue(new StringReader(sw.toString()), TestBeanWithAnnotationsAndDeprecation.class);

  assertThat(testBeanOut.getEndToEndId()).isEqualTo("endTo");
  assertThat(testBeanOut.getAccountOwnerUid()).isEqualTo(ownerUid);
  assertThat(testBeanOut.getOwnerUid()).isEqualTo(ownerUid);
}

static class TestBeanWithAnnotationsAndDeprecation {
  private final String endToEndId;
  private final UUID accountOwnerUid;

  TestBeanWithAnnotationsAndDeprecation(
      @JsonProperty("endToEndId") String endToEndId,
      @JsonProperty("ownerUid") UUID accountOwnerUid
  ) {
    this.endToEndId = endToEndId;
    this.accountOwnerUid = accountOwnerUid;
  }

  public String getEndToEndId() {
    return endToEndId;
  }

  public UUID getAccountOwnerUid() {
    return accountOwnerUid;
  }

  @Deprecated
  public UUID getOwnerUid() {
    return accountOwnerUid;
  }
}

The difference between running the test with the parameter names module is that _renameProperties in POJOPropertiesCollector decides that the property needs to be renamed. The breakpoint in there is not hit without the parameter names module being registered with the ObjectMapper.

cowtowncoder commented 5 years ago

Hmmh. Interesting.

I can see how/why property introspector gets confused: the problem comes from multiple factors...

  1. Implied name (ONLY found if parameter names module registered) for 2nd argument is accountOwnerUid
  2. There is also getAccountOwnerUid(), which is then linked with constructor argument
  3. Due to rename of (1), both of these essentially get renamed as "ownerUid"
  4. Alas, there is also the "real" ownerUid() getter, getOwnerUid()

Now: while I can see what intent is, question is whether it is possible to resolve it reliably without breaking something else.

Now... assuming @Deprecated means that that getter is NOT to be used, changing that to @JsonIgnore should resolve the problem as it will ignore just that accessor but not others, because of existence of @JsonProperty.

Alternatively, I think that adding @JsonProperty next to getAccountOwnerUid() (or, if I misunderstood, next to getOwnerUid()) would also resolve the situation as one accessor would be explicit, other not (just implicit based on name).

So: on short term I would add either @JsonIgnore or @JsonProperty to break the ambiguity (from Jackson POV).

I am open to suggestion on logic for other tie-breakers, but this seems like bit of a tricky case.