[Django] #22921: Model.clean_fields incorrectly skips validation for fields where null value is not allowed
Django
is not allowed
----------------------------------------------+----------------------------
Reporter: silveroff@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords: field
Triage Stage: Unreviewed | validation, model
Easy pickings: 0 | Has patch: 0
| UI/UX: 0
----------------------------------------------+----------------------------
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22921>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
is not allowed
Reporter: silveroff@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: field validation, | Needs documentation: 0
model | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Could you give a concrete example of a form that exhibits the unexpected
behavior?
--
Ticket URL: <https://code.djangoproject.com/ticket/22921#comment:1>
Django
is not allowed
Reporter: silveroff@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: field validation, | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by 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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22921#comment:2>
Django
is not allowed
Reporter: silveroff@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: field validation, | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
What does validating the field do?
--
Ticket URL: <https://code.djangoproject.com/ticket/22921#comment:3>
Django
is not allowed
Reporter: silveroff@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: field validation, | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22921#comment:4>
Django
is not allowed
Reporter: silveroff@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: field validation, | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
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."
--
Ticket URL: <https://code.djangoproject.com/ticket/22921#comment:5>
Django
is not allowed
Reporter: silveroff@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: field validation, | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by 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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22921#comment:6>
Django
is not allowed
Reporter: silveroff@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: field validation, | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22921#comment:7>
Django
is not allowed
Reporter: silveroff@… | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Database layer | Resolution: invalid
(models, ORM) | Triage Stage:
Severity: Normal | Unreviewed
Keywords: field validation, | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
Please reopen if you believe my analysis is incorrect.
--
Ticket URL: <https://code.djangoproject.com/ticket/22921#comment:8>