Opened 14 years ago

Closed 11 years ago

Last modified 11 years ago

#14877 closed Bug (fixed)

ModelFormSet.save() 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

Description

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.

Attachments (1)

django-formset-delete-bug.patch (1.1 KB ) - added by Ole Laursen 11 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 by andornaut, 14 years ago

Component: UncategorizedForms
Version: 1.21.3-alpha

comment:2 by andornaut, 14 years ago

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 by andornaut, 14 years ago

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

comment:4 by andornaut, 14 years ago

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

obj = self.instance

with

obj = form.instance

comment:5 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

comment:6 by James Addison, 14 years ago

Severity: Normal
Type: Bug

comment:7 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:8 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:9 by Patryk Zawadzki, 12 years ago

Cc: patrys@… added

comment:10 by Ole Laursen, 12 years ago

Cc: olau@… added

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

comment:11 by Ole Laursen, 12 years ago

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 by Ole Laursen, 12 years ago

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!

comment:13 by Ole Laursen, 12 years ago

Has patch: set

comment:14 by Anssi Kääriäinen, 12 years ago

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.

by Ole Laursen, 11 years ago

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

comment:15 by Ole Laursen, 11 years ago

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

comment:16 by Anssi Kääriäinen, 11 years ago

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

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

comment:17 by loic84, 11 years ago

Triage Stage: AcceptedReady for checkin

PR looks good to me, marking it as RFC.

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

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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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