Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#22747 closed Cleanup/optimization (fixed)

Add backwards compatibility tip for new behaviour of formset.save(commit=False)

Reported by: django@… Owned by: nobody
Component: Documentation Version: 1.7-beta-2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I just stumbled across the small 'Changed in Django 1.7' message at https://docs.djangoproject.com/en/dev/topics/forms/formsets/#django.forms.formsets.BaseFormSet.can_delete regarding formset.save(commit=False)

I have a worry that for people upgrading to 1.7 this might be overlooked. I know it's in the changelog as well, under 'miscellaneous', but should it come under 'backwards incompatible changes' or the such like? Just to make it more obvious.

Although it appears to be a small change, it's quite a big, backwards incompatible one really.

Use Case (untested):

Django Admin "Mother" form with "Child" formset. I've overridden
def save_formset() say to make sure that each 'Child' formset, when saved, has the correct 'Father' assigned.

def save_formset(self, request, form, formset, change):
    children = formset.save(commit=False)
    mother = form.save(commit=False)
    for child in children:
         child.father = mother.husband
         child.save()

As you can see, upgrading to 1.7 any children removed from the admin formset will no longer be deleted from the database.

My code for 10.7 would look like:

def save_formset(self, request, form, formset, change):
    children = formset.save(commit=False)
    mother = form.save(commit=False)
    for deleted_child in formset.deleted_objects:
         deleted.delete()
    for child in children:
         # Don't know if this check is required, haven't read the code
         if child in formset.deleted_objects:
              continue
         child.father = mother.husband
         child.save()

OK, fine. But this code is backwards incompatible
Try running this code on Django 1.6 and you'll raise [this](https://github.com/django/django/blob/master/django/db/models/base.py#L742) error, because save(commit=False) does delete the object from the database, giving the in-memory object a pk = None

I've set this as a 'release blocker' since it's easy to fix, and will certainly make life easier for those updating if rectified

Change History (8)

comment:1 by Tim Graham, 10 years ago

"Miscellaneous" is a subsection of "Backwards Incompatible Changes".

I'm not sure about raising a warning. Do we have a precedent for similar changes? Unlike a DeprecationWarning where you can update your code to silence the warning, I don't see a way to do that here besides using something like warnings.filterwarnings().

When one reads the release notes, I'd think it should be pretty easy to search your code for save(commit=False) and see if it needs to be updated.

comment:2 by django@…, 10 years ago

I did have Django's new system check framework in mind when I created this, but I don't know how you'd remove the warning, as you say.

Maybe the note on save(commit=False) should mention something like:

# save(commit=False) doesn't delete formsets in Django 1.7+
objects = formset.save(commit=False)
# You then have to delete them yourself...
# To maintain backwards compatibility:
try:
    # For Django 1.7+
    for recurring_order in formset.deleted_objects:
        recurring_order.delete()
except AssertionError:
    # Django 1.6 already deletes the objects, so don't try deleting them again.
    pass

comment:3 by Tim Graham, 10 years ago

Severity: Release blockerNormal
Summary: Warning should be issued on new behaviour of formset.save(commit=False)Add backwards compatibility tip for new behaviour of formset.save(commit=False)
Triage Stage: UnreviewedAccepted

As far as I know, the system check framework is not designed for a situation like this. Unlike models, forms are not registered in a central location... nor is the check framework designed to track what methods are called on objects.

The improvement to the existing note seems reasonable. Have you tested it? Is AssertionError is raised when trying to delete an object that's already been deleted?

comment:4 by django@…, 10 years ago

All sounds reasonable.

AssertionError is most definitely raised. If you look at the link I gave in the first issue post: (which it appears I got wrong)
again : https://github.com/django/django/blob/master/django/db/models/base.py#L739

This is the line that raises the assertion error. It's because the 1.6 save(commit=False) does delete the model, and whilst doing so sets the pk of the object to None
(I can confirm this from https://github.com/django/django/blob/master/django/db/models/deletion.py#L294 )

comment:5 by Russell Keith-Magee, 10 years ago

Confirming @timo's intuition - this isn't what the system check framework is for. System check doesn't do a scan of code looking for potentially problematic code usage; it just checks models and other formal declarations for problems that would prevent startup or cause known problems.

comment:6 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 2f7a7842baa50d74b9d4e93180375b7ff13b43e3:

Fixed #22747 -- Add backwards compatibility tip for new behavior of formset.save(commit=False).

Thanks django at patjack.co.uk.

comment:7 by Tim Graham <timograham@…>, 10 years ago

In 868b5e5e957eae6d88be12b2e52ca971de81ed8d:

[1.7.x] Fixed #22747 -- Add backwards compatibility tip for new behavior of formset.save(commit=False).

Thanks django at patjack.co.uk.

Backport of 2f7a7842ba from master

comment:8 by django@…, 10 years ago

Thanks to all you guys. Keep up the good work.

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