KotlinObjectSingletonDeserializer fails to deserialize previously serialized JSON as it doesn't delegate deserializeWithType

[george-hawkins]

The introduction of KotlinObjectSingletonDeserializer in 2.10.1 breaks the deserialization of Kotlin structures that could successfully be deserialized in 2.10.0.

I've forked jackson-module-kotlin and introduced three branches that demonstrate the problem and show a solution:

• I branched from tag 2.10.0 and introduced a test that passes successfully for this release - see the test here.
• I branched from tag 2.10.1 and in this branch, the same test now fails.
• I branched from master and introduced a change to KotlinObjectSingletonDeserializer that fixes this particular issue such that the test passes again - see the change here.

To quickly try this all out just do:

$ git clone git@github.com:george-hawkins/jackson-module-kotlin.git
$ cd jackson-module-kotlin

$ git checkout 2.10.0-object-id
$ mvn -Dtest=com.fasterxml.jackson.module.kotlin.test.TestGithubX test
...
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
...

$ git checkout 2.10.1-object-id-bug 
$ mvn -Dtest=com.fasterxml.jackson.module.kotlin.test.TestGithubX test
...
[ERROR] Errors: 
[ERROR]   TestGithubX.test reading involving type, id and object:63 » InvalidTypeId Miss...
...

$ git checkout master-object-id-fix 
$ mvn -Dtest=com.fasterxml.jackson.module.kotlin.test.TestGithubX test
...
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
...

Above I check out the three branches in turn and for each ask Maven to run the test that I've added - for the 2.10.0 based branch everything works fine, for the 2.10.1 based branch things break and then in the master based branch things are fixed.

You could merge my master-object-id-fix branch to fix this particular problem. But I'm actually going to suggest reverting the addition of KotlinObjectSingletonDeserializer instead.

KotlinObjectSingletonDeserializer was introduced to try and resolve issue #244. It's certainly confusing for people if they deserialize an object and get a different instance of what was supposed to be a singleton. But Jackson has so many special cases and so much clever logic for dealing with e.g. things like cycles (@JsonIdentityInfo etc.) and with things like generics (@JsonTypeInfo etc.) that it's extremely difficult to cover all situations where Jackson deserializes, stores and reuses objects. KotlinObjectSingletonDeserializer, as it is, is just the beginning and it would require quite invasive changes elsewhere in Jackson (not just the Kotlin module) to be able to handle all cases.

Even with my change, it's still fairly trivial to construct a case where a new instance is returned by deserialization rather than the original singleton.

E.g. see this test that fails (irrespective of whether the change I made above to KotlinObjectSingletonDeserializer is applied or not). To try this just do:

$ git checkout master-object-instance-bug
$ mvn -Dtest=com.fasterxml.jackson.module.kotlin.test.TestGithubY test
...
[ERROR] test reading a list of objects with id(com.fasterxml.jackson.module.kotlin.test.TestGithubY)  Time elapsed: 0.321 s  <<< FAILURE!
java.lang.AssertionError: second element is not the same instance as Child.
...

I've added a detailed explanation of what's going on in this particular situation below. But in short, I think the problem is too complex to be easily addressed for all possible cases. So I think it's maybe better to just accept that unfortunately Kotlin singletons get deserialized to a different instance than to go with a solution that only works in a certain subset of simpler cases. As such I think the addition of KotlinObjectSingletonDeserializer should be reverted.

---

Longer explanation

The new KotlinObjectSingletonDeserializer wraps an instance of JsonDeserializer. JsonDeserializer has quite a number of potentially overrideable methods (see below) but currently KotlinObjectSingletonDeserializer only delegates the three that I've marked with an asterisk. Of the remaining 16 methods, all but one of them (marked with a minus below) is overridden in at least one subclass in the standard Jackson libraries. So it would be surprising if the fact that KotlinObjectSingletonDeserializer doesn't delegate these methods doesn't cause problems in circumstances where the given behavior is overridden in the wrapped deserializer.

* createContextual(ctxt: DeserializationContext, property: BeanProperty ): JsonDeserializer<*>
* deserialize(p: JsonParser, ctxt: DeserializationContext): T
  deserialize(p: JsonParser, ctxt: DeserializationContext, intoValue: T): T
  deserializeWithType(p: JsonParser, ctxt: DeserializationContext, typeDeserializer: TypeDeserializer): Any
- deserializeWithType(p: JsonParser, ctxt: DeserializationContext, typeDeserializer: TypeDeserializer, intoValue: T): Any
  findBackReference(refName: String): SettableBeanProperty
  getDelegatee(): JsonDeserializer<*>
  getEmptyAccessPattern(): AccessPattern
  getEmptyValue(ctxt: DeserializationContext): Any
  getKnownPropertyNames(): Collection<Any>
  getNullAccessPattern(): AccessPattern
  getNullValue(ctxt: DeserializationContext): Any
  getObjectIdReader(ctxt: DeserializationContext): ObjectIdReader
  handledType(): Class<*>
  isCachable(): Boolean
  replaceDelegatee(delegatee: JsonDeserializer<*>): JsonDeserializer<*>
* resolve(ctxt: DeserializationContext)
  supportsUpdate(config: DeserializationConfig): Boolean
  unwrappingDeserializer(ctxt: DeserializationContext, unwrapper: NameTransformer): JsonDeserializer<T>

And even if you do delegate to the wrapped deserializer for these methods, there are still issues that KotlinObjectSingletonDeserializer can't address.

E.g. in the test I mentioned above @JsonIdentityInfo is used. When using @JsonIdentityInfo it's the delegate that stores identified instances. The following stacktrace shows where the ID based storage happens initially in this test. As you can see it happens in ObjectIdValueProperty.deserializeSetAndReturn which is called fairly deep down after KotlinObjectSingletonDeserializer has passed over control to the delegate:

* deserializeSetAndReturn:101, ObjectIdValueProperty (com.fasterxml.jackson.databind.deser.impl)
  deserializeAndSet:83, ObjectIdValueProperty (com.fasterxml.jackson.databind.deser.impl)
  deserializeFromObject:510, BeanDeserializer (com.fasterxml.jackson.databind.deser)
  deserializeWithObjectId:1212, BeanDeserializerBase (com.fasterxml.jackson.databind.deser)
  _deserializeOther:220, BeanDeserializer (com.fasterxml.jackson.databind.deser)
  deserialize:189, BeanDeserializer (com.fasterxml.jackson.databind.deser)
* deserialize:28, KotlinObjectSingletonDeserializer (com.fasterxml.jackson.module.kotlin)
  _deserializeTypedForId:122, AsPropertyTypeDeserializer (com.fasterxml.jackson.databind.jsontype.impl)
  deserializeTypedFromObject:89, AsPropertyTypeDeserializer (com.fasterxml.jackson.databind.jsontype.impl)
  deserializeWithType:237, AbstractDeserializer (com.fasterxml.jackson.databind.deser)
  _deserializeWithObjectId:384, CollectionDeserializer (com.fasterxml.jackson.databind.deser.std)

In the case of a singleton, the delegate stores its deserialized duplicate of the original object rather than the original singleton that only KotlinObjectSingletonDeserializer knows about.

When Jackson then encounters an ID for an already stored instance it will use this deserialized duplicate - it doesn't even give KotlinObjectSingletonDeserializer a chance to be involved in this process. In the above stacktrace we can see that we're deserializing a list (so CollectionDeserializer is being used) and that we're storing an element of this list that's identified with an ID. In this stacktrace below the same CollectionDeserializer has hit that ID again and, as you can see, it just looks up the ID directly, via findObjectId - it doesn't involve KotlinObjectSingletonDeserializer at all, so the stored duplicate is retrieved rather than the original singleton instance:

* findObjectId:78, DefaultDeserializationContext (com.fasterxml.jackson.databind.deser)
  _deserializeFromObjectId:303, AbstractDeserializer (com.fasterxml.jackson.databind.deser)
  deserializeWithType:220, AbstractDeserializer (com.fasterxml.jackson.databind.deser)
* _deserializeWithObjectId:384, CollectionDeserializer (com.fasterxml.jackson.databind.deser.std)

Hence if you look at the test you'll see the problem just with the second occurrence of the given singleton object (and you would see the same issue with any further occurrences in a longer list).

[cowtowncoder]

@george-hawkins Thank you for the extensive investigation and reporting of the issue. I agree that the sheer complexity makes overriding a fruitless and probably futile task. I was going back and forth originally whether added validation of the change (that is, verifying structure of content to match expected structure, by deserializing) was worth the hassle. But I probably should have gone the "just skip arbitrary content, produce singleton" approach, to avoid all the trouble.

Given this, would you be able to suggest a good PR that retains part of returning singleton, but replaces deserialization with simple skipping of JSON value? If I remember correctly, 2.10.0 before did regular deserialization losing singleton aspect.

@apatrida just an FYI; it's awkward timing-wise (to do this in patch), but that is due to my merging in a patch (should have waited till 2.11).

[george-hawkins]

Sorry, I was on holidays and I'm only catching up on this now.

The main gist of my original problem description is that you simply can't skip processing the JSON value corresponding to a Kotlin singleton as it can contain items that must be processed and stored in order to complete the parse of the remaining JSON structure.

E.g. look at the piece of JSON that I include in my test:

[
    {
        "entity": {
            "@id": 1,
            "@type": ".NumberEntity"
        },
        "value": 10
    },
    {
        "entity": 1,
        "value": 11
    }
]

Part of the substructure of the first element in the array corresponds to a Kotlin singleton. This singleton is an implementation of an interface that's been annotated with @JsonTypeInfo and @JsonIdentityInfo, hence the @id and @type fields.

So the JSON for this singleton must be parsed so that these fields can be picked up and it can be stored away by com.fasterxml.jackson.databind.deser.impl.ObjectIdValueProperty for when we get onto the second element in the array. When we hit the second element the parser has to know what to do with "entity": 1, i.e. retrieve the previously parsed object that had this ID (which happens in com.fasterxml.jackson.databind.deser.DefaultDeserializationContext).

This is all happening in logic well below the level that KotlinModule and its related classes can directly influence.

I showed (in the branch that I added to my fork of jackson-module-kotlin) that it is possible to resolve the first of my failing tests with changes at the KotlinModule level but then go on to show that it's easy to add yet another failing case.

It's simply not possible, at the KotlinModule level, to implement the KotlinObjectSingletonDeserializer, such that it can deserialize Kotlin singletons without introducing a new instance of the singleton class, while still coping with all the complexity that Jackson can handle at lower levels with annotations such as @JsonTypeInfo and @JsonIdentityInfo.

The current implementation will fail for all kinds of fairly normal cases. To get things to work would require supporting changes in jackson-databind and a very large amount of test cases (to basically cover the huge number of cases that are covered for standard POJOs and confirm that they also work for Kotlin singletons and mixtures of Kotlin singletons and POJOs).

I would guess that there's simply too much work to do to complete what initially looked like a small MR.

So, disappointing as it may be, I suggest reverting the merge for #244 and listing it as a known issue that Kotlin singletons deserialize to a new instance of the singleton class. Otherwise, users will face very hard to diagnose deserialization failures (as I have here) for situations where serialization can be done without issue and where deserialization was possible before this change - and for situations that work fine if the entity in question is not a Kotlin singleton.

[cowtowncoder]

Ugh. So I also remembered current status quo incorrectly.

I was first bit puzzled by actual usage of identity reference for singletons, but I guess it's more identity info for references/elements, and not type itself.
And yes, you are right, trying to make this work as expected would probably be non-trivial task, and to be successful require support from databind side. Perhaps by having a mechanism to indicate that identity of certain instances is of "singleton kind".
Not sure how feasible that'd be.

One thing to consider, too, is that while Jackson does attempt to bind arbitrary JSON (et al) to structures, fundamentally bigger goal is to support end-to-end handling (Jackson should be able to read what it writes, but not necessarily all kinds of input). There are types of JSON that Jackson will not work with (for example arbitrary Union types that things like JSON Schema can define), but this is more common with formats like XML.

Anyway. At this point my problem is that I am not sure which change I should make, if any, purely based on my understanding (even with your help outlining the problem), and need to reach out to Jackson/Kotlin user community.
It seems like there are 3 technically easyish choices:

1. Accept current state of Singletons blindly "read" from arbitrary JSON
2. Revert support for (1) and treat Singletons as regular POKOs (requiring other workarounds like factory methods to retain singleton-ness)
3. Add a feature flag for KotlinModule to choose between (1) or (2) in 2.11

I mention (3) since it seems like perhaps users choosing their poison might be the least bad option.
Question then would be that of default setting: I'd suggest defaulting to (2), simply because this is what the latest minor version had as its setting.

[george-hawkins]

Thanks for the followup @cowtowncoder - of the choices you suggest, I'm honestly inclined towards choice (2), i.e. revert this feature.

I'd thought previously about suggesting your choice (3), i.e. introducing a feature flag. However, I think that keeping KotlinObjectSingletonDeserializer (as it currently exists) has more downsides than upsides.

I understand that the fact that Jackson can serialize something to JSON has never been a guarantee that it can also deserialize it.

However the introduction of KotlinObjectSingletonDeserializer specifically breaks the deserialization of things that previously deserialized fine (and which can still be handled if one refrains from using object).

JSON as a format is fairly simple but much of the power of Jackson derives from its ability to handle non-trivial situations (like cycles, subtypes and generics). KotlinObjectSingletonDeserializer breaks this ability and in very non-obvious ways.

I think it's confusing to introduce a feature that works fine (as it's not involved) when standard POJOs are used but mysteriously breaks for identical structures involving Kotlin singletons (which worked previously with the caveat that deserialization introduced additional instances of the singletons).

Currently KotlinObjectSingletonDeserializer is not an almost complete feature that breaks some corner cases. It's an unfinished feature that breaks much of the value proposition of Jackson and which would require significant work at this level and below in core Jackson, e.g. databind, to get working.

I really appreciate your time looking into this issue. To sum up - I support choice (2), i.e. reverting. If you choose to go with choice (3), I'd make the default as you suggest, i.e. not enable KotlinObjectSingletonDeserializer, and I'd name the feature flag to make very clear that KotlinObjectSingletonDeserializer has serious issues. E.g. DeserializationFeature.EXPERIMENTAL_KOTLIN_SINGLETON_SUPPORT with KDoc to make sure the user is aware that this feature may break many of the annotations that make Jackson so useful and powerful.

Note to non-Kotlin users: the term "singleton" is used here in a rather confusing way - traditionally it means global (and often mutable) state. Like in Scala, the object keyword in Kotlin is often referred to as introducing a singleton but as both Scala and Kotlin favor immutable state these tend to be used in quite a different way to Java. So having multiple instances of such objects, while not desirable, is not generally the disaster it would be in Java. E.g. we use such objects to denote primitive types in AST trees that we serialize to JSON, so NumberType is an object (and is no more than a marker containing no mutable state). Similarly elsewhere we have what would be factory classes in Java as objects (again with no mutable state).

[cowtowncoder]

At this point, what is needed is someone deciding on what to do. I do not feel like I have time, focus or even knowledge of community to do that: especially for removing something that now is de facto behavior for 2.10. If this was about not adding a feature or making a change that'd be one thing (just leave it be), but with upcoming 2.11 -- the first point where change could be made -- I'd need someone else to chime in.

@apatrida What do you think?

I will also sent a note on mailing lists saying that this module needs someone (one or more individuals) to take ownership: otherwise I should probably stop maintaining it completely. I don't think it is fair for anyone for it to be in limbo.

[george-hawkins]

To summarize - in version 2.10.1 (released on November 10th, 2019) PR #244 was merged. This PR only superficially addresses the issue of deserializing Kotlin singletons (working only for very simple cases) while breaking one of Jackson's main value propositions (at least where Kotlin singletons are involved), i.e. the ability to handle all kinds of complex cases through the use of annotations such as @JsonTypeInfo and @JsonIdentityInfo.

So while this may be current behavior, I find it unlikely that, in the course of 4 months, it has become "de-facto behavior" on which many people depend. Given how it limits the usefulness of Jackson, I think the lack of further similar bugs, to this one, simply reflects how rarely Kotlin singletons are used in structures that people are serializing and deserializing using JSON (and perhaps how few people have actually picked up the 2.10.1 and 2.10.2 releases).

PR #244 does not come close to ensuring that Kotlin singletons are never deserialized to additional instances (see the examples, I provided when filing this issue) and breaks situations, where annotations such as @JsonTypeInfo and @JsonIdentityInfo are used, that previously worked.

Users are often prepared to work with clear and well-known limitations, however, PR #244 has introduced changes that cause deserialization to break in very non-obvious situations. So while it may not have resulted in many bug reports so far, I believe it will result in a steady stream of non-obvious deserialization issues (as opposed to ones where the error output can be easily traced back to a particular limitation and worked around).

To quote myself, from previous comments:

It's simply not possible, at the KotlinModule level, to implement the KotlinObjectSingletonDeserializer, such that it can deserialize Kotlin singletons without introducing a new instance of the singleton class, while still coping with all the complexity that Jackson can handle at lower levels with annotations such as @JsonTypeInfo and @JsonIdentityInfo.

And later:

Currently KotlinObjectSingletonDeserializer is not an almost complete feature that breaks some corner cases. It's an unfinished feature that breaks much of the value proposition of Jackson and which would require significant work at this level and below in core Jackson, e.g. databind, to get working.

As such, if no one has time to work on an actual solution to the original issue, that PR #244 attempted to address, then I suggest reverting the change before people really do start depending on this behavior (however broken it may be). And otherwise, I suggest, at the very least, disabling it by default and hiding it behind a feature flag with a name that reflects it's highly incomplete nature, e.g. EXPERIMENTAL_KOTLIN_SINGLETON_SUPPORT.

At the moment, in our large production application, we have to actively disable the behavior introduced by PR #244 through some unpleasant reflection and low-level hackery.

Thanks for flagging this issue as important to the release of 2.11 (in issue #302). Personally I would vote to revert PR #244, but the choice is clearly up to active committers like @cowtowncoder and @apatrida and to proposed owners from the community, i.e. @dinomite and @viartemev.

[cowtowncoder]

Thank you for summary @george-hawkins. I would like to defer solution here as well, but whichever path taken would really want it to be done for 2.11(.0).
Of choices suggested, I would be willing to merge configurable option between 2 methods of handling as a stop-gap -- since default is easy enough to change before release, that decision could be done as next step.

@george-hawkins one other question: would you be interested in being one of maintainers of this module? As I have mentioned, I would like to see at least one more member to resolve tricky decisions like this one.

[george-hawkins]

@cowtowncoder - thanks for the suggestion 🙂 At the moment my days are consumed by development work at a startup and I simply don't have the additional time necessary to properly investigate and work on jackson-module-kotlin issues as a project maintainer 😢

[dinomite]

I've read all of this, but I don't feel qualified to make a decision as I've never used a singleton with Jackson (and, not as a slight, I can't imagine when I'd want to deserialize something as a singleton). I'm happy to do the work to get (2) [revert] or (3) [hide behind a feature flag] as previously described implemented in preparation for 2.11.

Sounds like @george-hawkins would like to do (2), but perhaps a feature flag would be better to accommodate potential users like @ciderale, @lukas-krecan, and @Dico200 who worked & commented on #244.

[cowtowncoder]

@george-hawkins no problem: I ask because I know it is a non-trivial undertaking, even if "only" reviewing changes and actively making decisions as a group.

@dinomite at this point I think that going with (3) makes most sense. One open question there would be default state -- but that can be deferred until switch itself is available, and both cases are tested.
So I think that is the way to go.

[ciderale]

Thanks for the analysis and sorry for the problem caused. The current solution is clearly not optimal, but I still think that preserving the singleton property of kotlin's object should be a goal of jackson-module-kotlin. Otherwise, kotlin's equality operator == and pattern matching do not work as expected, resulting in subtle logic bugs. In fact, breaking == does not seem like a minor caveat to me.

This aspect becomes important when working with "sealed case classes", aka "algebraic data types". They are well supported in kotlin and excellent for modelling complex domains. The official kotlin documentaion provides a simple example that shows the combination of data class and object. (https://kotlinlang.org/docs/reference/sealed-classes.html). Working with them
relies heavily on pattern matching, which breaks down in the presence of multiple "kotlin object" instances. This is very unfortunate.

IMHO, a value of such a type should compare equal after a serialization/deserialization round trip. This is not the case when there are multiple instances of the singleton. Ideally, it will not even require any jackson annotations on such domain values, but only proper configuration of the object mapper.

This clearly is beyond the scope of this issue, but I hope it gives some context for why the singleton aspect is important. Maybe this is already possible with some clever configuration of the object mapper. I currently have a solution, but it does not feel clean; any hints are welcome. Or maybe it helps to find a good solution in the long run.

[lukas-krecan]

For me (3) makes most sense as well. Our use-case is to serialize simple structures which use sealed case classes (as described above) and the fact that pattern matching works differently for deserialized structures is really confusing.

On the other hand, I understand that some people use different features. So in my opinion the pragmatic way is to let people choose if they prefer to break Kotlin or edge cases of JSON parsing. Ideally Jackson should not break neither but I understand that it's quite complex issue to fix.

[cowtowncoder]

Ok. So, assuming that both behavior need to be preserved since there seem to be problems with both (as well as cases where one or the other does work), and there is no one-approach-works-well-all-the-time (or, alas, either, sometimes).

But some questions before adding simple on/off feature:

1. Are there are other reasonable default options to take: f.ex, would third choice be "throw exception if attempting to serialize/deserialize singleton instance/type"? If so, might need an enumeration
2. Is it likely that a pluggable handler would work; one that can provide deserializer for singleton types, and that would be implemented to support existing 2 options (or more, as per (1))?
   • handler could maybe be something used as post-processor for JsonDeserializer produced by default settings

I don't want to complicate solution but at the same time since apparently neither suggested approach works for some cases, pluggable custom handler would let users extend functionality in somewhat clean way.