Fix #340

[k163377]

Kotlin generates an getter named isXxx for a property named isXxx.
https://kotlinlang.org/docs/java-to-kotlin-interop.html#package-level-functions

On the other hand, Jackson treats a getter named isXxx as a property named xxx.
This has caused several problems in the past.

Currently jackson-module-kotlin avoids this problem by implementing findRenameByField(#287).
On the other hand, this approach causes unintended behavior when properties named xxx and isXxx are set (#340).

So I changed the name rewriting to do findImplicitPropertyName to fix this problem.

Concerns

@cowtowncoder
My main concern is that I am not fully aware of the scope of the impact of this change.

#287 changed from using findImplicitPropertyName to using findRenameByField, why is this?
All the tests that existed in jackson-module-kotlin were successful except for the failing test, and I could not read any intent in the implementation as far as I could find.
Any input on the impact of this change in terms of databind would be appreciated.

Also, even though this is a fix for a bug, it changes some of the behavior, do I need to be concerned about this?
I will add a note to the release notes in due course.

[cowtowncoder]

Ok: I think you should ONLY need to consider isXxx() case, given that getXxx() should work by default.

The intent of "implicit name" was originally to allow exposing names for constructor parameters, because names were not included in byte code, unlike with methods and fields. So if there is no addition "implicit" name, underlying method/field name is used. It seems better (at least conceptually) to only override cases where there is difference.

Otherwise, I think that use of implicit name route may be better than rename using findRenameByField... if it works as well I see no reason for other case.

However: use of findRenameByField() may make sense for a different case: case where property name starts with capital letter. Intent with this method is that instead of starting with getters, setters and fields all to unify names, instead start with field name. This tends to work better for Kotlin, Scala etc where developers specify property name as "plain", similar to it being field (and in fact being added as Field) and getter/setter name is created based on that.

[k163377]

Ok: I think you should ONLY need to consider isXxx() case, given that getXxx() should work by default.

This branching only applies when the getter name contains -, so I believe the behavior is as you say.
The reason for this is as per the comments on lines 39 and 40.

I think that use of implicit name route may be better than rename using findRenameByField.

I don't see how using findRenameByField would avoid the problem (#340) of confusion when two properties, isFoo and foo, are defined (this is the main problem I wanted to solve).

[cowtowncoder]

Ah. Ok, that - part I somehow missed although explicitly stated. Still, what I was trying to say is that I think you are right in preferring implicit name production over findRenameByFiedl() for cases you are thinking of solving.

[cowtowncoder]

@k163377 I do not (alas) know the logic of changes, but I think your intuition is right: with only minor change I'd suggest (only providing implicit name for isXxx() cases, leaving getXxx() as-is), I think that might be more robust mechanism to achieve desired outcome.

I do think that "rename-by-field" may have its uses, as it allows "field" (original property declaration) to drive binding, in cases where property name starts with one or more capital letters. But it should not be needed to solve "is-getter" case, I think.

As to backwards-compatibility; yes, that can be a concern. In some cases it may be useful to add a new on/off feature to allow changing behavior; but that may be difficult here -- not ability add such a setting, but to make it useful: most users "just want it fixed", and that requires changing default behavior. But then again for some users things may work the way they want and they want no change.
So I would probably suggest attempting to find a new behavior that overall works better, even at risk of some incompatibility.

But it really is up to your judgment. We do have Release Candidate phase (once we push 2.15.0-rc1 and so on), during which we get some feedback. But usually not enough to really know; and then users report their problems over next year or two... when behavior has already changed. :-/

[cowtowncoder]

As usual, feedback by @dinomite @viartemev would be valuable.

[k163377]

@cowtowncoder @dinomite @viartemev
I would like to include this change in 2.15. Is it difficult to get it approved?

[cowtowncoder]

@k163377 If others do not have opinion (or time to check), I think you are free to go with this change.

One thing that should be done first, tho, is to figure out how to merge changes in 2.15 into master, before merging this change, I think. master is a bit behind (also has some unit test failures already, but ignoring that for a moment).

[cowtowncoder]

@k163377 I was able to finally merge 2.15 -> master (I am not a git expert), without adding new test failures. So we should be good in that sense. And going forward should try to merge all fixes from 2.15 to master quite soon just to avoid accumulation.

[k163377]

OK, merge and reflect to master as well.