Opened 2 years ago

Closed 9 months ago

#17988 closed Bug (duplicate) doesn't honor commit=False when deleting objects.

Reported by: rem Owned by: tuxcanfly
Component: Forms Version: 1.3
Severity: Normal Keywords:
Cc: lemaire.adrien@…, akaariai, tuxcanfly, timograham@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no


When saving a ModelFormSet with some object(s) marked for deletion, calling still deletes the objects, which would then cause an obscure ValidationEror with invalid_choice when is called again.

The attached patches are against trunk, but this exists as of 1.3, if not earlier.

Attachments (2)

model_formset_check_commit_before_delete.diff (598 bytes) - added by rem 2 years ago.
Check commit=True before deleting existing objects.
test_model_formset_checks_commit_before_delete.diff (1.7 KB) - added by rem 2 years ago.
Regression test.

Download all attachments as: .zip

Change History (9)

Changed 2 years ago by rem

Check commit=True before deleting existing objects.

Changed 2 years ago by rem

Regression test.

comment:1 Changed 2 years ago by Fandekasp

  • Cc lemaire.adrien@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted


comment:2 Changed 2 years ago by akaariai

  • Cc akaariai added
  • Patch needs improvement set

Hmmh, I think this causes problems for this common use-case:

instances =
for instance in instances:
    instance.user = user

Currently I think things would work as follows: "deleted" instance are deleted by the call. The to-be-saved instances are returned, but not yet saved. After the change the deleted objects would still remain in the database after the snippet is run. Hence, the current patch is backwards incompatible.

I think a better fix would be to somehow remember that .save(commit=False) has already deleted the instances when calling the .save() again. This would get rid of the error, yet fix the problem.

comment:3 Changed 22 months ago by tuxcanfly

  • Cc tuxcanfly added
  • Owner changed from nobody to tuxcanfly
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Type changed from Uncategorized to Bug

Applied the patches and updated them according to akaariai's suggestions.

Pushed to my branch on github at:

Needs review.

comment:4 Changed 11 months ago by timo

Pull request for the branch:

comment:5 Changed 10 months ago by Kamu

Pull request requires rebasing.

comment:6 Changed 9 months ago by timo

  • Cc timograham@… added

Duplicate of #10284 which argues for changing the behavior and forcing users to handle formset.deleted_objects manually. Even though it's backwards incompatible, IMO, it's a cleaner fix as I wouldn't expect any data changing operations when using commit=False. Perhaps a django-developers discussion is needed.

comment:7 Changed 9 months ago by timo

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

Add Comment

Modify Ticket

Change Properties
<Author field>
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'

E-mail address and user name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.