Defer saving ClusterForm instances until formsets are saved

[tomkins]

Fixes wagtail/wagtail#13332
Fixes wagtail/wagtail#13701 (duplicate)

Description

Fixes IntegrityIssues or duplicate inlines when doing:

• Create/publish new page with inline
• Publish new version with a second inline
• Unpublish page
• Republish page

To test:

pip install wagtail
wagtail start mysite
cd mysite

cat > home/models.py <<EOF
from django.db import models

from modelcluster.fields import ParentalKey
from wagtail.admin.panels import FieldPanel, InlinePanel
from wagtail.models import Page


class HomePage(Page):
    content_panels = Page.content_panels + [
        InlinePanel("unique_inlines"),
    ]


class UniqueInline(models.Model):
    page = ParentalKey(Page, related_name="unique_inlines", on_delete=models.CASCADE)
    description = models.CharField(max_length=20)

    panels = [FieldPanel("description")]

    class Meta:
        constraints = [
            models.UniqueConstraint(
                fields=["page", "description"], name="unique_page_description"
            )
        ]
EOF

./manage.py makemigrations
./manage.py migrate
./manage.py createsuperuser
./manage.py runserver

• Go to: http://127.0.0.1:8000/admin/pages/3/edit/
• Add a single inline, "One", publish page
• Go to: http://127.0.0.1:8000/admin/pages/3/edit/
• Add another single inline, "Two", publish page
• Unpublish page: http://127.0.0.1:8000/admin/pages/3/unpublish/
• Try and publish again: http://127.0.0.1:8000/admin/pages/3/edit/

You'll see an IntegrityError 500 - which is wagtail/wagtail#13332.

• Go to: http://127.0.0.1:8000/admin/pages/3/edit/
• Change "Two" to "Dupe"
• Publish page

You'll see: One, Dupe, Two - which is wagtail/wagtail#13701.

Install this branch:

pip install -e django-modelcluster@git+https://github.com/tomkins/django-modelcluster.git@fix-clusterform-save

Repeat the same instructions, and you'll no longer get an IntegrityError or duplicates.

This is due to wagtail/wagtail#10104. That pull request changed various forms to .save(commit=True), which causes the following for the Revision model:

Initial creation of a page - the primary key is stored for inlines:

'unique_inlines': [{'pk': 8, 'page': 4, 'description': 'One'}]

Add an inline to the page:

'unique_inlines': [{'pk': 8, 'page': 4, 'description': 'One'}, {'pk': None, 'page': 4, 'description': 'Two'}]

The primary key for the new inline isn't saved, as the EditView saves the revision first, and then passes it to other code elsewhere to publish that (or, potentially publish later).

If you're editing that second version and just immediately publishing another one - it's all fine. However if you unpublish, Wagtail uses the stored Revision JSON when editing the page again, which has an empty PK.

Now, in theory this should all be handled by

django-modelcluster/modelcluster/forms.py

Lines 107 to 114 in 1b31175

	# If the manager has existing instances with a blank ID, we have no way of knowing
	# whether these correspond to items in the submitted data. We'll assume that they do,
	# as that's the most common case (i.e. the formset contains the full set of child objects,
	# not just a selection of additions / updates) and so we delete all ID-less objects here
	# on the basis that they will be re-added by the formset saving mechanism.
	no_id_instances = [obj for obj in manager.all() if obj.pk is None]
	if no_id_instances:
	manager.remove(*no_id_instances)

- but...

django-modelcluster/modelcluster/forms.py

Lines 377 to 399 in 1b31175

	instance = super().save(commit=(commit and not save_m2m_now))
	# 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()

Any call of super().save(commit=True) results in:

ipdb> self.instance.unique_inlines.all()
[<UniqueInline: UniqueInline object (10)>, <UniqueInline: UniqueInline object (None)>]

(let ipdb continue)

ipdb> self.instance.unique_inlines.all()
<QuerySet [<UniqueInline: UniqueInline object (10)>, <UniqueInline: UniqueInline object (14)>]>

Because super().save(commit=True) is effectively instance.save(). The instance gets saved, all the inlines get saved, and then the inline formsets try to save.

Anyway - I hope this is the right fix for the issue. Took me quite a lot of time to investigate this!

AI usage

Created the test myself (following other tests), then used Claude Opus 4.6 for the initial version of the fix (and changed/tweaked quite a lot before this PR).

[tomkins]

Actually, hold off on the review a little bit.

Need to ponder/tweak a bit for the "classic" M2M scenario. There's currently no cases covering it (only _need_commit_after_assignment cases).