ExclusionConstraint.expressions should be checked for foreign relationship references

[Simon Charette]

Unlike CheckConstraint.condition, UniqueConstraint.fields, .expressions, and .include the ExclusionConstraint.expressions members are not checked for references to foreign tables to emit models.E041 on violation which ​has been a source of confusion.

In order to avoid coupling the model constraint checks with third-party application and allow any third-party application to provide their own checks Model._check_constraint should delegate validation to BaseConstraint.check like _check_fields delegates to Field.check.

Unfortunately CheckConstraint already defines a .check attribute which I suggest we rename to ._check internally (better name welcome) to avoid collision with the proposed BaseConstraint.check() method and expose a coherent interface.

[Mariusz Felisiak]

​PR

[David Sanders]

Just on the ._check naming, imho this could confuse people by leading them to believe there's a relationship between .check() and ._check? 🤷‍♂️

As for suggestions, the only thing I could think of was to be more specific, perhaps ._check_expression ? Would be inline with ._db_default_expression. But then we have the dilemma of diverging from the kwarg check passed to CheckConstraint.

[Simon Charette]

But then we have the dilemma of diverging from the kwarg check passed to CheckConstraint.

yeah that's the crux of the issu, what Mariusz was referring to is that the attribute name is documented as the patch didn't suggest changing the kwarg name.

If we are going on the more explicit route I guess that def system_checks would be the most unambiguous choice? I pushed a version of the patch that opts to use _check for system checks as that maintains the public contract on ChekConstriant.check but it doesn't lift the ambiguity on the notion of check which might be warranted here to avoid any confusion.

[Mariusz Felisiak]

Replying to Simon Charette:

But then we have the dilemma of diverging from the kwarg check passed to CheckConstraint.

yeah that's the crux of the issue, what Mariusz was referring to is that the attribute name is documented as the patch didn't suggest changing the kwarg name.

If we are going on the more explicit route I guess that def system_checks would be the most unambiguous choice? I pushed a version of the patch that opts to use _check for system checks as that maintains the public contract on ChekConstriant.check but it doesn't lift the ambiguity on the notion of check which might be warranted here to avoid any confusion.

Yes, that's unfortunate. We will not be able to make it consistent without a proper deprecation of some kind. We can

• deprecate check() for ​fields, models, model managers, and database backends, and rename it to system_check(),
• deprecate check argument for CheckConstraint and rename it to the check_expression or condition.

[Simon Charette]

I have a slight inclination for the second option as it seems less disruptive? It involves a single method and CheckConstraint.check has been added quite more recently that all the other abstractions. So what about the following approach

1. Store the attribute in CheckExpression.condition and allow either condition or check to be passed to __init__
2. Raise a deprecation warning when check is provided.
3. Add a @property shim for CheckExpression.check that returns self.condition and raise a deprecation warning
4. Merge this patch that has Model._check_constraints call BaseConstraint._check for the time being to perform system checks
5. When the deprecation period ends remove the shim and have _check_constraints call into the newly instated BaseConstraint.check method and take the opportunity to document it.

[Mariusz Felisiak]

Replying to Simon Charette:

I have a slight inclination for the second option as it seems less disruptive? It involves a single method and CheckConstraint.check has been added quite more recently that all the other abstractions. So what about the following approach

1. Store the attribute in CheckExpression.condition and allow either condition or check to be passed to __init__
2. Raise a deprecation warning when check is provided.
3. Add a @property shim for CheckExpression.check that returns self.condition and raise a deprecation warning
4. Merge this patch that has Model._check_constraints call BaseConstraint._check for the time being to perform system checks
5. When the deprecation period ends remove the shim and have _check_constraints call into the newly instated BaseConstraint.check method and take the opportunity to document it.

Sounds like a plan 👍

[Simon Charette]

Adjusted the MR to this effect by adding a commit that deprecates CheckConstraint.check in favour of .condition.

The only part that might be another set of eyes is the stacklevel which I haven't confirmed is exactly what it should be for the warning to point at CheckConstraint(...) and .check interactions.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 0fb104dd:

Refs #35234 -- Moved constraint system checks to Check/UniqueConstraint methods.

[Mariusz Felisiak <felisiak.mariusz@…>]

In f82c67aa:

Fixed #35234 -- Added system checks for invalid model field names in ExclusionConstraint.expressions.

[Mariusz Felisiak <felisiak.mariusz@…>]

In daf7d48:

Refs #35234 -- Deprecated CheckConstraint.check in favor of .condition.

Once the deprecation period ends CheckConstraint.check() can become the
documented method that performs system checks for BaseConstraint
subclasses.

[GitHub <noreply@…>]

In 425b260:

Refs #35234 -- Skipped CheckConstraint system checks if not supported.

Thanks Tim Graham for the report.

Regression in 0fb104dda287431f5ab74532e45e8471e22b58c8.

[Stian Jensen]

After upgrading to Django 5.1 we are now getting lots of this message when running migrate:
"CheckConstraint.check is deprecated in favor of .condition."

The upgrade notes or documentation does not seem to say what the migration path is supposed to be, as just changing the code still leaves violating code in old migrations. Are we supposed to also change old migrations to the new syntax?

[Simon Charette]

The upgrade notes or documentation does not seem to say what the migration path is supposed to be, as just changing the code still leaves violating code in old migrations. Are we supposed to also change old migrations to the new syntax?

Just like with other deprecation of this form (think of ForeignKey(on_delete) ​being made a required argument) you have two options

1. Squash the migration containing the old definitions
2. Edit said migrations to use the new call signature

Maybe we could adjust the release notes to include a note similar to the ForeignKey(on_delete) one?