ModelForm regression when setting None into a ForeignKey

[John Paulett]

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.

[John Paulett]

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.

[Claude Paroz]

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.

[Tim Graham]

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.

[Przemek Lewandowski]

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.

[Przemek Lewandowski]

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 ​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 ​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 ​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 ​the tests I've proposed.

Having fixed foreign key field behaviour the solution for the submitted issue is in place out of the box.

[Tim Graham]

Thanks for the detailed explanation. I've ​written to django-developers to ask if anyone has objections to that rationale.

[Tim Graham]

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.

[Tim Graham <timograham@…>]

In 375e1cfe:

Fixed #25349 -- Allowed a ModelForm to unset a fields with blank=True, required=False.