Re: [Django] #36518: full_clean crashes on model with both a CheckConstraint and a GeneratedField with a Case expression

[Django]

#36518: full_clean crashes on model with both a CheckConstraint and a
GeneratedField with a Case expression
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* cc: Mariusz Felisiak (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/36518#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36518: full_clean crashes on model with both a CheckConstraint and a
GeneratedField with a Case expression
-------------------------------------+-------------------------------------

Reporter: Olivier Dalang | Owner: Simon
| Charette
Type: Bug | Status: assigned

Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* owner: (none) => Simon Charette
* status: new => assigned

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

[Django]

#36518: full_clean crashes on model with both a CheckConstraint and a
GeneratedField with a Case expression
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce):

Tried to look into this today a bit, note that the following is a possible
regression test:
{{{#!diff
--- a/tests/constraints/models.py
+++ b/tests/constraints/models.py
@@ -69,6 +69,29 @@ class GeneratedFieldVirtualProduct(models.Model):
required_db_features = {"supports_virtual_generated_columns"}


+class GeneratedFieldCaseWhenConstraint(models.Model):
+ age = models.IntegerField()
+ is_old_enough = models.GeneratedField(
+ expression=models.Case(
+ models.When(age__gte=18, then=models.Value(True)),
+ default=models.Value(False),
+ ),
+ db_persist=True,
+ output_field=models.BooleanField(),
+ )
+
+ class Meta:
+ required_db_features = {
+ "supports_stored_generated_columns",
+ "supports_table_check_constraints"
+ }
+ constraints = [
+ models.CheckConstraint(
+ name="age_valid", condition=models.Q(age__lt=100)
+ ),
+ ]
+
+
class UniqueConstraintProduct(models.Model):
name = models.CharField(max_length=255)
color = models.CharField(max_length=32, null=True)
diff --git a/tests/constraints/tests.py b/tests/constraints/tests.py
index bff8de8566..f13845c5d5 100644
--- a/tests/constraints/tests.py
+++ b/tests/constraints/tests.py
@@ -12,6 +12,7 @@ from django.test import SimpleTestCase, TestCase,
skipIfDBFeature, skipUnlessDBF
from .models import (
ChildModel,
ChildUniqueConstraintProduct,
+ GeneratedFieldCaseWhenConstraint,
GeneratedFieldStoredProduct,
GeneratedFieldVirtualProduct,
JSONFieldModel,
@@ -414,6 +415,15 @@ class CheckConstraintTests(TestCase):
constraint.validate(model, invalid_product, exclude={"price"})
constraint.validate(model, invalid_product, exclude={"rebate"})

+ @skipUnlessDBFeature(
+ "supports_stored_generated_columns",
"supports_table_check_constraints"
+ )
+ def test_full_clean_generated_field_case_when(self):
+ bob = GeneratedFieldCaseWhenConstraint(age=17)
+ bob.full_clean()
+ bob.save()
+ self.assertEqual(bob.is_old_enough, False)
+
def test_database_default(self):
models.CheckConstraint(
}}}

This test fails on main but passes once the current fix for #34871
(current PR https://github.com/django/django/pull/19190) is applied
--
Ticket URL: <https://code.djangoproject.com/ticket/36518#comment:5>

[Django]

#36518: full_clean crashes on model with both a CheckConstraint and a
GeneratedField with a Case expression
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Thanks for the ping and test Sarah, here's my analysis of the situation.

We've known that validation of constraints including unresolved lookups
(`Q`, `Case`) is since broken since 4.2 (#34871).

By fixing #35575 and requiring that `GeneratedField` expressions are
replaced with the validated against instance values we triggered the same
problem for `GeneratedField` making use of unresolved lookups hence why
the patch for #34871 happens to resolve this issue.

The interesting part is that since we have constraint asks for all fields
replacements via `_get_field_expression_map` (except for the ones part of
`excludes`) even if only a few ones are required by the constraint itself
the problem can now be triggered even if a generated field making use of
unresolved lookup is not referenced by the constraint. For example, in the
reported case the `age_valid` constraint doesn't refer to the
`is_old_enough` field.

Looking back at [https://github.com/django/django/pull/19190/ the proposed
patch] it seems like it would be adequate to address this issue even if it
duplicates `sql.Query.build_filter` logic as we likely want to avoid also
backporting a refactor. It would keep the unnecessary work in performed by
`_get_field_expression_map` though.

The only alternative I see is likely more invasive as it would require us
to flip the logic around by having constraint introspect their respective
expressions (e.g. `condition`, `constraint`) and only request that fields
of interests are considered by `_get_field_expression_map`. This seems
like a worthy optimization that would address this issue but it wouldn't
address the underlying cause of #34871 and is likely risky as a backport.
--
Ticket URL: <https://code.djangoproject.com/ticket/36518#comment:6>

[Django]

#36518: full_clean crashes on model with both a CheckConstraint and a
GeneratedField with a Case expression
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* has_patch: 0 => 1
* needs_docs: 0 => 1
* needs_tests: 0 => 1

Comment:

> Looking back at ​the proposed patch it seems like it would be adequate

to address this issue even if it duplicates `sql.Query.build_filter logic`
as we likely want to avoid also backporting a refactor. It would keep the
unnecessary work in performed by `_get_field_expression_map` though.

I agree the solution for #34871 is backportable and likely preferable to a
deeper refactor, would you mind adding a test to that patch relating to
this ticket and a release note?
--
Ticket URL: <https://code.djangoproject.com/ticket/36518#comment:7>

[Django]

#36518: full_clean crashes on model with both a CheckConstraint and a
GeneratedField with a Case expression
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

> would you mind adding a test to that patch relating to this ticket and a
release note?

yeah will do!
--
Ticket URL: <https://code.djangoproject.com/ticket/36518#comment:8>

[Django]

#36518: full_clean crashes on model with both a CheckConstraint and a
GeneratedField with a Case expression
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* has_patch: 1 => 0
* needs_docs: 1 => 0
* needs_tests: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36518#comment:9>

[Django]

#36518: full_clean crashes on model with both a CheckConstraint and a
GeneratedField with a Case expression
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | 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 Sarah Boyce):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36518#comment:10>

[Django]

#36518: full_clean crashes on model with both a CheckConstraint and a
GeneratedField with a Case expression
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36518#comment:11>

[Django]

#36518: full_clean crashes on model with both a CheckConstraint and a
GeneratedField with a Case expression
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: Simon
| Charette

Type: Bug | Status: closed

Component: Database layer | Version: 5.2
(models, ORM) |

Severity: Release blocker | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

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

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

Comment:

In [changeset:"079d31e698fa08dd92e2bc4f3fe9b4817a214419" 079d31e6]:
{{{#!CommitTicketReference repository=""
revision="079d31e698fa08dd92e2bc4f3fe9b4817a214419"
Fixed #34871, #36518 -- Implemented unresolved lookups expression
replacement.

This allows the proper resolving of lookups when performing constraint
validation involving Q and Case objects.

Thanks Andrew Roberts for the report and Sarah for the tests and review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36518#comment:12>

[Django]

#36518: full_clean crashes on model with both a CheckConstraint and a
GeneratedField with a Case expression
-------------------------------------+-------------------------------------
Reporter: Olivier Dalang | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
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:"b3bb7230e1225861b5c1f08931f2d82c2b04133a" b3bb723]:
{{{#!CommitTicketReference repository=""
revision="b3bb7230e1225861b5c1f08931f2d82c2b04133a"
[5.2.x] Fixed #34871, #36518 -- Implemented unresolved lookups expression

replacement.

This allows the proper resolving of lookups when performing constraint
validation involving Q and Case objects.

Thanks Andrew Roberts for the report and Sarah for the tests and review.

Backport of 079d31e698fa08dd92e2bc4f3fe9b4817a214419 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36518#comment:13>