Convert system checks for max_length and max_digits to __init__() checks.

[Jacob Walls]

#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.

[Mehraz Hossain Rumman]

PR link : ​https://github.com/django/django/pull/20439

[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.

[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?

[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, the validation of Index.name.

[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

[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.