From 069a1ae153fd95f55683c02722cff0ece151d74c Mon Sep 17 00:00:00 2001 From: Alex Tomkins Date: Sun, 12 Apr 2026 23:03:53 +0100 Subject: [PATCH] Defer saving ClusterForm instances until formsets are saved Calling `super().save(commit=True)` triggers an `instance.save()` in Django's ModelForm code. This results in a cluster model that saves all child relations unexpectedly early, causing collisions or duplicates when the formset does get saved. By saving formsets first (without a commit), modelcluster is able to update the inlines before the instance gets fully saved. --- modelcluster/forms.py | 90 +++++++++++----- ...alter_article_related_articles_and_more.py | 53 +++++++++ tests/models.py | 8 ++ tests/tests/test_cluster_form.py | 102 +++++++++++++++++- 4 files changed, 225 insertions(+), 28 deletions(-) create mode 100644 tests/migrations/0014_comment_alter_article_related_articles_and_more.py diff --git a/modelcluster/forms.py b/modelcluster/forms.py index 29e2e5d..33779ef 100644 --- a/modelcluster/forms.py +++ b/modelcluster/forms.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals +from itertools import chain from django.forms import ValidationError from django.core.exceptions import NON_FIELD_ERRORS from django.forms.formsets import TOTAL_FORM_COUNT @@ -10,7 +11,7 @@ from django.db.models.fields.related import ForeignObjectRel from django.utils.html import format_html_join - +from modelcluster.fields import ParentalManyToManyField from modelcluster.models import get_all_child_relations @@ -359,48 +360,83 @@ def media(self): media = media + formset.media return media - def save(self, commit=True): - # do we have any fields that expect us to call save_m2m immediately? - save_m2m_now = False + def save_cluster_m2m(self): + # Customised version of Django BaseModelForm.save_m2m for cluster M2M fields, limited to + # ParentalManyToManyField, or fields with the _need_commit_after_assignment flag. + cleaned_data = self.cleaned_data exclude = self._meta.exclude fields = self._meta.fields + opts = self.instance._meta + for f in chain(opts.many_to_many, opts.private_fields): + # Standard Django save_m2m filtering + if not hasattr(f, "save_form_data"): + continue + if fields and f.name not in fields: + continue + if exclude and f.name in exclude: + continue + + # Cluster M2M filtering + is_cluster_m2m = isinstance(f, ParentalManyToManyField) + save_early = getattr(f, '_need_commit_after_assignment', False) + if not is_cluster_m2m and not save_early: + continue - for f in self.instance._meta.get_fields(): + if f.name in cleaned_data: + f.save_form_data(self.instance, cleaned_data[f.name]) + + def _save_m2m(self): + # Customised version of Django BaseModelForm.save_m2m for standard M2M fields, any + # ParentalManyToManyField fields will be skipped as they're saved earlier with + # save_cluster_m2m. + cleaned_data = self.cleaned_data + exclude = self._meta.exclude + fields = self._meta.fields + opts = self.instance._meta + for f in chain(opts.many_to_many, opts.private_fields): + # Standard Django save_m2m filtering + if not hasattr(f, "save_form_data"): + continue if fields and f.name not in fields: continue if exclude and f.name in exclude: continue - if getattr(f, '_need_commit_after_assignment', False): - save_m2m_now = True - break - instance = super().save(commit=(commit and not save_m2m_now)) + # Cluster M2M filtering + if isinstance(f, ParentalManyToManyField): + continue + + if f.name in cleaned_data: + f.save_form_data(self.instance, cleaned_data[f.name]) + + def save(self, commit=True): + # Defer a full save until after formsets are saved. Calling an instance.save() with + # commit=True will save the instance *and* inlines too early, as modelcluster wants to + # save/update all the relations. + instance = super().save(commit=False) # The M2M-like fields designed for use with ClusterForm (currently # ParentalManyToManyField and ClusterTaggableManager) will manage their own in-memory # relations, and not immediately write to the database when we assign to them. # For these fields (identified by the _need_commit_after_assignment - # flag), save_m2m() is a safe operation that does not affect the database and is thus - # valid for commit=False. In the commit=True case, committing to the database happens - # in the subsequent instance.save (so this needs to happen after save_m2m to ensure - # we have the updated relation data in place). - - # For annoying legacy reasons we sometimes need to accommodate 'classic' M2M fields - # (particularly taggit.TaggableManager) within ClusterForm. These fields - # generally do require our instance to exist in the database at the point we call - # save_m2m() - for this reason, we only proceed with the customisation described above - # (i.e. postpone the instance.save() operation until after save_m2m) if there's a - # _need_commit_after_assignment field on the form that demands it. - - if save_m2m_now: - self.save_m2m() - - if commit: - instance.save() + # flag), save_cluster_m2m() is a safe operation that does not affect the database. + self.save_cluster_m2m() for formset in self.formsets.values(): formset.instance = instance - formset.save(commit=commit) + formset.save(commit=False) + + # For annoying legacy reasons we sometimes need to accommodate 'classic' M2M fields within + # ClusterForm. These fields generally do require our instance to exist in the database at + # the point we call save_m2m(). + if commit: + # Save the instance and M2M data + instance.save() + self._save_m2m() + else: + # Allow deferred saving of M2M data + self.save_m2m = self._save_m2m + return instance def has_changed(self): diff --git a/tests/migrations/0014_comment_alter_article_related_articles_and_more.py b/tests/migrations/0014_comment_alter_article_related_articles_and_more.py new file mode 100644 index 0000000..14940ea --- /dev/null +++ b/tests/migrations/0014_comment_alter_article_related_articles_and_more.py @@ -0,0 +1,53 @@ +# Generated by Django 5.2.13 on 2026-04-14 18:15 + +import django.db.models.deletion +import modelcluster.fields +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('taggit', '0006_rename_taggeditem_content_type_object_id_taggit_tagg_content_8fc721_idx'), + ('tests', '0013_add_log_category'), + ] + + operations = [ + migrations.CreateModel( + name='Comment', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('content', models.CharField(max_length=255)), + ], + ), + migrations.AlterField( + model_name='article', + name='related_articles', + field=modelcluster.fields.ParentalManyToManyField(blank=True, serialize=False, to='tests.article'), + ), + migrations.AlterField( + model_name='room', + name='features', + field=modelcluster.fields.ParentalManyToManyField(blank=True, related_name='rooms', to='tests.feature'), + ), + migrations.AlterField( + model_name='taggedarticle', + name='tag', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='%(app_label)s_%(class)s_items', to='taggit.tag'), + ), + migrations.AlterField( + model_name='taggednonclusterplace', + name='tag', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='%(app_label)s_%(class)s_items', to='taggit.tag'), + ), + migrations.AlterField( + model_name='taggedplace', + name='tag', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='%(app_label)s_%(class)s_items', to='taggit.tag'), + ), + migrations.AddField( + model_name='article', + name='comments', + field=models.ManyToManyField(blank=True, related_name='comments', to='tests.comment'), + ), + ] diff --git a/tests/models.py b/tests/models.py index 83b915b..c465c0f 100644 --- a/tests/models.py +++ b/tests/models.py @@ -192,6 +192,13 @@ class TaggedArticle(TaggedItemBase): content_object = ParentalKey('Article', related_name='tagged_items', on_delete=models.CASCADE) +class Comment(models.Model): + content = models.CharField(max_length=255) + + def __str__(self): + return self.content + + class Article(ClusterableModel): paper = ParentalKey(NewsPaper, blank=True, null=True, on_delete=models.CASCADE) title = models.CharField(max_length=255) @@ -200,6 +207,7 @@ class Article(ClusterableModel): tags = ClusterTaggableManager(through=TaggedArticle, blank=True) related_articles = ParentalManyToManyField('self', serialize=False, blank=True) view_count = models.IntegerField(null=True, blank=True, serialize=False) + comments = models.ManyToManyField('Comment', related_name='comments', blank=True) def __str__(self): return self.title diff --git a/tests/tests/test_cluster_form.py b/tests/tests/test_cluster_form.py index c0e93d9..99375d0 100644 --- a/tests/tests/test_cluster_form.py +++ b/tests/tests/test_cluster_form.py @@ -5,7 +5,7 @@ from django import VERSION as DJANGO_VERSION from django.core.exceptions import ValidationError from django.test import TestCase -from tests.models import Band, BandMember, Album, Log, Restaurant, Article, Author, Document, Gallery, Song +from tests.models import Band, BandMember, Album, Log, Restaurant, Article, Author, Document, Gallery, Song, Comment from modelcluster.forms import ClusterForm from django.forms import Textarea, CharField from django.forms.widgets import TextInput, FileInput @@ -113,6 +113,53 @@ class Meta: self.assertTrue(Band.objects.filter(name='The Beatles').exists()) self.assertTrue(BandMember.objects.filter(name='John Lennon').exists()) + def test_update_from_saved_revision(self): + class BandForm(ClusterForm): + class Meta: + model = Band + fields = ['name'] + formsets = ['members'] + + john = BandMember(name='John Lennon') + paul = BandMember(name='Paul McCartney') + beatles = Band(name='The Beatles', members=[ + john, + paul, + ]) + beatles.save() + + # To replicate a typical Wagtail saved revision, set the PK of one of the inlines to None, + # as these are saved into Revision content JSON *before* being saved to the database. + beatles_serialized = beatles.serializable_data() + beatles_serialized['members'][1]['pk'] = None + saved_revision = Band.from_serializable_data(beatles_serialized) + + form = BandForm({ + 'name': 'The Beatles', + + 'members-TOTAL_FORMS': 3, + 'members-INITIAL_FORMS': 1, + 'members-MAX_NUM_FORMS': 1000, + + 'members-0-name': 'John Lennon', + 'members-0-id': john.id, + + 'members-1-name': 'Paul McCartney', + 'members-1-id': '', + + 'members-2-name': 'George Harrison', + 'members-2-id': '', + }, instance=saved_revision) + + self.assertTrue(form.is_valid()) + result = form.save() + + self.assertEqual(result.members.count(), 3) + self.assertQuerySetEqual( + result.members.values_list('name', flat=True).order_by('name'), + ['George Harrison', 'John Lennon', 'Paul McCartney'], + ) + def test_explicit_formset_list(self): class BandForm(ClusterForm): class Meta: @@ -870,6 +917,59 @@ class Meta: self.assertEqual(db_article.title, 'Updated test article') self.assertEqual(list(db_article.authors.all()), [self.charles_dickens]) + def test_create_form_with_standard_m2m(self): + class ArticleForm(ClusterForm): + class Meta: + model = Article + fields = ['title', 'authors', 'comments'] + formsets = [] + + interesting_comment = Comment.objects.create(content='Interesting') + form = ArticleForm({ + 'title': 'New article', + 'authors': [self.james_joyce.id], + 'comments': [interesting_comment.id], + }) + self.assertTrue(form.is_valid()) + article = form.save() + + self.assertIsNotNone(article.pk) + self.assertEqual(article.title, 'New article') + self.assertQuerySetEqual(article.authors.all(), [self.james_joyce]) + self.assertQuerySetEqual(article.comments.all(), [interesting_comment]) + + db_article = Article.objects.get(pk=article.pk) + self.assertQuerySetEqual(db_article.authors.all(), [self.james_joyce]) + self.assertQuerySetEqual(db_article.comments.all(), [interesting_comment]) + + def test_defer_commit_with_standard_m2m(self): + class ArticleForm(ClusterForm): + class Meta: + model = Article + fields = ['title', 'authors', 'comments'] + formsets = [] + + interesting_comment = Comment.objects.create(content='Interesting') + form = ArticleForm({ + 'title': 'New article', + 'authors': [self.james_joyce.id], + 'comments': [interesting_comment.id], + }) + self.assertTrue(form.is_valid()) + article = form.save(commit=False) + + self.assertIsNone(article.pk) + + article.save() + self.assertIsNotNone(article.pk) + + db_article = Article.objects.get(pk=article.pk) + self.assertQuerySetEqual(db_article.comments.all(), []) + + form.save_m2m() + db_article.refresh_from_db() + self.assertQuerySetEqual(db_article.comments.all(), [interesting_comment]) + class NestedClusterFormTest(TestCase):