[Django] #36611: Model validation of constraint involving ForeignObject considers only first column

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 5.2 | 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
-------------------------------------+-------------------------------------
Discovered during #36580 in
[https://github.com/django/django/pull/19798/files#r2350234373 review].

Similar to #36431, where only the first column of a `ForeignObject` was
considered in `values()`, only the first column is considered during model
validation of constraints.

Composite PK's are not affected, because they raise system checks if you
try to use them in a constraint. But `ForeignObject` has been broken since
its introduction in this regard. Reproduced on 5.2, but thus, not a
release blocker.

Rough test (needs adjusting to avoid hijacking this model and
unnecessarily skipping tests on backends not supporting constraints):
{{{#!diff
diff --git a/tests/composite_pk/models/tenant.py
b/tests/composite_pk/models/tenant.py
index 65eb0feae8..c818ec4de7 100644
--- a/tests/composite_pk/models/tenant.py
+++ b/tests/composite_pk/models/tenant.py
@@ -48,6 +48,16 @@ class Comment(models.Model):
text = models.TextField(default="", blank=True)
integer = models.IntegerField(default=0)

+ class Meta:
+ # TODO: use new model instead
+ required_db_features = {"supports_table_check_constraints"}
+ constraints = [
+ models.CheckConstraint(
+ condition=models.Q(user__lt=(1000, 1000)),
+ name="user_limit",
+ ),
+ ]
+

class Post(models.Model):
pk = models.CompositePrimaryKey("tenant_id", "id")
diff --git a/tests/composite_pk/test_models.py
b/tests/composite_pk/test_models.py
index 27157a52ad..05aafd5306 100644
--- a/tests/composite_pk/test_models.py
+++ b/tests/composite_pk/test_models.py
@@ -1,6 +1,8 @@
from django.contrib.contenttypes.models import ContentType
from django.core.exceptions import ValidationError
+from django.db import connection
from django.test import TestCase
+from django.test.utils import CaptureQueriesContext

from .models import Comment, Tenant, Token, User

@@ -119,7 +121,23 @@ class CompositePKModelsTests(TestCase):
self.assertSequenceEqual(ctx.exception.messages,
messages)

def test_full_clean_update(self):
- with self.assertNumQueries(1):
+ with CaptureQueriesContext(connection) as ctx:
+ self.comment_1.full_clean()
+ select_queries = [
+ query["sql"]
+ for query in ctx.captured_queries
+ if "select" in query["sql"].lower()
+ ]
+ self.assertEqual(len(select_queries), 2, select_queries) # 1 on
5.2.x
+
+ def test_full_clean_update_invalid(self):
+ self.comment_1.tenant_id = 1001
+ with self.assertRaises(ValidationError):
+ self.comment_1.full_clean()
+
+ self.comment_1.tenant_id = 1
+ self.comment_1.user_id = 1001
+ with self.assertRaises(ValidationError):
self.comment_1.full_clean()

def test_field_conflicts(self):
diff --git a/tests/composite_pk/tests.py b/tests/composite_pk/tests.py
index 2245a472e4..5b7e34a0bc 100644
--- a/tests/composite_pk/tests.py
+++ b/tests/composite_pk/tests.py
@@ -187,12 +187,17 @@ class CompositePKTests(TestCase):
self.assertEqual(user.email, self.user.email)

def test_select_related(self):
- Comment.objects.create(tenant=self.tenant, id=2)
+ user2 = User.objects.create(
+ tenant=self.tenant,
+ id=2,
+ email="user...@example.com",
+ )
+ Comment.objects.create(tenant=self.tenant, id=2, user=user2)
with self.assertNumQueries(1):
comments =
list(Comment.objects.select_related("user").order_by("pk"))
self.assertEqual(len(comments), 2)
self.assertEqual(comments[0].user, self.user)
- self.assertIsNone(comments[1].user)
+ self.assertEqual(comments[1].user, user2)

def test_model_forms(self):
fields = ["tenant", "id", "user_id", "text", "integer"]
}}}
----
Notice `1001` only appears in the first query of the
`test_full_clean_update_invalid`.
{{{#!py
FAIL: test_full_clean_update_invalid
(composite_pk.test_models.CompositePKModelsTests.test_full_clean_update_invalid)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/jwalls/django/tests/composite_pk/test_models.py", line 140,
in test_full_clean_update_invalid
with self.assertRaises(ValidationError):
~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^
AssertionError: ValidationError not raised

----------------------------------------------------------------------
(0.000)
SELECT 1 AS "a"
FROM "composite_pk_tenant"
WHERE "composite_pk_tenant"."id" = 1001
LIMIT 1;

args=(1,
1001);

ALIAS=DEFAULT (0.000)
SELECT 1 AS "a"
FROM "composite_pk_tenant"
WHERE "composite_pk_tenant"."id" = 1
LIMIT 1;

args=(1,
1);

ALIAS=DEFAULT
----------------------------------------------------------------------
Ran 2 tests in 0.003s

FAILED (failures=1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36611>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | 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 Sarah Boyce):

* severity: Normal => Release blocker
* stage: Unreviewed => Accepted

Comment:

Thank you Jacob, replicated applying to 5.2

> Reproduced on 5.2, but thus, not a release blocker.

I think following the reasoning
[https://code.djangoproject.com/ticket/36431#comment:11 in this comment],
we should mark as a release blocker for 5.2
--
Ticket URL: <https://code.djangoproject.com/ticket/36611#comment:1>

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | 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 Samriddha Kumar Tripathi):

* cc: Samriddha Kumar Tripathi (added)

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

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | 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 Sarah Boyce):

* cc: JaeHyuckSa, Simon Charette (added)

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

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | 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 Devendra Tumu):

* cc: Devendra Tumu (added)

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

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | 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
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce):

Note that I have written a test
{{{#!diff
--- a/tests/foreign_object/models/__init__.py
+++ b/tests/foreign_object/models/__init__.py
@@ -1,5 +1,5 @@
from .article import Article, ArticleIdea, ArticleTag,
ArticleTranslation, NewsArticle
-from .customers import Address, Contact, Customer, CustomerTab
+from .customers import Address, Contact, ContactCheck, Customer,
CustomerTab
from .empty_join import SlugPage
from .person import Country, Friendship, Group, Membership, Person

@@ -10,6 +10,7 @@ __all__ = [
"ArticleTag",
"ArticleTranslation",
"Contact",
+ "ContactCheck",
"Country",
"Customer",
"CustomerTab",
diff --git a/tests/foreign_object/models/customers.py
b/tests/foreign_object/models/customers.py
index 085b7272e9..f9a2e932c5 100644
--- a/tests/foreign_object/models/customers.py
+++ b/tests/foreign_object/models/customers.py
@@ -41,6 +41,27 @@ class Contact(models.Model):
)


+class ContactCheck(models.Model):
+ company_code = models.CharField(max_length=1)
+ customer_code = models.IntegerField()
+ customer = models.ForeignObject(
+ Customer,
+ on_delete=models.CASCADE,
+ related_name="contact_checks",
+ to_fields=["customer_id", "company"],
+ from_fields=["customer_code", "company_code"],
+ )
+
+ class Meta:

+ required_db_features = {"supports_table_check_constraints"}
+ constraints = [
+ models.CheckConstraint(

+ condition=models.Q(customer__lt=(1000, "c")),
+ name="customer_company_limit",
+ ),
+ ]
+
+
class CustomerTab(models.Model):
customer_id = models.IntegerField()
customer = models.ForeignObject(
diff --git a/tests/foreign_object/tests.py b/tests/foreign_object/tests.py
index 09fb47e771..539c1a4fec 100644
--- a/tests/foreign_object/tests.py
+++ b/tests/foreign_object/tests.py
@@ -15,6 +15,7 @@ from .models import (
ArticleTag,
ArticleTranslation,
Country,
+ ContactCheck,
CustomerTab,
Friendship,
Group,
@@ -798,3 +799,9 @@ class ForeignObjectModelValidationTests(TestCase):
def test_validate_constraints_excluding_foreign_object_member(self):
customer_tab = CustomerTab(customer_id=150)
customer_tab.validate_constraints(exclude={"customer_id"})
+
+ @skipUnlessDBFeature("supports_table_check_constraints")
+ def
test_validate_constraints_with_foreign_object_multiple_fields(self):
+ contact = ContactCheck(company_code="d", customer_code=1500)
+ with self.assertRaisesMessage(ValidationError,
"customer_company_limit"):
+ contact.validate_constraints()
}}}

When applied to Django 5.2, this fails with `AssertionError:
ValidationError not raised`

When applied to Django main, this fails with:
{{{
======================================================================
ERROR: test_validate_constraints_with_foreign_object_multiple_fields
(foreign_object.tests.ForeignObjectModelValidationTests.test_validate_constraints_with_foreign_object_multiple_fields)

----------------------------------------------------------------------
Traceback (most recent call last):

File "/path_to_django/tests/foreign_object/tests.py", line 807, in
test_validate_constraints_with_foreign_object_multiple_fields
contact.validate_constraints()
File "/path_to_django/db/models/base.py", line 1653, in
validate_constraints
constraint.validate(model_class, self, exclude=exclude, using=using)
File "/path_to_django/django/db/models/constraints.py", line 212, in
validate
if not Q(self.condition).check(against, using=using):
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/query_utils.py", line 187, in
check
return compiler.execute_sql(SINGLE) is not None
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/sql/compiler.py", line 1611, in
execute_sql
sql, params = self.as_sql()
^^^^^^^^^^^^^
File "/path_to_django/django/db/models/sql/compiler.py", line 795, in
as_sql
self.compile(self.where) if self.where is not None else ("", [])
^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/sql/compiler.py", line 578, in
compile
sql, params = node.as_sql(self, self.connection)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/sql/where.py", line 151, in
as_sql
sql, params = compiler.compile(child)
^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/sql/compiler.py", line 578, in
compile
sql, params = node.as_sql(self, self.connection)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/lookups.py", line 404, in as_sql
lhs_sql, params = self.process_lhs(compiler, connection)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/lookups.py", line 230, in
process_lhs
lhs_sql, params = super().process_lhs(compiler, connection, lhs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/lookups.py", line 110, in
process_lhs
sql, params = compiler.compile(lhs)
^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/sql/compiler.py", line 576, in
compile
sql, params = vendor_impl(self, self.connection)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/expressions.py", line 29, in
as_sqlite
sql, params = self.as_sql(compiler, connection, **extra_context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/expressions.py", line 1107, in
as_sql
arg_sql, arg_params = compiler.compile(arg)
^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/db/models/sql/compiler.py", line 578, in compile
sql, params = node.as_sql(self, self.connection)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/sql/where.py", line 151, in
as_sql
sql, params = compiler.compile(child)
^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/sql/compiler.py", line 578, in
compile
sql, params = node.as_sql(self, self.connection)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/fields/related_lookups.py", line
128, in as_sql
return super().as_sql(compiler, connection)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/lookups.py", line 239, in as_sql
rhs_sql, rhs_params = self.process_rhs(compiler, connection)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/django/db/models/lookups.py", line 138, in
process_rhs
return self.get_db_prep_lookup(value, connection)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/db/models/lookups.py", line 259, in
get_db_prep_lookup
field = getattr(self.lhs.output_field, "target_field", None)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/path_to_django/db/models/fields/related.py", line 506, in
target_field
raise exceptions.FieldError(
django.core.exceptions.FieldError: The relation has multiple target
fields, but only single target field was asked for
}}}

So note to self that any fix, we should also make sure we have applied to
5.2 and tested (in case it is relying on a commit currently applied to
main, not to 5.2)
--
Ticket URL: <https://code.djangoproject.com/ticket/36611#comment:5>

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | 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
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

I suspect this will be very hard to to solve without solving #35956 of the
one ticket I can't find to add generic composite field support as it's
preventing us from performing lookups of the form


{{{#!python
ContactCheck.objects.filter(customer__lt=(1000, "c"))
}}}

in the first place.

Here's an attempt at using `Tuple` which results in the same problem as
`Tuple.output_field` is currently ''faked'' as `models.Field` due to the
lack of `CompositeField` existence

{{{#!diff
diff --git a/django/db/models/base.py b/django/db/models/base.py
index 36b16c1132..d7af0ebd1a 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -39,6 +39,7 @@
lazy_related_operation,
resolve_relation,
)
+from django.db.models.fields.tuple_lookups import Tuple
from django.db.models.functions import Coalesce
from django.db.models.manager import Manager
from django.db.models.options import Options
@@ -1380,11 +1381,27 @@ def _get_field_expression_map(self, meta,
exclude=None):
isinstance(field.remote_field, ForeignObjectRel)
and field not in meta.local_concrete_fields
):
- value = tuple(
- getattr(self, from_field) for from_field in
field.from_fields
- )
- if len(value) == 1:
- value = value[0]
+ composed_fields = field.local_related_fields
+ if len(composed_fields) == 1:
+ value = getattr(self, composed_fields[0].attname)
+ else:
+ composed_values = (
+ getattr(self, composed_field.attname)
+ for composed_field in composed_fields
+ )
+ value = Tuple(
+ *(
+ (
+ composed_value
+ if hasattr(composed_value,
"resolve_expression")
+ else Value(composed_value,
composed_field)
+ )
+ for composed_value, composed_field in zip(
+ composed_values, composed_fields
+ )
+ ),
+ output_field=field,
+ )
elif field.concrete:
value = getattr(self, field.attname)
else:
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36611#comment:6>

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | 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
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce):

In #35992, we added a system check to say that `CompositePrimaryKey`
fields cannot be used in indexes/constraints/unique_together
We could perhaps have approached #36580 with a similar system check, and
maybe that option is still open (so we disallow ForeignObject fields in
indexes/constraints/unique_together)
--
Ticket URL: <https://code.djangoproject.com/ticket/36611#comment:7>

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | 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
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Sarah, I know it represents reverting back work but I do like this
approach. We could revisit once we we add support for generic composite
fields but until then baking hacks to support what is essentially ORM
sugar (that can be expressed by pointing at the concrete fields directly)
makes me feel like we're opening the door to unbounded work.
--
Ticket URL: <https://code.djangoproject.com/ticket/36611#comment:8>

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | 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
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

We could add a check without reverting #36580, since anyone who might be
using this sugar already in 5.2 might be getting by just fine, for
instance if they don't use `ModelForm` or `full_clean`. They could just
silence the system check and carry on, and we can leave work in place we
think we're likely to want later anyway.
--
Ticket URL: <https://code.djangoproject.com/ticket/36611#comment:9>

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Sarah
| Boyce
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 Sarah Boyce):

* owner: (none) => Sarah Boyce
* status: new => assigned

Comment:

I will try and pull something together
--
Ticket URL: <https://code.djangoproject.com/ticket/36611#comment:10>

[Django]

#36611: Model validation of constraint involving ForeignObject considers only first
column
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Sarah
| Boyce
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/36611#comment:11>