From bd36438545c2bcfbbd9f8d09b8ac38e96e05210d Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Fri, 17 Oct 2014 16:46:46 +0100 Subject: [PATCH 1/2] Added system check for ForeignKey cascade Pages can be linked to other Pages, Images, etc through ForeignKeys. If the object being referenced by a page gets deleted, the page itself will be deleted as well. Also, the page will not be cleanly removed from the tree (leaving orphans, out of date num_child, etc). It is very common to accidentally forget to set the on_delete action to models.SET_NULL. This pull request checks all ForeignKey fields on pages and makes sure that they are not set to cascade. If they are set to cascade, the developer will be warned of their mistake. This only works for Django 1.7 users as it uses the system checks framework. --- wagtail/wagtailcore/models.py | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/wagtail/wagtailcore/models.py b/wagtail/wagtailcore/models.py index a4d0bed925..529bd9a023 100644 --- a/wagtail/wagtailcore/models.py +++ b/wagtail/wagtailcore/models.py @@ -26,6 +26,11 @@ from django.core.exceptions import ValidationError, ImproperlyConfigured from django.utils.functional import cached_property from django.utils.encoding import python_2_unicode_compatible +try: + from django.core import checks +except ImportError: + pass + from treebeard.mp_tree import MP_Node from wagtail.wagtailcore.utils import camelcase_to_underscore, resolve_model_string @@ -260,7 +265,7 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed live = models.BooleanField(default=True, editable=False) has_unpublished_changes = models.BooleanField(default=False, editable=False) url_path = models.CharField(max_length=255, blank=True, editable=False) - owner = models.ForeignKey(settings.AUTH_USER_MODEL, null=True, blank=True, editable=False, related_name='owned_pages') + owner = models.ForeignKey(settings.AUTH_USER_MODEL, null=True, blank=True, editable=False, on_delete=models.SET_NULL, related_name='owned_pages') seo_title = models.CharField(verbose_name=_("Page title"), max_length=255, blank=True, help_text=_("Optional. 'Search Engine Friendly' title. This will appear at the top of the browser window.")) show_in_menus = models.BooleanField(default=False, help_text=_("Whether a link to this page will appear in automatically generated menus")) @@ -343,6 +348,28 @@ class Page(six.with_metaclass(PageBase, MP_Node, ClusterableModel, index.Indexed return result + @classmethod + def check(cls, **kwargs): + errors = super(Page, cls).check(**kwargs) + + # Check that foreign keys from pages are not configured to cascade + # This is the default Django behaviour which must be explicitly overridden + # to prevent pages disappearing unexpectedly and the tree being corrupted + + for field in cls._meta.fields: + if isinstance(field, models.ForeignKey) and field.name not in ['page_ptr', 'content_type']: + if field.rel.on_delete == models.CASCADE: + errors.append( + checks.Error( + "Field hasn't specified on_delete action", + hint="Set on_delete=models.SET_NULL and make sure the field is nullable.", + obj=field, + id='wagtailcore.E001', + ) + ) + + return errors + def _update_descendant_url_paths(self, old_url_path, new_url_path): cursor = connection.cursor() if connection.vendor == 'sqlite': From f408dddfbea8db32b2de9e41bdcc3b2691eb2220 Mon Sep 17 00:00:00 2001 From: Matt Westcott Date: Mon, 27 Oct 2014 17:15:06 +0000 Subject: [PATCH 2/2] Release note for #718 --- CHANGELOG.txt | 1 + docs/releases/0.8.rst | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 5332c555cc..fe6701c431 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -7,6 +7,7 @@ Changelog * Added logging for page operations * The save/publish/submit buttons on the page edit page now redirects the user back to the edit page instead of the explorer * Signal handlers for ``wagtail.wagtailsearch`` and ``wagtail.contrib.wagtailfrontendcache`` are now automatically registered when using Django 1.7 or above. (Tim Heap) + * Added a Django 1.7 system check to ensure that foreign keys from Page models are set to on_delete=SET_NULL, to prevent inadvertent (and tree-breaking) page deletions * Fix: Replaced references of .username with .get_username() on users for better custom user model support (John-Scott Atlakson) * Fix: Unpinned dependency versions for six and requests to help prevent dependency conflicts * Fix: Fixed TypeError when getting embed HTML with oembed on Python 3 (John-Scott Atlakson) diff --git a/docs/releases/0.8.rst b/docs/releases/0.8.rst index 94cff59372..c2d89d1975 100644 --- a/docs/releases/0.8.rst +++ b/docs/releases/0.8.rst @@ -18,6 +18,7 @@ Minor features * Page operations (creation, publishing, copying etc) are now logged via Python's ``logging`` framework; to configure this, add a logger entry for ``'wagtail'`` or ``'wagtail.core'`` to the ``LOGGING`` setup in your settings file. * The save/publish/submit buttons on the page edit page now redirects the user back to the edit page instead of the explorer * Signal handlers for ``wagtail.wagtailsearch`` and ``wagtail.contrib.wagtailfrontendcache`` are now automatically registered when using Django 1.7 or above. + * Added a Django 1.7 system check to ensure that foreign keys from Page models are set to ``on_delete=SET_NULL``, to prevent inadvertent (and tree-breaking) page deletions Bug fixes