Permalink
Browse files

Merge pull request #2735 from mkoistinen/issue_2729

Issue 2729 - Ready for review/merge
  • Loading branch information...
2 parents aebd206 + 36e742c commit 1f2700803e8da1d69216494965d258030f25c427 @digi604 digi604 committed Feb 27, 2014
Showing with 206 additions and 13 deletions.
  1. +1 −2 cms/admin/pageadmin.py
  2. +25 −1 cms/models/pagemodel.py
  3. +7 −0 cms/signals/apphook.py
  4. +84 −5 cms/tests/cache.py
  5. +89 −5 cms/views.py
@@ -1160,8 +1160,7 @@ def change_innavigation(self, request, page_id):
"""
page = get_object_or_404(Page, pk=page_id)
if page.has_change_permission(request):
- page.in_navigation = not page.in_navigation
- page.save()
+ page.toggle_in_navigation()
return admin_utils.render_admin_menu_item(request, page)
return HttpResponseForbidden(force_unicode(_("You do not have permission to change this page's in_navigation status")))
@@ -4,7 +4,7 @@
from os.path import join
from cms import constants
from cms.constants import PUBLISHER_STATE_DEFAULT, PUBLISHER_STATE_PENDING, PUBLISHER_STATE_DIRTY, TEMPLATE_INHERITANCE_MAGIC
-from cms.exceptions import PublicIsUnmodifiable, LanguageError
+from cms.exceptions import PublicIsUnmodifiable, LanguageError, PublicVersionNeeded
from cms.models.managers import PageManager, PagePermissionsPermissionManager
from cms.models.metaclasses import PageMetaClass
from cms.models.placeholdermodel import Placeholder
@@ -193,6 +193,9 @@ def move_page(self, target, position='first-child'):
public_page.save()
page_utils.check_title_slugs(public_page)
+ from cms.views import invalidate_cms_page_cache
+ invalidate_cms_page_cache()
+
def _copy_titles(self, target, language, published):
"""
Copy all the titles to a new page (which must have a pk).
@@ -478,6 +481,19 @@ def is_published(self, language, force_reload=False):
except Title.DoesNotExist:
return False
+ def toggle_in_navigation(self, set_to=None):
+ '''
+ Toggles (or sets) in_navigation and invalidates the cms page cache
+ '''
+ if set_to in [True, False]:
+ self.in_navigation = set_to
+ else:
+ self.in_navigation = not self.in_navigation
+ self.save()
+ from cms.views import invalidate_cms_page_cache
+ invalidate_cms_page_cache()
+ return self.in_navigation
+
def get_publisher_state(self, language, force_reload=False):
from cms.models import Title
@@ -601,6 +617,9 @@ def publish(self, language):
cms_signals.post_publish.send(sender=Page, instance=self, language=language)
+ from cms.views import invalidate_cms_page_cache
+ invalidate_cms_page_cache()
+
return published
def unpublish(self, language):
@@ -631,8 +650,13 @@ def unpublish(self, language):
# trigger update home
self.save()
self.mark_descendants_pending(language)
+
+ from cms.views import invalidate_cms_page_cache
+ invalidate_cms_page_cache()
+
from cms.signals import post_unpublish
post_unpublish.send(sender=Page, instance=self, language=language)
+
return True
def mark_descendants_pending(self, language):
@@ -26,6 +26,9 @@ def apphook_post_page_checker(page):
if (old_page and (
old_page.application_urls != page.application_urls or old_page.application_namespace != page.application_namespace)) or (
not old_page and page.application_urls):
+
+ from cms.views import invalidate_cms_page_cache
+ invalidate_cms_page_cache()
request_finished.connect(trigger_restart, dispatch_uid=DISPATCH_UID)
@@ -53,6 +56,8 @@ def apphook_post_delete_title_checker(instance, **kwargs):
Check if this was an apphook
"""
if instance.page.application_urls:
+ from cms.views import invalidate_cms_page_cache
+ invalidate_cms_page_cache()
request_finished.connect(trigger_restart, dispatch_uid=DISPATCH_UID)
@@ -61,6 +66,8 @@ def apphook_post_delete_page_checker(instance, **kwargs):
Check if this was an apphook
"""
if instance.application_urls:
+ from cms.views import invalidate_cms_page_cache
+ invalidate_cms_page_cache()
request_finished.connect(trigger_restart, dispatch_uid=DISPATCH_UID)
# import the logging library
View
@@ -96,14 +96,93 @@ def test_no_cache_plugin(self):
plugin_pool.unregister_plugin(NoCachePlugin)
+
def test_cache_page(self):
- page1 = create_page('test page 1', 'nav_playground.html', 'en',
- published=True)
+ from cms.views import _get_cache_version
+ from cms.utils import get_cms_setting
+ from django.conf import settings
+
+ # We'll store the old MW so that we can play nice with the other tests
+ old_middleware = settings.MIDDLEWARE_CLASSES[:]
+
+ # Clear the entire cache for a clean slate
+ cache.clear()
+
+ # Ensure that we're testing in an environment WITHOUT the MW cache...
+ exclude = [
+ 'django.middleware.cache.UpdateCacheMiddleware',
+ 'django.middleware.cache.FetchFromCacheMiddleware'
+ ]
+ settings.MIDDLEWARE_CLASSES[:] = [mw for mw in settings.MIDDLEWARE_CLASSES if mw not in exclude]
+
+ # Silly to do these tests if this setting isn't True
+ page_cache_setting = get_cms_setting('PAGE_CACHE')
+ self.assertTrue(page_cache_setting)
+
+ # Create a test page
+ page1 = create_page('test page 1', 'nav_playground.html', 'en', published=True)
+
+ # Add some content
placeholder = page1.placeholders.filter(slot="body")[0]
add_plugin(placeholder, "TextPlugin", 'en', body="English")
add_plugin(placeholder, "TextPlugin", 'de', body="Deutsch")
- with self.assertNumQueries(FuzzyInt(10,20)):
- self.client.get('/en/')
+
+ # Create a request object
+ request = self.get_request(page1.get_path(), 'en')
+
+ # Ensure that user is NOT authenticated
+ self.assertFalse(request.user.is_authenticated())
+
+ # Test that the page is initially uncached
+ with self.assertNumQueries(FuzzyInt(1, 20)):
+ response = self.client.get('/en/')
+ self.assertEqual(response.status_code, 200)
+
+ #
+ # Test that subsequent requests of the same page are cached by
+ # asserting that they require fewer queries.
+ #
with self.assertNumQueries(0):
- self.client.get('/en/')
+ response = self.client.get('/en/')
+ self.assertEqual(response.status_code, 200)
+
+ #
+ # Test that the cache is invalidated on unpublishing the page
+ #
+ old_version = _get_cache_version()
+ page1.unpublish('en')
+ self.assertGreater(_get_cache_version(), old_version)
+
+ #
+ # Test that this means the page is actually not cached.
+ #
+ page1.publish('en')
+ with self.assertNumQueries(FuzzyInt(1, 20)):
+ response = self.client.get('/en/')
+ self.assertEqual(response.status_code, 200)
+
+ #
+ # Test that the above behavior is different when CMS_PAGE_CACHE is
+ # set to False (disabled)
+ #
+ cache.clear()
+ settings.CMS_PAGE_CACHE = False
+
+ # Test that the page is initially uncached
+ with self.assertNumQueries(FuzzyInt(1, 20)):
+ response = self.client.get('/en/')
+ self.assertEqual(response.status_code, 200)
+
+ #
+ # Test that subsequent requests of the same page are still requires DB
+ # access.
+ #
+ with self.assertNumQueries(FuzzyInt(1, 20)):
+ response = self.client.get('/en/')
+ self.assertEqual(response.status_code, 200)
+
+ #
+ # Let's reset the original middleware for the remaining tests...
+ #
+ settings.MIDDLEWARE_CLASSES = old_middleware[:]
View
@@ -22,6 +22,8 @@
from django.utils.timezone import get_current_timezone_name
+CMS_PAGE_CACHE_VERSION_KEY = 'CMS_PAGE_CACHE_VERSION'
+
def _handle_no_page(request, slug):
if not slug and settings.DEBUG:
return TemplateResponse(request, "cms/welcome.html", RequestContext(request))
@@ -35,9 +37,17 @@ def details(request, slug):
"""
from django.core.cache import cache
- if get_cms_setting("PAGE_CACHE") and (not hasattr(request, 'toolbar') or (
- not request.toolbar.edit_mode and not request.toolbar.show_toolbar and not request.user.is_authenticated())):
- cache_content = cache.get(_get_cache_key(request))
+ if get_cms_setting("PAGE_CACHE") and (
+ not hasattr(request, 'toolbar') or (
+ not request.toolbar.edit_mode and
+ not request.toolbar.show_toolbar and
+ not request.user.is_authenticated()
+ )
+ ):
+ cache_content = cache.get(
+ _get_cache_key(request),
+ version=_get_cache_version()
+ )
if not cache_content is None:
content, headers = cache_content
response = HttpResponse(content)
@@ -209,9 +219,22 @@ def _cache_page(response):
if not save_cache:
response
if save_cache:
- cache.set(_get_cache_key(request), (response.content, response._headers),
- get_cms_setting('CACHE_DURATIONS')['content'])
+ version = _get_cache_version()
+ ttl = get_cms_setting('CACHE_DURATIONS')['content']
+ cache.set(
+ _get_cache_key(request),
+ (response.content, response._headers),
+ ttl,
+ version=version
+ )
+ # See note in invalidate_cms_page_cache()
+ cache.set(
+ CMS_PAGE_CACHE_VERSION_KEY,
+ version,
+ ttl
+ )
+
def _get_cache_key(request):
#md5 key of current path
@@ -227,3 +250,64 @@ def _get_cache_key(request):
tz_name = force_text(get_current_timezone_name(), errors='ignore')
cache_key += '.%s' % tz_name.encode('ascii', 'ignore').decode('ascii').replace(' ', '_')
return cache_key
+
+def _get_cache_version():
+ from django.core.cache import cache
+
+ '''
+ Returns the current page cache version, explicitly setting one if not
+ defined.
+ '''
+
+ version = cache.get(CMS_PAGE_CACHE_VERSION_KEY)
+
+ if version:
+ return version
+ else:
+ cache.set(
+ CMS_PAGE_CACHE_VERSION_KEY,
+ 1,
+ get_cms_setting('CACHE_DURATIONS')['content']
+ )
+ return 1
+
+
+def invalidate_cms_page_cache():
+ from django.core.cache import cache
+
+ '''
+ Invalidates the CMS PAGE CACHE.
+ '''
+
+ #
+ # NOTE: We're using a cache versioning strategy for invalidating the page
+ # cache when necessary. Instead of wiping all the old entries, we simply
+ # increment the version number rendering all previous entries
+ # inaccessible and left to expire naturally.
+ #
+ # ALSO NOTE: According to the Django documentation, a timeout value of
+ # `None' (in version 1.6+) is supposed to mean "cache forever", however,
+ # this is actually only implemented as only slightly less than 30 days in
+ # some backends (memcached, in particular). In older Djangos, `None' means
+ # "use default value". To avoid issues arising from different Django
+ # versions and cache backend implementations, we will explicitly set the
+ # lifespan of the CMS_PAGE_CACHE_VERSION entry to whatever is set in
+ # settings.CACHE_DURATIONS['content']. This allows users to adjust as
+ # necessary for their backend.
+ #
+ # To prevent writing cache entries that will live longer than our version
+ # key, we will always re-write the current version number into the cache
+ # just after we write any new cache entries, thus ensuring that the
+ # version number will always outlive any entries written against that
+ # version. This is a cheap operation.
+ #
+ # If there are no new cache writes before the version key expires, its
+ # perfectly OK, since any previous entries cached against that version
+ # will have also expired, so, it'd be pointless to try to access them
+ # anyway.
+ #
+ try:
+ cache.incr(CMS_PAGE_CACHE_VERSION_KEY)
+ except ValueError:
+ # Key doesn't exist, so just set it to the default
+ cache.set(CMS_PAGE_CACHE_VERSION_KEY, 1, get_cms_setting('CACHE_DURATIONS')['content'])

0 comments on commit 1f27008

Please sign in to comment.