Deserialization of kotlin singleton as singletons

[ciderale]

Currently, serializing a kotlin 'object' instance and deserializing it again yields a new instance, while it is supposed to be a singleton instance in kotlin. This pull request resolves this problem by deserializing the instance (using the deserializer as before this commit), but returning the "canonical" singleton instance (rather than the newly created one).

I hope this PR gets a chance to be merged since using Sealed Case Classes are extremely valueable. As this PR reuses the original deserializer I think that the deserialization behaviour is unchanged except for the fact that singletons remain singleton.

The topic has already been discussed in #225 and PR #215. As far as I understand, the open discussion was how to handle singleton objects with internal mutable state. PR #215 completely ignored the internal state and only returned the singleton object. In contrast, this PR does also deserialize the internal state, but returns the "canonical" singleton instance rather than a new instance.

The PR includes two commit, the first one only includes test to assert the current behaviour, while the second commit ensures that the singleton property is preserved. The first commit shows, that deserialization creates a new instance (deserializationPreservesSingletonProperty). However, it also shows that the deserialized (new) instance is actually coupled to the actual singleton and the two instance behave like one (deserializedObjectsBehaveLikeSingletons). I'm not sure why exactly this is the case, but probably due to how kotlin defines accessor functions.

The second commit uses a BeanDeserializerModifier to get the desired singleton property. As a result, the canonical singleton is returned (change in deserializationPreservesSingletonProperty)
while the other tests remain unchanged. That means, the effects of deserialization on mutable members of an objects are the same as before this PR.

The open disussion in #225 and PR #215 were about whether the state should be handled or not, and if so how. For my purposes (Sealed Case Classes), the solution in PR #215 would have been good enough as that does not involve mutable state. For some reasons, PR #215 did not get merged, however. This PR provides a solution that does not change the previous behaviour with respect to handling internal mutable state.

[cowtowncoder]

@ciderale This sounds good to me at high level, but since I am not Kotlin developer and can not quite evaluate PR, unless @apatrida has time to look into this, it'd be great if you could bring this up on dev mailing list at:

https://groups.google.com/forum/#!pendingmsg/jackson-dev

A big challenge currently is that while there are contributors and good contributions, we do not have active maintainers with access. I'd be happy to grant more access if we can figure out who should have it, so that all PRs are properly (and ideally quickly) reviewed, feedback added, and we can get improvements in. I think that anyone who has gotten a PR or two merged would qualify (and yes, I realize that right now that's bit of catch-22 situation... bootstrapping needed).

[lukas-krecan]

Hi, is there anything we can do to help to get this merged? I can help with the Kotlin part, on the other hand I do not know enough about Jackson internals to say if using BeanDeserializerModifier is the best way or not.

[cowtowncoder]

Use of BeanDeserializerModifier makes sense if there is need to use "default" deserializer to ensure structural compatibility, and not just "read anything there is" to match (which would I assume be the alternative, simpler, path).

I do not have a problem with this, so let's see if we can get quick ok from others.

@viartemev @apatrida WDYT? Any concerns with this? I can merge it in, to be in 2.10.1. And based on longer discussions referred, it seems like this should get resolved; I agree that singleton status should be preserved (or put another way, Jackson should not break the invariant).

[ciderale]

That would be great! Having Jackson not breaking the "object" invariant would be nice.

If there is a simpler implementation that would be nice too. The downcasts (e.g. defaultDeserializer is ResolvableDeserializer) are a bit suboptimal. But I did not find another way to register a serializer and having access to the default bean serializer.

[lukas-krecan]

The code might be made more readable like this lukas-krecan@f7f9171

[cowtowncoder]

I'll see if we get any more comments during this week; if no concern, will get merged by end of the week so it will not be further delayed.

[cowtowncoder]

Added "CR needed" label: until we get something more formal, anyone (aside from submitter) can just add LGTM comment.

[Dico200]

Looks fine to me. I would suggest not (de)serializing any state whatsoever and treating these the same as enums, but the situation around kotlin objects gets a whole lot better here already.