Cannot reference FK relation from inline ModelForm.save()

[Stanislas Guerra]

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$

[Tim Graham]

Do you have time to add a test now? tests/model_formsets is probably a good place. If not, I can pick this up.

[Stanislas Guerra]

Replying to 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.

[Stanislas Guerra]

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).

[Stanislas Guerra]

Replying to 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

[Tim Graham]

Thanks, I'm reviewing the issue and patch now.

[Tim Graham]

I think this ​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.

[Stanislas Guerra]

Sounds perfect and much cleaner to me, thanks!

[Tim Graham]

Do you think we should make any updates to the documentation added in the linked ticket?

[Stanislas Guerra]

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.

[Tim Graham <timograham@…>]

In 4c2f546b55c029334d22e69bb29db97f9356faa3:

Fixed #24395 -- Ensured inline ModelsForms have an updated related instance.

[Tim Graham <timograham@…>]

In d298b1ba5043eaa40f3f4bebe3c7634b359ba34b:

Reverted "Fixed #24325 -- Documented change in ModelForm.save() foreign key access."

This reverts commit 0af3822dc362b6253bda1c9699466dd0bbbf6066.
It's obsoleted by refs #24395.

[Tim Graham <timograham@…>]

In a3fca05b05551fdd7089e7be49d48c454ea50a84:

[1.8.x] Fixed #24395 -- Ensured inline ModelsForms have an updated related instance.

Backport of 4c2f546b55c029334d22e69bb29db97f9356faa3 from master

[Tim Graham <timograham@…>]

In 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