android-record: Support deserializing polymorphic members of records

[HelloOO7]

This fixes #248 (see tests). As far as I could tell, the non-default ValueInstantiators do not have access to a DeserializationContext when created, so there's a fair bit of redundancy in place when using createContextual instead.

[eranl]

Thank you. LGTM. @cowtowncoder, your thougths?

[cowtowncoder]

First of all, thank you @HelloOO7 !

Correct, context is passed via createContextual() same as is done for serializers and deserializers (for (de)serializers the reason are cyclic types, cycles cannot be resolved otherwise; and probably same is true for ValueInstantiator).

Code looks ok, although somehow it feels like this should be doable via JsonDeserializer side, and not on ValueInstantiator, similar to how regular JDK Record deserializer creation works. But there may be reasons why this is not doable (jackson-databind might have access to something modules don't, for example).

Now: I really want to go through this in more detail, then merge, but am leaving for a vacation today. But will pick this up once I return in 2 weeks. So don't worry when you won't here much until then -- this is one of top-5 items for me to do before 2.18.0-rc1 is finalized.

[HelloOO7]

somehow it feels like this should be doable via JsonDeserializer side, and not on ValueInstantiator

I expanded upon ValueInstantiator in effort to adhere to the spirit of the original implementation. I may be wrong here, but wouldn't using JsonDeserializer needlessly couple the Android record module to the JSON format?

Upon looking into this further, I came up with a more similar-to-JDK approach to Android records that uses an AnnotationIntrospector, see here: https://github.com/HelloOO7/jackson-modules-base/tree/android-records-2 This approach is based on the fact that POJOPropertiesCollector handles records in _addCreators using:

if (_isRecordType) {
    primary = JDK14Util.findCanonicalRecordConstructor(_config, _classDef, constructors);
} else {
    primary = _annotationIntrospector.findDefaultCreator(_config, _classDef,
    constructors, factories);
}

However, this fails the testDeserializeSimpleRecord_DisableAnnotationIntrospector test (for apparent reasons). If that is a problem, it could probably be solved by overriding _addCreators itself in the POJOPropertiesCollector that the module overrides either way. It might also be of note that this change fixes 2 of the failing tests.

[cowtowncoder]

I added included test in core jackson-databind to make sure it works with "real" Records as well (there was one test class for @JsonTypeInfo annotated members but coverage pretty slim).

Will try to get this reviewed now.

[cowtowncoder]

but wouldn't using JsonDeserializer needlessly couple the Android record module to the JSON format?

No, despite its name, JsonDeserializer is format agnostic (really should be ValueDeserializer).

[cowtowncoder]

@HelloOO7 I like the looks of the alternate take -- could you do a PR for that instead, and I can get it quickly reviewed?

And yes, now that I looked at the original implementation it makes sense that you extended ValueInstantiator approach. But I much prefer AnnotationIntrospector approach, esp. with 2.18 addition of findDefaultCreator.

[HelloOO7]

Sure. Would it be okay to merge the new commits to the main branch of my fork and update this PR, or should I submit a new one?

[cowtowncoder]

@HelloOO7 Either way is fine, whatever is easiest to do?

[HelloOO7]

Alright, I merged it to my branch of 2.18. It's now awaiting review as part of this PR as far as I can see (commit has appeared before my last message probably 'cause it's sorted chronologically).

[HelloOO7]

I reckon I should probably update the tests to no longer be in the failing package, if this approch is the way to go forward.

[cowtowncoder]

Hmmmh. I wish indentation was not changed -- merging this into master (3.0) is pretty painful due to bogus changes... :-(

[cowtowncoder]

Thank you @HelloOO7 -- now merged in 2.18 for 2.18.0 as well as master for eventual 3.0.0.

[HelloOO7]

Oh well. I tried to configure it to keep 2 spaces indentation (which I think it did), but the tests had strangely nested static classes on a different indentation level than static methods, which my IDE couldn't keep up with:( Sorry.

[HelloOO7]

I'm really looking forward to have this fixed in 2.18. It's gonna be nice not to have to bother with workarounds. Thank you too!

[cowtowncoder]

@HelloOO7 yeah np, indentation gets tricky no worries. Glad to have gotten it all merged.