Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#14877 closed Bug (fixed) with a deleted form should work even if the model has already been deleted

Reported by: andornaut Owned by: Anssi Kääriäinen
Component: Forms Version: 1.3-alpha
Severity: Normal Keywords:
Cc: patrys@…, olau@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no



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


    def save_existing_objects(self, commit=True):
# ... snip ...
            pk_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)


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.

Attachments (1)

django-formset-delete-bug.patch (1.1 KB) - added by Ole Laursen 3 years ago.
Replace needless (re)-cleaning of form with just picking the already assigned form.instance

Download all attachments as: .zip

Change History (20)

comment:1 Changed 6 years ago by andornaut

Component: UncategorizedForms
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Version: 1.21.3-alpha

comment:2 Changed 6 years ago by 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.

comment:3 Changed 6 years ago by 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:
            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:
        return saved_instances

comment:4 Changed 6 years ago by andornaut

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

obj = self.instance


obj = form.instance

comment:5 Changed 6 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

comment:6 Changed 5 years ago by James Addison

Severity: Normal
Type: Bug

comment:7 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:8 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:9 Changed 4 years ago by Patryk Zawadzki

Cc: patrys@… added

comment:10 Changed 4 years ago by Ole Laursen

Cc: olau@… added

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

comment:11 Changed 4 years ago by 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.

comment:12 Changed 4 years ago by 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(): # validation exception here!

comment:13 Changed 4 years ago by Ole Laursen

Has patch: set

comment:14 Changed 4 years ago by Anssi Kääriäinen

Owner: changed from nobody to 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.

Changed 3 years ago by Ole Laursen

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

comment:15 Changed 3 years ago by Ole Laursen

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

comment:16 Changed 3 years ago by Anssi Kääriäinen

Patch at - should be ready for commit, and should solve #18508. Tests for #18508 will need to be added separately.

Last edited 3 years ago by Anssi Kääriäinen (previous) (diff)

comment:17 Changed 3 years ago by loic84

Triage Stage: AcceptedReady for checkin

PR looks good to me, marking it as RFC.

comment:18 Changed 3 years ago by Anssi Kääriäinen <akaariai@…>

Resolution: fixed
Status: newclosed

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.

comment:19 Changed 3 years ago by 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

Note: See TracTickets for help on using tickets.
Back to Top