mirror of
https://github.com/django/django.git
synced 2025-11-18 19:01:40 +00:00
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.
This commit is contained in:
parent
5a68f02498
commit
82ffd26c9f
2 changed files with 84 additions and 2 deletions
|
|
@ -399,6 +399,8 @@ class ModelState:
|
||||||
# on the actual save.
|
# on the actual save.
|
||||||
adding = True
|
adding = True
|
||||||
fields_cache = ModelStateFieldsCacheDescriptor()
|
fields_cache = ModelStateFieldsCacheDescriptor()
|
||||||
|
# Tracks whether the pk value came from the field's default during initialization
|
||||||
|
pk_from_default = False
|
||||||
|
|
||||||
|
|
||||||
class Model(metaclass=ModelBase):
|
class Model(metaclass=ModelBase):
|
||||||
|
|
@ -415,6 +417,22 @@ class Model(metaclass=ModelBase):
|
||||||
# Set up the storage for instance state
|
# Set up the storage for instance state
|
||||||
self._state = ModelState()
|
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
|
# 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
|
# 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
|
# The reason for the kwargs check is that standard iterator passes in by
|
||||||
|
|
@ -450,6 +468,8 @@ class Model(metaclass=ModelBase):
|
||||||
# Virtual field
|
# Virtual field
|
||||||
if field.attname not in kwargs and field.column is None:
|
if field.attname not in kwargs and field.column is None:
|
||||||
continue
|
continue
|
||||||
|
# Track if this field's value came from get_default()
|
||||||
|
from_default = False
|
||||||
if kwargs:
|
if kwargs:
|
||||||
if isinstance(field.remote_field, ForeignObjectRel):
|
if isinstance(field.remote_field, ForeignObjectRel):
|
||||||
try:
|
try:
|
||||||
|
|
@ -462,6 +482,7 @@ class Model(metaclass=ModelBase):
|
||||||
val = kwargs.pop(field.attname)
|
val = kwargs.pop(field.attname)
|
||||||
except KeyError:
|
except KeyError:
|
||||||
val = field.get_default()
|
val = field.get_default()
|
||||||
|
from_default = True
|
||||||
else:
|
else:
|
||||||
try:
|
try:
|
||||||
val = kwargs.pop(field.attname)
|
val = kwargs.pop(field.attname)
|
||||||
|
|
@ -471,8 +492,15 @@ class Model(metaclass=ModelBase):
|
||||||
# get_default() to be evaluated, and then not used.
|
# get_default() to be evaluated, and then not used.
|
||||||
# Refs #12057.
|
# Refs #12057.
|
||||||
val = field.get_default()
|
val = field.get_default()
|
||||||
|
from_default = True
|
||||||
else:
|
else:
|
||||||
val = field.get_default()
|
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 is_related_object:
|
||||||
# If we are passed a related instance, set it using the
|
# 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):
|
if not pk_set and (force_update or update_fields):
|
||||||
raise ValueError("Cannot force an update in save() with no primary key.")
|
raise ValueError("Cannot force an update in save() with no primary key.")
|
||||||
updated = False
|
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 (
|
if (
|
||||||
not force_insert and
|
not force_insert and
|
||||||
self._state.adding and
|
self._state.adding and
|
||||||
self._meta.pk.default 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
|
force_insert = True
|
||||||
# If possible, try an UPDATE. If that doesn't update anything, do an INSERT.
|
# If possible, try an UPDATE. If that doesn't update anything, do an INSERT.
|
||||||
|
|
|
||||||
|
|
@ -139,6 +139,58 @@ class ModelInstanceCreationTests(TestCase):
|
||||||
with self.assertNumQueries(1):
|
with self.assertNumQueries(1):
|
||||||
PrimaryKeyWithDefault().save()
|
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):
|
class ModelTest(TestCase):
|
||||||
def test_objects_attribute_is_only_available_on_the_class_itself(self):
|
def test_objects_attribute_is_only_available_on_the_class_itself(self):
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue