save() behavior for new model instance with default primary key does not apply to CompositePrimaryKey

[Jacob Walls]

Based on the ​description of how save() is different than update_or_create() if the model's primary key has a default, i.e. that only an INSERT is attempted if the pk is not set by the user, I would expect models with a CompositePrimaryKey to behave as if they "have a default" if all of their composed fields have a default, or else for this to be called out in the doc.

This test shows this is not the case. The first test showing 2 queries instead of 1 is not such a serious problem, but the second test not raising IntegrityError seems like it could bite a user who was expecting to catch IntegrityError and do a custom update (based on how other models behave since Django 3.0 and #29260).

tests/composite_pk/models/tenant.py

@@ +1 @@
+import uuid
 from django.db import models
 
 
@@ -46 +47 @@
 
 class Post(models.Model):
     pk = models.CompositePrimaryKey("tenant_id", "id")
-    tenant = models.ForeignKey(Tenant, on_delete=models.CASCADE)
-    id = models.UUIDField()
+    tenant = models.ForeignKey(Tenant, on_delete=models.CASCADE, default=1)
+    id = models.UUIDField(default=uuid.uuid4)

tests/composite_pk/test_create.py

@@ +1 @@
+from django.db import IntegrityError
 from django.test import TestCase
 
-from .models import Tenant, User
+from .models import Post, Tenant, User
 
 
 class CompositePKCreateTests(TestCase):
@@ -14 +15 @@
             id=1,
             email="user0001@example.com",
         )
+        cls.post = Post.objects.create()
+
+    def test_save_default_pk_not_set(self):
+        with self.assertNumQueries(1):
+            Post().save()
+
+    def test_save_default_pk_set(self):
+        with self.assertRaises(IntegrityError):
+            Post(tenant_id=1, id=self.post.id).save()
+
 
     def test_create_user(self):
         test_cases = (

[Sarah Boyce]

Thank you Jacob! Agree this needs either a fix or some docs

[mao xin chen]

This is my first time trying to submit code. Please see if it can meet the requirements. If so, I will learn to submit PR. Thanks

django/db/models/base.py

@@ -455 +455 @@
     # explicit (non-auto) PKs. This impacts validation only; it has no effect
     # on the actual save.
     adding = True
+    is_set_composite_pk = False
     fields_cache = ModelStateFieldsCacheDescriptor()
 
 
@@ -584 +585 @@
         new = cls(*values)
         new._state.adding = False
         new._state.db = db
+        if new._meta.is_composite_pk:
+            new._state.is_set_composite_pk = True
         return new
 
     def __repr__(self):
@@ -1104 +1107 @@
                 if f.name in update_fields or f.attname in update_fields
             ]
 
-        if not self._is_pk_set(meta):
-            pk_val = meta.pk.get_pk_value_on_save(self)
-            setattr(self, meta.pk.attname, pk_val)
-        pk_set = self._is_pk_set(meta)
+        if meta.is_composite_pk and not (force_update or update_fields):
+            pk_set = self._state.is_set_composite_pk
+        else:
+            pk_set = self._is_pk_set(meta)
+            if not pk_set:
+                pk_val = meta.pk.get_pk_value_on_save(self)
+                setattr(self, meta.pk.attname, pk_val)
+                pk_set = self._is_pk_set(meta)
+
         if not pk_set and (force_update or update_fields):
             raise ValueError("Cannot force an update in save() with no primary key.")
         updated = False

django/db/models/fields/composite.py

@@ -28 +28 @@
         attnames = self.attnames
         length = len(attnames)
 
-        if values is None:
+        is_clear_cp = values is None
+        if is_clear_cp:
             values = (None,) * length
 
         if not isinstance(values, (list, tuple)):
@@ -38 +39 @@
 
         for attname, value in zip(attnames, values):
             setattr(instance, attname, value)
+        instance._state.is_set_composite_pk = is_clear_cp
 
 
 class CompositePrimaryKey(Field):

tests/composite_pk/models/tenant.py

@@ +1 @@
+import uuid
+
 from django.db import models
 
 
@@ -46 +48 @@
 
 class Post(models.Model):
     pk = models.CompositePrimaryKey("tenant_id", "id")
-    tenant = models.ForeignKey(Tenant, on_delete=models.CASCADE)
-    id = models.UUIDField()
+    tenant = models.ForeignKey(Tenant, on_delete=models.CASCADE, default=1)
+    id = models.UUIDField(default=uuid.uuid4)

tests/composite_pk/test_create.py

@@ -1 +1 @@
 from django.test import TestCase
 
-from .models import Tenant, User
+from .models import Tenant, User, Post
 
 
 class CompositePKCreateTests(TestCase):
@@ -14 +14 @@
             id=1,
             email="user0001@example.com",
         )
+        cls.post = Post.objects.create()
+
+    def test_save_default_pk_not_set(self):
+        with self.assertNumQueries(1):
+            Post().save()
 
     def test_create_user(self):
         test_cases = (

tests/composite_pk/test_filter.py

@@ -37 +37 @@
         cls.comment_4 = Comment.objects.create(id=4, user=cls.user_3)
         cls.comment_5 = Comment.objects.create(id=5, user=cls.user_1)
 
+
     def test_filter_and_count_user_by_pk(self):
         test_cases = (
             ({"pk": self.user_1.pk}, 1),

[Csirmaz Bendegúz]

Replying to mao xin chen:

This is my first time trying to submit code. Please see if it can meet the requirements. If so, I will learn to submit PR. Thanks

Thanks for the patch! Please check the ​docs on how to contribute. If you open a PR on GitHub we can review your code.

[Csirmaz Bendegúz]

Thanks Jacob!

n	default	obj.save()	conflict
1	yes	insert	IntegrityError
2	no	update & insert	update

1. If pk has a default or db_default, saving a new object with a conflicting pk attempts an insert and raises an IntegrityError
2. If pk doesn't have a default or db_default, saving a new object with a conflicting pk attempts an update (if no rows affected attempts an insert)

Is that correct?

# 1. Model.id has a default
foo = Model(id=1)
foo.save() # INSERT
bar = Model(id=1)
bar.save() # INSERT & IntegrityError

# 2. Model.id doesn't have a default
foo = Model(id=1)
foo.save() # UPDATE & INSERT
bar = Model(id=1)
bar.save() # UPDATE