Preserve read after write

[dinomite]

Original description from PR 351:

Jackson with the Kotlin module does not choose the same json field names for both reading and writing if you have a data class with PascalCase field names, unless you go through a bunch of pain with @JsonProperty annotations. Not being able to read after write seems like a bug, so I fixed it.

Context: I'm working with a code generator that makes data classes based on DB schema. Because {reasons}, this makes classes with PascalCase field names, and it's not viable to change that to be more idiomatic for Kotlin.

[dinomite]

@popiel Have you signed the CLA? If not, would you fill it out and email a scan/photo of the result to info at fasterxml dot com?

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

[viartemev]

This is a tricky case.
Jackson (not Kotlin specific) serializes the class object:

data class NonIdiomaticPascalCasedClass(val Some_Number: Int, val Email_Address: String)
val obj = NonIdiomaticPascalCasedClass(6, "nobody@test.com") 

to

{"some_Number":6,"smail_Address":"nobody@test.com"}

Is it a known and documented feature?
Kotlin module can't deserialize the string because the object has different properties (names).
For me, it looks like correct behavior, but we have different behavior with Java module (it's a problem).
First of all, we need to understand is it correct for Java or not and based on the answer, decide what to do next.

Moreover, if you change deserialization in your test-case, the one works without any changes:

val deserialized = jacksonObjectMapper().readValue(serialized, NonIdiomaticPascalCasedClass::class.java)

@cowtowncoder do you have any insights about it?

[cowtowncoder]

This does not seem right to me -- more like an arbitrary change that makes specific case not fail the way it did.
And it is not a good place to make the change either, as name mangling aspects are not expected to be handled by AnnotationIntrospector.

I realize that there might not be a better way to fix it at the moment (2.12 or earlier), but, FWTW, I plan on revamping property introspection for 2.13. And this time try to keep module owners better in the loop regarding changes too.

Anyway: If change is to be made, I would recommend adding a comment with link to this PR and/or issue (yes, I know, git history can show it if one knows where to look) to explain the logic of making a change here.

[popiel]

If name mangling isn't expected in the introspector, why is it mangling (decapitalizing, etc) the names that start with 'get' and 'is'? Also, mangling is explicitly managed by the introspector in the next method (findRenameByField), so if this isn't the right place for mangling, a lot more changes are needed to move it to the right place...

[cowtowncoder]

It is possible that some other (relatively recent) changes are similarly sub-optimal (especially code right before your change in same method) but adding more of the same does not help.

But specifically, I think name-mangling in "findImplicitPropertyName()" seems wrong just due to its place in call sequences; it is a low-level accessor, whereas "findRenameByField()" has slightly different function.
I agree that "findRenameByField()" is not the best design choice either (and it is something I added).

I think my main concern here is just that I am not sure I understand intended logic, and strongly suspect that this is working around a real problem, but one that is not due to missing name-mangling but due to missing linkage between constructors and fields -- something that I know is an existing problem wrt auto-detected (non-annotated) constructors.

Still, if you could explain the intended logic in a comment, that would be helpful.

[popiel]

I shall add commentary in the morning, after finding references to where the standard getXxx and setXxx names get mangled, too (which is where I learned of the mangling routines in the first place).

[cowtowncoder]

Sounds good. Jackson 2.12 changed this handling a bit, trying to make it easier to replace pieces, moving away (ideally) from static helper methods.

I will also try to keep everyone updated on my plans for jackson-databind 2.13, via "jackson-dev" mailing list (and related Github issue for jackson-databind, once I get little bit further).
So even if there is a work-around here for 2.12, perhaps it could be further refactored into nicer packaging, once there is a better way (there might not be in 2.12).

[popiel]

@popiel Have you signed the CLA?

My signed copy of the CLA has been submitted. :-)

[popiel]

This is a tricky case.
Jackson (not Kotlin specific) serializes the class object:

data class NonIdiomaticPascalCasedClass(val Some_Number: Int, val Email_Address: String)
val obj = NonIdiomaticPascalCasedClass(6, "nobody@test.com") 

to

{"some_Number":6,"email_Address":"nobody@test.com"}

Is it a known and documented feature?

Yes, the down-casing of initial letters of Bean property names from the getXxx methods is a documented feature of Jackson, and the generation of getXxx accessors for data class members is a documented feature of Kotlin.

Moreover, if you change deserialization in your test-case, the one works without any changes:

val deserialized = jacksonObjectMapper().readValue(serialized, NonIdiomaticPascalCasedClass::class.java)

When I try that locally without my mainline changes, it fails. Did you make sure to revert the mainline and do a mvn clean?

[dinomite]

@popiel Did you have any more changes that needed to happen here?

[cowtowncoder]

I don't think this is active any more, should probably be closed?
WDYT @dinomite @k163377 ?

(but if not, cannot go in 2.12 branch, for example -- at minimum, 2.17 but probably 2.19)