[Django] #22224: Non-nullable blank string-based model field validation doesn't prevent or clean `None`
Django
clean `None`
-----------------------------------------+------------------------
Reporter: charettes | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Since
[https://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.Field.null
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`:
{{{#!python
from django.db import models
class A(models.Model):
b = models.CharField(blank=True)
a = A(b=None)
a.full_clean() # Passes
a.save() # 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 Py7/SQLite3.
--
Ticket URL: <https://code.djangoproject.com/ticket/22224>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
clean `None`
Reporter: charettes | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* has_patch: 0 => 1
* component: Uncategorized => Database layer (models, ORM)
Comment:
See https://github.com/django/django/pull/2409
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:1>
Django
clean `None`
Reporter: charettes | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by charettes:
Old description:
New description:
pass on Py2.7/SQLite3.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:2>
Django
clean `None`
Reporter: charettes | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by claudep):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:3>
Django
clean `None`
Reporter: charettes | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:4>
Django
clean `None`
Reporter: charettes | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:5>
Django
clean `None`
Reporter: charettes | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:6>
Django
clean `None`
Reporter: Simon Charette | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Jacob Walls):
Well, I came here to suggest `wontfix`, but
[https://code.djangoproject.com/ticket/22224#comment:5 Simon's batteries-
included proposal] is starting to tempt me.
This test case from 2010 indicates the supported design pattern that the
naive fix to this ticket (run model validation on blankable/not-nullable
fields instead of short-circuiting, as now) would break:
{{{
def test_validation_with_empty_blank_field(self):
# Since a value for pub_date wasn't provided and the field is
# blank=True, model-validation should pass.
# Also, Article.clean() should be run, so pub_date will be filled
after
# validation, so the form should save cleanly even though pub_date
is
# not allowed to be null.
data = {
'title': 'The state of model validation',
}
article = Article(author_id=self.author.id)
form = ArticleForm(data, instance=article)
self.assertEqual(list(form.errors), [])
self.assertIsNotNone(form.instance.pub_date)
article = form.save()
}}}
The preface to this test case says developers should be implementing an
Article.clean(), which looks like this:
{{{
def clean(self):
if self.pub_date is None:
self.pub_date = datetime.now()
}}}
If we run model validation on blankable not-nullable fields, we lose this
design pattern.
This is also in agreement with Carlton's assessment
[https://code.djangoproject.com/ticket/23130#comment:30 here] for
`BooleanField`:
> The validation weirdness when blank=True is a red-herring. It causes the
field to be skipped during full_clean(). At that point developers should
implementing clean_* to ensure they have an acceptable value. It's quite
feasible that blank=True doesn't really make sense for a BooleanField, but
I'm not sure we can tighten it up at this latter stage (no doubt there's
some use-case…)
I think the test case at the top demonstrates the use case Carlton was
reluctant to break for BooleanField, so I'm also reluctant to break it for
string-based fields.
But Simon's proposal is another side of the same coin: if you didn't write
a `clean()`, or your `clean()` missed a case, we could bail you out
instead of propagating db failures.
Simon, do you have an opinion about whether your proposal still makes
sense as a friendly guardrail? If so, then we should do something similar
for #27697, by providing a nice `{}` if `JSONField` is not nullable.
#20205 presents a similar set of issues for `PositiveIntegerField`, but I
think there *is* a fixable change there I plan to submit a patch for
shortly.
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:7>
Django
clean `None`
Reporter: Simon Charette | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: nobody => Jacob Walls
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* status: new => assigned
* component: Database layer (models, ORM) => Documentation
Comment:
[https://groups.google.com/g/django-developers/c/GlYM25fdRnA Per mailing
list:]
> It does come up. It _is_ surprising that `blank` has this effect, and
I'm not convinced it says clearly anywhere how you should approach it…
Having weighed the possible behavior changes and found them wanting, we're
looking at documenting the `blank=True` and `null=False` idiom.
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:8>
Django
clean `None`
Reporter: Simon Charette | Owner: Jacob
Cleanup/optimization | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* type: Bug => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:9>
Django
clean `None`
Reporter: Simon Charette | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: assigned
Component: Documentation | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/14920 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:10>
Django
clean `None`
Reporter: Simon Charette | Owner: Jacob
Type: | Walls
Component: Documentation | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"cd6bddd44e0a1c3c6a96a3677f8366ef0e4b6782" cd6bddd]:
{{{
#!CommitTicketReference repository=""
revision="cd6bddd44e0a1c3c6a96a3677f8366ef0e4b6782"
Fixed #22224 -- Added note about supplying missing values for non-nullable
fields with blank=True.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:11>
Django
clean `None`
Reporter: Simon Charette | Owner: Jacob
Type: | Walls
Cleanup/optimization | Status: closed
Component: Documentation | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"515d3c591cc65c77b8d46391d15ded576b037d85" 515d3c59]:
{{{
#!CommitTicketReference repository=""
revision="515d3c591cc65c77b8d46391d15ded576b037d85"
[4.0.x] Fixed #22224 -- Added note about supplying missing values for non-
nullable fields with blank=True.
Backport of cd6bddd44e0a1c3c6a96a3677f8366ef0e4b6782 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22224#comment:12>