Fixed serialization/deserialization failure when the name of getter contains -.

[k163377]

The current implementation determines whether a getter is for an value class(inline class) by checking whether the name of the getter contains -.
On the other hand, in Kotlin, it is possible to create a getter with - in its name by escaping with @JvmName or `, and serialization/deserialization will fail in such a pattern.

In this PR, I added a test case and modified it so that if there is a discrepancy between a property name retrieved from getter and a property name retrieved from KProperty, the latter is used.
Also, this change supports not only the value class, but all patterns where the getter is automatically compiled to a form containing -.

[dinomite]

Overall looks good to me, just a few comments

[dinomite]

I haven't opened this in an IDE so could be wrong, but does this work if you use when (member.name) and drop the .let {}?

            val propertyNameFromGetter = when (member.name) {
                    it.startsWith("get") -> it.substringAfter("get")
                    it.startsWith("is") -> it.substringAfter("is")
                    else -> throw IllegalStateException("Should not get here.")
            }.replaceFirstChar { it.lowercase(Locale.getDefault()) }

[k163377]

I tried, but it seemed that I could only write the following.

val propertyNameFromGetter = when  {
    member.name.startsWith("get") -> member.name.substringAfter("get")
    member.name.startsWith("is") -> member.name.substringAfter("is")
    else -> throw IllegalStateException("Should not get here.")
}.replaceFirstChar { it.lowercase(Locale.getDefault()) }

I thought it would be better to use let because member.name would be repeated many times.

[cowtowncoder]

Quick note: lower-casing may be incorrect here. Consider case of getURL() which should produce, I think, URL, as per Java Beans definition. But if code above simply lower-cases the first character, it'd return uRL.
(Jackson itself has bit of inconsistency, originally returning url for that case, lower-casing all leading upper-case chars; but then adding option for producing URL)

[k163377]

@cowtowncoder
In this scene, if the name retrieved from the getter is different from the name of the property, the name of the property will be adopted.
In other words, even if the name retrieved from the getter is wrong, the name of the property (= expected value) will be adopted as a result.
For this reason, I thought the problem you pointed out would not occur here.

[k163377]

I noticed that the pattern where getter is named with @JvmGetter @get:JvmName doesn't cover some areas.
It looks like the property names should not be used in this pattern.

I will revisit this later as I don't have much more time right now.

[k163377]

#451 (comment)

[cowtowncoder]

I am not sure exactly how much Kotlin module changes things, but the way basic Jackson property inspection works is that it first gets all "implicit" (and explicit) names of accessors, then joins similarly named entries (by implicit name), then does renaming of grouped accessors.
In this scheme, then, if implicit name from getter does not match those of fields, they would infer different non-related properties.

I have been hoping to change handling to let modules (Kotlin and Scala mostly) customize logic more to essentially "start with fields" and use different merging logic. But right now ways to override things tend to have unexpected side-effects.

For this particular case, also, I suspect that if matching of accessors used case-insensitive handling (not trivial to do as retrofit but something that has been requested and seems useful), mismatch could be avoided. And like you say, could then just use name from field (implicit name or explicit annotation)/

[k163377]

@cowtowncoder @dinomite
I reconsidered and made the following changes.

1. If JvmName annotation is given, it is not considered a generated getter.
2. Use KProperty.name without strict comparison if it roughly fits the characteristics of the generated getter.

1
The reason for doing it like 1 is to match the behavior expected by the user.

If a getter is named with a JvmName, the user will expect that the name actively given with the JvmName will be treated preferentially.

Therefore, if the JvmName annotation is given, it is not considered a generated getter.
The behavior of the Kotlin compiler also confirms that auto-generated suffixes are not given if JvmName is given.

2
The reason why I did it as 2 is that there seemed to be little need to make this decision completely.

First, if strictly compare the name of the getter with KProperty.name, we need to pay attention to the edge cases as @cowtowncoder pointed out, which complicates the logic.
On the other hand, I couldn't think of any pattern that should prioritize the name of getter over KProperty.name in the content that satisfies the condition of the generated getter.

Therefore, I decided to use KProperty.name without further checking if the pattern of the generated getter is satisfied.
This also subtly reduces the amount of computation.

[dinomite]

That makes sense to me, but @cowtowncoder is better at evaluating those sorts of things than I.

[cowtowncoder]

I think I agree that KProperty.name ought to be used, if that is what Kotlin logically thinks of as "true" name.
My main concern is just making this work both cleanly, correctly and (to some degree) efficiently.
Problems come mostly from Java side wherein getters/setters have sort of precedence due to historical reasons.

@k163377 I don't know how well you know jackson-databind side (specifically, POJOPropertiesCollector and POJOPropertyBuilder; which operate on information collected by JacksonAnnotationIntrospector via [Basic]BeanDescription), but that is part that may also be modified.
I really wish I had time to spend on planned rewrite of that setup: originally not so much wrt naming for Kotlin/Scala but to solve the issue of Creators (constructors/factory methods) being handled in a way that sometimes does not properly merge properties passed via Constructor parameters.
As part of that work it'd be good to also address needs of Kotlin module.

[k163377]

@dinomite @cowtowncoder
After checking the behavior again, I have confirmed that the KProperty does not exist in the getter like method, and this change is causing the failure to occur.

I don't think I have enough knowledge and organization of the problem to tackle this issue.
Since it doesn't seem to be a critical issue, I think I'll sort out the pattern of failure and leave it in an issue, then close the PR and add a separate test case, how about that?

I am very sorry for the trouble I caused you with my half-hearted understanding.

Additional context

As for AnnotatedMethod.isInlineClass, it looks like the decision in findImplicitPropertyName is either unnecessary or not behaving as intended (I didn't mention it because it's not the main topic of this PR).

The isInlineClass checks if there is a method with a name containing - in the class that defines the Method.
On the other hand, the findImplicitPropertyName checks if the name of the getter contains -.
This means that patterns that require an implicit name will always have isInlineClass set to true, making this check seem unnecessary.

I'm thinking of setting up a separate PR with only a light refactor and removal of this decision, how about that?

[dinomite]

Thanks for all the effort in exploring these ideas! Things don't always work out.

Separate PRs for testing that currently-untested getter like functionality and skipping the isInlineClass check would be great.

[k163377]

@dinomite
Created a separate PR for optimization and refactoring of findImplicitPropertyName.
#456

[dinomite]

@k163377 We've merged that other PR, is there still work to do here?

[k163377]

@dinomite
I forgot to close the PR, sorry about that.

I'll close this PR, but since there seems to be a demand for something like #503, I thought it would be nice to have an option to respect the Kotlin properties.