Move Index system checks from Model to Index.check()

[Tim Graham]

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.

[Simon Charette]

Agreed, this is coherent with #35234 (0fb104dda287431f5ab74532e45e8471e22b58c8) to move constraint checks to Constraint.check.

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

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

[Simon Charette]

Small note that this work landing would make implementing ticket:32770#comment:5 way more straightforward.

[Tim Graham]

I created a ​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 ​a lot argument validation and I don't see a reason why the validation of name should be treated differently.

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

[Tanishq]

Replying to 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

[Tim Graham]

Replying to 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.

[Sarah Boyce <42296566+sarahboyce@…>]

In 8638d8b:

Fixed #36273 -- Moved Index system checks from Model to Index.check().