[Django] #24395: Fixing #24325 (Cannot reference FK relation from inline ModelForm.save())
Django
----------------------------+--------------------
Reporter: Starou | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
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$
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24395>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
---------------------------------+-------------------------------------
Reporter: Starou | Owner: nobody
Type: Bug | Status: new
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:
New description:
Hi devs,
This is directly related to the closed ticket #24325 by Tim.
}}}
--
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
---------------------------------+-------------------------------------
Reporter: Starou | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Starou):
Replying to [comment:1 timgraham]:
> Do you have time to add a test now? `tests/model_formsets` is probably a
good place. If not, I can pick this up.
I can do that.
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:2>
Django
---------------------------------+-------------------------------------
Reporter: Starou | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Starou):
Jeez, Looks like I was Github logged-out when I submitted a comment maybe
a couple of hours ago. And same mistake now (and the textarea is
reset...).
Anyway, I was diging deeper in the Django admin code when I found out that
the `Author` instance (`new_object` [1]) binded to the `BookFormset`
instance was good where the one binded to the `BookFormset.forms` was not.
https://github.com/django/django/blob/master/django/contrib/admin/options.py#L1358
{{{
def changeform_view(self, request, object_id=None, form_url='',
extra_context=None):
...
ModelForm = self.get_form(request, obj)
if request.method == 'POST':
form = ModelForm(request.POST, request.FILES, instance=obj)
if form.is_valid():
form_validated = True
new_object = self.save_form(request, form, change=not add)
[1]
else:
form_validated = False
new_object = form.instance
formsets, inline_instances = self._create_formsets(request,
new_object, change=not add)
if all_valid(formsets) and form_validated:
self.save_model(request, new_object, form, not add)
self.save_related(request, form, formsets, not add)
if add:
self.log_addition(request, new_object)
return self.response_add(request, new_object)
else:
change_message =
self.construct_change_message(request, form, formsets)
self.log_change(request, new_object, change_message)
return self.response_change(request, new_object)
}}}
The problem could be in `BaseInlineFormSet._construct_form()`:
https://github.com/django/django/blob/master/django/forms/models.py#L884
{{{
def _construct_form(self, i, **kwargs):
form = super(BaseInlineFormSet, self)._construct_form(i, **kwargs)
if self.save_as_new:
# Remove the primary key from the form's data, we are only
# creating new instances
form.data[form.add_prefix(self._pk_field.name)] = None
# Remove the foreign key from the form's data
form.data[form.add_prefix(self.fk.name)] = None
# Set the fk value here so that the form can do its validation.
fk_value = self.instance.pk
if self.fk.rel.field_name != self.fk.rel.to._meta.pk.name:
fk_value = getattr(self.instance, self.fk.rel.field_name)
fk_value = getattr(fk_value, 'pk', fk_value)
setattr(form.instance, self.fk.get_attname(), fk_value)
return form
}}}
Using the following "patch" fixing the problem apparently:
{{{
- setattr(form.instance, self.fk.get_attname(), fk_value)
+ setattr(form.instance, self.fk.get_attname(), self.instance)
}}}
If you are not too much in a hurry, I can find some time tomorrow (GMT+1
here) to write a proper patch with tests (cannot right now).
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:3>
Django
---------------------------------+-------------------------------------
Reporter: Starou | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Starou):
Replying to [comment:3 Starou]:
>
> Using the following "patch" fixing the problem apparently:
>
> {{{
> - setattr(form.instance, self.fk.get_attname(), fk_value)
> + setattr(form.instance, self.fk.get_attname(), self.instance)
> }}}
>
That was a mistake because we'll get an `Author` instance in a
`Book.author_id` field which is casted in an integer later. For some
reason this only breaks if `self.instance` isn't saved in the database.
And of course - by design - we cannot do what was in my mind:
{{{
setattr(form.instance, self.fk.name, self.instance)
}}}
Without raising an exception with an unsaved object:
{{{
ValueError: Cannot assign "<Poet: >": "Poet" instance isn't saved in the
database.
}}}
So I'll stick with my first idea of updating the forms references in the
inline_formset.save() method:
https://github.com/Starou/django/compare/ticket_24395
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:4>
Django
---------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Forms | Version: 1.8alpha1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => assigned
* owner: nobody => timgraham
* has_patch: 0 => 1
Comment:
Thanks, I'm reviewing the issue and patch now.
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:5>
Django
---------------------------------+-------------------------------------
Reporter: Starou | Owner: timgraham
Type: Bug | Status: assigned
Component: Forms | Version: 1.8alpha1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I think this [https://github.com/django/django/pull/4204 simplified PR]
might be sufficient to address this. Mainly, there's only a need to make
the assignment when saving new objects, since existing objects must
already have a saved related instance. Let me know if you see any problems
with this solution and thank-you very much for the initial patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:6>
Django
---------------------------------+-------------------------------------
Reporter: Starou | Owner: timgraham
Type: Bug | Status: assigned
Component: Forms | Version: 1.8alpha1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Starou):
Sounds perfect and much cleaner to me, thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:7>
Django
---------------------------------+-------------------------------------
Reporter: Starou | Owner: timgraham
Type: Bug | Status: assigned
Component: Forms | Version: 1.8alpha1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Do you think we should make any updates to the documentation added in the
linked ticket?
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:8>
Django
---------------------------------+-------------------------------------
Reporter: Starou | Owner: timgraham
Type: Bug | Status: assigned
Component: Forms | Version: 1.8alpha1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Starou):
Maybe your release notes update from ticket #24325 is not relevant anymore
?
https://github.com/django/django/commit/8657e7caaac41266d9ac0b73a21af64edc681613
{{{
+Now if the related instance hasn't been saved (for example, when adding
an
+author and some inlined books in the admin), accessing the foreign key
+``book.author`` in the example) will raise ``RelatedObjectDoesNotExist``.
}}}
The example is not true, I think the related was saved in the admin.
But before the patch, although it was saved, you'd get the exception as
described.
With the patch, the instance is saved and now accessible in Form.save().
Concerning the documentation, I can't see what we can add unless the
previous behaviour was documented.
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:9>
Django
---------------------------------+-------------------------------------
Reporter: Starou | Owner: timgraham
Component: Forms | Version: 1.8alpha1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"4c2f546b55c029334d22e69bb29db97f9356faa3"]:
{{{
#!CommitTicketReference repository=""
revision="4c2f546b55c029334d22e69bb29db97f9356faa3"
Fixed #24395 -- Ensured inline ModelsForms have an updated related
instance.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:10>
Django
---------------------------------+-------------------------------------
Reporter: Starou | Owner: timgraham
Type: Bug | Status: closed
Component: Forms | Version: 1.8alpha1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"d298b1ba5043eaa40f3f4bebe3c7634b359ba34b"]:
{{{
#!CommitTicketReference repository=""
revision="d298b1ba5043eaa40f3f4bebe3c7634b359ba34b"
Reverted "Fixed #24325 -- Documented change in ModelForm.save() foreign
key access."
This reverts commit 0af3822dc362b6253bda1c9699466dd0bbbf6066.
It's obsoleted by refs #24395.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:11>
Django
---------------------------------+-------------------------------------
Reporter: Starou | Owner: timgraham
Type: Bug | Status: closed
Component: Forms | Version: 1.8alpha1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a3fca05b05551fdd7089e7be49d48c454ea50a84"]:
{{{
#!CommitTicketReference repository=""
revision="a3fca05b05551fdd7089e7be49d48c454ea50a84"
[1.8.x] Fixed #24395 -- Ensured inline ModelsForms have an updated related
instance.
Backport of 4c2f546b55c029334d22e69bb29db97f9356faa3 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:12>
Django
---------------------------------+-------------------------------------
Reporter: Starou | Owner: timgraham
Type: Bug | Status: closed
Component: Forms | Version: 1.8alpha1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"81911f29b70710d8f72916c49a69aa5df5cd7df8"]:
{{{
#!CommitTicketReference repository=""
revision="81911f29b70710d8f72916c49a69aa5df5cd7df8"
[1.8.x] Reverted "Fixed #24325 -- Documented change in ModelForm.save()
foreign key access."
This reverts commit 0af3822dc362b6253bda1c9699466dd0bbbf6066.
It's obsoleted by refs #24395.
Backport of d298b1ba5043eaa40f3f4bebe3c7634b359ba34b from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24395#comment:13>