[Django] #30181: Django's memcached default is returned if None is saved in cache
Django
-------------------------------------+-------------------------------------
Reporter: Jakub | Owner: nobody
Szafrański |
Type: Bug | Status: new
Component: | Version: 2.1
Uncategorized |
Severity: Normal | Keywords: memcached none
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/30181>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
| Szafrański
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Resolution:
Keywords: memcached none | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Jakub Szafrański):
* status: new => assigned
* cc: Jakub Szafrański (added)
* component: Uncategorized => Core (Cache system)
* owner: nobody => Jakub Szafrański
* easy: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:1>
Django
-------------------------------------+-------------------------------------
| Szafrański
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Jakub Szafrański):
PR that fixes this ticket: https://github.com/django/django/pull/10985
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:2>
Django
-------------------------------------+-------------------------------------
| Szafrański
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Jakub Szafrański):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:3>
Django
-------------------------------------+-------------------------------------
| Szafrański
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Jakub Szafrański):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:4>
Django
-------------------------------------+-------------------------------------
| Szafrański
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* easy: 1 => 0
* needs_tests: 0 => 1
Comment:
A test is required.
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:5>
Django
-------------------------------------+-------------------------------------
| Szafrański
Type: Bug | Status: assigned
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jakub Szafrański):
Test has been added
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Szafrański | Owner: Jakub
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* type: Bug => Cleanup/optimization
* needs_tests: 1 => 0
Comment:
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
[https://github.com/linsomniac/python-memcached/pull/158/files python-
memcached] will be fixed before then.
It looks like #29867 could also be addressed.
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Szafrański | Owner: Jakub
Type: | Szafrański
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jakub Szafrański):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Szafrański | Owner: Jakub
Type: | Szafrański
Cleanup/optimization | Status: assigned
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Szafrański | Owner: Jakub
Type: | Szafrański
Component: Core (Cache system) | Version: 2.1
Keywords: memcached none | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0104b5a41704430aaa1067da2281a86a83c8543a" 0104b5a]:
{{{
#!CommitTicketReference repository=""
revision="0104b5a41704430aaa1067da2281a86a83c8543a"
Fixed #30181 -- Made cache.get() with default work correctly on
PyLibMCCache if None is cached.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Szafrański | Owner: Jakub
Type: | Szafrański
Cleanup/optimization | Status: closed
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Resolution: fixed
Keywords: memcached none | 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:"d23dad5778b3610a5f870b4757ba628780924dd1" d23dad57]:
{{{
#!CommitTicketReference repository=""
revision="d23dad5778b3610a5f870b4757ba628780924dd1"
Refs #30181 -- Corrected note about storing None in the cache.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Szafrański | Owner: Jakub
Type: | Szafrański
Cleanup/optimization | Status: closed
Component: Core (Cache system) | Version: 2.1
Severity: Normal | Resolution: fixed
Keywords: memcached none | 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:"b044b417d22f8fc341fd6d2c4e1613ec3d7cb01c" b044b41]:
{{{
#!CommitTicketReference repository=""
revision="b044b417d22f8fc341fd6d2c4e1613ec3d7cb01c"
[3.1.x] Refs #30181 -- Corrected note about storing None in the cache.
Backport of d23dad5778b3610a5f870b4757ba628780924dd1 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30181#comment:12>