BaseGenericInlineFormSet runs validation methods before linking form instances to their related object

[Bouke Haarsma]

Given this model:

class Model(models.Model):
    content_type = models.ForeignKey(ContentType)
    object_id = models.PositiveIntegerField()
    object = generic.GenericForeignKey()

    def clean(self):
        assert self.object_id
        assert self.object

and this admin:

class ModelInline(generic.GenericStackedInline):
    model = Model

When saving a new Model, the assertion will fail. The object will not be linked, nor will the object_id be available to the clean function. This is because the fields are excluded from the formset and the values are set through the generic InlineAdmin, see this excerpt:

generic.py:412
    def save_new(self, form, commit=True):
        # Avoid a circular import.
        from django.contrib.contenttypes.models import ContentType
        kwargs = {
            self.ct_field.get_attname(): ContentType.objects.get_for_model(self.instance).pk,
            self.ct_fk_field.get_attname(): self.instance.pk,
        }
        new_obj = self.model(**kwargs)
        return save_instance(form, new_obj, commit=commit)

To hack around this issue, the forms and formsets involved should not cache the validation result. The clean function should then validate the value if it is present (the second time when cleaning). The hack is quite ugly and requires a lot of coding. It should be possible to easily validate the newly related-to foreign key object.

[matt@…]

I'm able to reproduce the problem by adding an extra field into the Model class and creating another models which Model can point to:

from django.db import models
from django.contrib.contenttypes.fields import GenericForeignKey
from django.contrib.contenttypes.models import ContentType

class Model(models.Model):
    number = models.IntegerField()
    content_type = models.ForeignKey(ContentType, related_name='content')
    object_id = models.PositiveIntegerField()
    object = GenericForeignKey()

    def clean(self):
        assert self.object_id

class AnotherModel(models.Model):
    another_number = models.IntegerField()

Then I create a model admin for the other model:

from django.contrib.contenttypes.admin import GenericStackedInline
from django.contrib import admin

from .models import Model, AnotherModel

class ModelInline(GenericStackedInline):
    model = Model

class AnotherModelAdmin(admin.ModelAdmin):
    inlines = (ModelInline,)

admin.site.register(AnotherModel, AnotherModelAdmin)

When I create a new AnotherModel and try to save it I get this:

Environment:


Request Method: POST
Request URL: http://127.0.0.1:8000/admin/testcase/anothermodel/add/

Django Version: 1.6.dev20121122090508
Python Version: 2.7.2
Installed Applications:
('django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.sites',
 'django.contrib.messages',
 'django.contrib.staticfiles',
 'testcase',
 'django.contrib.admin')
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware')


Traceback:
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/core/handlers/base.py" in get_response
  116.                         response = callback(request, *callback_args, **callback_kwargs)
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/contrib/admin/options.py" in wrapper
  369.                 return self.admin_site.admin_view(view)(*args, **kwargs)
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/utils/decorators.py" in _wrapped_view
  91.                     response = view_func(request, *args, **kwargs)
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/views/decorators/cache.py" in _wrapped_view_func
  89.         response = view_func(request, *args, **kwargs)
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/contrib/admin/sites.py" in inner
  202.             return view(request, *args, **kwargs)
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/utils/decorators.py" in _wrapper
  25.             return bound_func(*args, **kwargs)
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/utils/decorators.py" in _wrapped_view
  91.                     response = view_func(request, *args, **kwargs)
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/utils/decorators.py" in bound_func
  21.                 return func(self, *args2, **kwargs2)
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/db/transaction.py" in inner
  208.                 return func(*args, **kwargs)
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/contrib/admin/options.py" in add_view
  1035.             if all_valid(formsets) and form_validated:
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/forms/formsets.py" in all_valid
  380.         if not formset.is_valid():
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/forms/formsets.py" in is_valid
  277.         err = self.errors
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/forms/formsets.py" in errors
  259.             self.full_clean()
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/forms/formsets.py" in full_clean
  298.             self._errors.append(form.errors)
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/forms/forms.py" in _get_errors
  117.             self.full_clean()
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/forms/forms.py" in full_clean
  274.         self._post_clean()
File "/Users/matt/Code/envs/django/lib/python2.7/site-packages/django/forms/models.py" in _post_clean
  333.             self.instance.clean()
File "/Users/matt/Code/envs/django/tickets/19255/testsite/testcase/models.py" in clean
  12.         assert self.object_id

Exception Type: AssertionError at /admin/testcase/anothermodel/add/
Exception Value: 

[Tim Graham]

See #25488 for a duplicate which proposes a solution.

[Ariel Pontes]

Update

Consider these models:

class Child(models.Model):
    content_type = models.ForeignKey(ContentType)
    object_id = models.PositiveIntegerField()
    object = GenericForeignKey()

    def clean(self):
        assert self.object_id

class Parent(models.Model):
    parent_number = models.IntegerField()

And these admins:

class ChildInline(GenericStackedInline):
    model = Child

class ParentAdmin(admin.ModelAdmin):
    inlines = (ChildInline,)

admin.site.register(Parent, ParentAdmin)

The easier part of the problem (#25488)

I started investigating this problem as described in #25488 and I came up with the following solution:

1. Moving the linking from save_new to _construct_form. It solves the problem described in that ticket, which reported a case in which there was a FormSet which was always initialized with a ParentModel instance.

This solution, however, breaks ANY attempt of saving a child inline in a "create parent" admin form. This happens because the FormSet is not initialized with a Parent instance in the admin, after all the parent doesn't exist yet (it's being created in the same form submission), which brings me to the revised solution:

2. Doing the linking both in save_new AND _construct_form. This way, if a form is instantiated with an instance parameter, the clean will be able to access its instance's related object. It will only fail in the very particular case described above. It duplicates the linking but is still an improvement over the current behavior. It fixes the problem in #25488 without breaking anything (I ran the tests and they passed).

The harder part of the problem

Consider the following case:

• you have a Child model with a GenericRelation,
• validation in the Child's clean method that relies on the existence of the related object,
• a Parent model,
• a ChildInline,
• a ParentAdmin with a ChildInline,
• and you want to create a new Parent record while at the same time saving a new Child record (in the same form submission)

Currently, this is pretty much impossible. Any solution I can think of entails some of the following serious compromises:

3. We give up on the atomicity of the admin actions and save the parent record before validating the inlines. If there are errors, we show some (warning?) message like:

Your parent was created successfully but saving some of the children failed. Please correct the mistakes and re-submit the form to add them.

4. We enforce the transactionality of this operation in the application layer, creating a parent record and deleting it if validation of the inlines fail. I have no idea how acceptable this is. Intuitively I'm not a big fan.

5. We by default don't show inlines (inheriting from BaseInlineFormSet) in admin "new record" forms at all. It only appears once the parent is saved, in "edit record" forms.

Conclusion

Since I can't think of alternatives, in principle I'm going for the second solution. I'm thinking perhaps an improvement would be catching the django.db.models.fields.related.RelatedObjectDoesNotExist Exception in BaseInlineFormSet.full_clean and adding a more user-friendly message to the formset errors. Something along the lines of:

>>> formset.non_form_errors()
['Please make sure a parent record exists before trying to save a child.']

It might give the impression that this is never possible though, while in fact it's only not possible when there's a custom clean method trying to access the related object (the error would only be displayed in this situation).

Another idea would be throwing a more explanatory Django error, perhaps something like:

django.db.models.fields.related.RelatedObjectDoesNotExist: Cannot validate child's related object on parent creation

In any case, this solution wouldn't break any current code. The error would appear if there's a custom clean, otherwise the parent will be linked in save_new as usual.