Fixed #35957 -- Allowed auto fields to be used in composite primary keys. #19585

shangxiao · 2025-06-23T14:25:59Z

Trac ticket number

ticket-35957

Branch description

Allow auto fields to be used as part of a composite PK:

It appears all that was necessary was to remove the check and to remove setting the primary_key from the deconstruction...
SQLite does not allow this, ref https://www.sqlite.org/autoinc.html - whilst not explicitly stated it only talks about using AUTOINCREMENT after PRIMARY KEY on the column declaration

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.

…eys.

shangxiao · 2025-06-23T14:27:13Z

django/db/models/fields/__init__.py

@@ -2792,14 +2792,24 @@ def __init__(self, *args, **kwargs):
    def check(self, **kwargs):
        return [
            *super().check(**kwargs),
-            *self._check_primary_key(),
+            *self._check_primary_key(kwargs.get("databases", ["default"])),


One of the tests I saw was passing databases kwarg into check() but in another test it was absent. Is this a correct assumption here that databases may be passed in?

It should default to an empty list, see how CharField.check does it

django/django/db/models/fields/__init__.py

Lines 1219 to 1225 in 3f59711

def check(self, **kwargs):

databases = kwargs.get("databases") or []

return [

*super().check(**kwargs),

*self._check_db_collation(databases),

*self._check_max_length_attribute(**kwargs),

]

shangxiao · 2025-06-23T14:28:40Z

django/db/models/fields/__init__.py

+                all(
+                    connections[db].features.supports_autofields_in_composite_pk
+                    for db in databases
+                )


Simon had recently mentioned that only checking the default connection's features was harmful in a multi-engine setup. Is this what we want to do moving forward or just continue to use the default connection? If so we may need to abstract this as it will catch folks out.

I suppose what I should instead be doing here is iterating over connections in full 🤔

Also just wonder whether we may want to add something like this to Field:

@cached_property def part_of_pk(self): return self.primary_key or self in getattr(self.model._meta.pk, "pk", [])

Is this what we want to do moving forward or just continue to use the default connection? If so we may need to abstract this as it will catch folks out.

We should most definitely not use the default connection. If not specified the check should not be performed.

shangxiao · 2025-06-23T14:30:21Z

tests/composite_pk/models/tenant.py

+class AutoId(models.Model):
+    pk = models.CompositePrimaryKey("id", "name")
+    id = models.BigAutoField()
+    name = models.CharField(max_length=255)
+
+    class Meta:
+        required_db_features = ["supports_autofields_in_composite_pk"]


Does someone know how to skip checks when required_db_features is set? My tests were failing because it was still failing the check when run with SQLite.

Refer to how Constraint.check is implemented

e.g.

django/django/db/models/constraints.py

Lines 152 to 155 in 3f59711

if not (

connection.features.supports_table_check_constraints

or "supports_table_check_constraints" in model._meta.required_db_features

):

shangxiao · 2025-06-23T14:44:13Z

tests/composite_pk/tests.py

@@ -220,6 +220,12 @@ def test_model_forms(self):
        ):
            self.assertIsNone(modelform_factory(Comment, fields=["pk"]))

+    def test_autoid_in_composite_pk(self):
+        obj = AutoId.objects.create(name="Automatically generated ID")
+        self.assertEqual(obj.id, 1)


What's the correct way to check an auto field's initial value?

Assert it's not None?

charettes · 2025-06-27T18:15:35Z

django/db/models/fields/__init__.py

+                    connections[db].features.supports_autofields_in_composite_pk
+                    for db in databases
+                )
+                and self in getattr(self.model._meta.pk, "fields", [])


Suggested change

and self in getattr(self.model._meta.pk, "fields", [])

and self in self.model._meta.pk_fields

charettes · 2025-06-27T18:18:43Z

django/db/models/fields/__init__.py

@@ -2792,14 +2792,24 @@ def __init__(self, *args, **kwargs):
    def check(self, **kwargs):
        return [
            *super().check(**kwargs),
-            *self._check_primary_key(),
+            *self._check_primary_key(kwargs.get("databases", ["default"])),


It should default to an empty list, see how CharField.check does it

django/django/db/models/fields/__init__.py

Lines 1219 to 1225 in 3f59711

def check(self, **kwargs):

databases = kwargs.get("databases") or []

return [

*super().check(**kwargs),

*self._check_db_collation(databases),

*self._check_max_length_attribute(**kwargs),

]

charettes · 2025-06-27T18:19:20Z

django/db/models/fields/__init__.py

+                all(
+                    connections[db].features.supports_autofields_in_composite_pk
+                    for db in databases
+                )


Is this what we want to do moving forward or just continue to use the default connection? If so we may need to abstract this as it will catch folks out.

We should most definitely not use the default connection. If not specified the check should not be performed.

charettes · 2025-06-27T18:20:45Z

tests/composite_pk/models/tenant.py

+class AutoId(models.Model):
+    pk = models.CompositePrimaryKey("id", "name")
+    id = models.BigAutoField()
+    name = models.CharField(max_length=255)
+
+    class Meta:
+        required_db_features = ["supports_autofields_in_composite_pk"]


Refer to how Constraint.check is implemented

e.g.

django/django/db/models/constraints.py

Lines 152 to 155 in 3f59711

if not (

connection.features.supports_table_check_constraints

or "supports_table_check_constraints" in model._meta.required_db_features

):

charettes · 2025-06-27T18:21:45Z

tests/composite_pk/tests.py

@@ -220,6 +220,12 @@ def test_model_forms(self):
        ):
            self.assertIsNone(modelform_factory(Comment, fields=["pk"]))

+    def test_autoid_in_composite_pk(self):
+        obj = AutoId.objects.create(name="Automatically generated ID")
+        self.assertEqual(obj.id, 1)


Assert it's not None?

shangxiao · 2025-06-28T15:33:26Z

Thanks Simon, have applied your suggestions 👍

Fixed #35957 -- Allowed auto fields to be used in composite primary k…

2a94f8d

…eys.

shangxiao commented Jun 23, 2025

View reviewed changes

charettes reviewed Jun 27, 2025

View reviewed changes

shangxiao force-pushed the ticket-35957 branch from fc486e8 to 64d11e4 Compare June 28, 2025 15:16

Suggestions from Simon

c0fb099

shangxiao force-pushed the ticket-35957 branch from 64d11e4 to c0fb099 Compare June 28, 2025 15:32

	def check(self, **kwargs):
	databases = kwargs.get("databases") or []
	return [
	super().check(*kwargs),
	*self._check_db_collation(databases),
	self._check_max_length_attribute(*kwargs),
	]

	if not (
	connection.features.supports_table_check_constraints
	or "supports_table_check_constraints" in model._meta.required_db_features
	):

	and self in getattr(self.model._meta.pk, "fields", [])
	and self in self.model._meta.pk_fields

Uh oh!

Fixed #35957 -- Allowed auto fields to be used in composite primary keys. #19585

Are you sure you want to change the base?

Fixed #35957 -- Allowed auto fields to be used in composite primary keys. #19585

Conversation

shangxiao commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Trac ticket number

Branch description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shangxiao commented Jun 28, 2025

Uh oh!

Uh oh!

shangxiao commented Jun 23, 2025 •

edited

Loading