#33061 closed Bug (fixed)
ValueError not raised by some cache backends for incr() / decr() with negative delta when a key is missing
Reported by: | Chris Jerdonek | Owned by: | Sondre Lillebø Gundersen |
---|---|---|---|
Component: | Core (Cache system) | Version: | 3.2 |
Severity: | Normal | Keywords: | |
Cc: | Sondre Lillebø Gundersen | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
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.
Change History (16)
comment:1 by , 3 years ago
Description: | modified (diff) |
---|
comment:2 by , 3 years ago
Summary: | ValueError not raised for incr / decr with negative delta for some backends? → ValueError not raised by some cache backends for incr() / decr() with negative delta? |
---|
comment:3 by , 3 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
follow-up: 7 comment:4 by , 3 years ago
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. This would affect only some backends that derive from BaseMemcachedCache
. I'm including the incr()
method in full below to make it easier to see. The comment below has more details:
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
Also, here is where it's documented in the Django docs that incr()
/ decr()
should raise ValueError
when a key is missing:
A ValueError will be raised if you attempt to increment or decrement a nonexistent cache key.:
comment:5 by , 3 years ago
Summary: | ValueError not raised by some cache backends for incr() / decr() with negative delta? → ValueError not raised by some cache backends for incr() / decr() with negative delta when a key is missing |
---|
comment:6 by , 3 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
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.
follow-up: 8 comment:7 by , 3 years ago
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 ofLibraryValueNotFoundException
(or returningNone
) when a key is missing. If you look at the links to the code I provided, thedelta < 0
code path isn't inside the try-except block a line later where that conversion takes place for thedelta >= 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 🙂
follow-up: 9 comment:8 by , 3 years ago
Replying to Sondre Lillebø Gundersen:
The missing key would not be raised from
incr
but fromdecr
, right? Perhaps I'm missing the point 🙂
You're missing that it calls self._cache.decr()
not self.decr()
.
follow-up: 10 comment:9 by , 3 years ago
Replying to Mariusz Felisiak:
Replying to Sondre Lillebø Gundersen:
The missing key would not be raised from
incr
but fromdecr
, right? Perhaps I'm missing the point 🙂
You're missing that it calls
self._cache.decr()
notself.decr()
.
Can you please tell me what does self._cache.decr()
return for a missing key ?
comment:10 by , 3 years ago
comment:11 by , 3 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:12 by , 3 years ago
Cc: | added |
---|---|
Has patch: | set |
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.
comment:13 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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.