Model validation of constraint involving ForeignObject considers only first column

[Jacob Walls]

Discovered during #36580 in ​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):

tests/composite_pk/models/tenant.py

@@ -48 +48 @@
     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")

tests/composite_pk/test_models.py

@@ -1 +1 @@
 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 +121 @@
                 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):

tests/composite_pk/tests.py

@@ -187 +187 @@
             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="user0002@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.

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)

[Sarah Boyce]

Thank you Jacob, replicated applying to 5.2

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

I think following the reasoning in this comment, we should mark as a release blocker for 5.2

[Sarah Boyce]

Note that I have written a test

tests/foreign_object/models/__init__.py

@@ -1 +1 @@
 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 +10 @@
     "ArticleTag",
     "ArticleTranslation",
     "Contact",
+    "ContactCheck",
     "Country",
     "Customer",
     "CustomerTab",

tests/foreign_object/models/customers.py

@@ -41 +41 @@
     )
 
 
+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(

tests/foreign_object/tests.py

@@ -15 +15 @@
     ArticleTag,
     ArticleTranslation,
     Country,
+    ContactCheck,
     CustomerTab,
     Friendship,
     Group,
@@ -798 +799 @@
     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)

[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

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

django/db/models/base.py

@@ -39 +39 @@
     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 +1381 @@
                 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:

[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)

[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.

[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.

[Sarah Boyce]

I will try and pull something together

[Sarah Boyce]

Following further discussion, downgraded from a release blocker.
This bug technically exists prior to 5.2 and the preferred fix is not 100% backwards compatible.

[Natalia Bidart]

Setting back to Accepted until the final tweaks are pushed.

[Jacob Walls]

Ready for a final review.

[nessita <124304+nessita@…>]

In 5b51e6f:

Fixed #36611, Refs #36580 -- Added system check for multicolumn ForeignObject in Meta.indexes/constraints/unique_together.

ForeignObjects with multiple from_fields are not supported in these
options.

Co-authored-by: Jacob Walls <jacobtylerwalls@…>
Co-authored-by: Natalia <124304+nessita@…>

[Natalia <124304+nessita@…>]

In 0fa339c:

[6.0.x] Fixed #36611, Refs #36580 -- Added system check for multicolumn ForeignObject in Meta.indexes/constraints/unique_together.

ForeignObjects with multiple from_fields are not supported in these
options.

Co-authored-by: Jacob Walls <jacobtylerwalls@…>
Co-authored-by: Natalia <124304+nessita@…>

Backport of 5b51e6f759f2ba993219347435149173c756c478 from main.