general: keep cache of cell types used in a page (#24239)

combo/data/migrations/0035_page_related_cells.py

@@ -0,0 +1,21 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.12 on 2018-06-02 11:03
+from __future__ import unicode_literals
+
+from django.db import migrations
+import jsonfield.fields
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('data', '0034_redirect'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='page',
+            name='related_cells',
+            field=jsonfield.fields.JSONField(blank=True, default=dict),
+        ),
+    ]

combo/data/models.py

@@ -34,7 +34,7 @@ from django.core.exceptions import ObjectDoesNotExist, ValidationError, Permissi
 from django.core import serializers
 from django.db import models, transaction
 from django.db.models.base import ModelBase
-from django.db.models.signals import pre_save
+from django.db.models.signals import pre_save, post_save, post_delete
 from django.db.models import Max
 from django.dispatch import receiver
 from django.forms import models as model_forms
@@ -141,6 +141,9 @@ class Page(models.Model):
     snapshot = models.ForeignKey('PageSnapshot', on_delete=models.CASCADE, null=True,
             related_name='temporary_page')
 
+    # keep a cached list of cell types that are used in the pages.
+    related_cells = JSONField(blank=True)
+
     _level = None
     _children = None
 
@@ -154,6 +157,8 @@ class Page(models.Model):
         return (self.get_online_url().strip('/'), )
 
     def save(self, *args, **kwargs):
+        if not self.id:
+            self.related_cells = {'cell_types': []}
         if not self.order:
             max_order = Page.objects.all().aggregate(Max('order')).get('order__max') or 0
             self.order = max_order + 1
@@ -295,7 +300,16 @@ class Page(models.Model):
         return element_is_visible(self, user=user)
 
     def get_cells(self):
-        return CellBase.get_cells(page_id=self.id)
+        return CellBase.get_cells(page=self)
+
+    def build_cell_cache(self):
+        cells = CellBase.get_cells(page=self, skip_cell_cache=True)
+        cell_types = set()
+        for cell in cells:
+            cell_types.add(cell.get_cell_type_str())
+        if cell_types != set(self.related_cells.get('cell_types', [])):
+            self.related_cells['cell_types'] = list(cell_types)
+            self.save()
 
     def get_serialized_page(self):
         cells = [x for x in self.get_cells() if x.placeholder and not x.placeholder.startswith('_')]
@@ -304,6 +318,8 @@ class Page(models.Model):
         del serialized_page['model']
         if 'snapshot' in serialized_page:
             del serialized_page['snapshot']
+        if 'related_cells' in serialized_page['fields']:
+            del serialized_page['fields']['related_cells']
         serialized_page['cells'] = json.loads(serializers.serialize('json',
             cells, use_natural_foreign_keys=True, use_natural_primary_keys=True))
         serialized_page['fields']['groups'] = [x[0] for x in serialized_page['fields']['groups']]
@@ -499,10 +515,27 @@ class CellBase(models.Model):
         return cell_types
 
     @classmethod
-    def get_cells(cls, cell_filter=None, **kwargs):
+    def get_cells(cls, cell_filter=None, skip_cell_cache=False, **kwargs):
         """Returns the list of cells of various classes matching **kwargs"""
         cells = []
-        for klass in get_cell_classes():
+        pages = []
+        if 'page' in kwargs:
+            pages = [kwargs['page']]
+        elif 'page__in' in kwargs:
+            pages = kwargs['page__in']
+        cell_classes = get_cell_classes()
+        if pages and not skip_cell_cache:
+            # if there's a request for some specific pages, limit cell types
+            # to those that are actually in use in those pages.
+            cell_types = set()
+            for page in pages:
+                if page.related_cells and 'cell_types' in page.related_cells:
+                    cell_types |= set(page.related_cells['cell_types'])
+                else:
+                    break
+            else:
+                cell_classes = [get_cell_class(x) for x in cell_types]
+        for klass in cell_classes:
             if cell_filter and not cell_filter(klass):
                 continue
             cells.extend(klass.objects.filter(**kwargs))
@@ -935,26 +968,26 @@ class ParentContentCell(CellBase):
 
     def get_parents_cells(self, hierarchy):
         try:
-            page_ids = [Page.objects.get(slug='index', parent=None).id]
+            pages = [Page.objects.get(slug='index', parent=None)]
         except Page.DoesNotExist:
-            page_ids = []
+            pages = []
-        page_ids.extend([x.id for x in hierarchy])
+        pages.extend(hierarchy)
-        if not page_ids:
+        if not pages:
             return []
-        if len(page_ids) > 1 and page_ids[0] == page_ids[1]:
+        if len(pages) > 1 and pages[0].id == pages[1].id:
             # don't duplicate index cells for real children of the index page.
-            page_ids = page_ids[1:]
+            pages = pages[1:]
         cells_by_page = {}
-        for page_id in page_ids:
+        for page in pages:
-            cells_by_page[page_id] = []
+            cells_by_page[page.id] = []
-        for cell in list(CellBase.get_cells(placeholder=self.placeholder, page__in=page_ids)):
+        for cell in list(CellBase.get_cells(placeholder=self.placeholder, page__in=pages)):
             cells_by_page[cell.page_id].append(cell)
 
-        cells = cells_by_page[page_ids[-1]]
+        cells = cells_by_page[pages[-1].id]
-        for page_id in reversed(page_ids[:-1]):
+        for page in reversed(pages[:-1]):
             for i, cell in enumerate(cells):
                 if isinstance(cell, ParentContentCell):
-                    cells[i:i+1] = cells_by_page[page_id]
+                    cells[i:i+1] = cells_by_page[page.id]
                     break
             else:
                 # no more ParentContentCell, stop folloing the parent page chain
@@ -1302,3 +1335,13 @@ def create_redirects(sender, instance, raw, **kwargs):
         affected_pages.extend(level_pages)
     for page in affected_pages:
         Redirect(page=page, old_url=page.get_online_url()).save()
+
+@receiver(post_save)
+@receiver(post_delete)
+def cell_maintain_page_cell_cache(sender, instance=None, **kwargs):
+    if not issubclass(sender, CellBase):
+        return
+    if not instance.page_id:
+        return
+    page = instance.page
+    page.build_cell_cache()

combo/public/views.py

@@ -239,7 +239,7 @@ def skeleton(request):
             if placeholder.acquired:
                 cells.append(ParentContentCell(page=selected_page, placeholder=placeholder.key, order=0))
     else:
-        cells = CellBase.get_cells(page_id=selected_page.id)
+        cells = CellBase.get_cells(page=selected_page)
 
     pages = selected_page.get_parents_and_self()
     combo_template = settings.COMBO_PUBLIC_TEMPLATES[selected_page.template_name]
@@ -380,7 +380,7 @@ def publish_page(request, page, status=200, template_name=None):
     if page.redirect_url:
         return HttpResponseRedirect(page.get_redirect_url())
 
-    cells = CellBase.get_cells(page_id=page.id)
+    cells = CellBase.get_cells(page=page)
     extend_with_parent_cells(cells, hierarchy=pages)
     cells = [x for x in cells if x.is_visible(user=request.user)]
 

tests/test_cells.py

@@ -6,13 +6,16 @@ import requests
 
 from combo.data.models import Page, CellBase, TextCell, LinkCell, JsonCellBase, JsonCell, ConfigJsonCell
 from django.conf import settings
+from django.db import connection
 from django.forms.widgets import Media
 from django.template import Context
 from django.test import override_settings
 from django.test.client import RequestFactory
+from django.test.utils import CaptureQueriesContext
 from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
 
+from combo.data.library import get_cell_classes
 from combo.utils import NothingInCacheException
 
 from test_manager import admin_user, login
@@ -627,3 +630,44 @@ def test_page_cell_placeholder_restricted_visibility(app, admin_user):
 
     assert "<p>Public text</p>" not in resp.content
     assert "<p>Private text</p>" in resp.content
+
+def test_related_cell_types_tracking():
+    page = Page(title='example page', slug='example-page')
+    page.save()
+    assert page.related_cells['cell_types'] == []
+
+    TextCell(page=page, placeholder='content', order=0, text='hello').save()
+    assert Page.objects.get(id=page.id).related_cells['cell_types'] == ['data_textcell']
+
+    TextCell(page=page, placeholder='content', order=1, text='hello').save()
+    assert Page.objects.get(id=page.id).related_cells['cell_types'] == ['data_textcell']
+
+    LinkCell(page=page, placeholder='content', order=0, title='Test', url='http://example.net').save()
+    assert set(Page.objects.get(id=page.id).related_cells['cell_types']) == set(['data_textcell', 'data_linkcell'])
+
+    with CaptureQueriesContext(connection) as ctx:
+        assert len(CellBase.get_cells(page=Page.objects.get(id=page.id))) == 3
+        assert len(ctx.captured_queries) == 1 + 2
+
+    TextCell.objects.get(order=1).delete()
+    assert set(Page.objects.get(id=page.id).related_cells['cell_types']) == set(['data_textcell', 'data_linkcell'])
+
+    TextCell.objects.get(order=0).delete()
+    assert set(Page.objects.get(id=page.id).related_cells['cell_types']) == set(['data_linkcell'])
+
+    with CaptureQueriesContext(connection) as ctx:
+        assert len(CellBase.get_cells(page=Page.objects.get(id=page.id))) == 1
+        assert len(ctx.captured_queries) == 1 + 1
+
+    # remove tracker, check it is rebuilt correctly
+    page.related_cells = {}
+    page.save()
+
+    with CaptureQueriesContext(connection) as ctx:
+        assert len(CellBase.get_cells(page=Page.objects.get(id=page.id))) == 1
+        assert len(ctx.captured_queries) == len(get_cell_classes())
+
+    Page.objects.get(id=page.id).get_cells()
+
+    TextCell(page=page, placeholder='content', order=0, text='hello').save()
+    assert set(Page.objects.get(id=page.id).related_cells['cell_types']) == set(['data_textcell', 'data_linkcell'])

tests/test_fargo.py

@@ -28,7 +28,7 @@ def test_manager_fargo_cell_setup(app, admin_user):
         resp = app.get(resp.html.find('option',
             **{'data-add-url': re.compile('recentdoc')})['data-add-url'])
 
-        cells = page.get_cells()
+        cells = Page.objects.get(id=page.id).get_cells()
         assert len(cells) == 1
         assert isinstance(cells[0], RecentDocumentsCell)
         resp = app.get('/manage/pages/%s/' % page.id)
@@ -42,10 +42,10 @@ def test_manager_fargo_cell_setup(app, admin_user):
                         'backoffice-menu-url': 'http://example.org/manage/',},
                      }}):
         resp = app.get('/manage/pages/%s/' % page.id)
-        cells = page.get_cells()
+        cells = Page.objects.get(id=page.id).get_cells()
         resp.forms[0]['c%s-fargo_site' % cells[0].get_reference()].value = 'second'
         resp = resp.forms[0].submit()
-        cells = page.get_cells()
+        cells = Page.objects.get(id=page.id).get_cells()
         assert cells[0].fargo_site == 'second'
 
 

tests/test_wcs.py

@@ -271,8 +271,9 @@ def test_form_cell_load():
     cell.save()
     site_export = [page.get_serialized_page()]
     cell.delete()
-    assert not page.get_cells()
+    assert not Page.objects.get(id=page.id).get_cells()
     Page.load_serialized_pages(site_export)
+    page = Page.objects.get(slug='test_form_cell_save_cache')
     cells = page.get_cells()
     assert len(cells) == 1
     cell = cells[0]
@@ -518,7 +519,7 @@ def test_manager_forms_of_category_cell(app, admin_user):
     resp = app.get(resp.html.find('option',
                    **{'data-add-url': re.compile('wcsformsofcategorycell')})['data-add-url'])
 
-    cells = page.get_cells()
+    cells = Page.objects.get(id=page.id).get_cells()
     assert len(cells) == 1
     assert isinstance(cells[0], WcsFormsOfCategoryCell)
 
@@ -538,7 +539,7 @@ def test_manager_current_forms(app, admin_user):
     resp = app.get(resp.html.find('option',
                    **{'data-add-url': re.compile('wcscurrentformscell')})['data-add-url'])
 
-    cells = page.get_cells()
+    cells = Page.objects.get(id=page.id).get_cells()
     assert len(cells) == 1
     assert isinstance(cells[0], WcsCurrentFormsCell)