save() behavior when updating model with default primary keys

Using Django Using the ORM
8

Thanks, this is why I came to django-users first–I wasn’t sure of my footing!

What you can’t tell from this is whether the pk that is set currently exists in the database. Nor, as I’m reading that method, is it able to identify whether the pk value being set comes from the default.

Ugh, the early patch never gets the worm. :slight_smile:

I tried this another way: setting ._state.adding = False at the moment the default is not used. It passes the tests, pending some issues with passing pk= instead of fieldname=. Diff below.

I think that would actually bring things closer to the spirit of the paragraph we’re parsing. I read it to say something like:

Are you providing a primary key?
– Do you intend to be creating a new object?
---- You should be prepared for a primary-key collision to issue an update instead! Beware data loss!

Whereas my scenario was:
Are you providing a primary key? Yes.
Do you intend to create an object? idk, I’m in an overwriting (update-or-create) posture.
You should be prepared…! Wait, I thought the ORM abstracted away the difference between INSERT and UPDATE?

What I’m finding frustrating is that I can’t seem to find any messages in the mailing lists (Django Users and Django Developers) from the timeframe of 29260 (Feb / March 2018) with any sort of discussion about this or why this was an issue to be solved.

I think it was just to optimizing out one UPDATE query on models with auto-generated UUID pk’s. But now it’s costing queries for folks who now need to fetch an instance (directly, or via update_or_create()). It seems like it was implicit from the PR discussions that force_update=True would still be an option for folks. (To paraphrase Carlton, we’re trading one user’s need to call force_insert for someone else’s need to call force_update." But, alas–force_update wasn’t tested.

I think force_update=True not working here is a bug. But I think something like this draft patch would accomplish what the participants on the PRs imagined in the first place (but is it worth it… :thinking: )

diff --git a/django/db/models/base.py b/django/db/models/base.py
index 4a9150bf37..a42448dd12 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -489,6 +489,8 @@ class Model(AltersData, metaclass=ModelBase):
                 if val is _DEFERRED:
                     continue
                 _setattr(self, field.attname, val)
+                if field.primary_key:
+                    self._state.adding = False
         else:
             # Slower, kwargs-ready version.
             fields_iter = iter(opts.fields)
@@ -501,6 +503,8 @@ class Model(AltersData, metaclass=ModelBase):
                         f"{cls.__qualname__}() got both positional and "
                         f"keyword arguments for field '{field.name}'."
                     )
+                if field.primary_key:
+                    self._state.adding = False
 
         # Now we're left with the unprocessed fields that *must* come from
         # keywords, or default.
@@ -522,6 +526,9 @@ class Model(AltersData, metaclass=ModelBase):
                             val = kwargs.pop(field.attname)
                         except KeyError:
                             val = field.get_default()
+                        else:
+                            if field.primary_key:
+                                self._state.adding = False
                 else:
                     try:
                         val = kwargs.pop(field.attname)
@@ -531,6 +538,9 @@ class Model(AltersData, metaclass=ModelBase):
                         # get_default() to be evaluated, and then not used.
                         # Refs #12057.
                         val = field.get_default()
+                    else:
+                        if field.primary_key:
+                            self._state.adding = False
             else:
                 val = field.get_default()
 
diff --git a/tests/basic/tests.py b/tests/basic/tests.py
index ad82cffe8c..2aded16233 100644
--- a/tests/basic/tests.py
+++ b/tests/basic/tests.py
@@ -1,5 +1,6 @@
 import inspect
 import threading
+import uuid
 from datetime import datetime, timedelta
 from unittest import mock
 
@@ -172,7 +173,14 @@ class ModelInstanceCreationTests(TestCase):
         self.assertTrue(Article.objects.filter(id=a.id).exists())
 
     def test_save_primary_with_default(self):
-        # An UPDATE attempt is skipped when a primary key has default.
+        known_uuid = uuid.uuid4()
+        # An UPDATE attempt is made when a primary key is provided.
+        with self.assertNumQueries(2):
+            PrimaryKeyWithDefault(uuid=known_uuid).save()  # fails if pk= is given
+        with self.assertNumQueries(1):
+            PrimaryKeyWithDefault(uuid=known_uuid).save()
+
+        # Otherwise only an INSERT is attempted.
         with self.assertNumQueries(1):
             PrimaryKeyWithDefault().save()
1 Like