#26804: update_or_create() updates all columns
----------------------------------------------+----------------------------
Reporter: jensen-cochran | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
| update_or_create
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+----------------------------
The update_or_create() helper function in Django is passed a 'defaults'
kwarg, which contains the fields that we expect the function to update.
When .save() is called in the function to do the update, it does not pass
the update_fields parameter to .save(), and so the SQL generated to
complete the update sets every column (except pk), even those not passed
in 'defaults'.
This creates a potential race condition. In the period of time between
when the function calls .get() to retrieve the object and when it calls
.save(), some other process may have updated a column in that row that has
nothing to do with this update. When save() is called, these changes will
be overwritten. This is not an issue for the fields passed into
'defaults' since we expected those to be updated anyway. The main issue
is that update_or_create updates fields/columns other than those passed in
'defaults', and does so in a way that could cause data loss.
Example:
{{{
class Authors(models.Model):
id = models.AutoField(primary_key=True)
name = models.CharField(max_length=128, unique=True)
newest_release = models.DateTimeField()
active = models.BooleanField(default=True)
Authors.objects.update_or_create(
name='Bob',
defaults={
'newest_release': timezone.now()
})
# Django update_or_create code from
https://github.com/django/django/blob/master/django/db/models/query.py
def update_or_create(self, defaults=None, **kwargs):
"""
Looks up an object with the given kwargs, updating one with
defaults
if it exists, otherwise creates a new one.
Returns a tuple (object, created), where created is a boolean
specifying whether an object was created.
"""
defaults = defaults or {}
lookup, params = self._extract_model_params(defaults, **kwargs)
self._for_write = True
try:
obj = self.get(**lookup)
except self.model.DoesNotExist:
obj, created = self._create_object_from_params(lookup, params)
if created:
return obj, created
for k, v in six.iteritems(defaults):
setattr(obj, k, v)
obj.save(using=self.db)
return obj, False
}}}
Assume that the author Bob already exists in the database.
update_or_create retrieves the Author object using .get().
Meanwhile, another process runs the following SQL:
{{{ UPDATE authors SET active = FALSE WHERE id = 1234; }}}
Then, update_or_create sets the values from 'defaults' on the object, then
calls .save() on the object. Generated SQL would look something like
{{{ UPDATE authors SET name='Bob', active=TRUE,
newest_release='#######'::timestamp, WHERE id = 1234" }}}
Notice that both 'name' and 'active' are set in the SQL, even though they
were not passed in defaults. Bob has been wrongly made active again, even
though the update portion of the update_or_create() call had nothing to do
with the 'active' field. Besides the data issues, it is also not
efficient to have the database update every field/column when there is no
need.
I have tested this behavior by putting a time.sleep() into
update_or_create right after the .get() and updating the database during
this sleep.
Once the sleep finished, my changes were overwritten by .save()
My initial thought to patch this was to change
{{{ obj.save(using=self.db) }}}
in update_or_create into
{{{ obj.save(using=self.db, update_fields=defaults.keys()) }}}
However, this causes a single test to fail in generic_relations, since
update_fields in .save() does not accept GenericForeignKey fields, which
may be bug in and of itself. There are other ways to fix this like
locking the row, but that would be less efficient and not fix the
underlying issue.
{{{
ERROR: test_update_or_create_defaults
(generic_relations.tests.GenericRelationsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib64/python2.7/unittest/case.py", line 365, in run
testMethod()
File "/home/jensen/sandbox/django/tests/generic_relations/tests.py",
line 540, in test_update_or_create_defaults
tag, created = TaggedItem.objects.update_or_create(tag="shiny",
defaults={'content_object': diamond})
File "/home/jensen/sandbox/django/django/db/models/manager.py", line 85,
in manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
File "/home/jensen/sandbox/django/django/db/models/query.py", line 493,
in update_or_create
obj.save(using=self.db, update_fields=defaults.keys())
File "/home/jensen/sandbox/django/django/db/models/base.py", line 782,
in save
% ', '.join(non_model_fields))
ValueError: The following fields do not exist in this model or are m2m
fields: content_object
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26804>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/057.6e26b997668a25238044f30459e8b96b%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
