ModelFormSet.save() with a deleted form should work even if the model has already been deleted
|Reported by:||andornaut||Owned by:||akaariai|
|Cc:||patrys@…, olau@…||Triage Stage:||Ready for checkin|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||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.
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
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)
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.
Change History (20)
comment:1 Changed 3 years ago by andornaut
- Component changed from Uncategorized to Forms
- Needs documentation unset
- Needs tests unset
- Patch needs improvement unset
- Version changed from 1.2 to 1.3-alpha
Changed 4 months ago by olau
comment:18 Changed 4 months ago by Anssi Kääriäinen <akaariai@…>
- Resolution set to fixed
- Status changed from new to closed