Page urls are now cached (and cleared when appropriate) so that a cal…

…l to get_absolute_url() isn't as expensive anymore. Fixes issue #25

demoproject/src/odemo/templates/base.html

@@ -17,7 +17,7 @@
         <h1>Ostinato Demo</h1>
 
         <nav>
-            {% navbar path=request.path %}
+            {% navbar %}
         </nav>
 
         {% if page %}

ostinato/pages/forms.py

@@ -16,4 +16,6 @@ def save(self, *args, **kwargs):
 
         page = Page.objects.get(id=page_id)
         target= Page.objects.get(id=target_id)
+        # IMPORTANT: Clear the url cache!
+        Page.objects.clear_url_cache()
         page.move_to(target, position)

ostinato/pages/managers.py

@@ -1,4 +1,5 @@
 from django.db import models
+from django.core.cache import get_cache
 from django.utils import timezone
 
 from mptt.managers import TreeManager
@@ -73,3 +74,13 @@ def get_from_path(self, url_path):
             except:
                 pass
 
+    def generate_url_cache(self):
+        cache = get_cache('default')
+        for page in self.get_query_set().all():
+            page.get_absolute_url()
+
+    def clear_url_cache(self):
+        cache = get_cache('default')
+        page_ids = list(self.get_query_set().values_list('id', flat=True))
+        cache.delete_many(['ostinato:pages:page:%s:url' % i for i in page_ids])
+

ostinato/pages/models.py

@@ -79,7 +79,13 @@ def save(self, *args, **kwargs):
                 self.publish_date = now
 
         self.modified_date = now
-        super(Page, self).save(*args, **kwargs)
+
+        page = super(Page, self).save(*args, **kwargs)
+
+        # Now that the page is saved, we reset and cache the url
+        Page.objects.clear_url_cache()
+
+        return page
 
 
     def get_short_title(self):
@@ -95,11 +101,14 @@ def perma_url(self, data):
         return data
 
 
-    def get_absolute_url(self):
+    def get_absolute_url(self, clear_cache=False):
         """ Cycle through the parents and generate the path """
         cache = get_cache('default')
         cache_key = 'ostinato:pages:page:%s:url' % self.id
 
+        if clear_cache:
+            cache.delete(cache_key)
+
         # Try to get the path from the cache
         url = cache.get(cache_key)
 
@@ -108,15 +117,14 @@ def get_absolute_url(self):
             if self.redirect:
                 return self.redirect
 
-            if self.is_root_node() and self._mpttfield('tree_id') == 1:
-                return reverse('ostinato_page_home')
-
-            path = list(self.get_ancestors().values_list('slug', flat=True))
-            path.append(self.slug)
-
-            url = self.perma_url(('ostinato_page_view', None, {
-                'path': '/'.join(path)
-            }))
+            if self.is_root_node() and self == Page.objects.root_nodes()[0]:
+                url = reverse('ostinato_page_home')
+            else:
+                path = list(self.get_ancestors().values_list('slug', flat=True))
+                path.append(self.slug)
+                url = self.perma_url(('ostinato_page_view', None, {
+                    'path': '/'.join(path)
+                }))
 
             # Set the cache
             cache.set(cache_key, url, 1 * 60 * 60)

ostinato/pages/templatetags/pages_tags.py

@@ -7,31 +7,20 @@
 
 
 @register.inclusion_tag('pages/navbar.html', takes_context=True)
-def navbar(context, for_page=None, path=''):
+def navbar(context, for_page=None):
     """
     Renders a basic navigation bar.
 
     ``for_page`` is used to specify a navbar for a specific
         page (it's children); defaults to root level pages
-
-    ``path`` can be used in special cases where a page might not exist on
-    the current path, but you would like the page to be "discovered" from the
-    url. This will basically mark the active page in the navbar; if it can
-    be found in the path of course.
-
     """
-    if 'page' not in context:
-        for_page = Page.objects.get_from_path(path)
-    else:
-        page = context['page']
+    page = context.get('page', None)
 
-    # if not for_page:
-    #     if 'page' in context:
-    #         for_page = context['page']
-    #     else:
-    #         for_page = Page.objects.get_from_path(path)
-
-    navbar = Page.objects.get_navbar(for_page=for_page)
+    if for_page and not for_page.is_leaf_node():
+        navbar = Page.objects.get_navbar(for_page=for_page)
+    else:
+        # Return root level pages
+        navbar = Page.objects.get_navbar(for_page=None)
 
     return {
         'page': page,

ostinato/pages/tests.py

@@ -5,6 +5,7 @@
 from django.contrib.auth.models import User
 from django.contrib.contenttypes.models import ContentType
 from django.core.urlresolvers import reverse
+from django.core.cache import get_cache
 from django.template import Context, Template
 from django.template.response import SimpleTemplateResponse, TemplateResponse
 from django.utils import simplejson as json
@@ -14,6 +15,7 @@
 from ostinato.pages.registry import page_content
 from ostinato.pages.models import Page, PageContent
 from ostinato.pages.views import PageView, PageReorderView, page_dispatch
+from ostinato.utils import benchmark
 
 
 page_content.setup()  # Clear the registry before we start the tests
@@ -172,6 +174,42 @@ def test_absolute_url(self):
         self.assertEqual('/page-2/', p2.get_absolute_url())
         self.assertEqual('/page-1/page-3/', p3.get_absolute_url())
 
+    def test_absolute_url_is_cached(self):
+        p3 = Page.objects.get(slug='page-3')
+        cache = get_cache('default')
+        cache_key = 'ostinato:pages:page:3:url'
+
+        # First the get_absolute_url should cache the url
+        # Lets make sure that this url wasn't previously cached
+        cache.set(cache_key, None)
+        self.assertEqual('/page-1/page-3/', p3.get_absolute_url())
+        self.assertEqual('/page-1/page-3/', cache.get(cache_key))
+
+    def test_urls_updated_after_move(self):
+        p = Page.objects.get(slug='page-1')
+        p2 = Page.objects.get(slug='page-2')
+        p3 = Page.objects.get(slug='page-3')
+        p3.parent = p2
+        p3.save()
+
+        self.assertEqual('/page-2/page-3/', p3.get_absolute_url())
+
+        # We need to test a couple of other moves also, just to make sure
+        # changes are propagated up the tree
+        # p2.move_to(p, position='first-child')
+        p = Page.objects.get(slug='page-1')
+        p2 = Page.objects.get(slug='page-2')
+        p3 = Page.objects.get(slug='page-3')
+        p2.parent = p
+        p2.save()
+
+        # Get fresh variables so that mptt does the right thing
+        p2 = Page.objects.get(slug='page-2')
+        self.assertEqual('/page-1/page-2/', p2.get_absolute_url())
+
+        p3 = Page.objects.get(slug='page-3')
+        self.assertEqual('/page-1/page-2/page-3/', p3.get_absolute_url())
+
     def test_absolute_url_based_on_location(self):
         p = Page.objects.get(slug='page-1')
         p4 = Page.objects.create(
@@ -254,6 +292,32 @@ def test_get_page_from_path(self):
         self.assertEqual('page-3',
             Page.objects.get_from_path(request.path).slug)
 
+    def test_generate_url_cache(self):
+        cache = get_cache('default')
+        cache_url = lambda id: cache.get('ostinato:pages:page:%s:url' % id)
+
+        # Make sure the url cache is empty
+        for i in range(3):
+            cache.set('ostinato:pages:page:%s:url' % i, None)
+
+        Page.objects.generate_url_cache()
+
+        self.assertEqual('/', cache_url(1))
+        self.assertEqual('/page-2/', cache_url(2))
+        self.assertEqual('/page-1/page-3/', cache_url(3))
+
+
+    def test_clear_url_cache(self):
+        cache = get_cache('default')
+        cache_url = lambda id: cache.get('ostinato:pages:page:%s:url' % id)
+
+        Page.objects.generate_url_cache()  # Make sure there is a cache
+        Page.objects.clear_url_cache()
+
+        self.assertEqual(None, cache_url(1))
+        self.assertEqual(None, cache_url(2))
+        self.assertEqual(None, cache_url(3))
+
 
 class PageContentModelTestCase(TestCase):