Opened 9 months ago

Closed 8 months ago

Last modified 3 weeks ago

#35234 closed Cleanup/optimization (fixed)

ExclusionConstraint.expressions should be checked for foreign relationship references

Reported by: Simon Charette Owned by: Simon Charette
Component: contrib.postgres Version: 5.0
Severity: Normal Keywords:
Cc: David Sanders Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (16)

comment:1 by Mariusz Felisiak, 9 months ago

Has patch: set
Triage Stage: UnreviewedAccepted

comment:2 by Mariusz Felisiak, 9 months ago

Owner: set to Simon Charette

comment:3 by Mariusz Felisiak, 9 months ago

Patch needs improvement: set

comment:4 by Simon Charette, 9 months ago

Patch needs improvement: unset

comment:5 by David Sanders, 9 months ago

Cc: David Sanders added

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.

comment:6 by Simon Charette, 9 months ago

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.

in reply to:  6 comment:7 by Mariusz Felisiak, 9 months ago

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

comment:8 by Simon Charette, 9 months ago

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.
Last edited 9 months ago by Simon Charette (previous) (diff)

in reply to:  8 comment:9 by Mariusz Felisiak, 9 months ago

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 👍

comment:10 by Simon Charette, 8 months ago

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.

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 8 months ago

In 0fb104dd:

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

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 8 months ago

Resolution: fixed
Status: assignedclosed

In f82c67aa:

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

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 8 months ago

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.

comment:14 by GitHub <noreply@…>, 7 months ago

In 425b260:

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

Thanks Tim Graham for the report.

Regression in 0fb104dda287431f5ab74532e45e8471e22b58c8.

comment:15 by Stian Jensen, 3 weeks ago

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?

comment:16 by Simon Charette, 3 weeks ago

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?

Note: See TracTickets for help on using tickets.
Back to Top