#30181 closed Cleanup/optimization (fixed)
Fix cache.get() with default on PyLibMCCache if None is cached
Reported by: | Jakub Szafrański | Owned by: | Jakub Szafrański |
---|---|---|---|
Component: | Core (Cache system) | Version: | 2.1 |
Severity: | Normal | Keywords: | memcached none |
Cc: | Jakub Szafrański | 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
If None
is saved to the cache, the default
keyword argument in cache.get()
shadows it if using any Memcached backend. This is not consistent with LocMemCache.
The issue can be very easily fixed within PyLibMCCache
, as it supports retrieving a default value if key is not present.
For MemcachedCache
, unfortunatelly, this is not possible because of python-memcached
limitations, although I've opened up an issue and PR to the upstream package maintainer that would add functionality that would allow this in MemcachedCache
as well: https://github.com/linsomniac/python-memcached/issues/159
I will fix this issue for PyLibMCCache
for now, and once (if) the upstream PR for python-memcached
gets approved, merged and released, I'll also submit a PR that would fix this issue for MemcachedCache
(although that would require a minimum version of python-memcached
, so that one would probably be a little more complex).
Change History (12)
comment:1 by , 6 years ago
Cc: | added |
---|---|
Component: | Uncategorized → Core (Cache system) |
Easy pickings: | set |
Owner: | changed from | to
Status: | new → assigned |
comment:3 by , 6 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 6 years ago
Has patch: | set |
---|
comment:7 by , 6 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | set |
Summary: | Django's memcached default is returned if None is saved in cache → Allow memcached backends to cache None |
Type: | Bug → Cleanup/optimization |
There's documentation in topics/cache.txt about this that would need to be updated: "We advise against storing the literal value None in the cache, because you won't be able to distinguish between your stored None value and a cache miss signified by a return value of None."
I'm not sure if fixing this on only one backend is a good idea. Django 3.0 won't be released until December so we can't wait a bit and hopefully python-memcached will be fixed before then.
It looks like #29867 could also be addressed.
comment:8 by , 6 years ago
I fear that the python-memcached package may be abandoned, as it looks like there are many issues that have been reported but are not addressed. Anyway, right now I see a bunch of options we could go for:
1) Wait for python-memcached to accept my PR (or wait if somebody will take over maintenance of this package) and make an unified change to BaseMemcachedCache
class (although this will introduce a change that's breaking to all users who have older python-memcached
package installed)
2) Introduce only the change to PyLibMCCache
and change the documentation to only mention the None
inconsistency at MemcachedCache
(because that will be the only core backend that's going to have this issue, more on that later)
3) Remove support for python-memcached
alltogether and remove the None
inconsistency mention from the docs entirely.
As for backends, I tested the current 2.1.7 Django release against the None
and default
keyword param issue and it looks like LocMemCache
, FileBasedCache
and DatabaseCache
all support None
+ default
properly. Only the Memcached-based backends are the ones that are not.
Personally I think that even introducing this change to one backend for now is good, as you'll have 1/5 backends not working correctly instead of 2/5, which is an improvement IMO, but this is up to you.
comment:9 by , 6 years ago
Patch needs improvement: | unset |
---|---|
Summary: | Allow memcached backends to cache None → Fix cache.get() with default on PyLibMCCache if None is cached |
Triage Stage: | Accepted → Ready for checkin |
PR that fixes this ticket: PR