From 438f95893830f5407428c76cfdb02f46cfbe3664 Mon Sep 17 00:00:00 2001 From: Sonny Baker Date: Fri, 19 Jun 2020 13:32:44 +0100 Subject: [PATCH 01/10] First draft of Swappable Page Model RFC --- text/000-swappable-page-model.md | 135 +++++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 text/000-swappable-page-model.md diff --git a/text/000-swappable-page-model.md b/text/000-swappable-page-model.md new file mode 100644 index 00000000..c5bcd150 --- /dev/null +++ b/text/000-swappable-page-model.md @@ -0,0 +1,135 @@ +# RFC 000: Swappable Page model + +* RFC: 000 +* Author: Sonny Baker +* Created: 2020-06-19 +* Last Modified: 2020-06-19 + +## Abstract + +This RFC proposes that Wagtail should give users the option to specify a custom `Page` model, +rather than inheriting the concrete one provided at `wagtail.core.models.Page`. + +## Specification + +Django has undocumented support for [swappable models](https://code.djangoproject.com/ticket/19103), +most commonly used when specifying a custom `auth.User` model. +[A relatively simple third party library](https://github.com/wq/django-swappable-models) provides an +API for leveraging this feature. + +Wagtail core would provide an abstract version of the existing `Page` model +(with default attributes, methods, object manager, queryset etc), and helper methods to retrieve the +correct page model, falling back to a default: + +```python +class AbstractPage(MP_Node, index.Indexed, ClusterableModel, metaclass=PageBase): + + objects = PageManager() + + title = models.CharField( + verbose_name=_('title'), + max_length=255, + help_text=_("The page title as you'd like it to be seen by the public") + ) + + # ... + + class Meta: + swappable = swapper.swappable_setting('wagtailcore', 'Page') + +class Page(AbstractPage): + + class Meta(AbstractPage.Meta): + pass + + +def get_page_model(): + """ + Get the site model from the ``WAGTAILCORE_PAGE_MODEL`` setting. + Defaults to the standard :class:`~wagtail.sites.models.Page` model + if no custom model is defined. + """ + Page = swapper.load_model("wagtailcore", "Page") + return Page + + +def get_page_model_string(): + """ + Get the dotted ``app.Model`` name for the page model as a string. + """ + return getattr(settings, 'WAGTAILCORE_PAGE_MODEL', 'wagtailcore.Page') + +``` + +This would be inherited by the user's custom `Page` class: + +```python +from wagtail.core.models import AbstractPage + +class Page(AbstractPage): + thumbnail = models.ForeignKey( + get_image_model_string(), + null=True, + blank=True, + on_delete=models.SET_NULL, + related_name='+' + ) + + class Meta(AbstractPage.Meta): + pass +``` + +And the project `settings.py` file will point to the dot notated string representation of that model: + +```python +WAGTAILCORE_PAGE_MODEL = 'foo.Page' +``` + +This would mean future references to the page model can be made like so: + +```python +class FooPage(Page): + related_page = models.ForeignKey( + get_page_model_string(), + null=True, + blank=True, + on_delete=models.SET_NULL, + related_name='+' + ) +``` + +### Rationale +* Improves performance - fewer lookups as shared page attributes will be on the concrete `Page` model +* Reduces code complexity - filtering by custom attributes like `Page.objects.filter(language='fr')` +* Removing `.specific()` from queries will allow annotations of `Page` querysets +* Allows developers to control routing and URL generation +* Consistent with custom `Image` and `Document` (and [potentially `Site`](https://github.com/wagtail/wagtail/pull/5457)) models +* Simplifies managing future extensions with mixins - e.g Translation, Experiments, Personalisation + +### Caveats / Considerations +* This is a major change to Wagtail, and must be tested thoroughly. It would be be best soft-launched +as an experimental feature, to collect user feedback. +* Swappable models is an _intentionally undocumented_ (or "stealth alpha") feature - +[User models were the pilot program for this](https://code.djangoproject.com/ticket/19103). Though its +use in the `auth` library has shown it to be stable, relying on a private API is still a risk, and another solution may be preferable. +* Would mean updating core migrations. +* Page revisions will need to be adapted to work with generic foreign keys. +* Could present issues when adding (or removing) fields from the abstract Page base (ie. clashing attribute names). +* Potentially adds a layer of upfront complexity for new users. The existing custom `Image` and +`Document` model implementations are good examples of optional features available to those who need it, +but ignorable for common Wagtail installations. +* Could make future support queries harder to deal with. +* Difficult to switch to a custom model mid-project. Documentation should specify this in the same +manner as Django does with [custom user models](https://docs.djangoproject.com/en/3.0/topics/auth/customizing/#changing-to-a-custom-user-model-mid-project). + +## Open Questions +* How do we implement this in a way that is frictionless for new starters? +* What are the implications for third-party packages? +* What other consequences might there be for introducing this change? + +## References / further reading +* [Open PR for swappable `Site` model](https://github.com/wagtail/wagtail/pull/5457/) +* [Open feature request](https://github.com/wagtail/wagtail/issues/3282) +* [Open GitHub issue](https://github.com/wagtail/wagtail/issues/836) +* [Google groups discussion](https://groups.google.com/forum/#!msg/wagtail/4459qj1tNiU/D91COykMSmcJ) + From 2c9d2c2201bf70320b733474ca6058d27bdd5e8d Mon Sep 17 00:00:00 2001 From: Sonny Baker Date: Fri, 19 Jun 2020 15:30:10 +0100 Subject: [PATCH 02/10] Adding more detail to Swappable Page Model --- text/000-swappable-page-model.md | 65 ++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/text/000-swappable-page-model.md b/text/000-swappable-page-model.md index c5bcd150..da3d4aca 100644 --- a/text/000-swappable-page-model.md +++ b/text/000-swappable-page-model.md @@ -10,6 +10,71 @@ This RFC proposes that Wagtail should give users the option to specify a custom `Page` model, rather than inheriting the concrete one provided at `wagtail.core.models.Page`. +## Why? + +Consider a Wagtail site with 3 page types - `BlogPage`, `PressReleasePage` and `CompetitionPage`. +Each of these pages can be assigned a category and marked as featured content: + +```python + +class Category(models.Model): + name = models.CharField( + max_length=255, + null=True, + blank=True + ) + +class PageBase(Page): + class Meta: + abstract = True + + category = models.ForeignKey( + 'foo.Category', + null=True, + blank=False, + on_delete=models.SET_NULL, + related_name='+' + ) + + is_featured = models.BooleanField( + verbose_name="Feature this page on the home page", + default=False + ) + +class BlogPage(PageBase): + pass + +class PressReleasePage(PageBase): + pass + +class CompetitionPage(PageBase): + pass + +``` + +On the homepage of the site, we'd like to show all the featured pages, and display the name +of the category they belong to. Currently, this query would fail: + +```python +featured = Page.objects.filter(is_featured=True).select_related('category', 'owner') +``` + +Because `is_featured` and `cateogy` do not exist on the concrete `Page` model. + +We may also want to show a list of the most recently published pages of multiple types and their category. +The `specific()` method exists on `PageQuerySet`, but it comes at the cost of extra database queries. +Additionally, [annotations are not currently possible when using `specific()`](https://github.com/wagtail/wagtail/issues/2340#issuecomment-496987132), +meaning given the following query: + +```python +news_pages = Page.objects.type((BlogPage, PressReleasePage)).specific().annotate( + author_name=Concat('owner__first_name', models.Value(' '), 'owner__last_name') +).order_by('-first_published_at') +``` + +Would return a list of pages as their specific classes and `news_pages.first().category` would work, +but `news_pages.first().owner_name` will throw an exception, even though the query ran without error. + ## Specification Django has undocumented support for [swappable models](https://code.djangoproject.com/ticket/19103), From fade01b31c5aeedbdcee41e5b70e145e686a8e87 Mon Sep 17 00:00:00 2001 From: Sonny Baker Date: Fri, 19 Jun 2020 15:39:27 +0100 Subject: [PATCH 03/10] Consolidation examples and formatting --- text/000-swappable-page-model.md | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/text/000-swappable-page-model.md b/text/000-swappable-page-model.md index da3d4aca..4a4b5c24 100644 --- a/text/000-swappable-page-model.md +++ b/text/000-swappable-page-model.md @@ -67,9 +67,15 @@ Additionally, [annotations are not currently possible when using `specific()`](h meaning given the following query: ```python -news_pages = Page.objects.type((BlogPage, PressReleasePage)).specific().annotate( - author_name=Concat('owner__first_name', models.Value(' '), 'owner__last_name') -).order_by('-first_published_at') +page_types = tuple([BlogPage, PressReleasePage, ]) +author_name = Concat('owner__first_name', models.Value(' '), 'owner__last_name') +news_pages = ( + Page.objects + .type(page_types) + .specific() + .annotate(author_name=author_name) + .order_by('-first_published_at') +) ``` Would return a list of pages as their specific classes and `news_pages.first().category` would work, @@ -97,7 +103,7 @@ class AbstractPage(MP_Node, index.Indexed, ClusterableModel, metaclass=PageBase) help_text=_("The page title as you'd like it to be seen by the public") ) - # ... + # other default fields... class Meta: swappable = swapper.swappable_setting('wagtailcore', 'Page') @@ -132,14 +138,19 @@ This would be inherited by the user's custom `Page` class: from wagtail.core.models import AbstractPage class Page(AbstractPage): - thumbnail = models.ForeignKey( - get_image_model_string(), + category = models.ForeignKey( + 'foo.Category', null=True, - blank=True, + blank=False, on_delete=models.SET_NULL, related_name='+' ) + is_featured = models.BooleanField( + verbose_name="Feature this page on the home page", + default=False + ) + class Meta(AbstractPage.Meta): pass ``` From 1f79f27e2e4f0999b9ee923fa80fb02054d01a4d Mon Sep 17 00:00:00 2001 From: Sonny Baker Date: Fri, 19 Jun 2020 15:45:42 +0100 Subject: [PATCH 04/10] Fixing a typo --- text/000-swappable-page-model.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/000-swappable-page-model.md b/text/000-swappable-page-model.md index 4a4b5c24..54569c55 100644 --- a/text/000-swappable-page-model.md +++ b/text/000-swappable-page-model.md @@ -59,7 +59,7 @@ of the category they belong to. Currently, this query would fail: featured = Page.objects.filter(is_featured=True).select_related('category', 'owner') ``` -Because `is_featured` and `cateogy` do not exist on the concrete `Page` model. +Because `is_featured` and `category` do not exist on the concrete `Page` model. We may also want to show a list of the most recently published pages of multiple types and their category. The `specific()` method exists on `PageQuerySet`, but it comes at the cost of extra database queries. From ca1144e0c32410e4b20695ba72e39f429c11196a Mon Sep 17 00:00:00 2001 From: Sonny Baker Date: Fri, 19 Jun 2020 16:13:59 +0100 Subject: [PATCH 05/10] Adding more rationale and examples --- text/000-swappable-page-model.md | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/text/000-swappable-page-model.md b/text/000-swappable-page-model.md index 54569c55..15aebf69 100644 --- a/text/000-swappable-page-model.md +++ b/text/000-swappable-page-model.md @@ -56,15 +56,17 @@ On the homepage of the site, we'd like to show all the featured pages, and displ of the category they belong to. Currently, this query would fail: ```python -featured = Page.objects.filter(is_featured=True).select_related('category', 'owner') +featured = Page.objects.select_related('category', 'owner').filter(is_featured=True) ``` -Because `is_featured` and `category` do not exist on the concrete `Page` model. +Because `is_featured` and `category` do not exist on the concrete `Page` model. The solution would +be to query each model indiviually, and then combine the querysets - a cumbersome and slow operation +that is difficult to scacle. We may also want to show a list of the most recently published pages of multiple types and their category. The `specific()` method exists on `PageQuerySet`, but it comes at the cost of extra database queries. Additionally, [annotations are not currently possible when using `specific()`](https://github.com/wagtail/wagtail/issues/2340#issuecomment-496987132), -meaning given the following query: +meaning the following query: ```python page_types = tuple([BlogPage, PressReleasePage, ]) @@ -174,6 +176,25 @@ class FooPage(Page): ) ``` +And previously complex multi-model Page queries become trivial: + +```python +author_name = Concat('owner__first_name', models.Value(' '), 'owner__last_name') + +featured_foo_posts = ( + Page.objects + .live() + .filter(category='foo', is_featured=True) + .annotate( + tag_count=Count('tags'), + author_name=author_name + ) + .order_by('-first_published_at') + .values('title', 'slug', 'tag_count', 'author_name') +) +``` + + ### Rationale * Improves performance - fewer lookups as shared page attributes will be on the concrete `Page` model * Reduces code complexity - filtering by custom attributes like `Page.objects.filter(language='fr')` @@ -201,6 +222,7 @@ manner as Django does with [custom user models](https://docs.djangoproject.com/e ## Open Questions * How do we implement this in a way that is frictionless for new starters? * What are the implications for third-party packages? +* What happens to required attributes like `title` if user has the option to override? Do we protect them somehow? * What other consequences might there be for introducing this change? ## References / further reading From 37c8888616e257a155dc18c8a072b37eff1e790d Mon Sep 17 00:00:00 2001 From: Sonny Baker Date: Fri, 19 Jun 2020 16:15:28 +0100 Subject: [PATCH 06/10] Show more realistic category example --- text/000-swappable-page-model.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/text/000-swappable-page-model.md b/text/000-swappable-page-model.md index 15aebf69..ea5bbe3c 100644 --- a/text/000-swappable-page-model.md +++ b/text/000-swappable-page-model.md @@ -180,11 +180,10 @@ And previously complex multi-model Page queries become trivial: ```python author_name = Concat('owner__first_name', models.Value(' '), 'owner__last_name') - featured_foo_posts = ( Page.objects .live() - .filter(category='foo', is_featured=True) + .filter(category__name='Foo', is_featured=True) .annotate( tag_count=Count('tags'), author_name=author_name From d0d92fcdd8880d3e10b2747e2cff7ed05fed5db4 Mon Sep 17 00:00:00 2001 From: Sonny Baker Date: Mon, 22 Jun 2020 11:07:55 +0100 Subject: [PATCH 07/10] Updating RFC number to match Wagtail pull request --- text/000-swappable-page-model.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/text/000-swappable-page-model.md b/text/000-swappable-page-model.md index ea5bbe3c..40a7b722 100644 --- a/text/000-swappable-page-model.md +++ b/text/000-swappable-page-model.md @@ -1,9 +1,9 @@ -# RFC 000: Swappable Page model +# RFC 53: Swappable Page model -* RFC: 000 +* RFC: 53 * Author: Sonny Baker * Created: 2020-06-19 -* Last Modified: 2020-06-19 +* Last Modified: 2020-06-20 ## Abstract From d2094b68e56371c9ea6d726da59c8117acf1d283 Mon Sep 17 00:00:00 2001 From: Sonny Baker Date: Mon, 22 Jun 2020 11:10:21 +0100 Subject: [PATCH 08/10] Renaming markdown file to match PR --- text/{000-swappable-page-model.md => 053-swappable-page-model.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename text/{000-swappable-page-model.md => 053-swappable-page-model.md} (100%) diff --git a/text/000-swappable-page-model.md b/text/053-swappable-page-model.md similarity index 100% rename from text/000-swappable-page-model.md rename to text/053-swappable-page-model.md From 7b63594a99fa26693531f503e6a377a6aa23b71b Mon Sep 17 00:00:00 2001 From: Sonny Baker Date: Fri, 12 Mar 2021 11:49:39 +0000 Subject: [PATCH 09/10] Update 053-swappable-page-model.md Fixing a typo --- text/053-swappable-page-model.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/053-swappable-page-model.md b/text/053-swappable-page-model.md index 40a7b722..674c23d2 100644 --- a/text/053-swappable-page-model.md +++ b/text/053-swappable-page-model.md @@ -60,7 +60,7 @@ featured = Page.objects.select_related('category', 'owner').filter(is_featured=T ``` Because `is_featured` and `category` do not exist on the concrete `Page` model. The solution would -be to query each model indiviually, and then combine the querysets - a cumbersome and slow operation +be to query each model individually, and then combine the querysets - a cumbersome and slow operation that is difficult to scacle. We may also want to show a list of the most recently published pages of multiple types and their category. From 92dd5b2202663c7f0cadb544414d2d0f5766d3a9 Mon Sep 17 00:00:00 2001 From: Sonny Baker Date: Fri, 12 Mar 2021 11:51:05 +0000 Subject: [PATCH 10/10] Update 053-swappable-page-model.md Fixing _another_ typo... --- text/053-swappable-page-model.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/053-swappable-page-model.md b/text/053-swappable-page-model.md index 674c23d2..85890294 100644 --- a/text/053-swappable-page-model.md +++ b/text/053-swappable-page-model.md @@ -61,7 +61,7 @@ featured = Page.objects.select_related('category', 'owner').filter(is_featured=T Because `is_featured` and `category` do not exist on the concrete `Page` model. The solution would be to query each model individually, and then combine the querysets - a cumbersome and slow operation -that is difficult to scacle. +that is difficult to scale. We may also want to show a list of the most recently published pages of multiple types and their category. The `specific()` method exists on `PageQuerySet`, but it comes at the cost of extra database queries.