[Django] #22747: Warning should be issued on new behaviour of formset.save(commit=False)
Django
--------------------------------------+------------------------
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.7-beta-2
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/22747>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: django@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version:
Component: Documentation | 1.7-beta-2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timo):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
"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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22747#comment:1>
Django
Reporter: django@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version:
Component: Documentation | 1.7-beta-2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by django@…):
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22747#comment:2>
Django
formset.save(commit=False)
-------------------------------------+-------------------------------------
Reporter: django@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version:
Component: Documentation | 1.7-beta-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* stage: Unreviewed => Accepted
* severity: Release blocker => Normal
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22747#comment:3>
Django
formset.save(commit=False)
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization | Version:
Component: Documentation | 1.7-beta-2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by django@…):
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
)
--
Ticket URL: <https://code.djangoproject.com/ticket/22747#comment:4>
Django
formset.save(commit=False)
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization | Version:
Component: Documentation | 1.7-beta-2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by russellm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22747#comment:5>
Django
formset.save(commit=False)
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization | Version:
Component: Documentation | 1.7-beta-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"2f7a7842baa50d74b9d4e93180375b7ff13b43e3"]:
{{{
#!CommitTicketReference repository=""
revision="2f7a7842baa50d74b9d4e93180375b7ff13b43e3"
Fixed #22747 -- Add backwards compatibility tip for new behavior of
formset.save(commit=False).
Thanks django at patjack.co.uk.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22747#comment:6>
Django
formset.save(commit=False)
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization | Version:
Component: Documentation | 1.7-beta-2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"868b5e5e957eae6d88be12b2e52ca971de81ed8d"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22747#comment:7>
Django
formset.save(commit=False)
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization | Version:
Component: Documentation | 1.7-beta-2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by django@…):
Thanks to all you guys. Keep up the good work.
--
Ticket URL: <https://code.djangoproject.com/ticket/22747#comment:8>