Skip to content
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

ReflectionCache not Serializable #295

Closed
elgbar opened this issue Feb 4, 2020 · 11 comments · Fixed by #634
Closed

ReflectionCache not Serializable #295

elgbar opened this issue Feb 4, 2020 · 11 comments · Fixed by #634

Comments

@elgbar
Copy link

elgbar commented Feb 4, 2020

Ref issue #245, it it possible to make ReflectionCache Serializable?

It should be relatively easy, but I would suggest to only serialize it as empty.
Serializing actual contents may be complicated and unlikely to be worth the hassle.

Originally posted by @cowtowncoder in #245 (comment)

I do need to pass a configured instance of an ObjectMapper between applications.

If this is infeasible, any other suggestions for how to deal with configuring ObjectMapper cross application?

@cowtowncoder
Copy link
Member

cowtowncoder commented Feb 4, 2020

Yes I would think so. Someone would need to make the change, test serializability (databind has some tests, just initialize mapper with module, serialize + deserialize back, use for reading/writing [simple cases ok]), and provide a patch.

With 3.x things will change a bit to make things much easier so that modules do not usually need to do anything to keep them serializable (mapper serializes modules and NOT things modules register; problem with 2.x is that mappers are serialized with full state, 3.0 only serializes configuration).

@k163377
Copy link
Contributor

k163377 commented Feb 18, 2023

@elgbar
Sorry for the long wait.

Is this still a necessary issue?
Personally, I thought it could be accomplished by making KotlinModule.Builder to Serializable.

@cowtowncoder
Copy link
Member

I think the problem is that even if Builder is serializable, there are use cases where ObjectMappers are passed in a way that requires JDK serialization.

If there is need for ReflectionCache to be java.io.Serializable, it should almost certainly be done in a way that nothing is serialized (actual cache fields are marked as transient) and that on deserialization an empty ReflectionCache is constructed instead (in a state that works for mapper & Kotlin module).

A reasonable example of how to do that can be found from jackson-databind, class LRUMap (in package com.fasterxml.jackson.databind.util) -- aside from transient fields, addition of:

    protected Object readResolve() {
        return new LRUMap<K,V>(_initialEntries, _maxEntries);
    }

handles reconstruction (in this case retaining configuration settings; that might not be needed with ReflectionCache)

@elgbar
Copy link
Author

elgbar commented Feb 20, 2023

The application I was wringing at the time is now EOL, so I don't personally need it now.

The solution I came up with was including a file in an Uber jar with the absolute classpath to an object instance which I used to do serialisation stuff with.

But as a feature I still see it's usage

@cowtowncoder
Copy link
Member

I agree: an ObjectMapper with Kotlin module registered should be JDK serializable.

k163377 added a commit to k163377/jackson-module-kotlin that referenced this issue Feb 22, 2023
@k163377
Copy link
Contributor

k163377 commented Feb 22, 2023

As far as I understood it, it looked like simply inheriting Serializable and setting serialVersionUID, so I created a PR anyway.
#634

@k163377
Copy link
Contributor

k163377 commented Feb 26, 2023

It will be released in 2.15.

@cowtowncoder
Copy link
Member

@k163377 One follow-up thing: could you please merge this from 2.15 into master? Due to differences in Jackson 2.x/3.0 APIs there are some conflicts and I am not knowledgeable enough to resolve those.

Some things are due to renaming:

  • LRUMap becomes SimpleLookupCache
  • JsonSerializer/JsonDeserializer -> ValueSerializer/ValueDeserializer

and then there is Java package name change from com.fasterxml.jackson.* into tools.jackson.*.

This merge should be done after merging every new feature in 2.15. Similarly, if a fix goes in an earlier branch (backport), like 2.14, it needs to be merged to all later branches (to 2.15, then master)

@k163377
Copy link
Contributor

k163377 commented Feb 26, 2023

@cowtowncoder Created #636

@cowtowncoder
Copy link
Member

Thank you @k163377 -- I merged it; I know there are pre-existing failures in master but I don't think this significantly changed the situation. And we don't want codebases to further diverge.

@k163377
Copy link
Contributor

k163377 commented Feb 28, 2023

Done in #636

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants