Model.clean_fields incorrectly skips validation for fields where null value is not allowed

[silveroff@…]

I'm curious why does this method skips validation for a field with None value. As a side effect if field has null=False, skipping validation will result in DatabaseError saying that coulmn cannot be null. Why don't we check if None is a valid value for a field, taking into account current status of null option? This would save developers from nasty bugs and boilerplate in their model forms.

def clean_fields(self, exclude=None):
        """
        Cleans all fields and raises a ValidationError containing message_dict
        of all validation errors if any occur.
        """
        if exclude is None:
            exclude = []

        errors = {}
        for f in self._meta.fields:
            if f.name in exclude:
                continue
            # Skip validation for empty fields with blank=True. The developer
            # is responsible for making sure they have a valid value.
            raw_value = getattr(self, f.attname)
            if f.blank and raw_value in f.empty_values:
                continue
            try:
                setattr(self, f.attname, f.clean(raw_value, self))
            except ValidationError as e:
                errors[f.name] = e.error_list

        if errors:
            raise ValidationError(errors)

[Tim Graham]

Could you give a concrete example of a form that exhibits the unexpected behavior?

[anonymous]

Sure,

# Model
class Example(models.Model):
    optional_decimal = models.DecimalField(
        blank=True,
        null=False,   # so I want this field to be optional in my forms, but I dont want null values in db
        default=Decimal(0),
        validators=[MinValueValidator(0)],
        max_digits=8,
        decimal_places=2,
    )

# Form
class ExampleForm(ModelForm):
    class Meta:
        model = Example

Now if one create object using this form without providing value for optional_decimal field this would result in IntegrityError: (1048, "Column 'optional_decimal' cannot be null")
So developer has to create ExampleForm.clean_optional_decimal method to handle this issue, and thats not DRY and wouldn't be needed if model.clean_fields run validation at the first place. Something like this
would suffice:

def clean_fields(self, exclude=None):
        """
        Cleans all fields and raises a ValidationError containing message_dict
        of all validation errors if any occur.
        """
        if exclude is None:
            exclude = []

        errors = {}
        for f in self._meta.fields:
            if f.name in exclude:
                continue
            # Skip validation for empty fields with blank=True. The developer
            # is responsible for making sure they have a valid value.
            raw_value = getattr(self, f.attname)
            if f.blank and raw_value in f.empty_values:
                # do not skip validation if raw_value is None and field does not accept null values to be saved!
                if raw_value is None and not f.null:
                    pass
                else:
                    continue
            try:
                setattr(self, f.attname, f.clean(raw_value, self))
            except ValidationError as e:
                errors[f.name] = e.error_list

        if errors:
            raise ValidationError(errors)

[Tim Graham]

What does validating the field do?

[silveroff@…]

@timo,

Currently

In [1]: obj = Example()

In [2]: obj.optional_decimal = None

In [3]: obj.full_clean()  # no ValidationError, so it looks like model is valid which is not true :(

In [4]: obj.save()
---------------------------------------------------------------------------
...
Warning: Column 'optional_decimal' cannot be null

With my fix proposal for django.db.models.base.Model#clean_fields:

In [1]: obj = Example()

In [2]: obj.optional_decimal = None

In [3]: obj.full_clean()
---------------------------------------------------------------------------
...
ValidationError: {'optional_decimal': [u'This field cannot be null.']}

Latter looks like proper behaviour, don't you think?

[Tim Graham]

Did you try to run Django's test suite with your proposed change? I suspect there will be failures. If you want the field to be optional, you need to set a value before it's saved as noted in the comment: "Skip validation for empty fields with blank=True. The developer is responsible for making sure they have a valid value."

[pysilver]

Yes, and there are just a few simple tests fails.

I've seen that "Skip validation for empty" many times, and yet many times I've seen bugs caused by this behaviour... This is just does not feel right – docs says "use model.full_clean() to make sure model is valid" and turns out thats not true.

======================================================================
FAIL: test_abstract_inherited_unique (model_forms.tests.UniqueTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/Silver/Projects/Interesting Repos/django-repo/tests/model_forms/tests.py", line 721, in test_abstract_inherited_unique
    self.assertEqual(len(form.errors), 1)
AssertionError: 3 != 1

======================================================================
FAIL: test_inherited_unique (model_forms.tests.UniqueTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/Silver/Projects/Interesting Repos/django-repo/tests/model_forms/tests.py", line 702, in test_inherited_unique
    self.assertEqual(len(form.errors), 1)
AssertionError: 3 != 1

======================================================================
FAIL: test_inherited_unique_together (model_forms.tests.UniqueTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/Silver/Projects/Interesting Repos/django-repo/tests/model_forms/tests.py", line 712, in test_inherited_unique_together
    self.assertEqual(len(form.errors), 1)
AssertionError: 3 != 1

======================================================================
FAIL: test_validation_with_empty_blank_field (validation.tests.ModelFormsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/Silver/Projects/Interesting Repos/django-repo/tests/validation/tests.py", line 104, in test_validation_with_empty_blank_field
    self.assertEqual(list(form.errors), [])
AssertionError: Lists differ: ['pub_date'] != []

First list contains 1 additional elements.
First extra element 0:
pub_date

- ['pub_date']
+ []

----------------------------------------------------------------------
Ran 7142 tests in 321.586s

FAILED (failures=4, skipped=571, expected failures=8)

[Tim Graham]

It is valid based on blank=True though. A use case for blank=True, null=False would be a field that gets populated in save(), for example. If we changed the behavior, it would be backwards incompatible and wouldn't allow this use case anymore. If you want the field required for model validation but not for form validation, then you should drop blank=True on the model and customize the form.

[Tim Graham]

Please reopen if you believe my analysis is incorrect.