Skip to content

Loading…

Fixed #22085 - Added a feature that allows the Cache settings argument to be None, for "no timeout". #2365

Closed
wants to merge 9 commits into from

2 participants

@zedr

Django 1.6 introduced a feature that allows the passing of None as the timeout argument when setting a cache key, using the Django caching framework. This is interpreted as "no timeout", so the cache object will set a non-expirable key as the default, unless a different value for the timeout is passed.

However, if the TIMEOUT was set in the settings file, the default timeout was still 300 seconds.

Ticket #22085 was opened to address this, and was accepted as a new feature.

This fix implements the feature, and adds four new tests specific to this case.

To set a never-expiring timeout for cache keys as the default behaviour, simply define the CACHES setting in this way:

CACHES = {
    "default": {
        # define the backend here...

        # set the timeout to None, meaning "no timeout".
        TIMEOUT = None
    }
}

Now, by default behavior of the Cache instances will be to set non-expiring keys ("cache forever").

For more information, see ticket: https://code.djangoproject.com/ticket/22085

zedr added some commits
@zedr zedr Fixed #22085 - Add a feature for setting non-expiring keys as the def…
…ault.

This feature allows the default `TIMEOUT` Cache argument to be set to `None`,
so that Cache instances can set a non-expiring key as the default,
instead of using the default value of 5 minutes.

Previously, this was possible only by passing `None` as an argument to
the set() method of objects of type `BaseCache` (and subtypes).
fdb5e0b
@zedr zedr Removed the import of `DEFAULT_CACHE_ALIAS` that redefines a previous…
… import

The GetCacheTests test case import the constant `DEFAULT_CACHE_ALIAS`
inside one of its test methods, redefining a previous import. Removing
this second import does not break the test.
9a1f860
@zedr zedr Enhance fix for #22085: update the cache documentation to mention the…
… possibility of using as the default timeout
d57b14e
@zedr zedr Enhance fix for #22085: add a paragraph to the release documentation …
…for 1.7, describing the new feature.
8cfb732
@zedr

I've updated the pull request with the relative items for the documentation and the release notes.

@bmispelon bmispelon commented on an outdated diff
docs/releases/1.7.txt
@@ -437,6 +437,12 @@ Cache
thread-safe any more, as :data:`django.core.cache.caches` now yields
different instances per thread.
+* Defining the :setting:`TIMEOUT` argument of the CACHES setting as ``None``
@bmispelon Django member

Use :setting:`CACHES`, but simply ``TIMEOUT`` (the :setting: thingy makes Django autolink the the reference docs).

@bmispelon Django member

My bad, you can actually use :setting:`TIMEOUT <CACHES-TIMEOUT>`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@zedr

I made the suggested modifications.

@bmispelon bmispelon commented on the diff
docs/topics/cache.txt
((5 lines not shown))
* :setting:`TIMEOUT <CACHES-TIMEOUT>`: The default timeout, in
- seconds, to use for the cache. This argument defaults to ``300``
- seconds (5 minutes).
+ seconds, to use for the cache. This argument defaults to ``300`` seconds (5 minutes).
+
+ .. versionchanged:: 1.7
@bmispelon Django member

I think versionadded might be more appropriate here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bmispelon bmispelon commented on the diff
docs/topics/cache.txt
@@ -363,9 +363,13 @@ Each cache backend can be given additional arguments to control caching
behavior. These arguments are provided as additional keys in the
:setting:`CACHES` setting. Valid arguments are as follows:
+
@bmispelon Django member

I don't think the extra newline is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bmispelon bmispelon commented on the diff
docs/releases/1.7.txt
@@ -437,6 +437,12 @@ Cache
thread-safe any more, as :data:`django.core.cache.caches` now yields
different instances per thread.
+* Defining the :setting:`TIMEOUT <CACHES-TIMEOUT>` argument of the CACHES setting as ``None``
@bmispelon Django member

Can you link CACHES to the appropriate :setting: too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bmispelon bmispelon commented on an outdated diff
docs/releases/1.7.txt
@@ -437,6 +437,12 @@ Cache
thread-safe any more, as :data:`django.core.cache.caches` now yields
different instances per thread.
+* Defining the :setting:`TIMEOUT <CACHES-TIMEOUT>` argument of the CACHES setting as ``None``
+ will set the cache keys as "non-expiring" by default. Previously, it was
+ still being set to 300 seconds, although explicitly passing ``None`` to the
+ ```set()``` method of a subtype of
+ :class:`django.core.cache.backends.base.BaseCache` works as intended.
@bmispelon Django member

Is that link working when you build the docs? I can't find the reference documentation for caches so you might need to just use double backticks (``django.core.cache.backends.base.BaseCache``).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bmispelon
Django member

The patch applies, but there's a warning about trailing whitespace:

$ git apply 2365.patch
2365.patch:316: trailing whitespace.
  If the ``TIMEOUT`` is set to ``None``, the cache keys will never expire. 
warning: 1 line adds whitespace errors.
@zedr

I have removed the trailing whitespace character.

Regarding the link to the django.core.cache.backends.base.BaseCache: the syntax is correct, but I assume the link isn't built because it the association with the documentation for the class isn't made anywhere by Sphinx.

Any advice on this?

@bmispelon
Django member

Yes, the syntax is correct but as you say, the link is broken because BaseCache is not documented anywhere in our documentation.

I see two ways you can fix this:

1) Add reference documentation for BaseCache
2) Don't use a link here, but a simple double-backtick quoting.

While option 1) might be nice, it's probably a lot of work and out of the scope of this ticket so I'd sugest going with option 2 (if you grep the documentation for BaseCache, you'll find that other parts of our documentation do this already).

@zedr

I have changed the formatting to just use the double backticks.

@bmispelon
Django member

I squashed everything together, tweaked the docs a bit and merged it in 6fe22b3.

Thanks!

@bmispelon bmispelon closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 23, 2014
  1. @zedr

    Fixed #22085 - Add a feature for setting non-expiring keys as the def…

    zedr committed
    …ault.
    
    This feature allows the default `TIMEOUT` Cache argument to be set to `None`,
    so that Cache instances can set a non-expiring key as the default,
    instead of using the default value of 5 minutes.
    
    Previously, this was possible only by passing `None` as an argument to
    the set() method of objects of type `BaseCache` (and subtypes).
  2. @zedr

    Removed the import of `DEFAULT_CACHE_ALIAS` that redefines a previous…

    zedr committed
    … import
    
    The GetCacheTests test case import the constant `DEFAULT_CACHE_ALIAS`
    inside one of its test methods, redefining a previous import. Removing
    this second import does not break the test.
Commits on Mar 4, 2014
  1. @zedr

    Enhance fix for #22085: update the cache documentation to mention the…

    zedr committed
    … possibility of using as the default timeout
  2. @zedr

    Enhance fix for #22085: add a paragraph to the release documentation …

    zedr committed
    …for 1.7, describing the new feature.
  3. @zedr
  4. @zedr
  5. @zedr
  6. @zedr

    Amended the fix for #22085 with minor edits, removing the whitespacin…

    zedr committed
    …g and using the correct amount of backticks
  7. @zedr
This page is out of date. Refresh to see the latest.
Showing with 97 additions and 8 deletions.
  1. +5 −4 django/core/cache/backends/base.py
  2. +6 −0 docs/releases/1.7.txt
  3. +6 −2 docs/topics/cache.txt
  4. +80 −2 tests/cache/tests.py
View
9 django/core/cache/backends/base.py
@@ -52,10 +52,11 @@ def get_key_func(key_func):
class BaseCache(object):
def __init__(self, params):
timeout = params.get('timeout', params.get('TIMEOUT', 300))
- try:
- timeout = int(timeout)
- except (ValueError, TypeError):
- timeout = 300
+ if timeout is not None:
+ try:
+ timeout = int(timeout)
+ except (ValueError, TypeError):
+ timeout = 300
self.default_timeout = timeout
options = params.get('OPTIONS', {})
View
6 docs/releases/1.7.txt
@@ -437,6 +437,12 @@ Cache
thread-safe any more, as :data:`django.core.cache.caches` now yields
different instances per thread.
+* Defining the :setting:`TIMEOUT <CACHES-TIMEOUT>` argument of the CACHES setting as ``None``
@bmispelon Django member

Can you link CACHES to the appropriate :setting: too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ will set the cache keys as "non-expiring" by default. Previously, it was
+ still being set to 300 seconds, although explicitly passing ``None`` to the
+ ``set()`` method of a subtype of
+ ``django.core.cache.backends.base.BaseCache`` works as intended.
+
Email
^^^^^
View
8 docs/topics/cache.txt
@@ -363,9 +363,13 @@ Each cache backend can be given additional arguments to control caching
behavior. These arguments are provided as additional keys in the
:setting:`CACHES` setting. Valid arguments are as follows:
+
@bmispelon Django member

I don't think the extra newline is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
* :setting:`TIMEOUT <CACHES-TIMEOUT>`: The default timeout, in
- seconds, to use for the cache. This argument defaults to ``300``
- seconds (5 minutes).
+ seconds, to use for the cache. This argument defaults to ``300`` seconds (5 minutes).
+
+ .. versionchanged:: 1.7
@bmispelon Django member

I think versionadded might be more appropriate here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ If the ``TIMEOUT`` is set to ``None``, the cache keys will never expire.
* :setting:`OPTIONS <CACHES-OPTIONS>`: Any options that should be
passed to the cache backend. The list of valid options will vary
View
82 tests/cache/tests.py
@@ -6,6 +6,7 @@
import os
import re
+import copy
import shutil
import tempfile
import threading
@@ -15,7 +16,8 @@
from django.conf import settings
from django.core import management
-from django.core.cache import cache, caches, CacheKeyWarning, InvalidCacheBackendError
+from django.core.cache import (cache, caches, CacheKeyWarning,
+ InvalidCacheBackendError, DEFAULT_CACHE_ALIAS)
from django.db import connection, router, transaction
from django.core.cache.utils import make_template_fragment_key
from django.http import HttpResponse, StreamingHttpResponse
@@ -1175,7 +1177,7 @@ def test_custom_key_validation(self):
class GetCacheTests(IgnorePendingDeprecationWarningsMixin, TestCase):
def test_simple(self):
- from django.core.cache import caches, DEFAULT_CACHE_ALIAS, get_cache
+ from django.core.cache import caches, get_cache
self.assertIsInstance(
caches[DEFAULT_CACHE_ALIAS],
get_cache('default').__class__
@@ -1204,6 +1206,82 @@ def test_close_deprecated(self):
self.assertTrue(cache.closed)
+DEFAULT_MEMORY_CACHES_SETTINGS = {
+ 'default': {
+ 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache',
+ 'LOCATION': 'unique-snowflake',
+ }
+}
+NEVER_EXPIRING_CACHES_SETTINGS = copy.deepcopy(DEFAULT_MEMORY_CACHES_SETTINGS)
+NEVER_EXPIRING_CACHES_SETTINGS['default']['TIMEOUT'] = None
+
+
+class DefaultNonExpiringCacheKeyTests(TestCase):
+ """Tests that verify that settings having Cache arguments with a TIMEOUT
+ set to `None` will create Caches that will set non-expiring keys.
+
+ This fixes ticket #22085.
+ """
+ def setUp(self):
+ # The 5 minute (300 seconds) default expiration time for keys is
+ # defined in the implementation of the initializer method of the
+ # BaseCache type.
+ self.DEFAULT_TIMEOUT = caches[DEFAULT_CACHE_ALIAS].default_timeout
+
+ def tearDown(self):
+ del(self.DEFAULT_TIMEOUT)
+
+ def test_default_expiration_time_for_keys_is_5_minutes(self):
+ """The default expiration time of a cache key is 5 minutes.
+
+ This value is defined inside the __init__() method of the
+ :class:`django.core.cache.backends.base.BaseCache` type.
+ """
+ self.assertEquals(300, self.DEFAULT_TIMEOUT)
+
+ def test_caches_with_unset_timeout_has_correct_default_timeout(self):
+ """Caches that have the TIMEOUT parameter undefined in the default
+ settings will use the default 5 minute timeout.
+ """
+ cache = caches[DEFAULT_CACHE_ALIAS]
+ self.assertEquals(self.DEFAULT_TIMEOUT, cache.default_timeout)
+
+ @override_settings(CACHES=NEVER_EXPIRING_CACHES_SETTINGS)
+ def test_caches_set_with_timeout_as_none_has_correct_default_timeout(self):
+ """Memory caches that have the TIMEOUT parameter set to `None` in the
+ default settings with have `None` as the default timeout.
+
+ This means "no timeout".
+ """
+ cache = caches[DEFAULT_CACHE_ALIAS]
+ self.assertIs(None, cache.default_timeout)
+ self.assertEquals(None, cache.get_backend_timeout())
+
+ @override_settings(CACHES=DEFAULT_MEMORY_CACHES_SETTINGS)
+ def test_caches_with_unset_timeout_set_expiring_key(self):
+ """Memory caches that have the TIMEOUT parameter unset will set cache
+ keys having the default 5 minute timeout.
+ """
+ key = "my-key"
+ value = "my-value"
+ cache = caches[DEFAULT_CACHE_ALIAS]
+ cache.set(key, value)
+ cache_key = cache.make_key(key)
+ self.assertNotEquals(None, cache._expire_info[cache_key])
+
+ @override_settings(CACHES=NEVER_EXPIRING_CACHES_SETTINGS)
+ def text_caches_set_with_timeout_as_none_set_non_expiring_key(self):
+ """Memory caches that have the TIMEOUT parameter set to `None` will set
+ a non expiring key by default.
+ """
+ key = "another-key"
+ value = "another-value"
+ cache = caches[DEFAULT_CACHE_ALIAS]
+ cache.set(key, value)
+ cache_key = cache.make_key(key)
+ self.assertEquals(None, cache._expire_info[cache_key])
+
+
@override_settings(
CACHE_MIDDLEWARE_KEY_PREFIX='settingsprefix',
CACHE_MIDDLEWARE_SECONDS=1,
Something went wrong with that request. Please try again.