[Django] #19255: Cannot validate generic foreignkey in new instance
Django
--------------------------------------+--------------------
Reporter: bouke | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.contenttypes | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19255>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: bouke | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.contenttypes | Version: 1.4
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 matt@…):
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
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 import generic
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 = generic.GenericForeignKey()
def clean(self):
assert self.object_id
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 import generic
from django.contrib import admin
from testcase.models import Model, AnotherModel
class ModelInline(generic.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:
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/19255#comment:1>
Django
instances to their related object
--------------------------------------+------------------------------------
Reporter: bouke | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
See #25488 for a duplicate which proposes a solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/19255#comment:3>
Django
instances to their related object
Reporter: bouke | Owner:
| arielpontes
Type: Bug | Status: assigned
Component: | Version: 1.4
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by arielpontes):
* status: new => assigned
* owner: nobody => arielpontes
--
Ticket URL: <https://code.djangoproject.com/ticket/19255#comment:4>
Django
instances to their related object
-------------------------------------+-------------------------------------
Reporter: bouke | Owner:
| arielpontes
Type: Bug | Status: assigned
Component: | Version: 1.4
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by arielpontes):
= 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/19255#comment:5>