OK, super.
Would you then fancy looking at that docs change? 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β¦
Thanks for your efforts digging into this! π
C.
On Tuesday, 27 July 2021 at 14:22:50 UTC+2 jacobty...@gmail.com wrote:
> This makes good sense, Carlton, and I didn't mean to be rushing your reply!
>
> "That way lies madness" is entirely correct. Just wanted to map the
> terrain after I had so confidently commented on one Trac ticket about
> wontfix-ing it, since on it there were various opinions about possible
> courses of action. I think closing the behavior tickets is the right course
> to follow here.
>
> All best,
> Jacob
>
>
> On Tuesday, July 27, 2021 at 6:08:31 AM UTC-4 carlton...@gmail.com wrote:
>
>> 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/a0609e5a-bf1c-4441-a1fc-6455502acc2cn%40googlegroups.com.
