#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)
Change History (20)
comment:1 by , 14 years ago
Component: | Uncategorized → Forms |
---|---|
Version: | 1.2 → 1.3-alpha |
comment:2 by , 14 years ago
comment:3 by , 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 , 14 years ago
Sorry, there was an error in the previous code. Please replace:
obj = self.instance
with
obj = form.instance
comment:5 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:6 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:9 by , 12 years ago
Cc: | added |
---|
comment:10 by , 12 years ago
Cc: | added |
---|
Just hit this bug too on a site with a lot of writes. andornauts suggestion seems to work.
comment:11 by , 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 , 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 , 12 years ago
Has patch: | set |
---|
comment:14 by , 12 years ago
Owner: | changed from | to
---|
by , 11 years ago
Attachment: | django-formset-delete-bug.patch added |
---|
Replace needless (re)-cleaning of form with just picking the already assigned form.instance
comment:15 by , 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 , 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.
comment:17 by , 11 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
PR looks good to me, marking it as RFC.
comment:18 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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 thesave_existing_objects()
code path at all.