[Django] #19221: Cache keys can't be integers
Django
-------------------------------------+--------------------
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+--------------------
In current master cache keys can't be integers whilst they can in 1.4.
Attempts to do this result in:
{{{
File "/home/ismdj/src/django/django/core/cache/backends/dummy.py", line
15, in get
key = self.make_key(key, version=version)
File "/home/ismdj/src/django/django/core/cache/backends/base.py", line
80, in make_key
new_key = self.key_func(key, self.key_prefix, version)
File "/home/ismdj/src/django/django/core/cache/backends/base.py", line
26, in default_key_func
return ':'.join([key_prefix, str(version), key])
TypeError: sequence item 2: expected string or Unicode, int found
}}}
The issue seems to have been introduced with this commit:
https://github.com/django/django/commit/45baaabafb6cf911afad9ec63c86753b284f7269
--
Ticket URL: <https://code.djangoproject.com/ticket/19221>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
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 russellm):
* needs_better_patch: => 0
* needs_docs: => 0
* severity: Normal => Release blocker
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
Since this is an apparent regression in behavior, I'm marking this as a
release blocker.
Whether this is an actual bug will depend on whether we've ever
contractually guaranteed that a cache key can be any type. I'm not
necessarily convinced that an integer *should* be allowed to be a cache
key (other than the fact that historically, it's apparently been allowed).
So - the resolution here might be documenting the type guarantee, rather
than fixing the 'bug'.
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:1>
Django
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
This is another case of Django's source historically abusing
`smart_bytes`, which makes integers work where they shouldn't. The docs
don't promise anything and the cache keys are clearly intended to be
strings.
I recently had the same problem in `HttpResponse`. I ended up implementing
explicit support for integers because it was tested, but it's probably
going to be deprecated in the long run.
In this case, I believe that `cache.get(42)` and `cache.get('42')`
shouldn't be the same -- this isn't PHP! In Django 1.4, they are. I'm +0
on classifying this as a bugfix, and maybe documenting it as a backwards
incompatible change.
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:2>
Django
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Anssi agrees with just documenting the change (on IRC).
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:3>
Django
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Upon further thought, we could take this opportunity to:
- document that cache keys *must* be strings,
- document how unicode and byte strings behave: at least with the
memcached backend, a unicode string and the corresponding utf-8 bytestring
map to the same cache key; it could be considered a bug that two non-equal
Python objects map to the same cache key;
- (maybe) normalize that behavior across cache backends.
Overall this would result in a simpler and more correct implementation —
without `smart_str` or `smart_bytes` that often do more work than is
desirable.
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:4>
Django
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
One more thing: with the current code, cache keys have to be `str` on
Python 3, not `bytes`. This is a rather annoying limitation.
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:5>
Django
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by ptone):
If we can easily coerce to a str at the right place, I don't see why we
can't do this for users. Since we have a finite known set of components
for the key - we should be able to do this with more robust string
formatting instead of join.
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:6>
Django
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Since this is a release blocker for 1.5, we have to do something quickly;
but I'm not ready to commit to supporting integers as cache keys in Django
right now.
I'm not specifically against using integers as cache keys. I'm against
different Python values mapping to the same cache key, and that's what
happens currently with integers (as well as any non-string type):
{{{
>>> a, b = 42, "42"
>>> a == b
False
>>> cache.get(a)
>>> cache.set(b, "foobar")
>>> cache.get(a)
"foobar"
}}}
----
As a compromise, in the short term, I suggest this small change:
{{{
--- a/django/core/cache/backends/base.py
+++ b/django/core/cache/backends/base.py
@@ -26,7 +26,7 @@ def default_key_func(key, key_prefix, version):
the `key_prefix'. KEY_FUNCTION can be used to specify an alternate
function with custom key making behavior.
"""
- return ':'.join([key_prefix, str(version), key])
+ return '%s:%s:%s' % (key_prefix, version, key)
def get_key_func(key_func):
}}}
No tests, because tests create a promise of backwards-compatibility.
And then move the ticket to DDN.
----
This will make the cache work with any type of key that's convertible to a
unicode string (`unicode_literals` is turned on in this module).
It isn't 100% backwards compatible: Django used to call `smart_bytes`, and
with this change it will implicitly call `six.text_type`. But I'm strongly
against using the `smart_*` or `force_*` here. These functions do much
more work than necessary. They are only suitable for use on not-so-well-
specified input, or in extreme cases (like the 500 view) where we want
*something* representing *any* object (even an instance of
`UnicodeDecodeError`).
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:7>
Django
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"6c69de80bdcd2744bc64cb933c2d863dd5e74a33"]:
{{{
#!CommitTicketReference repository=""
revision="6c69de80bdcd2744bc64cb933c2d863dd5e74a33"
Tweaked cache key creation to avoid strict typing.
This is a provisional change. See #19221 for details.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:8>
Django
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Design
Has patch: 0 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* severity: Release blocker => Normal
* stage: Accepted => Design decision needed
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:9>
Django
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Design
Has patch: 0 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"3db2aeec988f7bc9ad8a0844809a5bd4d7211669"]:
{{{
#!CommitTicketReference repository=""
revision="3db2aeec988f7bc9ad8a0844809a5bd4d7211669"
[1.5.x] Tweaked cache key creation to avoid strict typing.
This is a provisional change. See #19221 for details.
Backport of 6c69de8 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:10>
Django
-------------------------------------+------------------------------------
Reporter: mhsparks | Owner: nobody
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* stage: Design decision needed => Accepted
Comment:
Since no one had a better idea, let's do what I explained above.
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:11>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
I agree that implicitly converting `42` and `"42"` to be the same cache
key is bad, and we should migrate away from it with an appropriate
deprecation path. (I think the patch to do so should take some care to
"assume string and only do more work if that assumption fails" rather than
"check pre-emptively", in order to preserve all possible speed in the
common case.)
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:12>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* cc: Carl Meyer (added)
Comment:
Carl, I thought this might be a good task for a new-ish contributor but
I'm not sure what the implementation of "assume string and only do more
work if that assumption fails" might look like. It looks like you're
suggesting not to use `isinstance` but I'm not sure what the alternative
is.
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:13>
Django
Reporter: Mark Hughes | Owner: Dan
| Stephenson
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Dan Stephenson):
* status: new => assigned
* cc: dan@… (added)
* owner: nobody => Dan Stephenson
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Mark Hughes | Owner: Dan
| Stephenson
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Dan Stephenson):
Can i propose prepending the name of the keys object type, to fix the
issue of `42` and `"42"` pointing at same data as it does currently.
{{{
return '%s:%s:%s_%s' % (key_prefix, version, type(key).__name__, key)
}}}
The challenge for this change would be in how we handle pre-existing cache
keys after Django is upgraded.
- We could add some code into the **has_key** function temporarily, so
that in scenarios where it returns None, there is an additional check
against the legacy format, if something is found, we would update the
cache key (delete the old one), and return it.
or
- Add an advisory to the release notes that the internal cache naming has
changed, and to prevent orphaned keys taking up space, its recommended to
flush existing caches alongside the version upgrade.
comments welcome!
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Mark Hughes | Owner: Dan
| Stephenson
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Balazs Endresz):
The `default_key_func` is still the old version in the docs, which fails
with any non-string arguments:
https://github.com/django/django/blob/stable/3.0.x/docs/ref/settings.txt#L171
Btw, not just the `key` but the `key_prefix` can be non-string too. What
tripped me up was this in the test settings:
`CACHES['default']['KEY_PREFIX'] = random()` when used with the
`KEY_PREFIX` code snippet from the docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:16>
Django
Reporter: Mark Hughes | Owner: (none)
Type: Bug | Status: new
Component: Core (Cache system) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Dan Stephenson => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:17>
Django
-------------------------------------+------------------------------------
Reporter: Mark Hughes | Owner: (none)
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:18>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by DKPHUONG):
* owner: (none) => DKPHUONG
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:19>
Django
-------------------------------------+------------------------------------
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by DKPHUONG):
* owner: DKPHUONG => (none)
Ticket URL: <https://code.djangoproject.com/ticket/19221#comment:20>