Opened 6 years ago

Closed 22 months ago

Last modified 17 months ago

#10284 closed Bug (fixed)

ModelFormSet - objects are deleted even if commit=False

Reported by: laureline.guerin@… Owned by: Wedg
Component: Forms Version: master
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)

django_modelformset_commit_false_delete.diff (635 bytes) - added by laureline.guerin@… 6 years ago.
Patch for described issue
modelformset_false_delete.diff (624 bytes) - added by Wedg 5 years ago.
Patch against latest trunk.
models.py (881 bytes) - added by Wedg 5 years ago.
Regression test for issue.
modelformset_doc.diff (763 bytes) - added by Wedg 5 years ago.
Doc patch describing new/fixed behavior so people don't forget to delete.

Download all attachments as: .zip

Change History (23)

Changed 6 years ago by laureline.guerin@…

Patch for described issue

comment:1 Changed 6 years ago by jacob

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to 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 6 years ago by damien_szczyt

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 5 years ago by Wedg

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 5 years ago by Alex

  • Needs tests set

Changed 5 years ago by Wedg

Patch against latest trunk.

Changed 5 years ago by Wedg

Regression test for issue.

comment:5 Changed 5 years ago by Wedg

  • Needs documentation set
  • Version changed from 1.0 to 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 5 years ago by Wedg

Doc patch describing new/fixed behavior so people don't forget to delete.

comment:6 Changed 5 years ago by Wedg

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to Wedg
  • Status changed from new to assigned

comment:7 Changed 4 years ago by denilsonsa

  • Cc denilsonsa@… added

comment:8 Changed 4 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Bug

comment:9 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:11 Changed 2 years ago by carljm

  • Triage Stage changed from Design decision needed to 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:12 Changed 22 months ago by timo

  • Cc timograham@… added

Updated PR. Seems straightforward to me.

comment:13 Changed 22 months ago by timo

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 22 months ago by timo

django-dev thread hasn't received any responses in an attempt to get a design decision.

comment:15 Changed 22 months ago by loic84

+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 22 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 65e03a424e82e157b4513cdebb500891f5c78363:

Fixed #10284 -- ModelFormSet.save(commit=False) no longer deletes objects

Thanks laureline.guerin@ and Wedg.

comment:17 Changed 17 months ago by Simon Charette <charette.s@…>

In de1d5d5df5238136e8cd114e36065857bee1ace4:

[1.6.x] Fixed #21594 -- Added note about model formsets deleting objects.

This behavior has been fixed in 65e03a424e. refs #10284.

comment:18 Changed 17 months ago by Simon Charette <charette.s@…>

In a53820b1b1747d6f9b5ee5723aa7526ededdaba7:

[1.5.x] Fixed #21594 -- Added note about model formsets deleting objects.

This behavior has been fixed in 65e03a424e. refs #10284.

Backport of de1d5d5df5 from stable/1.6.x.

comment:19 Changed 17 months ago by Simon Charette <charette.s@…>

In 474e7dd6d08105fce66d7f62670a6c120c0b1198:

[1.4.x] Fixed #21594 -- Added note about model formsets deleting objects.

This behavior has been fixed in 65e03a424e. refs #10284.

Backport of de1d5d5df5 from stable/1.6.x.

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