System check for ManyToManyField & Composite Primary Key checks only one direction

[Jacob Walls]

#36034 added the system check E347 for ManyToManyField when the "to_model" has a composite pk, but the situation is just as unsupported when the composite primary key model is the "from_model", i.e. the one with *both* the ManyToManyField and the CompositePrimaryKey.

We should be able to just adjust ​this system check condition to check the "from" table also.

---

e.g. this (purely for demo, not suggesting we edit this test model):

tests/composite_pk/models/tenant.py

@@ -60 +60 @@
     id = models.SmallIntegerField(unique=True)
     created = models.DateTimeField(auto_now_add=True)
     text = models.TextField(default="", blank=True)
+    tenants = models.ManyToManyField(Tenant)

gives:

ERROR: test_serialize_datetime (composite_pk.tests.CompositePKFixturesTests.test_serialize_datetime)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jwalls/django/tests/composite_pk/tests.py", line 368, in test_serialize_datetime
    result = serializers.serialize("json", TimeStamped.objects.all())
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/django/core/serializers/__init__.py", line 134, in serialize
    s.serialize(queryset, **options)
  File "/Users/jwalls/django/django/core/serializers/base.py", line 144, in serialize
    self.handle_m2m_field(obj, field)
  File "/Users/jwalls/django/django/core/serializers/python.py", line 95, in handle_m2m_field
    queryset_iterator(obj, field),
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/django/core/serializers/python.py", line 89, in queryset_iterator
    query_set = getattr(obj, field.name).select_related(None).only("pk")
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/django/db/models/query.py", line 1632, in select_related
    self._not_support_combined_queries("select_related")
  File "/Users/jwalls/django/django/db/models/query.py", line 2052, in _not_support_combined_queries
    if self.query.combinator:
       ^^^^^^^^^^
  File "/Users/jwalls/django/django/db/models/query.py", line 302, in query
    self._filter_or_exclude_inplace(negate, args, kwargs)
  File "/Users/jwalls/django/django/db/models/query.py", line 1549, in _filter_or_exclude_inplace
    self._query.add_q(Q(*args, **kwargs))
  File "/Users/jwalls/django/django/db/models/sql/query.py", line 1667, in add_q
    clause, _ = self._add_q(q_object, can_reuse)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/django/db/models/sql/query.py", line 1699, in _add_q
    child_clause, needed_inner = self.build_filter(
                                 ^^^^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/django/db/models/sql/query.py", line 1609, in build_filter
    condition = self.build_lookup(lookups, col, value)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/django/db/models/sql/query.py", line 1436, in build_lookup
    lookup = lookup_class(lhs, rhs)
             ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/django/db/models/lookups.py", line 35, in __init__
    self.rhs = self.get_prep_lookup()
               ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jwalls/django/django/db/models/fields/tuple_lookups.py", line 55, in get_prep_lookup
    self.check_rhs_length_equals_lhs_length()
  File "/Users/jwalls/django/django/db/models/fields/tuple_lookups.py", line 69, in check_rhs_length_equals_lhs_length
    len_lhs = len(self.lhs)
              ^^^^^^^^^^^^^
TypeError: object of type 'Col' has no len()

----------------------------------------------------------------------
Ran 170 tests in 0.183s

FAILED (errors=1)

[Natalia Bidart]

Thank you Jacob! How about this for a regression test?

    def test_many_to_many_from_model_with_composite_primary_key(self):
        class Parent(models.Model):
            name = models.CharField(max_length=20)

        class Child(models.Model):
            pk = models.CompositePrimaryKey("version", "name")
            version = models.IntegerField()
            name = models.CharField(max_length=20)
            parents = models.ManyToManyField(Parent)

        field = Child._meta.get_field("parents")
        self.assertEqual(
            field.check(from_model=Child),
            [
                Error(
                    "Field has a CompositePrimaryKey and defines a relation with model "
                    "'Parent' which is not supported.",
                    obj=field,
                    id="fields.E348",
                ),
            ],
        )

[Jason Hall]

I was just trying to reproduce this issue with the following test:

@isolate_apps("model_fields")
class M2MCompositePKCheckTests(SimpleTestCase):
    def test_m2m_from_model_composite_pk_should_raise_e347(self):
        class Target(models.Model):
            name = models.CharField(max_length=100)

            class Meta:
                app_label = "model_fields"
        
        class TimeStamped(models.Model):
            id = models.IntegerField()
            created = models.DateTimeField()
            tenants = models.ManyToManyField(Target)

            class Meta:
                app_label = "model_fields"
                constraints = [
                    models.UniqueConstraint(fields=["id", "created"], name="composite_pk")
                ]
        
        errors = TimeStamped.check()
        self.assertTrue(
            any(e.id == "models.E347" for e in errors),
            msg="Expected E347 for ManyToManyField declared on a model with composite primary key",
        )

I'm not sure if this approach (using UniqueConstraint to simulate a composite PK) is appropriate for the final test, or if we should prefer the version that uses CompositePrimaryKey and field.check(from_model=...).

Just wanted to share what I came up with to help confirm the issue before working on the fix. Happy to adjust based on feedback!

[Jacob Walls]

We should use CompositePrimaryKey, as that ensures the check guards what is unsupported in fact.

Natalia's version riffs on the tests from #36034, which is what I had in mind. And incidentally why I thought this was an easy picking, but I suppose I should go to the forum before taking up a 1-person campaign to use that flag more ;)

[Simon Charette]

Please refer to the contributing documentation, ​you can't mark your own patch RFC.

[nessita <124304+nessita@…>]

In 2013092:

Fixed #36530 -- Extended fields.E347 to check for ManyToManyField involving CompositePrimaryKey on either side.

Thanks to Jacob Walls for the report.

[Natalia <124304+nessita@…>]

In bdc3f9e:

[5.2.x] Fixed #36530 -- Extended fields.E347 to check for ManyToManyField involving CompositePrimaryKey on either side.

Thanks to Jacob Walls for the report.

Backport of 2013092b693be0ebdf36f41dc61615a2de1bbe31 from main.