[Django] #24787: ModelForm validation crashes for ForeignKey field with blank=True and null=False
Django
null=False
-------------------------------+--------------------
Reporter: mssnlayam | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
Here's my model code
{{{
from django.db import models
from django.contrib.auth.models import User
class MyModel(models.Model):
user = models.ForeignKey(User, blank=True, null=False)
def clean(self):
defaultuser = User.objects.first()
if not defaultuser:
raise ValueError('Need at least one user')
self.user = defaultuser
return super(MyModel, self).clean()
}}}
Here's my code where I create and initialize a `ModelForm`
{{{
from __future__ import print_function
import django
django.setup()
from myapp.models import MyModel
from django.forms.models import modelform_factory
MyForm = modelform_factory(MyModel, fields=('user', ))
myform = MyForm({})
print(myform.is_valid())
}}}
Here's the crash on running the above program:
{{{
Traceback (most recent call last):
File "a.py", line 10, in <module>
print myform.is_valid()
File "/home/vagrant/bookshelf.git/my-env/local/lib/python2.7/site-
packages/django/forms/forms.py", line 184, in is_valid
return self.is_bound and not self.errors
File "/home/vagrant/bookshelf.git/my-env/local/lib/python2.7/site-
packages/django/forms/forms.py", line 176, in errors
self.full_clean()
File "/home/vagrant/bookshelf.git/my-env/local/lib/python2.7/site-
packages/django/forms/forms.py", line 394, in full_clean
self._post_clean()
File "/home/vagrant/bookshelf.git/my-env/local/lib/python2.7/site-
packages/django/forms/models.py", line 427, in _post_clean
self.instance = construct_instance(self, self.instance, opts.fields,
construct_instance_exclude)
File "/home/vagrant/bookshelf.git/my-env/local/lib/python2.7/site-
packages/django/forms/models.py", line 62, in construct_instance
f.save_form_data(instance, cleaned_data[f.name])
File "/home/vagrant/bookshelf.git/my-env/local/lib/python2.7/site-
packages/django/db/models/fields/__init__.py", line 874, in save_form_data
setattr(instance, self.name, data)
File "/home/vagrant/bookshelf.git/my-env/local/lib/python2.7/site-
packages/django/db/models/fields/related.py", line 619, in __set__
(instance._meta.object_name, self.field.name)
ValueError: Cannot assign None: "MyModel.user" does not allow null values.
}}}
What's the right way to support a `ForeignKey` with `blank=True` and
`null=False`? Form validation does not even raise `ValidationError`. It
simply crashes.
Had a conversation with @apollo13 on IRC who said that this is a bug to be
reported.
--
Ticket URL: <https://code.djangoproject.com/ticket/24787>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
null=False
Reporter: mssnlayam | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.8
Keywords: | Triage Stage: Unreviewed
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
* needs_docs: => 0
Comment:
In the `clean()` method, I believe you need to add the user to
`self.cleaned_data['user']` instead of simply assigning it to `self.user`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24787#comment:1>
Django
null=False
Reporter: mssnlayam | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mssnlayam):
The `clean()` method is for the model, not the form. The model has no
`cleaned_data`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24787#comment:2>
Django
null=False
Reporter: mssnlayam | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Right, I missed that. In your example code, do you need the field on the
form at all? If you always assign `defaultuser` in `clean()`, I think you
could move that to the `model.save()` method.
As model validation doesn’t run until after the instance is constructed
(where the error is being thrown), I'm not sure there's a simple fix
besides moving the logic to the `form.clean()` method as I hinted at
before.
--
Ticket URL: <https://code.djangoproject.com/ticket/24787#comment:3>
Django
null=False
Reporter: mssnlayam | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mssnlayam):
In my specific situation, the field is optional in the form. If the user
does not specify a value for the field, then `MyModel.clean()` will set
the field. That's what hoped to do, but I encountered the crash before I
got there. I will override `Form.clean()` and see if that works for me.
Thank you for your replies.
The code sample I have written is just to illustrate what I think is a bug
--- there is no good way to use a `ForeignKey` with `blank=True` and
`null=False` in a form. Maybe this should be documented somewhere. You can
keep this bug open if you think this is a code/documentation bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/24787#comment:4>
Django
null=False
Reporter: mssnlayam | Owner: nobody
Type: Uncategorized | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by abhaga):
* component: Uncategorized => Forms
--
Ticket URL: <https://code.djangoproject.com/ticket/24787#comment:5>
Django
Model.clean()
-----------------------------+------------------------------------
Reporter: mssnlayam | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: 1.8
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
* type: Uncategorized => New feature
Comment:
Not quite sure what the proper resolution would look like (i.e. if there's
a good way to allow the use case), but we can accept the ticket and
perhaps document the limitation if needed (although it seems a bit of an
edge case so I'm not sure where the best place to do so would be).
--
Ticket URL: <https://code.djangoproject.com/ticket/24787#comment:6>
Django
Model.clean()
-----------------------------+------------------------------------
Reporter: mssnlayam | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by monikasulik):
Maybe I'm missing something, but isn't the code in the clean method wrong
in the example? Shouldn't you be raising ValidationError instead of
ValueError?
--
Ticket URL: <https://code.djangoproject.com/ticket/24787#comment:7>
Django
Model.clean()
-----------------------------+------------------------------------
Reporter: mssnlayam | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mssnlayam):
The code in the clean method is correct. I deliberately raise ValueError
because this error is not related to field validation, but related to the
fact that there is not valid default value of user (in this example). The
key point of this example is that `clean()` tries to set the value of a
`blank=True` and `null=False` field. However model form validation causes
a crash even before that.
--
Ticket URL: <https://code.djangoproject.com/ticket/24787#comment:8>
Django
Model.clean()
-----------------------------+------------------------------------
Reporter: mssnlayam | Owner: nobody
Type: New feature | Status: closed
Component: Forms | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* resolution: => fixed
Comment:
The null assignment exception in the ticket description was removed in
04e13c89138d48c20e774a2b6bf06796f73ac0fe (#26179) which will be in Django
1.10. I believe that resolves this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/24787#comment:9>