Empty String Stored when `cache_nils: false` and `raw: true`

[nickamorim]

Description

When storing a nil value with the raw: true request option, Dalli stores an empty string even though cache_nils is false [ref]. This especially doesn't play nice with a custom serializer. @petergoldstein I'm curious to get your thoughts on whether we should instead raise an ArgumentError in this situation instead of storing an empty string (although it would be a breaking change).

irb(main):001> require "dalli"
irb(main):002> c = Dalli::Client.new("127.0.0.1:11211", cache_nils: false)
irb(main):003> c.set("foo", nil, 0, raw: true)
irb(main):004> c.get("foo")
=> ""

Expected Behaviour

irb(main):001> require "dalli"
irb(main):002> c = Dalli::Client.new("127.0.0.1:11211", cache_nils: false)
irb(main):003> c.set("foo", nil, 0, raw: true) <= ArgumentError

[petergoldstein]

@nickamorim I agree that the current behavior is problematic, and we should adjust with what amounts to a breaking change.

How does this behave when cache_nils is true? I'm not averse to raising an error, but I want to make sure we're handling all of the relevant cases. Can you please enumerate the two cases and describe what currently happens (and what you think should happen) in both cases? Then we can make sure we have a sensible, consistent behavior.

[nickamorim]

How does this behave when cache_nils is true?

An empty string is stored:

irb(main):001> require "dalli"
irb(main):002> c = Dalli::Client.new("127.0.0.1", cache_nils: true)
irb(main):003> c.set("foo", nil, 0, raw: true)
irb(main):004> c.get("foo")
=> ""

Can you please enumerate the two cases and describe what currently happens (and what you think should happen) in both cases?

The current behaviour is such that if the raw storage request option is true, we store an empty string in Memcached regardless of what cache_nils is set to.

My proposed behaviour is that if cache_nils is true, we store a sentinel value in Memcached to represent nil and the subsequent get call returns nil. If cache_nils is false, we return an ArgumentError:

irb(main):002> c = Dalli::Client.new("127.0.0.1", cache_nils: true)
irb(main):003> c.set("foo", nil, 0, raw: true) # store a sentinel value (e.g. "__nil__") in the cache
irb(main):004> c.get("foo"
=> nil
irb(main):005> c = Dalli::Client.new("127.0.0.1", cache_nils: false)
irb(main):006> c.set("foo", nil, 0, raw: true)
=> ArgumentError attempting to store a nil value with cache_nils set to false

WDYT @petergoldstein?

[grcooper]

I think the issue here comes from the original line that Nick linked in the problem statement: https://github.com/petergoldstein/dalli/blob/main/lib/dalli/protocol/value_serializer.rb#L31

We call raw: true but then convert it automatically to a string. I think this is kind of nice to the user, but can really lead to some lazy and unexpected behaviour, as we have just seen.

We have 4 use cases for cache_nils and raw:

What happens now:

# Nils allowed cases:
c_nils = Dalli::Client.new("127.0.0.1", cache_nils: true)

# Raw false && nils allowed
c_nils.set("foo", nil, 0, raw: false)
=> 360287970189639680
c_nils.get("foo")
=> nil
# Memcached output
VALUE foo 1 3
0
END

# Raw true && nils allowed
c_nils.set("foo", nil, 0, raw: true)
=> 432345564227567616
c_nils.get("foo")
=> ""
# Memcached output
VALUE foo 0 0

END

# Nils not allowed cases:
c_no_nils = Dalli::Client.new("127.0.0.1", cache_nils: false)

# Raw false && nils not allowed
c_no_nills.set("bar", nil, 0, raw: false)
=> 504403158265495552
c_no_nills.get("bar")
=> nil
# Memcached output
VALUE bar 1 3
0
END

# Raw true && nils not allowed
c_no_nills.set("bar", nil, 0, raw: true)
=> 576460752303423488
c_no_nills.get("bar")
=> ""
# Memcached output:
get bar
VALUE bar 0 0

END

case 3 seems a bit odd to me, since we are saying cache_nils to be false, but still caching them, but they are ruby objects that we serialize, and they do end up in memcached as some bytes.

I think the behaviour should be:

1. Raw false && Nils allowed -> Current behaviour, store nil, show off nil, misses are returned as NilObject instead
2. Raw true && Nils allowed -> Should fail, anything that is not a string should be an argument error
3. Raw false && nils not allowed -> current behaviour is fine for the sake of not adding more breaking changes than we need to
4. Raw true && nils not allowed -> Follows 2, anything that is not a string should be an argument error.

Raw true for me, is a more important forcing function than cache_nils. Looking back at the PR that added it, it was added to support behaviour that behaved more like ruby hashes where you can lookup to see if a key exists, even if the value is nil. I think that is valuable, but I would assume it is a very niche case, where you are working with specific objects. If you are ever writing raw data, writing an nil and auto-converting to "" seems like dangerous behaviour.

I think the other way around is that if we wanted to support a good mix of raw + cache_nils, we could convert "" to the NilObject if returned? But I think that might mess with anyone who actually wants to cache the empty string.