[Django] #25349: ModelForm regression when setting None into a ForeignKey
Django
-------------------------------+--------------------
Reporter: jpaulett | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
It appears that when using a ModelForm to "unset" a non-required foreign
key field, Django 1.8 no longer will set the foreign key to None when
saving the ModelForm, and the prior value for the ForeignKey remains.
In practice, imagine a form where users can pick a category to assign to
the instance, but can un-assign its category by picking the empty_label
value in the <select>.
The behaviour worked previously in Django, including 1.6 and 1.7.9, but
appears when testing against Django 1.8.5. I have also tested the
stable/1.8.x and master branches on github, which still show the issue. I
have reviewed the Release Notes and ModelForms documentation and can not
see anything that would indicate this behaviour has changed.
models.py:
{{{
from django.db import models
class Child(models.Model):
name = models.TextField()
def __unicode__(self):
return self.name
class Parent(models.Model):
name = models.TextField()
child = models.ForeignKey(Child, null=True)
def __unicode__(self):
return self.name
}}}
forms.py:
{{{
from django import forms
from .models import Parent, Child
class ParentForm(forms.ModelForm):
# field definition only set so `required = False`. Have also tried
# a custom ParentForm.__init__ with `self.fields['child'].required =
False`
child = forms.ModelChoiceField(
queryset=Child.objects.all(),
required=False
)
class Meta:
model = Parent
fields = ('name', 'child')
def __init__(self, *args, **kwargs):
super(ParentForm, self).__init__(*args, **kwargs)
# self.fields['child'].required = False
}}}
tests.py:
{{{
from django.test import TestCase
from .forms import ParentForm
from .models import Parent, Child
class ParentFormTest(TestCase):
def test_unset(self):
p = Parent.objects.create(
name='p',
child=Child.objects.create(name='c')
)
form = ParentForm(
data={'name': 'new name', 'child': None},
instance=p
)
self.assertTrue(form.is_valid())
form.save()
obj = Parent.objects.get()
self.assertEqual(obj.name, 'new name')
self.assertIsNone(obj.child) # ASSERTION FAILS
def test_set_alternative(self):
p = Parent.objects.create(
name='p',
child=Child.objects.create(name='c')
)
form = ParentForm(
data={'name': 'new name',
'child': Child.objects.create(name='alt').id},
instance=p
)
self.assertTrue(form.is_valid())
form.save()
obj = Parent.objects.get()
self.assertEqual(obj.name, 'new name')
self.assertEqual(obj.child.name, 'alt') # ASSERTION WORKS
}}}
Result when running against Django 1.8.5:
{{{
Creating test database for alias 'default'...
.F
======================================================================
FAIL: test_unset (demo.tests.ParentFormTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/user/modelform-issue/demo/tests.py", line 24, in test_unset
self.assertIsNone(obj.child) # ASSERTION FAILS
AssertionError: <Child: c> is not None
----------------------------------------------------------------------
Ran 2 tests in 0.007s
FAILED (failures=1)
Destroying test database for alias 'default'...
}}}
I have tried tracing through the ModelForm.save code with pdb. The
form.cleaned_data appears correct (`{'name': u'new name', 'child': None}`.
At the point that `save_instance` is called, it appears that the
`instance.child`.
--
Ticket URL: <https://code.djangoproject.com/ticket/25349>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jpaulett | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | 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 jpaulett):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
It appears that adding `blank=True` to the model's field definition
results in the expected behaviour.
Conceptually, it does make sense that at the model level, I'm allowing
blank values. However, it seems a bit surprising that the original
apparent illogical combination (blank=False, required=False) does not
result in a validation error or system check warning, thus keeping the
prior value in the field.
Should this result in a ValidationError? Or perhaps a small note in the
1.8 release notes to state that for ModelForms with required=False
ModelChoiceFields, that blank=True is required on the model's ForeignKey
field?
I'd be happy to assist, but just seek clarification on intended / desired
behaviour in this case.
--
Ticket URL: <https://code.djangoproject.com/ticket/25349#comment:1>
Django
Reporter: jpaulett | Owner: nobody
Type: Bug | 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
Changes (by claudep):
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
I've not investigated myself, but I think at first sight that setting the
empty choice to a `ForeignKey` with `blank=False` should raise a
ValidationError.
If it worked in the past, would be nice to git bisect the change.
--
Ticket URL: <https://code.djangoproject.com/ticket/25349#comment:2>
Django
Reporter: jpaulett | Owner: nobody
Type: Bug | 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 timgraham):
If I had to guess 45e049937d2564d11035827ca9a9221b86215e45 might be the
related commit.
System checks don't really work for forms (at least we don't have any
now), since options like `Field.required` can be set dynamically in
`Form.__init__()` and system checks are static analysis. I don't know if
we must require `blank=True` on model fields for `required=False`, but I
agree it would be good to investigate and document the findings.
--
Ticket URL: <https://code.djangoproject.com/ticket/25349#comment:3>
Django
Reporter: jpaulett | Owner: haxoza
Type: Bug | Status: assigned
Component: Forms | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by haxoza):
* owner: nobody => haxoza
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/25349#comment:4>
Django
Reporter: jpaulett | Owner: haxoza
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by haxoza):
Hi! I was digging this issue for a few hours and here are my findings:
* I run the tests proposed above by jpaulett and they failed as it's
described
* I was trying to prepare a fix digging in `django/forms/models.py` but
with no luck
* then I discovered then it is a wider misbehaviour of model forms fields
with `required=False` and generated from model fields with `blank=False`
(below there is a test)
* I've tried to run git bisect but with no luck - however the sure thing
is the issue wasn't there in 1.7.10
Here's a test for more general issue I found:
models.py:
{{{
from django.db import models
class Writer(models.Model):
name = models.CharField(max_length=30, blank=False)
}}}
tests.py:
{{{
from django import forms
from django.test.testcases import TestCase
from .models import Writer
class CustomWriterForm(forms.ModelForm):
name = forms.CharField(required=False)
class Meta(object):
model = Writer
fields = '__all__'
class ModelFormTest(TestCase):
def test_save_blank_false_with_required_false(self):
obj = Writer.objects.create(name='test')
value = ''
form = CustomWriterForm(data={'name': value}, instance=obj)
self.assertTrue(form.is_valid())
obj = form.save()
self.assertEqual(obj.name, value) # ASSERTION FAILS
}}}
When running the test in master branch:
{{{
$ python tests/runtests.py
aaaaa.tests.ModelFormTest.test_save_blank_false_with_required_false
Testing against Django installed in
'/Users/haxoza/dev/projects/django/django' with up to 4 processes
Creating test database for alias 'default'...
Creating test database for alias 'other'...
F
======================================================================
FAIL: test_save_blank_false_with_required_false
(aaaaa.tests.ModelFormTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/haxoza/dev/projects/django/tests/aaaaa/tests.py", line 24,
in test_save_blank_false_with_required_false
self.assertEqual(obj.name, value)
AssertionError: 'test' != ''
----------------------------------------------------------------------
Ran 1 test in 0.002s
FAILED (failures=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...
}}}
The test fails and I think what should actually happen is that the model
should be saved with empty string value.
In my opinion it should be reported as a separate issue and referenced to
this one because it looks quite serious.
--
Ticket URL: <https://code.djangoproject.com/ticket/25349#comment:5>
Django
Reporter: jpaulett | Owner:
Type: Bug | Status: new
Component: Forms | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by haxoza):
* owner: haxoza =>
* status: assigned => new
* has_patch: 0 => 1
Comment:
As I showed in tests from my previous comment the submitted bug is a
little bit more general. The main thing is that when model field with
`blank=False` is overridden by form field with `required=False` then
saving the model doesn't change the value in model instance nor in
database. This test is added
[https://github.com/django/django/pull/5658/files#diff-
74c67aea7429ebe5e21f206f24b4418bR266 here in my pull request].
I run git bisect tool to find which commit introduced this more general
bug. Tim was right on his first guess that commit
45e049937d2564d11035827ca9a9221b86215e45 broke the model forms behaviour
(#13776).
After further digging I came into the fact that in `_post_clean()` method
on `BaseModelForm` class `exclude` parameter in the following line:
{{{
self.instance = construct_instance(self, self.instance, opts.fields,
exclude)
}}}
has to be changed to `opts.exclude`. What's more the instance has to be
constructed once exclusions for further validation are prepared (because
`_get_validation_exclusions()` methods looks into `self._errors`). Here's
[https://github.com/django/django/pull/5658/files#diff-
70af885c2725fe87eb3b99a393268d10L379 the change I made].
The solution described above fixes the bug I found but it still doesn't
fix it for the bug originally described by jpaulett. The problem here is
that the None value cannot be assigned to foreign key field in a process
of model instance construction.
Long time ago Jacob
[https://github.com/haxoza/django/commit/1452d46240609ff39dacf9ea2f759ed600c2f09f
#diff-3010fc5a498b7171c342520f34507968R192 introduced a check for foreign
keys] which prevents `None` value assignment if the field is `null=False`.
In that time this decision was probably right.
However currently there are a few points against this check:
* Conceptually it is a programmer's responsibility to validate the data
assigned to foreign key in a right moment - not necessarily during
assignment to the model field.
* This behaviour is not symmetrical to other database-related constraints
as for example `unique=True` which is deferred to `save()` method (raising
IntegrityError) or `full_clean()` method (raising ValidationError).
* In #13776 the decision was made that None value for foreign key field
with `null=False` and corresponding form field with `required=False`
should be valid. It means that after model instantiation by such form the
value of given field is empty or just unset (not defined). Whatever the
state is, it doesn't matter. It shows only that currently discussed check
does **not** prevent in 100% against having foreign key field with
`null=False` set to `None`. It undermines the legitimacy of presence for
the discussed check.
As pointed above it has much more logical sense to defer validation of
non-nullable foreign key field to `save()` method and `full_clean()`
method. What's interesting deletion of the code introduced by Jacob
results in expected behaviour as highlighted in
[https://github.com/django/django/pull/5658/files#diff-
e4aa8dc49d7522ae55a932ffc7d09c1cR250 the tests I've proposed].
Having fixed foreign key field behaviour the solution for the submitted
issue is in place out of the box.
--
Ticket URL: <https://code.djangoproject.com/ticket/25349#comment:6>
Django
Reporter: jpaulett | Owner:
Type: Bug | Status: new
Component: Forms | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Thanks for the detailed explanation. I've
[https://groups.google.com/d/topic/django-
developers/4UL_FkDXX-w/discussion written to django-developers] to ask if
anyone has objections to that rationale.
--
Ticket URL: <https://code.djangoproject.com/ticket/25349#comment:7>
Django
Reporter: jpaulett | Owner:
Type: Bug | Status: new
Component: Forms | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
The consensus is to remove the check that prevents assigning `None` to a
foreign key field if `null=False` as proposed here. For clarity and
completeness (there's another similar check which the current PR doesn't
remove), let's do that removal as a separate ticket (#26179) and then
rebase the fix for this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/25349#comment:8>
Django
Reporter: jpaulett | Owner: Tim
| Graham <timograham@…>
Type: Bug | Status: closed
Component: Forms | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tim Graham <timograham@…>):
* owner: => Tim Graham <timograham@…>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"375e1cfe2b2e1c3c57f882147c34902c6e8189ac" 375e1cfe]:
{{{
#!CommitTicketReference repository=""
revision="375e1cfe2b2e1c3c57f882147c34902c6e8189ac"
Fixed #25349 -- Allowed a ModelForm to unset a fields with blank=True,
required=False.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25349#comment:9>