[Django] #33061: ValueError not raised for incr / decr with negative delta for some backends?
Django
backends?
-----------------------------------------------+------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 3.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-----------------------------------------------+------------------------
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
[https://github.com/django/django/blob/3445c50a3affc5ae7b1c2712a139d4a5105aeaf5/django/core/cache/backends/memcached.py#L111-L112
incr()] and
[https://github.com/django/django/blob/3445c50a3affc5ae7b1c2712a139d4a5105aeaf5/django/core/cache/backends/memcached.py#L128-L129
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
--
Ticket URL: <https://code.djangoproject.com/ticket/33061>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
backends?
Reporter: Chris Jerdonek | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Description changed by Chris Jerdonek:
Old description:
> 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
> [https://github.com/django/django/blob/3445c50a3affc5ae7b1c2712a139d4a5105aeaf5/django/core/cache/backends/memcached.py#L111-L112
> incr()] and
> [https://github.com/django/django/blob/3445c50a3affc5ae7b1c2712a139d4a5105aeaf5/django/core/cache/backends/memcached.py#L128-L129
> 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
New description:
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
[https://github.com/django/django/blob/3445c50a3affc5ae7b1c2712a139d4a5105aeaf5/django/core/cache/backends/memcached.py#L111-L112
incr()] and
[https://github.com/django/django/blob/3445c50a3affc5ae7b1c2712a139d4a5105aeaf5/django/core/cache/backends/memcached.py#L128-L129
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:1>
Django
negative delta?
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:2>
Django
negative delta?
-------------------------------------+-------------------------------------
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* resolution: => needsinfo
Comment:
> ... 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:3>
Django
negative delta?
-------------------------------------+-------------------------------------
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: nobody
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:5>
Django
negative delta when a key is missing
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: closed => new
* resolution: needsinfo =>
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:6>
Django
negative delta when a key is missing
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Jonny Park):
Made the pull request at https://github.com/django/django/pull/14807
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:7>
Django
negative delta when a key is missing
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Jonny Park):
Replying to [comment:7 Jonny Park]:
> Made the pull request at https://github.com/django/django/pull/14807
Sorry, please ignore this. It's a comment in wrong ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:8>
Django
negative delta when a key is missing
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Sondre Lillebø Gundersen):
Replying to [comment:4 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?
{{{#!python
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 🙂
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:7>
Django
negative delta when a key is missing
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:7 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()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:8>
Django
negative delta when a key is missing
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Vijay ):
Replying to [comment:8 Mariusz Felisiak]:
> Replying to [comment:7 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 non-
existing key ?
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:9>
Django
negative delta when a key is missing
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:9 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
[https://github.com/django/django/blob/main/django/core/cache/backends/memcached.py
code]. This doesn't really matter for the ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:10>
Django
negative delta when a key is missing
Reporter: Chris Jerdonek | Owner: Sondre
| Lillebø Gundersen
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Sondre Lillebø Gundersen):
* owner: nobody => Sondre Lillebø Gundersen
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:11>
Django
negative delta when a key is missing
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Sondre
| Lillebø Gundersen
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Sondre Lillebø Gundersen):
* cc: Sondre Lillebø Gundersen (added)
* has_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:12>
Django
negative delta when a key is missing
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Sondre
| Lillebø Gundersen
Type: Bug | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:13>
Django
negative delta when a key is missing
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Sondre
| Lillebø Gundersen
Type: Bug | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"96ab3a13792c129b9a16c5788e9e0bf3d828564d" 96ab3a1]:
{{{
#!CommitTicketReference repository=""
revision="96ab3a13792c129b9a16c5788e9e0bf3d828564d"
Refs #33061 -- Added DummyCache.incr()/decr() tests for nonexistent keys
with negative deltas.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:14>
Django
negative delta when a key is missing
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Sondre
| Lillebø Gundersen
Component: Core (Cache system) | Version: 3.2
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2c912c348808ee66a4fd164bda68b494243c6c54" 2c912c34]:
{{{
#!CommitTicketReference repository=""
revision="2c912c348808ee66a4fd164bda68b494243c6c54"
Fixed #33061 -- Fixed handling nonexistent keys with negative deltas in
incr()/decr() in memcached backends.
Thanks Chris Jerdonek for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:15>
Django
negative delta when a key is missing
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Sondre
| Lillebø Gundersen
Type: Bug | Status: closed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"93e06f29787fce6f7c58acd9d1e17a7192a66c94" 93e06f29]:
{{{
#!CommitTicketReference repository=""
revision="93e06f29787fce6f7c58acd9d1e17a7192a66c94"
Refs #33061 -- Removed unnecessary BaseMemcachedCache.decr().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33061#comment:16>