Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#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 Jakub Szafrański, 6 years ago

Cc: Jakub Szafrański added
Component: UncategorizedCore (Cache system)
Easy pickings: set
Owner: changed from nobody to Jakub Szafrański
Status: newassigned

comment:2 by Jakub Szafrański, 6 years ago

PR that fixes this ticket: PR

Last edited 6 years ago by Jakub Szafrański (previous) (diff)

comment:3 by Jakub Szafrański, 6 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Jakub Szafrański, 6 years ago

Has patch: set

comment:5 by Tim Graham, 6 years ago

Easy pickings: unset
Needs tests: set

A test is required.

comment:6 by Jakub Szafrański, 6 years ago

Test has been added

comment:7 by Tim Graham, 6 years ago

Needs tests: unset
Patch needs improvement: set
Summary: Django's memcached default is returned if None is saved in cacheAllow memcached backends to cache None
Type: BugCleanup/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 Jakub Szafrański, 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 Tim Graham, 6 years ago

Patch needs improvement: unset
Summary: Allow memcached backends to cache NoneFix cache.get() with default on PyLibMCCache if None is cached
Triage Stage: AcceptedReady for checkin

comment:10 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In 0104b5a:

Fixed #30181 -- Made cache.get() with default work correctly on PyLibMCCache if None is cached.

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In d23dad57:

Refs #30181 -- Corrected note about storing None in the cache.

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In b044b41:

[3.1.x] Refs #30181 -- Corrected note about storing None in the cache.

Backport of d23dad5778b3610a5f870b4757ba628780924dd1 from master

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