JsonMappingException: Conflicting getter / setter definitions when using property names that start with set, get or is

[janeisklar]

I started to experience the following issue starting from 2.10 in combination with the jackson scala module.

Consider a POJO like this:

class SamplePojo {
    private String settlementDate;
    private String getaways;
    private Boolean island;

    public void settlementDate(String v) { throw new Error("Should not get called"); }
    public String settlementDate() { throw new Error("Should not get called"); }
    public void setSettlementDate(String v) { settlementDate = v; }
    public String getSettlementDate() { return settlementDate; }

    public void getaways(String v) { throw new Error("Should not get called"); }
    public String getaways() { throw new Error("Should not get called"); }
    public void setGetaways(String v) { getaways = v; }
    public String getGetaways() { return getaways; }

    public void island(Boolean v) { throw new Error("Should not get called"); }
    public Boolean island() { throw new Error("Should not get called"); }
    public void setIsland(Boolean v) { island = v; }
    public Boolean isIsland() { return island; }
  }

When serializing, one will receive a JsonMappingException because both getaways() and getGetaways(), as well as island() and isIsland() are considered conflicting getters with the same priority.

When deserializing, both settlementDate(String) and setSettlementDate(String) will be conflicting setters with the same priority.

Case-sensitivity should be respected when determining the priority of setters and getters, i.e. settlementDate() should not be the same as setTlementDate().

Also see _setterPriority() in POJOPropertyBuilder (as well as _getterPriority()):

    protected int _setterPriority(AnnotatedMethod m)
    {
        final String name = m.getName();
        if (name.startsWith("set") && name.length() > 3) {
            // should we check capitalization?
            return 1;
        }
        return 2;
    }

I'll submit a pull request to have this fixed later today.

[cowtowncoder]

As per my comment on patch, I am not convinced that premises here are correct: and that it is likely that there is significant usage for such naming. I am not saying using such naming is a good idea (capitalization oddities will cause problems wrt matching to fields, probably), but I would not just go and significantly change name mangling.

[cowtowncoder]

Ah. I missed one part: note on Scala module. This can complicate things a lot... I do not think pure Java reproduction of class would have these issues (since method like settlementDate() would not be auto-detected as setter or getter, but only if explicitly annotated).

But I will try to reproduce this with just jackson-databind to make sure there isn't some odd regression (if there is, that definitely needs addressing).

[cowtowncoder]

I can not reproduce conflicting setters/getters part with jackson-databind, so issue would seem to be Scala-specific, and if so needs to be filed against jackson-module-scala.
I can transfer this ticket, but not sure what would happen to related PR (I assume it'd stay here), if that makes sense.

But there seems to be the other aspect: that of whether getter/setter detection should require that the character following "get", "set" or "is" marker should be capitalized or not. As behavior of Jackson since beginning has been not to require this, alternate behavior would need to be opt-in: this could be done by adding something like MapperFeature.REQUIRE_CAPITALIZED_PROPERTY_NAME (just an example name, I'm sure there are better alternatives :) ) and then allowing different logic.

Wrt this second aspect it would be good to find definitions that require such capitalization limits (Bean naming spec, maybe?) to refer to.

[janeisklar]

In order not to comment on both the pull request and the issue at the same time I'll copy my comment from the pull request over here once more:

Sure, see the specification of JavaBeans chapter 8.3 (page 55ff):

If we discover a matching pair of "get<PropertyName>" and "set<PropertyName>" methods that take and return the same type, then we regard these methods as defining a read-write property whose name will be "<propertyName>". We will use the "get<PropertyName>" method to get the property value and the "set<PropertyName>" method to set the property value.

If you decide not to follow this convention how else would you differentiate a setter from a property name that simply starts with "set"?

[janeisklar]

Since capitalization in my view is a standard for getters and setters I would much rather see an opt-out toggle rather than an opt-in toggle. Would you accept the pull request if I were to add such a MapperFeature?

Moving the issue to jackson-module-scala would not help as the issue is with jackson-databind itself (please see the respective code change). But having a toggle to opt-out might be a possible compromise if you insist on such a toggle.

[janeisklar]

To maybe give you a bit more background: we are using Spring Boot and generate our DTOs using swagger-codegen, which I would consider a pretty standard setup.
For a property settlementDate in a DTO called Payment, swagger will generate both the java beans setter void setSettlementDate(date) as well as setter for fluent interfaces Payment settlementDate(date) (which can only be altered by providing custom generator templates).
I'm not sure what caused it, but deserialization has stopped working for us with 2.10 and hence I'm trying to work out a solution to the issue.

[janeisklar]

I added a second pull request that contains a new feature MapperFeature.REQUIRE_CAPITALIZED_PROPERTY_ACCESSOR_NAME which allows to turn this behaviour off. I hope that you agree with me that this should be the default behaviour, but feel free to disable it per default.

[cowtowncoder]

@janeisklar Thank you for PR! I feel quite strongly about not enabling this by default for jackson-databind 2.x, just because my gut feeling is that there is existing usage that would break with the change. I will change default in 3.0 (master), however, and can just remove feature there.

But I think Scala-module could change this feature by default on registration (even in 2.x) if that makes sense.

Still... I am wondering if there is an issue with logic Scala module uses on detecting properties.
I know there are challenges in getting more advanced data/case class models of Scala and Kotlin to map to Jackson's "old school" Java centric beans. But it does not seem right to assume that method like

settlementDate(date)

should be auto-detected by default, to match with settlementDate field. At least jackson-databind does not do that by default, ever (although one can configure this for Builder case by defining "empty" prefix).

It could then be that there are 2 issues: one is (to me) minor deviation from Bean naming convention.
I am also beginning to think that it is necessary to somehow make name-based auto-detection more extensible; adding more on/off features will not scale very far.

[cowtowncoder]

After writing all of above, I think I will have to think a bit more about this.

There is an existing feature, MapperFeature.USE_STD_BEAN_NAMING, with semantics to use proper compliant handling of getURL() (to infer URL instead of earlier url).
This change could actually piggy-back on that change.

I will send a note on user mailing list.

Such change would have to go in 2.12(.0), being opt-out change essentially.

[cowtowncoder]

At this point, I am not certain I will want to proceed with this since there has not been any recent change to name handling for years (unless I miss something), so it seems more likely something has changed either in Scala module, or some interaction between the two. So instead of trying to make a potentially risky change it would be necessary to first check what might have triggered the problem.

@pjfanning any guesses as to what might be going on here? I wonder if I should move this to jackson-module-scala

[pjfanning]

I do not see any Scala code so far - and the Java code would not map to any Scala coding norms that I am aware of. If there is a reproducible scala test case, I'll look at it.

[cowtowncoder]

@pjfanning while original code is in Java, it would not fail the way described without Scala module: I think Scala module applies rules to connect methods like public void settlementDate(String v) to field named the same (private String settlementDate;).
I assume this is the case since basic jackson-databind does not use this logic, but other extensions (Kotlin module) have added handling for some cases (various types of value classes).
I may be wrong here of course; esp. if Scala handling only applies to specific types like case classes.

Another thing that might help would be to know fixes that went in 2.10 -- I know release notes have sometimes been incomplete for Scala module (and/or I haven't know where to look?) -- so

https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.10

only lists one fix which does not seem related. But maybe some other fix could be related.

[pjfanning]

@cowtowncoder https://github.com/pjfanning/jackson-databind/tree/issue-2729 and https://github.com/pjfanning/jackson-databind/tree/issue-2729-2.10 show the issue exists in jackson-databind 2.11 and 2.10 respectively - neither case has the jackson-scala-module enabled

the same test fails on the 2.9 branch too

[cowtowncoder]

@pjfanning I think there is a bit of (understandable) confusion here.

So, the problem test reports wrt there:

        public String getaways() { throw new Error("Should not get called"); }
        public Boolean island() { throw new Error("Should not get called"); }

is something separate from what title suggest, conflicting getters.

At this point, and for past 12+ years or so, these 2 methods have been and are considered valid getters to infer property. This may not be Bean Naming compliant, although I don't know if other Java tools would exclude them.
So for now the solution would be to add @JsonIgnore on them (for existing versions). Going forward, adding a MapperFeature (or something similar... there may be a set of specific introspection aspects to extract into separate set) would be an option. That option can not be default for 2.x but could be for 3.x.

But what was originally claimed, as I understand it, was not just that "hey do not recognize these methods as accessors", but also that there would be conflict between (I thought):

        private Boolean island;

        public Boolean island() { return island; }
        public Boolean isIsland() { return island; }

(and various combinations)

Regular databinding derives property island from only Field above, and from isIsland() (or, getIsland() or even isisland()). It does not recognize method public Boolean island() as accessor).
So I was guessing that if that was to happen, it would have to be additional introspection by Scala and/or Kotlin modules, as they are trying to solve bit different problem: that of certain classes being composed from field definitions, adding synthetic accessors.

If this is not the case, and I just misunderstood part of "conflicting getters" -- conflict meaning there are multiple ones causing exception -- and what is meant is "using wrong method", then that's my bad.

@janeisklar Do you have the original exception stack trace available? That would help validate the 2nd issue (conflicting methods); compared to 1st issue which is clear (that of Bean Naming incompatibility).

[janeisklar]

To hopefully make everything a bit clearer I added a minimal project that reproduces the issue within the setup we are using.

When running the project with clean verify --fail-at-end you will see the following versions fail due to the altered behaviour:

[INFO] dto ................................................ SUCCESS [ 4.133 s]
[INFO] jackson-10-9 ....................................... SUCCESS [ 1.649 s]
[INFO] jackson-10-10 ...................................... SUCCESS [ 1.211 s]
[INFO] jackson-10-11 ...................................... SUCCESS [ 1.204 s]
[INFO] jackson-10-9-with-scala ............................ SUCCESS [ 1.777 s]
[INFO] jackson-10-10-with-scala ........................... FAILURE [ 1.774 s]
[INFO] jackson-10-11-with-scala ........................... FAILURE [ 1.960 s]

The relevant exception is this one:

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Conflicting setter definitions for property "settlementDate": test.model.SetInSetter#settlementDate(1 params) vs test.model.SetInSetter#setSettlementDate(1 params)
at [Source: (String)"{"settlementDate":"foo"}"; line: 1, column: 1]

at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:62)
at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.buildBeanDeserializer(BeanDeserializerFactory.java:227)
at com.fasterxml.jackson.databind.deser.BeanDeserializerFactory.createBeanDeserializer(BeanDeserializerFactory.java:143)
at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer2(DeserializerCache.java:414)
at com.fasterxml.jackson.databind.deser.DeserializerCache._createDeserializer(DeserializerCache.java:349)
at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:264)
at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:244)
at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:142)
at com.fasterxml.jackson.databind.DeserializationContext.findRootValueDeserializer(DeserializationContext.java:491)
at com.fasterxml.jackson.databind.ObjectMapper._findRootDeserializer(ObjectMapper.java:4669)
at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4478)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3434)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3402)
at test.SerializationTest.setInSetterTest(SerializationTest.java:33)
...

If I changed the pull request such that this becomes an optional flag that is deactivated by default, would that be an acceptable approach to move this forward?

[pjfanning]

A fix could take a while - a potential workaround (other than using the jackson 2.9) is to use separate ObjectMapper instances for scala and java classes. You don't need to register the scalamodule if you are working with plain java classes.

[janeisklar]

We are currently using a custom patch build based on my pull request. That's not ideal, but takes off the pressure for the moment. The internal framework we are using is also using scala and it's a bit hard to use a different ObjectMapper as it is used by Spring Boot implicitly. Nevertheless, let me know if I can help.

[cowtowncoder]

Thank you for the update @janeisklar and apologies for slow follow up. I think the PR suggested makes sense, and the only hold up I have has to do with thoughts on MapperFeature -- right now there is bit of limitation on features to add, and I have concurrent plans of maybe creating more granular feature sets.

Also thank you for adding stack trace: it does indeed look like settlementDate() is detected as setter for Scala and not "plain" mapper -- that is something I suspected.
It makes sense for Scala and Kotlin to support type classes that are defined in terms of logical property name (instead of getter/setter pair), but not something core databind implements.

[cowtowncoder]

One new (for 2.12) feature that could be helpful is this one: #2800

wherein a new pluggable handle (AccessorNamingStrategy) may be registered (by configuring AccessorNamingStrategy.Provider) and it will be called to determine if given signature-compatible mutator (setter, "withX" for builder) or accessor (getter, is-getter) implies a property, returning name of such property if matching. It can also apply name-mangling to fields similarly.
Formerly this functionality was accessed through static methods in BeanUtil.

Strategy implementation is initialized on per-type basis so that it should also be possible to inspect the whole class first, and thereby consider combination of field+method and other per-class aspects.

This could open door both for per-module (Scala, Kotlin) default alternatives and, perhaps, reusable alternate implementations.

[pjfanning]

FasterXML/jackson-module-scala#455 might help too

[cowtowncoder]

@pjfanning ah cool. Yes.

[cowtowncoder]

One further thought: I can quite easily make default AccessorNamingStrategy implementation allow further logic regarding the first character of tentative property name. Two main alternatives would be:

1. Have a boolean property to require capital letter as the first letter -- so isLand() -> Land is ok, but neither island() nor, say, is2go() or is_open() would qualify
2. Have a boolean property allow/deny name that starts with lower-case letter -- if allowed, all of isLand() and is2go(), is_open() would qualify, but NOT island().

Configuration would be done with something like:

        ObjectMapper mapper = JsonMapper.builder()
                .accessorNaming(new DefaultAccessorNamingStrategy.Provider()
                        .allowLowerCaseFirstCharacter(true)
                )
                .build();

and would apply to all accessors (setter/with-method, getter/is-getter).

[cowtowncoder]

Implemented: configuration would be done by using:

ObjectMapper mapper = JsonMapper.builder()
                .accessorNaming(new DefaultAccessorNamingStrategy.Provider()
                        // accept lower-case first? no - accept non-letter first? no:
                        .withFirstCharAcceptance(false, false))
                .build();

but it is possible to define other validation rules as well.

I will file a follow-up request to change default handling for 3.0, to change accepted naming as suggested here.

[cowtowncoder]

Created #2882