mumoshu/play2-memcached

Support removing key by passing null and 0

Enalmada opened this issue · 4 comments

The documentation states that the only way to remove a key from cache is to pass in a null with 0
Cache.set("item.key", null, 0);
https://github.com/playframework/Play20/wiki/JavaCache

I see that serialize throws null if null is passed in, but I think perhaps passing null and 0 should be a special case and internally call delete. I am thking something like this?

def set(key: String, value: Any, expiration: Int) {
if(value == null && expiration == 0) {
client.remove(key)
} else {
client.set(key, expiration, value, tc)
}
}

Actually, there is probably no good reason to ever support null so this could be better but you may have reason to know better...

def set(key: String, value: Any, expiration: Int) {
  if (value == null) {
    remove(key);
  } else {
    client.set(key, expiration, value, tc)
  }
}

Thanks for your proposal!

I guessed that removing the key on set(key, null, 0) or set(key, null nonZero), as you propsed, is too much.
But making it not to throw exceptions on setting null:

override protected def serialize(obj: java.lang.Object) = {
  val bos: ByteArrayOutputStream = new ByteArrayOutputStream()
  // Just allow serializing `null`
  new ObjectOutputStream(bos).writeObject(obj)
  bos.toByteArray()
}

is O.K.

How do you think of this change?


Let me explain what I thought.

The documentation of the Cache API for Scala states that in Scala, you can remove keys with:

import play.api.cache.{EhCachePlugin, Cache}
import play.api.Play
Play.current.plugin[EhCachePlugin].map {
      ehcache =>
        ehcache.cache.remove(key)
}.getOrElse(false) 

Actually, EhCachePlugin does not remove a key on calling set(key, null, 0), but does set null value for the key eternally.

In Java Cache API, get(key) returns null when the key doesn't exist or the value for the key is null, thus "setting the value for key to 0 eternally" results as if the key was removed.
(BTW, the key actually remains and keeps taking up tiny space in memcached! But we call this removing the key for the convenience)

In Scala Cache API, get(key) returns None when the key doesn't exist, but returns Some(null) when the value for the key is null.

I thought that returning Some(null), does not make sense, but confusing, for Scala Cache API.
That's why I decided to throw exceptions on setting null to encourage people not set null.

But you noticed me that it disallows Java developers remove keys in the ways the documentation states.

Now, my proposal:

override protected def serialize(obj: java.lang.Object) = {
  val bos: ByteArrayOutputStream = new ByteArrayOutputStream()
  // Just allow serializing `null`
  new ObjectOutputStream(bos).writeObject(obj)
  bos.toByteArray()
}

allows both Java and Scala developers remove keys in the ways the documentation states.

How do you think of this proposal?

It sounds good to me. It is great that you found a way to allow both the java and scala side to work well.

I have pushed the changes to master.
You can now remove keys as documented in both Java and Scala!


BTW, EhCache and Memcached implementations of Cache API have a subtle difference, when used from Scala, about how it behaves for nulls.
The EhCache impl returns Some(null) when getting null value. Meanwhile, the Memcached impl returns None in that case.
(See MemcachedIntegrationSpec.scala for detail)

That's because the Java memcached client does not differentiate the absence of key from null.
I'm going to keep the subtle difference remaining, until I could come up with a straightforward way to make the Memcached impl compliant with the EhCache impl.