Opened 12 years ago

Closed 11 years ago

#17988 closed Bug (duplicate)

BaseModelFormset.save doesn't honor commit=False when deleting objects.

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

Description

When saving a ModelFormSet with some object(s) marked for deletion, calling formset.save(commit=False) still deletes the objects, which would then cause an obscure ValidationEror with invalid_choice when formset.save() 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 Remoun Metyas 12 years ago.
Check commit=True before deleting existing objects.
test_model_formset_checks_commit_before_delete.diff (1.7 KB ) - added by Remoun Metyas 12 years ago.
Regression test.

Download all attachments as: .zip

Change History (9)

by Remoun Metyas, 12 years ago

Check commit=True before deleting existing objects.

by Remoun Metyas, 12 years ago

Regression test.

comment:1 by Adrien Lemaire, 12 years ago

Cc: lemaire.adrien@… added
Triage Stage: UnreviewedAccepted

LGTM

comment:2 by Anssi Kääriäinen, 12 years ago

Cc: Anssi Kääriäinen added
Patch needs improvement: set

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

instances = formset.save(commit=False)
for instance in instances:
    instance.user = user
    instance.save()

Currently I think things would work as follows: "deleted" instance are deleted by the formset.save(commit=False) 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 by tuxcanfly, 12 years ago

Cc: tuxcanfly added
Owner: changed from nobody to tuxcanfly
Patch needs improvement: unset
Status: newassigned
Type: UncategorizedBug

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

Pushed to my branch on github at: https://github.com/tuxcanfly/django/tree/ticket_17988

Needs review.

comment:4 by Tim Graham, 11 years ago

Pull request for the branch: https://github.com/django/django/pull/165

comment:5 by Kamu, 11 years ago

Pull request requires rebasing.

comment:6 by Tim Graham, 11 years ago

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 by Tim Graham, 11 years ago

Resolution: duplicate
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top