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

AbstractValueAdaptingCache does not allow for flexible null value serialization [SPR-15693] #20252

Closed
spring-issuemaster opened this issue Jun 23, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Jun 23, 2017

Stéphane Nicoll opened SPR-15693 and commented

AbstractAdaptingValueCache has been refactored to offer a NullValue holder that would resolve to a unique instance when deserialized (see #18129).

This infrastructure cannot be used if the cache manager implementation uses a custom serialization mechanism (for instance if the data is serialized in text using json). The redis cache manager is affected by this issue.

A simple/naive fix would be to make that instance public so that they can return what the base class is expecting.


Affects: 4.3.9

Issue Links:

  • #18129 JCacheCache doesn't recognize null values in other JVMs
  • #19739 AbstractValueAdaptingCache#toStoreValue should throw an exception if the value is null and allowNulls is false

Referenced from: commits spring-projects/spring-data-redis@c998abf

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2017

Juergen Hoeller commented

Stéphane Nicoll, Christoph Strobl, we could easily make NullValue.INSTANCE public but I'd really like to understand the need better: What does the Redis cache manager do specifically? Aren't the existing fromStoreValue and toStoreValue template methods wide-ranging enough to allow for a custom null value already?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2017

Christoph Strobl commented

Redis itself does not allow null values for keys and just removes those entries. That's why we need to store the binary representation of NullValue for those otherwise we could not tell the difference between a key representing null and a cache miss.

RedisCache allows usage of different serializers converting the value into the actual byte[] stored. Using eg. Jackson to serialize NullValue fails when used with the defaults. Still, lookup needs to return NullValue in order to have fromStoreValue behave the expected way. So we need to get NullValue.INSTANCE from some place, which we currently do via reflection.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2017

Juergen Hoeller commented

Hmm, AbstractValueAdaptingCache is meant to be used with general Java serialization, and NullValue does work there without a need to expose its instance. If you prefer to extend from it but not use serialization, you should be overriding fromStoreValue and toStoreValue accordingly, directly adapting to the store representation without a need to mimic NullValue.

That said, maybe Redis simply isn't a case for extending AbstractValueAdaptingCache to begin with?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2017

Christoph Strobl commented

It works fine without reflection using the default JdkSerializer we have. It the some special cases with other serializers it does not play well with. I'd rather rely on AbstractValueAdaptingCache than copy the approach to RedisCache having its own flavor of NullValue.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 5, 2017

Juergen Hoeller commented

Alright, for your implementation convenience, NullValue.INSTANCE is public now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.