#24395: Cannot reference FK relation from inline ModelForm.save()
---------------------------------+-------------------------------------
Reporter: Starou | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8alpha1
Severity: Release blocker | Resolution:
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 timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* version: master => 1.8alpha1
* needs_docs: => 0
* stage: Unreviewed => Accepted
* severity: Normal => Release blocker
Old description:
> Hi devs,
>
> This is directly related to the closed ticket #24235 by Tim.
>
> The problem is that the instance of the related object (an `Author`
> instance in my example) in the formset's forms instances (`BookForm`) is
> not up-to-date.
>
> In the admin, the call stack is:
>
> {{{
> # django.contrib.admin.options.py
> ModelAdmin.changeform_view():
> ...
> new_object = self.save_form(request, form, change=not add)
> ...
> self.save_model(request, new_object, form, not add)
> self.save_related(...) [1]
>
> def save_related():
> self.save_formset(request, form, formset, change=change) [2]
>
> def save_formset(self, request, form, formset, change):
> formset.save() [3]
> }}}
>
> At this point [1], the main form has already been saved and the
> `new_object` (an `Author` instance) is in the database.
> This `new_object` instance is correctly bound to the formset(s)
> instance(s) but not to the formsets' forms.
>
> IMAO this is a bug.
> Since the `new_object` is saved in the database and all the forms have
> been validated, there is no reason to have an old instance attached to
> the ModelForm.instance attribute (which is a `Book` instance in my
> example).
>
> A basic patch could be something like that:
>
> {{{
> diff --git a/django/forms/models.py b/django/forms/models.py
> index 98f84a0..40217f4 100644
> --- a/django/forms/models.py
> +++ b/django/forms/models.py
> @@ -649,6 +649,7 @@ class BaseModelFormSet(BaseFormSet):
> for form in self.saved_forms:
> form.save_m2m()
> self.save_m2m = save_m2m
> + self.set_forms_related_object(self.instance)
> return self.save_existing_objects(commit) +
> self.save_new_objects(commit)
>
> save.alters_data = True
> @@ -656,6 +657,11 @@ class BaseModelFormSet(BaseFormSet):
> def clean(self):
> self.validate_unique()
>
> + def set_forms_related_object(self, obj):
> + rel_name = self.fk.name
> + for form in self.forms:
> + setattr(form.instance, rel_name, obj)
> +
> def validate_unique(self):
> # Collect unique_checks and date_checks to run from all the
> forms.
> all_unique_checks = set()
>
> }}}
>
> What do you guys think ?
>
> This patch passed most of admin tests. But I got a segfault with the last
> master checkout either I use the patch or not.
>
> {{{
> stan@stanislrrasimac:tests$ PYTHONPATH=..:$PYTHONPATH python runtests.py
> admin_views -v 2
> Testing against Django installed in
> '/Users/stan/src/Django/repos/django/django/django'
> Importing application admin_views
> Creating test database for alias 'default' (':memory:')...
> ...
> test_logout_and_password_change_URLs
> (admin_views.tests.AdminViewBasicTest) ... ok
> test_multiple_sort_same_field (admin_views.tests.AdminViewBasicTest) ...
> ok
> test_named_group_field_choices_change_list
> (admin_views.tests.AdminViewBasicTest) ... ok
> test_named_group_field_choices_filter
> (admin_views.tests.AdminViewBasicTest) ... ok
> test_popup_add_POST (admin_views.tests.AdminViewBasicTest) ... ok
> test_popup_dismiss_related (admin_views.tests.AdminViewBasicTest) ... ok
> test_proxy_model_content_type_is_used_for_log_entries
> (admin_views.tests.AdminViewBasicTest) ... ok
> test_relation_spanning_filters (admin_views.tests.AdminViewBasicTest) ...
> ok
> test_resolve_admin_views (admin_views.tests.AdminViewBasicTest) ... ok
> test_sort_indicators_admin_order (admin_views.tests.AdminViewBasicTest)
> ... ok
> Segmentation fault: 11
> stan@stanislrrasimac:tests$
> }}}
New description:
Hi devs,
This is directly related to the closed ticket #24325 by Tim.
The problem is that the instance of the related object (an `Author`
instance in my example) in the formset's forms instances (`BookForm`) is
not up-to-date.
In the admin, the call stack is:
{{{
# django.contrib.admin.options.py
ModelAdmin.changeform_view():
...
new_object = self.save_form(request, form, change=not add)
...
self.save_model(request, new_object, form, not add)
self.save_related(...) [1]
def save_related():
self.save_formset(request, form, formset, change=change) [2]
def save_formset(self, request, form, formset, change):
formset.save() [3]
}}}
At this point [1], the main form has already been saved and the
`new_object` (an `Author` instance) is in the database.
This `new_object` instance is correctly bound to the formset(s)
instance(s) but not to the formsets' forms.
IMAO this is a bug.
Since the `new_object` is saved in the database and all the forms have
been validated, there is no reason to have an old instance attached to the
ModelForm.instance attribute (which is a `Book` instance in my example).
A basic patch could be something like that:
{{{
diff --git a/django/forms/models.py b/django/forms/models.py
index 98f84a0..40217f4 100644
--- a/django/forms/models.py
+++ b/django/forms/models.py
@@ -649,6 +649,7 @@ class BaseModelFormSet(BaseFormSet):
for form in self.saved_forms:
form.save_m2m()
self.save_m2m = save_m2m
+ self.set_forms_related_object(self.instance)
return self.save_existing_objects(commit) +
self.save_new_objects(commit)
save.alters_data = True
@@ -656,6 +657,11 @@ class BaseModelFormSet(BaseFormSet):
def clean(self):
self.validate_unique()
+ def set_forms_related_object(self, obj):
+ rel_name = self.fk.name
+ for form in self.forms:
+ setattr(form.instance, rel_name, obj)
+
def validate_unique(self):
# Collect unique_checks and date_checks to run from all the
forms.
all_unique_checks = set()
}}}
What do you guys think ?
This patch passed most of admin tests. But I got a segfault with the last
master checkout either I use the patch or not.
{{{
stan@stanislrrasimac:tests$ PYTHONPATH=..:$PYTHONPATH python runtests.py
admin_views -v 2
Testing against Django installed in
'/Users/stan/src/Django/repos/django/django/django'
Importing application admin_views
Creating test database for alias 'default' (':memory:')...
...
test_logout_and_password_change_URLs
(admin_views.tests.AdminViewBasicTest) ... ok
test_multiple_sort_same_field (admin_views.tests.AdminViewBasicTest) ...
ok
test_named_group_field_choices_change_list
(admin_views.tests.AdminViewBasicTest) ... ok
test_named_group_field_choices_filter
(admin_views.tests.AdminViewBasicTest) ... ok
test_popup_add_POST (admin_views.tests.AdminViewBasicTest) ... ok
test_popup_dismiss_related (admin_views.tests.AdminViewBasicTest) ... ok
test_proxy_model_content_type_is_used_for_log_entries
(admin_views.tests.AdminViewBasicTest) ... ok
test_relation_spanning_filters (admin_views.tests.AdminViewBasicTest) ...
ok
test_resolve_admin_views (admin_views.tests.AdminViewBasicTest) ... ok
test_sort_indicators_admin_order (admin_views.tests.AdminViewBasicTest)
... ok
Segmentation fault: 11
stan@stanislrrasimac:tests$
}}}
--
Comment:
Do you have time to add a test now? `tests/model_formsets` is probably a
good place. If not, I can pick this up.
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/064.d32ac4325b7f0001fb048d7d87b3d6c2%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
