Hi Jacob.
Thanks for the follow-up. Good hustle! π
I saw your comments on Trac last week, and was preparing a response.
The trouble with these 8, 7, and 5 year old tickets is that they're trickyβ¦
Let me put the take-home first, I think that in-themselves we should close
all
three of these tickets as wontfix.
* The issues arise due to the `blank=True` usage, which is as expected, but
folks need to be using the right forms fields, or implementing `clean` on
their
model, or various `clean_<field>` methods on their forms to ensure that
sane
values are provided, or β¦ β there are lots of options here.
* Silently adjusting the value the ORM receives on the basis of `blank`
would
lead to more issues than it would resolve.
* If users want to implement custom fields that automatically map given
values,
then the API is there to do this. **BUT** that belongs properly to the
form
layer: they'd be better off correcting the issue there. In any case, it
is not
something we should do by default.
* `blank` is under-documented. We should expand those docs to spell-out that
validation is skipped once it's in play, and so the extra steps that a
user
needs to take.
Take the simplest example, ignoring `blank`, and validation:
```
from django.db import models
class A(models.Model):
b = models.CharField(max_length=100)
a = A(b=None)
a.save() # IntegrityError: NOT NULL constraint failed: app_a.b
```
That **is** the behaviour we want. I set bad values on the model instance. I
try to save. The DB error is raised.
Let's call `full_clean()` there, with no other changes:
```
a = A(b=None)
a.full_clean() # ValidationError: This field cannot be null.
```
All good. We can call `full_clean()` prior to `save()` to run the field
validation.
The issue comes adding `blank=True`, which causes the field to be excluded
from
`full_clean()`:
```
class A(models.Model):
b = models.CharField(max_length=100, blank=True)
a = A(b=None)
a.full_clean() # This now passes.
a.save() # IntegrityError: NOT NULL constraint failed: app_a.b
```
The issues report this as surprising. But it's precisely what setting
`blank=True` does: exclude the field from validation. It **is** the expected
result.
Given that this issue comes up, we should do a better job of documenting
it.
What I don't think we can/should do is to silently map to a correct value.
Here
we're still at the ORM layer. We positively **want** the IntegrityError
when we
pass bad values and try to save. As tricky as these issues are, how much
harder
would they be if you instantiate a model with `None` but what's actually
passed
to the DB is `''`:
```
a = A(b=None)
```
β that way lies madness. (If I've opted into this mapping with a custom
field, that's a different topic: I'm presumably expecting it then.)
**Rather**, the issue here lies at the user's form layer. The ORM is
protecting
me by raising the IntegrityError if I give it bad data, but my job is to
make
sure my form (or if I'm using DRF serialiser) outputs the correct data types
for my field.
For #20205, the form for the nullable PositiveIntegerfield should give me
`None`, not `''`. (This was noted on the ticket.)
Similarly, for #22224 the form for a CharField should give me an empty
string
`''`, rather than `None`.
#27697 is more complex. By the time we reach the model, we're (again)
expecting
#the form to provided us a sensible empty value (`{}`) β but there are
related
#questions about how we render the initial and so on. Those, though, belong
at
#the form/widget level: it's still not the ORM's business.
Summary there: the ORM is protecting you from gaps in your form layer. We
like
that.
Can we do **something** at the model layer? β Yes, we can implement
`clean()`
to do the right thing for `blank=True` fields. Or we can have fields do
something clever in `get_db_prep_value()`.
Of those two, I much prefer the first:
* It's the long-standing one-way to do things.
* When I'm writing my model, my clean method is right in front of me, in the
same place as my field definitions. (I don't need to go digging into Field
internals to see how/what mapping is applied.)
Expanding the `blank` docs[0] in the model field reference to point to the
`Model.clean()` docs[1], and maybe adding an example for a `blank=True`
field
in the latter, would probably help folks to see what's missing. π€
[0]: https://docs.djangoproject.com/en/3.2/ref/models/fields/#s-blank
[1]:
https://docs.djangoproject.com/en/3.2/ref/models/instances/#django.db.models.Model.clean
But beyond that, I don't think a change is in order. So I'd close as
wontfix.
I think from your comments you more or less already think that?
Ref your PR[2], I don't think adding the `and (f.empty_strings_allowed or
raw_value != '')` special case in `django/db/models/base.py` is the right
way
to go.
For one, any of the other `empty_values` except `''` will still exhibit the
issue (e.g. the test case you added fails with `{}`).
But more, `BaseModelForm._get_validation_exclusions()` will already add the
relevant fields to the `exclude` list, so assuming normal usage the
ValidationError won't be raised anyway. (We're back to the form layer.)
[2]: https://github.com/django/django/pull/14666/
There's some good stuff in the discussion on #20205, but I think it's
relevant
to _Advanced techniques with custom model fields_, rather than a change we
can
realistically make.
I hope that all makes sense. What do you think?
Kind Regards,
Carlton
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-developers+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/11d530c4-d4f5-461d-a291-8ea1f1e366f1n%40googlegroups.com.
