Opened 5 years ago

Closed 5 years ago

#24756 closed Bug (invalid)

Unexpected behavior of ModelForm.save for fields with blank=False and overridden required attribute

Reported by: bboogaard Owned by: nobody
Component: Forms Version: 1.8
Severity: Release blocker Keywords: forms models save blank required
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Since upgrading to Django 1.8.1, I'm getting unexpected behavior when setting the required attribute for ModelForm fields to False for fields with blank=False in their field declaration on the model. Contrary to previous Django versions, the field is now ignored when calling the save() method of the ModelForm.

The model:

from django.db import models


class CodeName(models.Model):

    code = models.CharField('Code', max_length=20)

    name = models.CharField('Name', max_length=100)

The form:

from django import forms

from .models import CodeName


class CodeNameForm(forms.ModelForm):

    class Meta:
        model = CodeName
        fields = ['code', 'name']

    def __init__(self, *args, **kwargs):
        super(CodeNameForm, self).__init__(*args, **kwargs)
        self.fields['code'].required = False

Failing test:

from django.test import TestCase

from .forms import CodeNameForm
from .models import CodeName


class TestCodeNameForm(TestCase):

    def test_save(self):
        instance = CodeName(code='foo', name='Foobar')
        data = {
            'code': '',
            'name': 'Foobar'
        }
        form = CodeNameForm(data, instance=instance)
        form.full_clean()
        instance = form.save()
        self.assertEqual(instance.code, '')

Attachments (1)

models.patch (542 bytes) - added by bboogaard 5 years ago.
Proposed patch (django/forms/models.py)

Download all attachments as: .zip

Change History (6)

Changed 5 years ago by bboogaard

Attachment: models.patch added

Proposed patch (django/forms/models.py)

comment:1 Changed 5 years ago by Tim Graham

I am not sure if we should support this use case because with code='', the submitted model is invalid from a model validation standpoint. Could you elaborate on the use case to omit blank=True from the model field definition but accept blank values on the form?

The relevant code in forms/models.py.

Your proposed patch doesn't pass the test suite:

======================================================================
ERROR: test_blank_with_null_foreign_key_field (model_forms.tests.ModelFormBaseTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tim/code/django/tests/model_forms/tests.py", line 231, in test_blank_with_null_foreign_key_field
    self.assertTrue(f1.is_valid())
  File "/home/tim/code/django/django/forms/forms.py", line 168, in is_valid
    return self.is_bound and not self.errors
  File "/home/tim/code/django/django/forms/forms.py", line 160, in errors
    self.full_clean()
  File "/home/tim/code/django/django/forms/forms.py", line 378, in full_clean
    self._post_clean()
  File "/home/tim/code/django/django/forms/models.py", line 433, in _post_clean
    self.instance = construct_instance(self, self.instance, opts.fields, construct_instance_exclude)
  File "/home/tim/code/django/django/forms/models.py", line 60, in construct_instance
    f.save_form_data(instance, cleaned_data[f.name])
  File "/home/tim/code/django/django/db/models/fields/__init__.py", line 856, in save_form_data
    setattr(instance, self.name, data)
  File "/home/tim/code/django/django/db/models/fields/related.py", line 634, in __set__
    (instance._meta.object_name, self.field.name)
ValueError: Cannot assign None: "Student.character" does not allow null values.

comment:2 Changed 5 years ago by bboogaard

I am not sure if we should support this use case because with code=, the submitted model is invalid from a model validation standpoint.

Do you mean that the pre-1.8 behavior was unintended?

Last edited 5 years ago by bboogaard (previous) (diff)

comment:3 Changed 5 years ago by Tim Graham

In my mind, yes. There weren't any tests in Django's test suite to claim it was officially supported, at least.

comment:4 Changed 5 years ago by bboogaard

All right, if it was an unintended effect of previous versions, I'll simply change my model field declarations and code logic. I thought the override was supposed to work the way I described (i.e. in principle required, but for some form classes not required).

comment:5 Changed 5 years ago by Tim Graham

Resolution: invalid
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top