-
Notifications
You must be signed in to change notification settings - Fork 175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KotlinObjectSingletonDeserializer fails to deserialize previously serialized JSON as it doesn't delegate deserializeWithType #281
Comments
@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). |
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 So the JSON for this singleton must be parsed so that these fields can be picked up and it can be stored away by This is all happening in logic well below the level that 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 It's simply not possible, at the 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. |
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. 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.
I mention (3) since it seems like perhaps users choosing their poison might be the least bad option. |
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 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 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). 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 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 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 |
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. |
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 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 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:
And later:
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. 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. |
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). @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. |
@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 😢 |
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. |
@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. |
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 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 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. |
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. |
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:
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. |
Concerning question 2, I'm not sure if I understand that "pluggable handler" correctly. Is there currently a cleaner option to "post-process" the result of a JsonDeserializer? Because that was the original intention, deserialize "as normal/before the change" and, as post-processing, return the "canonical" singleton object instead of the newly created one. Although there are multiple instances, potential member variables are shared (if I remember correctly). But I didn't find such a mechanism and therefore wrapped the defaultSerializer and forwarded method calls. |
@ciderale What I meant by handler referred to changing deserializer to use, sort of like what Another way to think of this would be that Kotlin module would add a Does this make more sense? |
Ok. So, assuming we will go with either "on/off" setting (enable/disable Singleton instance canonicalization), or enumerated set (use-default-deserializer / deserialize-and-canonicalize / skip-content-return-singleton), question next is that of how to configure module. Currently all configuration is passed via constructor, which is actually bit different from other modules. While immutability is good, this approach will not scale well with increased configurability. So perhaps it would make sense to create Builder-based alternative? Existing on/off choices for As to Singleton handling, I think there might be 3 (and not just 2) options:
although I guess addition of (3) is optional, and would be more of an optimization (and could also hide possible issues). But I think that it could also support handling of some singletons where deserialization would otherwise fail. Brainstorming bit more, here's another idea: |
Option (3) for singletons is actually an interesting choice, especially if it optimizes or hides possible issues. I assume that this would still leverage type information (as provided by Concerning the @cowtowncoder I'm still not sure if I understand your previous comment about pluggable handler. As mentioned earlier, I'm particularly interested in handling sealed case classes. It is not 100% related to this issue and yet, it depends directly on singletons. I think they are quite idiomatic for kotlin and thus an important use case. I'd like to share my current solution, as others (e.g. @lukas-krecan) are also interested in that topic and the solution seems not obvious. It describes the goal and how it works. There may be simpler solution (any feedback is welcome), but it works quite well. Maybe this concrete example helps to find a good configuration mechanism for such situations too. |
Option 3 would get applied after On annotation: yes, this would only be used to override whatever default handling, in case a specific type would require alternate handling. But there would be some default handling to use in absence of annotation. On pluggable handler: we can forget about it now; I have only general idea of allowing something that gets invoked by kotlin module when detecting singleton type being deserialized, and then letting that handler determine action to take, in case none of default strategies would work. But I do not have any concrete idea of exactly what arguments it would be passed, or how it should interact with default |
I've started looking at things—progress may be a bit slow as the entire Jackson codebase is mostly new to me. I'll link here when I have a draft PR. |
@dinomite Thank you for stepping up on this -- I'll have a look at PR. |
Merged the PR to add builder; good start. Then just need an Enum for singleton-handling choices, builder for configuring that, passing to module & wiring. |
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:
KotlinObjectSingletonDeserializer
that fixes this particular issue such that the test passes again - see the change here.To quickly try this all out just do:
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 anobject
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: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 ofJsonDeserializer
.JsonDeserializer
has quite a number of potentially overrideable methods (see below) but currentlyKotlinObjectSingletonDeserializer
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 thatKotlinObjectSingletonDeserializer
doesn't delegate these methods doesn't cause problems in circumstances where the given behavior is overridden in the wrapped deserializer.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 inObjectIdValueProperty.deserializeSetAndReturn
which is called fairly deep down afterKotlinObjectSingletonDeserializer
has passed over control to the delegate: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 (soCollectionDeserializer
is being used) and that we're storing an element of this list that's identified with an ID. In this stacktrace below the sameCollectionDeserializer
has hit that ID again and, as you can see, it just looks up the ID directly, viafindObjectId
- it doesn't involveKotlinObjectSingletonDeserializer
at all, so the stored duplicate is retrieved rather than the original singleton instance: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).
The text was updated successfully, but these errors were encountered: