ValueError not raised by some cache backends for incr() / decr() with negative delta when a key is missing

[Chris Jerdonek]

I was looking at the code for some of the cache backends, and I noticed that in a couple places, it looks like ValueError isn't being raised when it should be. This is in the ​incr() and ​decr() methods of BaseMemcachedCache when delta is less than zero. The commit where this code was added is here: ​https://github.com/django/django/commit/79dd751b0b9e428bf386d7304f442fa9815fdbba

This is just by looking at the code, so I could be wrong. But I wanted to flag it.

[Mariusz Felisiak]

... it looks like ValueError isn't being raised when it should be.

Why do you think that we should raise an error in this case? I found nothing in our docs or code to suggest that negative delta is forbidden 🤔 It works on all cache backends.

[Chris Jerdonek]

Why do you think that we should raise an error in this case?

It's not that an error should be raised from nothing. It's that ValueError should be raised instead of LibraryValueNotFoundException. If you look at the links to the code I provided, the delta < 0 code path isn't inside the try-except block a line later where that conversion takes place for the delta >= 0 case. Please look, and you will see what I mean. This would just affect backends that derive from BaseMemcachedCache.

[Mariusz Felisiak]

Thanks for the clarification. I misunderstood your original intentions, sorry 🤦.

Also, it looks like we can completely remove BaseMemcachedCache.decr() (as a separate cleanup) after fixing this.

[Sondre Lillebø Gundersen]

Replying to Chris Jerdonek:

Why do you think that we should raise an error in this case?

It's not that an error should be raised from nothing. It's that ValueError should be raised instead of LibraryValueNotFoundException (or returning None) when a key is missing. If you look at the links to the code I provided, the delta < 0 code path isn't inside the try-except block a line later where that conversion takes place for the delta >= 0 case.

Hi!

I was looking at trying to implement this ticket, but I'm confused by one thing (perhaps I'm missing something):

*Won't* the delta < 0 code path for incr raise a ValueError even though it's not in the incr try/except block? Since it calls decr and that method has its own mirrored logic, it seems like you would end up with a ValueError either way, wouldn't you?

    def incr(self, key, delta=1, version=None):
        key = self.make_key(key, version=version)
        # memcached doesn't support a negative delta
        if delta < 0:
            return self._cache.decr(key, -delta)
        try:
            val = self._cache.incr(key, delta)

        # python-memcache responds to incr on non-existent keys by
        # raising a ValueError, pylibmc by raising a pylibmc.NotFound
        # and Cmemcache returns None. In all cases,
        # we should raise a ValueError though.
        except self.LibraryValueNotFoundException:
            val = None
        if val is None:
            raise ValueError("Key '%s' not found" % key)
        return val

    def decr(self, key, delta=1, version=None):
        key = self.make_key(key, version=version)
        # memcached doesn't support a negative delta
        if delta < 0:
            return self._cache.incr(key, -delta)
        try:
            val = self._cache.decr(key, delta)

        # python-memcache responds to incr on non-existent keys by
        # raising a ValueError, pylibmc by raising a pylibmc.NotFound
        # and Cmemcache returns None. In all cases,
        # we should raise a ValueError though.
        except self.LibraryValueNotFoundException:
            val = None
        if val is None:
            raise ValueError("Key '%s' not found" % key)
        return val

The missing key would not be raised from incr but from decr, right? Perhaps I'm missing the point 🙂

[Mariusz Felisiak]

Replying to Sondre Lillebø Gundersen:

The missing key would not be raised from incr but from decr, right? Perhaps I'm missing the point 🙂

You're missing that it calls self._cache.decr() not self.decr().

[Vijay]

Replying to Mariusz Felisiak:

Replying to Sondre Lillebø Gundersen:

The missing key would not be raised from incr but from decr, right? Perhaps I'm missing the point 🙂

You're missing that it calls self._cache.decr() not self.decr().

Can you please tell me what does self._cache.decr() return for a missing key ?

[Mariusz Felisiak]

Replying to Vijay :

Can you please tell me what does self._cache.decr() return for a missing key ?

The same as self._cache.incr(), it depends on the library and it's described in the ​code. This doesn't really matter for the ticket.

[Sondre Lillebø Gundersen]

Opened at PR for this: ​https://github.com/django/django/pull/14810 :)

As far as I can tell, it should now work as I expected it to originally - and I think that fixes any uncertainty around the handling of a missing key, since negative deltas are now treated the same as positive deltas, just with the extra step.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 96ab3a1:

Refs #33061 -- Added DummyCache.incr()/decr() tests for nonexistent keys with negative deltas.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 2c912c34:

Fixed #33061 -- Fixed handling nonexistent keys with negative deltas in incr()/decr() in memcached backends.

Thanks Chris Jerdonek for the review.

[GitHub <noreply@…>]

In 93e06f29:

Refs #33061 -- Removed unnecessary BaseMemcachedCache.decr().