[Django] #36273: Move Index system checks from Model to Index.check()

[Django]

#36273: Move Index system checks from Model to Index.check()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Type:
| Cleanup/optimization
Status: new | Component: Core
| (System checks)
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
-------------------------------------+-------------------------------------
In #30613 (53209f78302a639032afabf5326d28d4ddd9d03c), some Index
validation was moved from `Index.__init__()` to `Model.check()`. This
prevents `Index` subclasses from customizing or extending the validation.
The logic should be in `Index.check()`, following the pattern of fields,
constraints, etc.
--
Ticket URL: <https://code.djangoproject.com/ticket/36273>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36273: Move Index system checks from Model to Index.check()

--------------------------------------+------------------------------------
Reporter: Tim Graham | Owner: (none)

Type: Cleanup/optimization | Status: new
Component: Core (System checks) | Version: dev

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

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

--------------------------------------+------------------------------------
Changes (by Simon Charette):

* stage: Unreviewed => Accepted

Comment:

Agreed, this is coherent with #35234
(0fb104dda287431f5ab74532e45e8471e22b58c8) to move constraint checks to
`Constraint.check`.
--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:1>

[Django]

#36273: Move Index system checks from Model to Index.check()
--------------------------------------+------------------------------------
Reporter: Tim Graham | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Core (System checks) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by Tanishq):

@Simon Charette

Hi,

I'd like to claim ticket #36273 and work on moving the Index system checks
from **Model.check()** to **Index.check()**, following the pattern used
for fields and constraints. I'll submit a PR once the changes are ready.

Let me know if there are any specific considerations or related
discussions I should be aware of before proceeding.
--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:2>

[Django]

#36273: Move Index system checks from Model to Index.check()
--------------------------------------+------------------------------------
Reporter: Tim Graham | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Core (System checks) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Comment (by Simon Charette):

The ticket is yours Tanishq, make sure to assign it to you.

As mentioned above the required changes can borrow a lot from
0fb104dda287431f5ab74532e45e8471e22b58c8 except for the fact that we can
name the method `Index.check` from the get go (there's no conflict with a
pre-existing attribute).
--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:3>

[Django]

#36273: Move Index system checks from Model to Index.check()
--------------------------------------+------------------------------------
Reporter: Tim Graham | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Core (System checks) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Simon Charette):

Small note that this work landing would make implementing
ticket:32770#comment:5 way more straightforward.
--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:4>

[Django]

#36273: Move Index system checks from Model to Index.check()
--------------------------------------+------------------------------------

Reporter: Tim Graham | Owner: Tanishq
Type: Cleanup/optimization | Status: assigned

Component: Core (System checks) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Changes (by Tanishq):

* owner: (none) => Tanishq
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:5>

[Django]

#36273: Move Index system checks from Model to Index.check()
--------------------------------------+------------------------------------
Reporter: Tim Graham | Owner: Tanishq
Type: Cleanup/optimization | Status: assigned
Component: Core (System checks) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------

Changes (by Clifford Gama):

* cc: Clifford Gama (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:6>

[Django]

#36273: Move Index system checks from Model to Index.check()

-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
Type: | Graham
Cleanup/optimization | Status: assigned
Component: Core (System | Version: dev
checks) |

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Tim Graham):

* owner: Tanishq => Tim Graham

Comment:

I created a [https://github.com/django/django/pull/19395 draft PR],
however, there's an undesired behavior change in this patch. The checks
that don't require a database like index name validation are no longer run
at the `makemigrations` stage since `makemigrations` does not provide the
`databases` argument to system checks and `Index.check()` is guarded by
`router.allow_migrate_model(db, ...)` (see also ticket:31286#comment:7).

Possibly this could be mitigated independently, however, I'm not sure it
was a good idea to move validation that can be done in `Index.__init__()`
to system checks (was done in #30613 /
53209f78302a639032afabf5326d28d4ddd9d03c). `Index.__init__()` does
[https://github.com/django/django/blob/1831f7733d3ef03d1ca7fac3e8d9f4c5e3e3375e/django/db/models/indexes.py#L29-L64
a lot argument validation] and I don't see a reason why the validation of
`name` should be treated differently.
--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:7>

[Django]

#36273: Move Index system checks from Model to Index.check()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
Type: | Graham
Cleanup/optimization | Status: assigned
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* cc: Simon Charette (added)

Comment:

Tanish, you haven't provided any updates in the past month, are you still
actively working on this?

----

> `Index.__init__()` does ​a lot argument validation and I don't see a

reason why the validation of name should be treated differently.

I suspect that one of the intent was that this validation could be
silenced on databases that don't have Oracle-like restriction in index
names; if the validation is performed at `__init__` time there's no way to
silence it.

I'm not sure I agree with the ''cleaner and more consistent'' rationale of
#30613 either but the move to `Index.check(model, connection)` should
allow at least allow these checks to be eventually truly database specific
(as they truly are only relevant on Oracle). In all cases I don't think it
should be a blocker for this work and that we should focus our efforts on
whether or not we believe ticket:31286#comment:9 is an adequate solution
for the general problem of database related checks not running for
database related commands.
--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:8>

[Django]

#36273: Move Index system checks from Model to Index.check()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
Type: | Graham
Cleanup/optimization | Status: assigned
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Tanishq):

Replying to [comment:8 Simon Charette]:

> Tanish, you haven't provided any updates in the past month, are you
still actively working on this?
>
> ----
>
> > `Index.__init__()` does ​a lot argument validation and I don't see a
reason why the validation of name should be treated differently.
>
> I suspect that one of the intent was that this validation could be
silenced on databases that don't have Oracle-like restriction in index
names; if the validation is performed at `__init__` time there's no way to
silence it.
>
> I'm not sure I agree with the ''cleaner and more consistent'' rationale
of #30613 either but the move to `Index.check(model, connection)` should
allow at least allow these checks to be eventually truly database specific
(as they truly are only relevant on Oracle). In all cases I don't think it
should be a blocker for this work and that we should focus our efforts on
whether or not we believe ticket:31286#comment:9 is an adequate solution
for the general problem of database related checks not running for
database related commands.
>

sorry, about that, I was working on this actively, but was encountering
database issues which is resolved now I
guess
--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:9>

[Django]

Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:10>

[Django]

#36273: Move Index system checks from Model to Index.check()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
Type: | Graham
Cleanup/optimization | Status: assigned
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham):

* has_patch: 0 => 1

Comment:

Replying to [comment:8 Simon Charette]:

> In all cases I don't think it should be a blocker for this work...

Okay, then I suppose my PR is ready for review.
--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:11>

[Django]

#36273: Move Index system checks from Model to Index.check()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
Type: | Graham
Cleanup/optimization | Status: assigned
Component: Core (System | Version: dev
checks) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

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

Changes (by Simon Charette):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:12>

[Django]

#36273: Move Index system checks from Model to Index.check()
-------------------------------------+-------------------------------------
Reporter: Tim Graham | Owner: Tim
Type: | Graham

Cleanup/optimization | Status: closed

Component: Core (System | Version: dev
checks) |

Severity: Normal | Resolution: fixed

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

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"8638d8bf74c1a58302c97d4436ad2eb08438145b" 8638d8b]:
{{{#!CommitTicketReference repository=""
revision="8638d8bf74c1a58302c97d4436ad2eb08438145b"
Fixed #36273 -- Moved Index system checks from Model to Index.check().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36273#comment:13>