#22224: Non-nullable blank string-based model field validation doesn't prevent
or
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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/067.6eef392d1c94d2849e0dcad6917495cf%40djangoproject.com.
