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

Cache deserialization fails with NPE for null valued entries #140

Closed
cowtowncoder opened this issue Dec 20, 2023 · 7 comments
Closed

Cache deserialization fails with NPE for null valued entries #140

cowtowncoder opened this issue Dec 20, 2023 · 7 comments

Comments

@cowtowncoder
Copy link
Member

Since most Guava containers are null-averse, looks like deserializing Cache with null values fails with NPE.

Longer term it may make sense to add more options for handling wrt Guava module, but for now:

  1. Users can use @JsonSetter annotation on field (or equivalent Config Overrides) to "skip nulls"
  2. We should throw JsonMappingException (specifically, MismatchedInputException) instead of NPE in failure case
@arthurscchan
Copy link
Contributor

@cowtowncoder You are acting fast. I just received a new issue earlier today from OSS-Fuzz (https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65183), which is exactly an NPE from GuavaCacheDeserializer. Since you have already fixed that. I would wait for this issue to be resolved.

@cowtowncoder
Copy link
Member Author

@arthurscchan Yeah in this case it was just so similar to other Guava NPEs, easy to reproduce.

Also wondering if this (unrelated) Fuzz issue:

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65181

might have been fixed by one of your PRs. At least I seem unable to reproduce it.

@ben-manes
Copy link

I'm not sure if this matters but fyi wrt the contract for native serialization of Cache.

 * <p>The caches produced by {@code CacheBuilder} are serializable, and the deserialized caches
 * retain all the configuration properties of the original cache. Note that the serialized form does
 * <i>not</i> include cache contents, but only configuration.

@arthurscchan
Copy link
Contributor

arthurscchan commented Dec 20, 2023

@arthurscchan Yeah in this case it was just so similar to other Guava NPEs, easy to reproduce.

Also wondering if this (unrelated) Fuzz issue:

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=65181

might have been fixed by one of your PRs. At least I seem unable to reproduce it.
@cowtowncoder
Yes. That should be fixed and will need some time to resolve.

@cowtowncoder
Copy link
Member Author

@ben-manes Good point. I don't know if configuration may be retained in JSON serialization, but the point about dropping contents is ... interesting. Unfortunately implementation was for #90 where content serialization was explicitly desired.
So change at this point would probably need configuration for opt-in.

So if anyone would want "drop the contents" on serialization, they should file a new issue as RFE.

@ben-manes
Copy link

I think it was for FlumeJava (Spark, Apache Beam predecessor) so the computations could be sent to the data, caching as it was processed on the local node. A very different usage and since a cache is transient data, perhaps the only reasonable expectation. Just an fyi, this seems fine for Jackson

@cowtowncoder
Copy link
Member Author

@ben-manes Yeah it is what it is. But conceptually I think it was wrong thing to do -- I wish I had thought it through.
Not worth worrying too much about but just one of those live-and-learn cases.

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

No branches or pull requests

3 participants