Opened 6 years ago

Last modified 6 years ago

#22224 new Bug

Non-nullable blank string-based model field validation doesn't prevent or clean `None`

Reported by: Simon Charette Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Simon Charette)

Since we discourage the use of NULLable string-based fields the assumed way of holding text that might be empty would be to use the blank option.

In this case I would expect full_clean to raise a ValidationError when an empty non-nullable field is assigned None:

from django.db import models

class A(models.Model):
    b = models.CharField(blank=True)

a = A(b=None)
a.full_clean()  # Passes  # Integrity error

Unfortunately specifying blank=True disable all validation against the b field, leading to database integrity error upon calling save(). The obvious solution here would be to override A.clean to convert b to an empty string when it equals None but it strikes me as a non trivial change for a quite common use case: correctly allowing a string-based field to be empty.

This is not an issue when cleaning data retrieved from the usual application/form-url-encoded request.GET or request.POST since an empty value is always represented by the empty string. However, in the case of an application/json encoded payload one may either provide None or '' (null or '') to express emptiness. Which will trigger the issue described above.

Attaching a patch that special case the empty_values of non-nullable string based fields based on the empty_strings_allowed flag. All tests pass on Py2.7/SQLite3.

Change History (6)

comment:1 Changed 6 years ago by Simon Charette

Component: UncategorizedDatabase layer (models, ORM)
Has patch: set

comment:2 Changed 6 years ago by Simon Charette

Description: modified (diff)

comment:3 Changed 6 years ago by Claude Paroz

Triage Stage: UnreviewedAccepted

comment:4 Changed 6 years ago by Simon Charette

As pointed out by Loïc this issue is related to #20205 which propose to clean '' to None for fields disallowing empty strings.

Maybe the correct solution here would be to clean None to '' for non-null fields allowing empty strings?

comment:5 Changed 6 years ago by Simon Charette

Another solution might be to special case get_db_prep_value to convert None to '' when the field is not NULL'able and empty_strings_allowed.

This would be completely backward compatible in the case of a model specific clean() with a self.non_null_string_based_field is None conditional.

comment:6 Changed 6 years ago by Anssi Kääriäinen

Patch needs improvement: set

It seems the problem here is that for HTML forms, "" is the natural None value, so for example IntegerField="" should validate and set the field to Zero. But for model validation this isn't the case. In that use case providing "" for IntegerField doesn't make sense. We could maybe make forms treat "" specially (convert to None unless the underlying field accepts empty strings), and then remove "" from empty_values for practically every field. Unfortunately changes here seem problematic as the problem isn't that large currently, but the backwards compatibility issues will be large.

Also, maybe we could just remove "" from empty values for all fields that accept "" directly as a value. The logic is that while "" is kinda special value for CharField, it is just one value in its domain. However, for IntegerField (and other fields that do not accept "" directly) the empty string is not in their domain, and thus it can be treated as None.

I'm marking patch needs improvement for lack of better marker. What is needed here is a design decision of how we want to treat "" in CharFields.

Note: See TracTickets for help on using tickets.
Back to Top