#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 , 9 months ago
Has patch: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 9 months ago
Owner: | set to |
---|
comment:3 by , 9 months ago
Patch needs improvement: | set |
---|
comment:4 by , 9 months ago
Patch needs improvement: | unset |
---|
comment:5 by , 9 months ago
Cc: | 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.
follow-up: 7 comment:6 by , 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.
comment:7 by , 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 onChekConstriant.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 tosystem_check()
, - deprecate
check
argument forCheckConstraint
and rename it to thecheck_expression
orcondition
.
follow-up: 9 comment:8 by , 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
- Store the attribute in
CheckExpression.condition
and allow eithercondition
orcheck
to be passed to__init__
- Raise a deprecation warning when
check
is provided. - Add a
@property
shim forCheckExpression.check
that returnsself.condition
and raise a deprecation warning - Merge this patch that has
Model._check_constraints
callBaseConstraint._check
for the time being to perform system checks - When the deprecation period ends remove the shim and have
_check_constraints
call into the newly instatedBaseConstraint.check
method and take the opportunity to document it.
comment:9 by , 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
- Store the attribute in
CheckExpression.condition
and allow eithercondition
orcheck
to be passed to__init__
- Raise a deprecation warning when
check
is provided.- Add a
@property
shim forCheckExpression.check
that returnsself.condition
and raise a deprecation warning- Merge this patch that has
Model._check_constraints
callBaseConstraint._check
for the time being to perform system checks- When the deprecation period ends remove the shim and have
_check_constraints
call into the newly instatedBaseConstraint.check
method and take the opportunity to document it.
Sounds like a plan 👍
comment:10 by , 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:15 by , 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 , 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
- Squash the migration containing the old definitions
- 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?
PR