Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#33361 closed Bug (fixed)

Redis cache backend doesn't allow storing bool values

Reported by: Jeremy Lainé Owned by: Jeremy Lainé
Component: Core (Cache system) Version: 4.0
Severity: Release blocker Keywords:
Cc: Nick Pope, Daniyal Abbasi Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Mariusz Felisiak)

The following code raises an exception: redis.exceptions.DataError: Invalid input of type: 'bool'. Convert to a bytes, string, int or float first.

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.

Change History (9)

comment:1 Changed 2 years ago by Mariusz Felisiak

Cc: Nick Pope Daniyal Abbasi added
Description: modified (diff)
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Thanks for report! It should be enough to special-case bool, e.g.

  • django/core/cache/backends/redis.py

    diff --git a/django/core/cache/backends/redis.py b/django/core/cache/backends/redis.py
    index 16556b1ded..bbb0e0320d 100644
    a b from django.utils.module_loading import import_string 
    1111
    1212class RedisSerializer(PickleSerializer):
    1313    def dumps(self, obj):
    14         if isinstance(obj, int):
     14        if isinstance(obj, int) and not isinstance(obj, bool):
    1515            return obj
    1616        return super().dumps(obj)
    1717

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 discussion).

comment:2 Changed 2 years ago by Jeremy Lainé

Owner: changed from nobody to Jeremy Lainé
Status: newassigned

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:

  • django/core/cache/backends/redis.py

    diff --git a/django/core/cache/backends/redis.py b/django/core/cache/backends/redis.py
    index 16556b1ded..bbb0e0320d 100644
    a b from django.utils.module_loading import import_string 
    1111
    1212class RedisSerializer(PickleSerializer):
    1313    def dumps(self, obj):
    14         if isinstance(obj, int):
     14        if type(obj) is int:
    1515            return obj
    1616        return super().dumps(obj)
    1717

comment:3 in reply to:  2 Changed 2 years ago by Mariusz Felisiak

Replying to 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:

  • django/core/cache/backends/redis.py

    diff --git a/django/core/cache/backends/redis.py b/django/core/cache/backends/redis.py
    index 16556b1ded..bbb0e0320d 100644
    a b from django.utils.module_loading import import_string 
    1111
    1212class RedisSerializer(PickleSerializer):
    1313    def dumps(self, obj):
    14         if isinstance(obj, int):
     14        if type(obj) is int:
    1515            return obj
    1616        return super().dumps(obj)
    1717

Yes, looks good, a small comment could de helpful.

comment:4 Changed 2 years ago 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.

comment:5 Changed 2 years ago by Claude Paroz

Has patch: set

comment:6 Changed 2 years ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:7 Changed 2 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In c7902612:

Refs #33361 -- Added Added DummyCache.set() test for boolean values.

comment:8 Changed 2 years ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 2f33217:

Fixed #33361 -- Fixed Redis cache backend crash on booleans.

comment:9 Changed 2 years ago by Mariusz Felisiak <felisiak.mariusz@…>

In 3b03bce1:

[4.0.x] Fixed #33361 -- Fixed Redis cache backend crash on booleans.

Backport of 2f33217ea2cad688040dd6044cdda946c62e5b65 from main

Note: See TracTickets for help on using tickets.
Back to Top