[Django] #35234: ExclusionConstraint.expressions should be checked for foreign relationship references

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
------------------------------------------------+--------------------------
Reporter: Simon Charette | Owner: (none)
Type: Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0
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 |
------------------------------------------------+--------------------------
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 [https://forum.djangoproject.com/t
/exclusionconstraint-with-expression-does-not-apply-migration/21574 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35234>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references

--------------------------------------+------------------------------------

Reporter: Simon Charette | Owner: (none)
Type: Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0

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 Mariusz Felisiak):

* has_patch: 0 => 1
* stage: Unreviewed => Accepted

Comment:

[https://github.com/django/django/pull/17876 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:1>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references

-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette

Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0
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 Mariusz Felisiak):

* owner: (none) => Simon Charette

--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:2>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:3>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0
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 Simon Charette):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:4>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0
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 David Sanders):

* cc: David Sanders (added)

Comment:

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:5>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0
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
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:6>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0
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
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

Replying to [comment:6 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
[https://docs.djangoproject.com/en/5.0/topics/checks/#field-model-manager-
and-database-checks 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:7>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0
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
-------------------------------------+-------------------------------------

Comment (by 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` deprecation shim for `CheckExpression.check` that
returns `self.condition`
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 the
`_check_constraints` call into the newly instated `BaseConstraint.check`
method and take the opportunity to document it.
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:8>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0
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
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

Replying to [comment:8 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 👍
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:9>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0
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
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:10>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 5.0
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
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"0fb104dda287431f5ab74532e45e8471e22b58c8" 0fb104dd]:
{{{#!CommitTicketReference repository=""
revision="0fb104dda287431f5ab74532e45e8471e22b58c8"
Refs #35234 -- Moved constraint system checks to Check/UniqueConstraint
methods.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:11>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette

Cleanup/optimization | Status: closed
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution: fixed

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 Mariusz Felisiak <felisiak.mariusz@…>):

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

Comment:

In [changeset:"f82c67aa217c8dd4320ef697b04a6da1681aa799" f82c67aa]:
{{{#!CommitTicketReference repository=""
revision="f82c67aa217c8dd4320ef697b04a6da1681aa799"
Fixed #35234 -- Added system checks for invalid model field names in
ExclusionConstraint.expressions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:12>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: closed
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"daf7d482dbaaa2604241a994c49f442fa15142c1" daf7d48]:
{{{#!CommitTicketReference repository=""
revision="daf7d482dbaaa2604241a994c49f442fa15142c1"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:13>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: closed
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by GitHub <noreply@…>):

In [changeset:"425b26092f038accd2a5c5fc5a9bd3f82d4dd847" 425b260]:
{{{#!CommitTicketReference repository=""
revision="425b26092f038accd2a5c5fc5a9bd3f82d4dd847"
Refs #35234 -- Skipped CheckConstraint system checks if not supported.

Thanks Tim Graham for the report.

Regression in 0fb104dda287431f5ab74532e45e8471e22b58c8.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:14>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: closed
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:15>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: closed
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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)`
[https://docs.djangoproject.com/en/5.1/releases/1.9/#foreignkey-and-
onetoonefield-on-delete-argument 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:16>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: closed
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

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

In [changeset:"85750bd2f8ed7e595aac25c7e5fd7218528a25b1" 85750bd2]:
{{{#!CommitTicketReference repository=""
revision="85750bd2f8ed7e595aac25c7e5fd7218528a25b1"
Refs #35234 -- Removed CheckConstraint.check per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:17>

[Django]

#35234: ExclusionConstraint.expressions should be checked for foreign relationship
references
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: closed
Component: contrib.postgres | Version: 5.0
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by GitHub <noreply@…>):

In [changeset:"65c46d6932c0956d2988d13ec3d9ab3ef9d96d61" 65c46d69]:
{{{#!CommitTicketReference repository=""
revision="65c46d6932c0956d2988d13ec3d9ab3ef9d96d61"
Fixed #35358, Refs #35234 -- Renamed _check() methods to check() for
constraints.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35234#comment:18>