From 82ffd26c9fc2e0ac7fc54c431f1247c285d1cbc7 Mon Sep 17 00:00:00 2001 From: "utkarsh.arya@zomato.com" Date: Sat, 15 Nov 2025 23:02:45 +0000 Subject: [PATCH] Fix duplicate INSERT when saving model with explicit pk Restore pre-3.0 behavior: only skip UPDATE if pk is from default. This avoids INSERT failures when saving instances with explicit pk values and primary key fields with defaults. Refs #29260. --- django/db/models/base.py | 34 ++++++++++++++++++++++++-- tests/basic/tests.py | 52 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/django/db/models/base.py b/django/db/models/base.py index e3b14a41a0..d36c808f3f 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -399,6 +399,8 @@ class ModelState: # on the actual save. adding = True fields_cache = ModelStateFieldsCacheDescriptor() + # Tracks whether the pk value came from the field's default during initialization + pk_from_default = False class Model(metaclass=ModelBase): @@ -415,6 +417,22 @@ class Model(metaclass=ModelBase): # Set up the storage for instance state self._state = ModelState() + # Check if 'pk' was explicitly provided in kwargs or args before processing fields. + # This is needed to correctly track whether the primary key value came from + # the field's default or was explicitly set by the user. + pk_explicitly_provided = 'pk' in kwargs or opts.pk.attname in kwargs or opts.pk.name in kwargs + # Check if pk was provided as a positional argument + if args: + # Find the position of the pk field in concrete_fields + pk_position = None + for i, field in enumerate(opts.concrete_fields): + if field.primary_key: + pk_position = i + break + # If pk field position is within the provided args, it was explicitly provided + if pk_position is not None and pk_position < len(args): + pk_explicitly_provided = True + # There is a rather weird disparity here; if kwargs, it's set, then args # overrides it. It should be one or the other; don't duplicate the work # The reason for the kwargs check is that standard iterator passes in by @@ -450,6 +468,8 @@ class Model(metaclass=ModelBase): # Virtual field if field.attname not in kwargs and field.column is None: continue + # Track if this field's value came from get_default() + from_default = False if kwargs: if isinstance(field.remote_field, ForeignObjectRel): try: @@ -462,6 +482,7 @@ class Model(metaclass=ModelBase): val = kwargs.pop(field.attname) except KeyError: val = field.get_default() + from_default = True else: try: val = kwargs.pop(field.attname) @@ -471,8 +492,15 @@ class Model(metaclass=ModelBase): # get_default() to be evaluated, and then not used. # Refs #12057. val = field.get_default() + from_default = True else: val = field.get_default() + from_default = True + + # Track if the primary key value came from the default. + # Only set this if the pk was not explicitly provided in kwargs. + if field.primary_key and from_default and not pk_explicitly_provided: + self._state.pk_from_default = True if is_related_object: # If we are passed a related instance, set it using the @@ -847,12 +875,14 @@ class Model(metaclass=ModelBase): if not pk_set and (force_update or update_fields): raise ValueError("Cannot force an update in save() with no primary key.") updated = False - # Skip an UPDATE when adding an instance and primary key has a default. + # Skip an UPDATE when adding an instance and primary key has a default, + # but only if the pk value came from the default during initialization. if ( not force_insert and self._state.adding and self._meta.pk.default and - self._meta.pk.default is not NOT_PROVIDED + self._meta.pk.default is not NOT_PROVIDED and + self._state.pk_from_default ): force_insert = True # If possible, try an UPDATE. If that doesn't update anything, do an INSERT. diff --git a/tests/basic/tests.py b/tests/basic/tests.py index 5eada343e1..a61e42fbe9 100644 --- a/tests/basic/tests.py +++ b/tests/basic/tests.py @@ -139,6 +139,58 @@ class ModelInstanceCreationTests(TestCase): with self.assertNumQueries(1): PrimaryKeyWithDefault().save() + def test_save_with_explicit_pk_when_pk_has_default(self): + """ + When saving a model instance with an explicit pk value that already + exists in the database, it should perform an UPDATE, not an INSERT, + even if the pk field has a default value. + Regression test for issue where Django 3.0+ incorrectly tries to + INSERT instead of UPDATE when pk field has a default. + """ + # Create an initial instance + obj1 = PrimaryKeyWithDefault.objects.create() + initial_pk = obj1.pk + + # Create a new instance with the same explicit pk + obj2 = PrimaryKeyWithDefault(pk=initial_pk) + obj2.uuid = initial_pk # Explicitly set the pk + + # This should perform an UPDATE, not an INSERT + obj2.save() + + # Verify there's still only one object + self.assertEqual(PrimaryKeyWithDefault.objects.count(), 1) + self.assertEqual(PrimaryKeyWithDefault.objects.get().pk, initial_pk) + + def test_save_with_explicit_pk_after_objects_create(self): + """ + Test the exact scenario from the ticket: + After creating an object, creating a new instance with the explicit pk + of the existing object should perform an UPDATE, not an INSERT. + This is particularly important for loaddata to work correctly when + loading fixtures multiple times. + """ + # This mirrors the scenario from the ticket description: + # s0 = Sample.objects.create() + s0 = PrimaryKeyWithDefault.objects.create() + + # s1 = Sample(pk=s0.pk, name='Test 1') + # Since PrimaryKeyWithDefault doesn't have a name field, + # we'll just test with the pk + s1 = PrimaryKeyWithDefault(pk=s0.pk) + + # s1.save() + # In Django 2.2 and earlier, this would result in an INSERT followed by an UPDATE. + # In Django 3.0+ with the bug, this would result in two INSERTs (second one fails). + # With our fix, this should attempt UPDATE first, which succeeds. + s1.save() + + # Verify that there's still only one object in the database + self.assertEqual(PrimaryKeyWithDefault.objects.count(), 1) + + # Verify the pk hasn't changed + self.assertEqual(s1.pk, s0.pk) + class ModelTest(TestCase): def test_objects_attribute_is_only_available_on_the_class_itself(self):