#10284 closed Bug (fixed)
ModelFormSet - objects are deleted even if commit=False
| Reported by: | Owned by: | Wedg | |
|---|---|---|---|
| Component: | Forms | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | denilsonsa@…, timograham@… | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
In ModelFormSet, when the deletion checkbox is checked for an existing object, and the formset is saved with commit=False:
formset.save(commit=False)
the object is deleted.
Attachments (4)
Change History (23)
Changed 15 years ago by
| Attachment: | django_modelformset_commit_false_delete.diff added |
|---|
comment:1 Changed 15 years ago by
| Triage Stage: | Unreviewed → Design decision needed |
|---|
This patch isn't correct, since the related objects will never get deleted by anyone. This is a tricky issue to solve.
comment:2 Changed 14 years ago by
I can't understand why this patch isn't correct. Can you tell us more ?
I had the bug yesterday, and the actual design for delete related objects with formsets is very dangerous. When we check the "delete", and preview the formset before submit, the related objects are deleted. There should be a lot of data lost because of this issue.
comment:3 Changed 14 years ago by
I'm also not understanding why the patch won't work - do we need to document the fact that if you call formset.save(commit=False) you'd need to call delete() for each instance in the formset.deleted_objects just like you'd have to call save() for other changed/new instances? (If the patch were applied...)
From what I can tell, this is all that's happening in the BaseModelFormSet.save_existing_objects method, and all the objects that would have delete() called are stored in formset.deleted_objects.
comment:4 Changed 14 years ago by
| Needs tests: | set |
|---|
Changed 14 years ago by
| Attachment: | modelformset_false_delete.diff added |
|---|
Patch against latest trunk.
comment:5 Changed 14 years ago by
| Needs documentation: | set |
|---|---|
| Version: | 1.0 → SVN |
So... hopefully I did this right. The current trunk fails the regression test (deletes the model even though commit=False), but with the patch applied it passes. Please let me know if I need to make any corrections/whatever. :D
Oh, and I'll try to get a doc patch for the behavior too, unless someone beats me to it.
Changed 14 years ago by
| Attachment: | modelformset_doc.diff added |
|---|
Doc patch describing new/fixed behavior so people don't forget to delete.
comment:6 Changed 13 years ago by
| Needs documentation: | unset |
|---|---|
| Needs tests: | unset |
| Owner: | changed from nobody to Wedg |
| Status: | new → assigned |
comment:7 Changed 13 years ago by
| Cc: | denilsonsa@… added |
|---|
comment:8 Changed 13 years ago by
| Severity: | → Normal |
|---|---|
| Type: | → Bug |
comment:11 Changed 11 years ago by
| Triage Stage: | Design decision needed → Accepted |
|---|
I don't understand the rationale jacob gave for rejecting the initial patch ("related objects will never get deleted by anyone.") Flipping back to Accepted status; even if the current patch is wrong for some reason I'm not getting, I think this is still clearly a bug that should be fixed.
comment:13 Changed 10 years ago by
Duplicate of #17988 in which @akaariai suggests changing the current behavior would be backwards-incompatible (of course, all bug fixes are -- I guess it's whether or not you consider the current design a bug or a feature). This is what Jacob meant by "related objects will never get deleted by anyone" -- the pull request solves this by a documentation update which informs users they must handle formset.deleted_objects manually.
IMO, the justification for changing the behavior is that it's unexpected that anything DB related happens with commit=False.
comment:14 Changed 10 years ago by
django-dev thread hasn't received any responses in an attempt to get a design decision.
comment:15 Changed 10 years ago by
+1 for this PR.
As mentioned on IRC, this together with formally documenting ModelFormSet.(new|changed|deleted)_objects is IMO the cleanest solution to this problem.
comment:16 Changed 10 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
Patch for described issue