[Django] #33361: Redis cache backend doesn't allow storing bool values
Django
-----------------------------------------------+------------------------
Reporter: Jeremy Lainé | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 4.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------------+------------------------
The following code raises an exception: `redis.exceptions.DataError:
Invalid input of type: 'bool'. Convert to a bytes, string, int or float
first.`
{{{#!python
from django.core.cache import cache
cache.set("foo", True)
}}}
This contradicts the documentation which states that any data type
supported by pickle can be stored to the cache.
The root cause seems to be because instances of `int` are special-cased
and not send through pickle, but redis-py cannot send booleans to redis:
What was the rationale behind the `int` special-case? `django-redis` for
instance consistently sends all data through pickle.
--
Ticket URL: <https://code.djangoproject.com/ticket/33361>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Jeremy Lainé | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: 4.0
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Nick Pope, Daniyal Abbasi (added)
* stage: Unreviewed => Accepted
* severity: Normal => Release blocker
Old description:
> The following code raises an exception: `redis.exceptions.DataError:
> Invalid input of type: 'bool'. Convert to a bytes, string, int or float
> first.`
>
> {{{#!python
> from django.core.cache import cache
> cache.set("foo", True)
> }}}
>
> This contradicts the documentation which states that any data type
> supported by pickle can be stored to the cache.
>
> The root cause seems to be because instances of `int` are special-cased
> and not send through pickle, but redis-py cannot send booleans to redis:
>
> https://github.com/django/django/blob/2f73e5406d54cb8945e187eff302a3a3373350be/django/core/cache/backends/redis.py#L14
>
> What was the rationale behind the `int` special-case? `django-redis` for
> instance consistently sends all data through pickle.
New description:
The following code raises an exception: `redis.exceptions.DataError:
Invalid input of type: 'bool'. Convert to a bytes, string, int or float
first.`
{{{#!python
from django.core.cache import cache
cache.set("foo", True)
}}}
This contradicts the documentation which states that any data type
supported by pickle can be stored to the cache.
The root cause seems to be because instances of `int` are special-cased
and not send through pickle, but redis-py cannot send booleans to redis:
What was the rationale behind the `int` special-case? `django-redis` for
instance consistently sends all data through pickle.
--
Comment:
Thanks for report! It should be enough to special-case `bool`, e.g.
{{{#!diff
diff --git a/django/core/cache/backends/redis.py
b/django/core/cache/backends/redis.py
index 16556b1ded..bbb0e0320d 100644
--- a/django/core/cache/backends/redis.py
+++ b/django/core/cache/backends/redis.py
@@ -11,7 +11,7 @@ from django.utils.module_loading import import_string
class RedisSerializer(PickleSerializer):
def dumps(self, obj):
- if isinstance(obj, int):
+ if isinstance(obj, int) and not isinstance(obj, bool):
return obj
return super().dumps(obj)
}}}
Would you like to prepare a patch?
> What was the rationale behind the `int` special-case? `django-redis` for
instance consistently sends all data through pickle.
We do this to have better atomicity of `incr()` and `decr()` operations
(see [https://github.com/django/django/pull/14437#issuecomment-915046037
discussion]).
--
Ticket URL: <https://code.djangoproject.com/ticket/33361#comment:1>
Django
Reporter: Jeremy Lainé | Owner: Jeremy
| Lainé
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jeremy Lainé):
* owner: nobody => Jeremy Lainé
* status: new => assigned
Comment:
Thanks for the reply and the link to the discussion. I'm assigning myself
to the ticket as I'd be happy to prepare a patch and corresponding test.
Regarding the proposed fix, it feels like we're just moving the problem as
I'd expect similar issues with an user-defined subclasses of `int`. Would
it be acceptable to do:
{{{#!diff
diff --git a/django/core/cache/backends/redis.py
b/django/core/cache/backends/redis.py
index 16556b1ded..bbb0e0320d 100644
--- a/django/core/cache/backends/redis.py
+++ b/django/core/cache/backends/redis.py
@@ -11,7 +11,7 @@ from django.utils.module_loading import import_string
class RedisSerializer(PickleSerializer):
def dumps(self, obj):
- if isinstance(obj, int):
+ if type(obj) is int:
return obj
return super().dumps(obj)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33361#comment:2>
Django
Reporter: Jeremy Lainé | Owner: Jeremy
| Lainé
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:2 Jeremy Lainé]:
> Thanks for the reply and the link to the discussion. I'm assigning
myself to the ticket as I'd be happy to prepare a patch and corresponding
test.
>
> Regarding the proposed fix, it feels like we're just moving the problem
as I'd expect similar issues with an user-defined subclasses of `int`.
Would it be acceptable to do:
>
> {{{#!diff
> diff --git a/django/core/cache/backends/redis.py
b/django/core/cache/backends/redis.py
> index 16556b1ded..bbb0e0320d 100644
> --- a/django/core/cache/backends/redis.py
> +++ b/django/core/cache/backends/redis.py
> @@ -11,7 +11,7 @@ from django.utils.module_loading import import_string
>
> class RedisSerializer(PickleSerializer):
> def dumps(self, obj):
> - if isinstance(obj, int):
> + if type(obj) is int:
> return obj
> return super().dumps(obj)
>
> }}}
Yes, looks good, a small comment could de helpful.
--
Ticket URL: <https://code.djangoproject.com/ticket/33361#comment:3>
Django
Reporter: Jeremy Lainé | Owner: Jeremy
| Lainé
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jeremy Lainé):
I've submitted a PR which includes a simple unit test on serialization. I
checked the tests pass by running it locally with a configured Redis
cache, I'm not sure if CI executes backend-specific tests though.
--
Ticket URL: <https://code.djangoproject.com/ticket/33361#comment:4>
Django
Reporter: Jeremy Lainé | Owner: Jeremy
| Lainé
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/15191 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33361#comment:5>
Django
Reporter: Jeremy Lainé | Owner: Jeremy
| Lainé
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: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33361#comment:6>
Django
Reporter: Jeremy Lainé | Owner: Jeremy
| Lainé
Type: Bug | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"c7902612ca722f25f6e461c82e6d4d5f0c22f16a" c7902612]:
{{{
#!CommitTicketReference repository=""
revision="c7902612ca722f25f6e461c82e6d4d5f0c22f16a"
Refs #33361 -- Added Added DummyCache.set() test for boolean values.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33361#comment:7>
Django
Reporter: Jeremy Lainé | Owner: Jeremy
| Lainé
Component: Core (Cache system) | Version: 4.0
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2f33217ea2cad688040dd6044cdda946c62e5b65" 2f33217]:
{{{
#!CommitTicketReference repository=""
revision="2f33217ea2cad688040dd6044cdda946c62e5b65"
Fixed #33361 -- Fixed Redis cache backend crash on booleans.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33361#comment:8>
Django
Reporter: Jeremy Lainé | Owner: Jeremy
| Lainé
Type: Bug | Status: closed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"3b03bce122b0f7336a90e3cc111a717c1a8f3700" 3b03bce1]:
{{{
#!CommitTicketReference repository=""
revision="3b03bce122b0f7336a90e3cc111a717c1a8f3700"
[4.0.x] Fixed #33361 -- Fixed Redis cache backend crash on booleans.
Backport of 2f33217ea2cad688040dd6044cdda946c62e5b65 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33361#comment:9>