ModelFormSet.save() with a deleted form should work even if the model has already been deleted

[andornaut]

Background

This issue occurs when you have a ModelFormSet that contains a deleted Form that has an already deleted instance. When trying to save the ModelFormSet, Django will try and fail during save() because it is not able to lookup the instance by the raw_pk_value.

Use Case

An edit page with two formsets. The formset at the top allows you to edit or delete instances of model Foo. The formset at the bottom allows you to edit or delete instances of model Bar, each of which have a FK to one of the instance of Foo in the top formset.

If the user deletes Foo above, the the related Bars below are marked as deleted (fields['DELETE']=True). If a Foo is deleted and save() is called on the top formset before the bottom formset, then the Bars that have a FK to the deleted Foo will be deleted. Then when ModelFormSet tries to delete the Bar forms marked as deleted it will fail when it tries to lookup the deleted Bar by its PK.

This may can also occur outside of a single request if a Foo is deleted, for example, in another browser tab (untested):

1) Bar edit page GET request.
2) Foo edit page GET request.
3) Foo edit page POST: Delete 1 Foo, which cascade deletes some Bars
4) Bar edit page POST: Delete 1 Bar (the same one deleted in #3) - fails on save() during instance lookup

Relevant Code

django.forms.models.BaseModelFormSet.save_existing_objects()#603

    def save_existing_objects(self, commit=True):
# ... snip ...
            pk_name = self._pk_field.name
            raw_pk_value = form._raw_value(pk_name)

            # clean() for different types of PK fields can sometimes return
            # the model instance, and sometimes the PK. Handle either.
            pk_value = form.fields[pk_name].clean(raw_pk_value)
            pk_value = getattr(pk_value, 'pk', pk_value)

            obj = self._existing_object(pk_value)

Suggestion

It may make more sense to use form.instance instead of looking up the instance by its raw_pk_value again. This would also require fewer calls to the database.

Perhaps, consider replacing the code above with:

obj = self.instance

Of course, other changes may also be necessary to fix this use case.

[andornaut]

I forgot to mention that at line #617, the method should check if the object has a pk before attempting to call delete(). Alternatively, perhaps, objects with None PKs should not go down the save_existing_objects() code path at all.

[andornaut]

The following seems to work, though would require further testing. Substitute for django.forms.models.BaseModelFormSet.save_existing_objects()#603

    def save_existing_objects(self, commit=True):
        self.changed_objects = []
        self.deleted_objects = []
        if not self.get_queryset():
            return []

        saved_instances = []
        for form in self.initial_forms:
            obj = self.instance
            if self.can_delete:
                raw_delete_value = form._raw_value(DELETION_FIELD_NAME)
                should_delete = form.fields[DELETION_FIELD_NAME].clean(raw_delete_value)
                if should_delete:
                    self.deleted_objects.append(obj)
                    if obj.pk:
                        obj.delete()
                    continue
            if form.has_changed():
                self.changed_objects.append((obj, form.changed_data))
                saved_instances.append(self.save_existing(form, obj, commit=commit))
                if not commit:
                    self.saved_forms.append(form)
        return saved_instances

[andornaut]

Sorry, there was an error in the previous code. Please replace:

obj = self.instance

with

obj = form.instance

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Ole Laursen]

Just hit this bug too on a site with a lot of writes. andornauts suggestion seems to work.

[Ole Laursen]

I added a test case in the related issue #18508 which also reproduces this bug. So if it's stalled on a on a missing automated test, this can perhaps help.

[Ole Laursen]

What the heck, I added a patch for fixing it based on andornaut's suggestion (it didn't apply verbatim to HEAD). With this the test case in #18508 passes.

Just to recap, the problem here is that the code tries to call clean on the pk field which a) makes an extraneous DB query to validate it (extraneous because we just went through a form.is_valid()), b) fails if the object is already gone from the database.

In my case, I basically had

    if formset.is_valid():
        formset.save() # validation exception here!

[Anssi Kääriäinen]

This seems to be somewhat related to #18508 which I just assigned to myself. So, I will also work on this ticket while investigating #18508 - assigning to myself.

[Ole Laursen]

Replace needless (re)-cleaning of form with just picking the already assigned form.instance

[Ole Laursen]

I just uploaded a slightly revised patch that checks for obj.pk != None instead of just checking for obj.pk evaluating to true.

[Anssi Kääriäinen]

Patch at ​https://github.com/django/django/pull/1829 - should be ready for commit, and should solve #18508. Tests for #18505 will need to be added separately.

[loic84]

PR looks good to me, marking it as RFC.

[Anssi Kääriäinen <akaariai@…>]

In efb0100ee67931329f17bc9988ecd5f0619cea14:

Fixed #14877 -- repeated deletion using formsets

When a formset contained deletion for an existing instance, and the
instance was already deleted, django threw an exception. A common cause for
this was resubmit of the formset.

Original patch by Trac alias olau.

In addition this commit cleaned some code in _construct_form(). This
was needed as the primary key value the user submitted wasn't converted
correctly to python value in case the primary key field was also a
related field.

[Anssi Kääriäinen <akaariai@…>]

In ed516a5fc8061615ec94bee37425d4fee8ca872b:

Merge pull request #1829 from akaariai/ticket_14877

Fixed #14877 -- repeated deletion using formsets

Reviewed by Loic Bistuer