Rewrite Bean Property Introspection logic in Jackson 2.x

[cowtowncoder]

Describe your Issue

We need to rewrite the Bean Property introspection (see #3719 for background); and while this is ambitious undertaking, I think it should be done in Jackson 2.x timeframe, not 3.x. That way code remains potentially mergeable from 2.x to 3.x.

The main drivers for rewrite are to solve problems that are difficult if not impossible to solve with the current mechanism:

1. Merging of annotations between Field/Method accessors and Creator (constructor/factory method) arguments
2. Handling of Record types and in particular alternate (non-canonical) constructors
3. Support for canonical constructors for non-Java JVM languages (Kotlin, Scala)

This issue is an umbrella ticket as refactoring will likely be spread across multiple PRs; they can all refer to this issue,

[cowtowncoder]

Started reading the code and clearly more of code wrt Creator detection needs to move from BeanDeserializerFactory/BasicDeserializerFactory (later part of processing) into POJOPropertiesCollector (earlier part of processing).

But not sure whether to try to move abstractions from Factory part, or rewrite; and whether to try move things incrementally or not.

Started removing deprecated methods from code paths, to help isolate actual in-use code.

[JooHyukKim]

But not sure whether to try to move abstractions from Factory part, or rewrite; and whether to try move things incrementally or not.

If we agree that we have enough test coverage, I guess.... rewrite, move big-step-incrementally?

[cowtowncoder]

@JooHyukKim Yes, I think test coverage is sufficient to allow refactoring. I am struggling at finding the steps tho. Especially in a way that would allow merging 2.18 -> master.
So it's not yet about mechanics but even conceptually which things to move or rewrite/re-implement.

And one problem part is that the latest BeanDeserializerFactory code for processing Creators relies on [Basic]BeanDescription, which would not be available during POJOPropertiesCollector (since BeanDescription is mostly used as wrapper for actual POJOPropertiesCollector.

[cowtowncoder]

Ok one big challenge that is coming up now, looking through functionality in BasicDeserializerFactory is the coupling to deserialization side in general (DeserializationConfig and DeserializationContext), and also dependency on context (DeserializationContext). This because POJOPropertiesCollector only operates on generic MapperConfig (base config of DeserializationConfig and SerializationConfig), and does not need or use context.
Dependencies are not trivial to remove; but they are also not exactly essential. But just means it is not easy to start by "lift-and-shift"ing (that is, by basically moving code from one place to the other), some rewriting required.

In the meantime I am making small incremental changes, including removal of deprecated code in affected areas.

[JooHyukKim]

Right. Also from my experience, many times it was necessary to both modify existing and add additional "internal" structure to make the code base in better shape. More of "tidy up just before making changes".

Just out of curiosity, are we going to introduce POJOPropertiesCollector, RecordPropertiesCollector and such for new version? This question is just from my imagination 😆. I am asking this because, though not necessary, if there's any plan in written format that I can refer to, so I can help.

[cowtowncoder]

Just out of curiosity, are we going to introduce POJOPropertiesCollector, RecordPropertiesCollector and such for new version? 

Not planned yet. I am thinking it should be possible to make things work without fully separate implementation. But of course if it becomes necessary that could be done.

At this first I would really want to make existing logic work same as now, but get Creator properties discovered around time others are... so move it out of BasicDeserializerFactory into either POJOPropertiesCollector or where BasicBeanDescription is resolved.

[cowtowncoder]

Hmmmh. Despite incremental refactoring, I don't think functional from BasicDeserializerFactory can moved as-is into POJOPropertiesCollector -- it does "too much", including instantiation of deserializers and so on.
So what we need is a subset. Need to re-think things a bit.

...

Ok. So, in POJOPropertiesCollector I think handling needs to incorporate selection of actual Creator method and NOT just individual properties. And that information will then be used by BasicDeserializerFactory.

[cowtowncoder]

Good progress:

1. All JDK-8 tests pass with re-factored POJOPropertiesCollector
2. Only 4 Record tests fail
3. Still have duplication with BasicDeserializerFactory: will leave resolving that to second PR

Hoping to solve remaining Record tests, to get first PR merged into 2.18, then master.

[cowtowncoder]

Forgot to add an update: #4532 was merged 2 days ago, and it handles front-end half of changes:

• POJOPropertiesCollector fully resolves explicit Properties-based creators, linking properties
• Code in BasicDeserializerFactory still handles rest of introspection, including linking from (1) to Creators. This is to be removed as the next step.

I also started to look into the second half (reducing/removing code from BasicDeserializerFactory); it is somewhat complicated too, but can hopefully be tackled incrementally as well.

[cowtowncoder]

Ok some more progress: now BasicDeserializerFactory actually uses primary Properties-based Creator discovered by POJOPropertiesCollector. Additionally POJOPropertiesCollector keeps Creator-properties from that primary one better in-sync with getter/setter/field accessor info (in fact making failing test for #4119 pass -- but will not yet marked as fixed until getting further with refactoring).

But quite a bit remains to rewrite in BasicDeserializerFactory still.

[cowtowncoder]

Solved (1): only one fail, and that is due to changed exception message (although not sure if the new message is good)

[JooHyukKim]

Lemme look into the Kotlin one. Theres already bunch of master branch failures

[cowtowncoder]

Yeah first 2.18 (hopefully with a small number -- just 1 if I saw it right).

[JooHyukKim]

Yup, just 1 actually. TestGithub145.testPerson7()

[JooHyukKim]

Okay I haven't found solution to resolve or pinpoint root cause for the 🔗 failing test testPerson7() yet.
At this point, I am not sure if it should pass ... 🤔 or @k163377 might know?

Until then, in case it helps anyone, below is the failing test in Kotlin module.

    // Cannot have companion object in class declared within function
    class Person7 constructor(val preName: String, val lastName: String) {
        private constructor(preNameAndLastName: String) : this(
            preNameAndLastName.substringBefore(","),
            preNameAndLastName.substringAfter(",")
        )

        companion object {
            @JsonCreator
            @JvmStatic
            fun createFromJson(preNameAndLastName: String): Person7 {
                return Person7(preNameAndLastName)
            }
        }
    }

    @Test
    fun testPerson7() {
        val person7Json = objectMapper.readValue<Person7>("""{"preName":"TestPreName","lastName":"TestLastname"}""")
    }

It seems to be that Jackson 2.17 managed to discover PropertyCreator for constructor(val preName: String, val lastName: String) constructor.
But 2.18 does not and fails with message...

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.fasterxml.jackson.module.kotlin.test.github.TestGithub145$Person7` (although at least one Creator exists): cannot deserialize from Object value (no delegate- or property-based Creator)
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 2]

	at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:63)
	at com.fasterxml.jackson.databind.DeserializationContext.reportInputMismatch(DeserializationContext.java:1754)
	at com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1379)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1510)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:348)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:185)
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:342)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4907)

Will get back to it tommorow.

[k163377]

Sorry, I am sick with COVID and don't have the energy to investigate.
If I recover this weekend I will look at it, but it may be next weekend.

[JooHyukKim]

Oh, I am so sorry to hear about your illness 😭. Please take care and get well soon.
I will look at it as much as I could.

[cowtowncoder]

@k163377 I hope you get better soon! Don't worry about issues while you are recovering.

[cowtowncoder]

@JooHyukKim One possible reason for failure could be that auto-detection of properties-based creation with new code only occurs if there are no alternative constructors (including no default constructor). I am not 100% if behavior in 2.17 and before allowed existence of other constructors; there was one test that failed if not, yet another that failed if allowed.
I added further logic based on ConstructorDetector state that solved that issue on databind side, but it is quite specific and might not help here.

I do still need to extend 2.18 with extension points to let Kotlin (and Scala) module indicate if it has "canonical" Creator to use -- that would probably solve the issue here.
But that extension point does not exist.

One possibility, on short term, would be to change this test to be considered "failing", to be addressed with follow-up: this way build would remain green for now.

[JooHyukKim]

One possibility, on short term, would be to change this test to be considered "failing", to be addressed with follow-up: this way build would remain green for now.

Agreed, as per the issue FasterXML/jackson-module-kotlin#145 the failing test belongs to, the test was already problematic.

So what we can do is

1. move to failing test,
2. File a Github issue against 2.18
3. Fix it in 2.18

[cowtowncoder]

@JooHyukKim one quick note: while fixing issues with jackson-databind/3.0, I backported them into 2.x -- basically these are uncovered in 3.0 since it included parameter names module (unlike 2.x where it is separate). So there's a slight chance that something wrt Kotlin could be fixed by these changes.

[JooHyukKim]

Kotlin module still failing. So I filed a PR moving the test to failing FasterXML/jackson-module-kotlin#802.