-
Notifications
You must be signed in to change notification settings - Fork 192
Allow caching null objects. #1303
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
Conversation
Closes #939.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with some nits.
/** | ||
* This is a standalone class (vs. inner) to allow Serialization of all fields to work. | ||
* If it was an inner class of CouchbaseCacheIntegrationTests, then it would have a | ||
* this$0 field = CouchbaseCacheIntegrationTests and would not serialize. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If CacheUser were a static nested class, would it it still need to be in a separate file?
https://docs.oracle.com/javase/tutorial/java/javaOO/nested.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with some nits.
* @author Michael Reiche | ||
*/ | ||
@IgnoreWhen(clusterTypes = ClusterType.MOCKED) | ||
class CouchbaseCacheIntegrationTests extends JavaIntegrationTests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are good. Are they related to the "Allow caching null objects" change?
Should we have a test that caches and retrieves a null value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cacheHitMiss() tests a cache hit on a stored null.
It also tests a cache-miss - which is the same for a null as a non-null.
The other tests have cache-hits on non-null (the normal case).
Closes #939. Co-authored-by: mikereiche <[email protected]>
Closes #939. Co-authored-by: mikereiche <[email protected]>
Use toStoreValue(value) to substitute null objects with NullValue.INSTANCE when putting. fromStoreValue(value) does the opposite when getting.
Closes #939.