[Django] #36813: Convert system checks for max_length and max_digits to __init__() checks.

[Django]

#36813: Convert system checks for max_length and max_digits to __init__() checks.
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type:
| Cleanup/optimization
Status: new | Component: Database
| layer (models, ORM)
Version: dev | 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
-------------------------------------+-------------------------------------
#28120 extended a system check validating `CharField.max_length` to also
validate that booleans are not provided.

However, system checks only check model fields, not fields constructed for
use in the `output_field` argument of an `Expression`, e.g. when using
`Cast()`. (Invalid inputs to `Cast()` error out at the db level instead of
raising nicely.)

I'm not suggesting to move ''a bunch'' of checks over to
`*Field.__init__()`, but I ''am'' suggesting that for simple checks on
`isinstance` and `> 0` that we could error out in
`{Char,Decimal}Field.__init__` instead of via a system check, to catch
additional usage mistakes.

Another use case besides `output_field` would be for database setup before
tests run checks, as in #34727.
--
Ticket URL: <https://code.djangoproject.com/ticket/36813>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36813: Convert system checks for max_length and max_digits to __init__() checks.
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Mehraz
Type: | Hossain Rumman
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mehraz Hossain Rumman):

* cc: Mehraz Hossain Rumman (added)
* owner: (none) => Mehraz Hossain Rumman
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36813#comment:1>

[Django]

#36813: Convert system checks for max_length and max_digits to __init__() checks.
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Mehraz
Type: | Hossain Rumman
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mehraz Hossain Rumman):

PR link : https://github.com/django/django/pull/20439
--
Ticket URL: <https://code.djangoproject.com/ticket/36813#comment:2>

[Django]

#36813: Convert system checks for max_length and max_digits to __init__() checks.
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Mehraz
Type: | Hossain Rumman
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mehraz Hossain Rumman):

* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36813#comment:3>

[Django]

#36813: Convert system checks for max_length and max_digits to __init__() checks.
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Mehraz
Type: | Hossain Rumman
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

I've followed the discussion but am a little skeptical. This introduces
uncertainty about where field validation should be done. Also, field
validation that relies on `connection` can't be done in `Model.__init__()`
so this approach may always be not quite right.

Alternatively, I wonder if this could be addressed by calling
`output_field.check(connection)` somewhere in `Expression`. There is some
complication with this approach because `Field.model` won't be set, so
field checks will have to be aware of that possibility.
--
Ticket URL: <https://code.djangoproject.com/ticket/36813#comment:4>

[Django]

#36813: Convert system checks for max_length and max_digits to __init__() checks.
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Mehraz
Type: | Hossain Rumman
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

I hear that. I was definitely suggesting to bifurcate where validation is
done given what you point out about `connection`-aware validation.

I guess I thought we'd already started down that path, e.g. the following
classes all have static, non-connection-aware validation in their
constructors:

- `CompositePrimaryKey`
- `GeneratedField`
- `JSONField`
- `FileField`
- `ManyToManyField`

I get bifurcation without a reason is to be avoided, but I think there's
something compelling about validating types as close to the perimeter as
possible instead of whackamoling checks throughout the ORM (or doing it
too late, say, inside `db_type()` and then suffering from a lack of parity
in third-party expressions that don't implement that checking). If there's
an elegant solution to do it once in the ORM, I'd definitely take a look.

Should we accept this for investigation and ask a contributor to look into
multiple approaches?
--
Ticket URL: <https://code.djangoproject.com/ticket/36813#comment:5>

[Django]

#36813: Convert system checks for max_length and max_digits to __init__() checks.
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Mehraz
Type: | Hossain Rumman
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tim Graham):

I suppose if there's consensus that using the system check framework for
this sort of validation was a design mistake, it can be corrected. A
rather clear line of delineation is that anything that can feasibly be
validated in `__init__()` should be done there, although I also note
there's some debate about what's truly an "error" and there might be a
case for silencing/skipping some validation, for example,
[https://code.djangoproject.com/ticket/36273#comment:8 the validation of
Index.name].
--
Ticket URL: <https://code.djangoproject.com/ticket/36813#comment:6>

[Django]

#36813: Convert system checks for max_length and max_digits to __init__() checks.
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Mehraz
Type: | Hossain Rumman
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

A proposed rule of thumb:
- `__init__()`: cheap, simple, static checks primarily raising
`ValueError` or `TypeError`
- `check()`: database-dependent, debatable, elaborate, or expensive checks
--
Ticket URL: <https://code.djangoproject.com/ticket/36813#comment:7>

[Django]

#36813: Convert system checks for max_length and max_digits to __init__() checks.
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Mehraz
Type: | Hossain Rumman
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

I like that rule of thumb and it seems coherent with your initial request

> I am suggesting that for simple checks on `isinstance` and `> 0` that we
could error out in `{Char,Decimal}Field.__init__` instead of via a system
check, to catch additional usage mistakes.

I guess it's kind of a mitigative measure to our lack of type hints usage.

Anything that is somehow database dependent should be performed at check
and compilation time though.
--
Ticket URL: <https://code.djangoproject.com/ticket/36813#comment:8>

[Django]

#36813: Convert system checks for max_length and max_digits to __init__() checks.
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Mehraz
Type: | Hossain Rumman
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/36813#comment:9>