diff options
| -rw-r--r-- | AUTHORS | 1 | ||||
| -rw-r--r-- | django/db/models/query.py | 9 | ||||
| -rw-r--r-- | docs/releases/1.11.16.txt | 13 | ||||
| -rw-r--r-- | docs/releases/2.0.9.txt | 13 | ||||
| -rw-r--r-- | docs/releases/2.1.1.txt | 3 | ||||
| -rw-r--r-- | docs/releases/index.txt | 2 | ||||
| -rw-r--r-- | tests/get_or_create/models.py | 2 | ||||
| -rw-r--r-- | tests/get_or_create/tests.py | 58 |
8 files changed, 96 insertions, 5 deletions
@@ -574,6 +574,7 @@ answer newbie questions, and generally made Django that much better: michael.mcewan@gmail.com Michael Placentra II <someone@michaelplacentra2.net> Michael Radziej <mir@noris.de> + Michael Sanders <m.r.sanders@gmail.com> Michael Schwarz <michi.schwarz@gmail.com> Michael Sinov <sihaelov@gmail.com> Michael Thornhill <michael.thornhill@gmail.com> diff --git a/django/db/models/query.py b/django/db/models/query.py index 5ae451879c..8642d6f798 100644 --- a/django/db/models/query.py +++ b/django/db/models/query.py @@ -501,7 +501,9 @@ class QuerySet: try: obj = self.select_for_update().get(**lookup) except self.model.DoesNotExist: - obj, created = self._create_object_from_params(lookup, params) + # Lock the row so that a concurrent update is blocked until + # after update_or_create() has performed its save. + obj, created = self._create_object_from_params(lookup, params, lock=True) if created: return obj, created for k, v in defaults.items(): @@ -509,7 +511,7 @@ class QuerySet: obj.save(using=self.db) return obj, False - def _create_object_from_params(self, lookup, params): + def _create_object_from_params(self, lookup, params, lock=False): """ Try to create an object using passed params. Used by get_or_create() and update_or_create(). @@ -521,7 +523,8 @@ class QuerySet: return obj, True except IntegrityError as e: try: - return self.get(**lookup), False + qs = self.select_for_update() if lock else self + return qs.get(**lookup), False except self.model.DoesNotExist: pass raise e diff --git a/docs/releases/1.11.16.txt b/docs/releases/1.11.16.txt new file mode 100644 index 0000000000..04335f9439 --- /dev/null +++ b/docs/releases/1.11.16.txt @@ -0,0 +1,13 @@ +============================ +Django 1.11.16 release notes +============================ + +*Expected September 1, 2018* + +Django 1.11.16 fixes a data loss bug in 1.11.15. + +Bugfixes +======== + +* Fixed a race condition in ``QuerySet.update_or_create()`` that could result + in data loss (:ticket:`29499`). diff --git a/docs/releases/2.0.9.txt b/docs/releases/2.0.9.txt new file mode 100644 index 0000000000..2def94ef73 --- /dev/null +++ b/docs/releases/2.0.9.txt @@ -0,0 +1,13 @@ +========================== +Django 2.0.9 release notes +========================== + +*Expected September 1, 2018* + +Django 2.0.9 fixes a data loss bug in 2.0.8. + +Bugfixes +======== + +* Fixed a race condition in ``QuerySet.update_or_create()`` that could result + in data loss (:ticket:`29499`). diff --git a/docs/releases/2.1.1.txt b/docs/releases/2.1.1.txt index 42fe6edeb3..dd9662118d 100644 --- a/docs/releases/2.1.1.txt +++ b/docs/releases/2.1.1.txt @@ -9,4 +9,5 @@ Django 2.1.1 fixes several bugs in 2.1. Bugfixes ======== -* ... +* Fixed a race condition in ``QuerySet.update_or_create()`` that could result + in data loss (:ticket:`29499`). diff --git a/docs/releases/index.txt b/docs/releases/index.txt index 364fc7f3d1..1f9a4ecd58 100644 --- a/docs/releases/index.txt +++ b/docs/releases/index.txt @@ -33,6 +33,7 @@ versions of the documentation contain the release notes for any later releases. .. toctree:: :maxdepth: 1 + 2.0.9 2.0.8 2.0.7 2.0.6 @@ -48,6 +49,7 @@ versions of the documentation contain the release notes for any later releases. .. toctree:: :maxdepth: 1 + 1.11.16 1.11.15 1.11.14 1.11.13 diff --git a/tests/get_or_create/models.py b/tests/get_or_create/models.py index 4a33a809bb..6510bb9464 100644 --- a/tests/get_or_create/models.py +++ b/tests/get_or_create/models.py @@ -2,7 +2,7 @@ from django.db import models class Person(models.Model): - first_name = models.CharField(max_length=100) + first_name = models.CharField(max_length=100, unique=True) last_name = models.CharField(max_length=100) birthday = models.DateField() defaults = models.TextField() diff --git a/tests/get_or_create/tests.py b/tests/get_or_create/tests.py index 1aed318608..ac21b86260 100644 --- a/tests/get_or_create/tests.py +++ b/tests/get_or_create/tests.py @@ -514,6 +514,64 @@ class UpdateOrCreateTransactionTests(TransactionTestCase): self.assertGreater(after_update - before_start, timedelta(seconds=0.5)) self.assertEqual(updated_person.last_name, 'NotLennon') + @skipUnlessDBFeature('has_select_for_update') + @skipUnlessDBFeature('supports_transactions') + def test_creation_in_transaction(self): + """ + Objects are selected and updated in a transaction to avoid race + conditions. This test checks the behavior of update_or_create() when + the object doesn't already exist, but another thread creates the + object before update_or_create() does and then attempts to update the + object, also before update_or_create(). It forces update_or_create() to + hold the lock in another thread for a relatively long time so that it + can update while it holds the lock. The updated field isn't a field in + 'defaults', so update_or_create() shouldn't have an effect on it. + """ + lock_status = {'lock_count': 0} + + def birthday_sleep(): + lock_status['lock_count'] += 1 + time.sleep(0.5) + return date(1940, 10, 10) + + def update_birthday_slowly(): + try: + Person.objects.update_or_create(first_name='John', defaults={'birthday': birthday_sleep}) + finally: + # Avoid leaking connection for Oracle + connection.close() + + def lock_wait(expected_lock_count): + # timeout after ~0.5 seconds + for i in range(20): + time.sleep(0.025) + if lock_status['lock_count'] == expected_lock_count: + return True + self.skipTest('Database took too long to lock the row') + + # update_or_create in a separate thread. + t = Thread(target=update_birthday_slowly) + before_start = datetime.now() + t.start() + lock_wait(1) + # Create object *after* initial attempt by update_or_create to get obj + # but before creation attempt. + Person.objects.create(first_name='John', last_name='Lennon', birthday=date(1940, 10, 9)) + lock_wait(2) + # At this point, the thread is pausing for 0.5 seconds, so now attempt + # to modify object before update_or_create() calls save(). This should + # be blocked until after the save(). + Person.objects.filter(first_name='John').update(last_name='NotLennon') + after_update = datetime.now() + # Wait for thread to finish + t.join() + # Check call to update_or_create() succeeded and the subsequent + # (blocked) call to update(). + updated_person = Person.objects.get(first_name='John') + self.assertEqual(updated_person.birthday, date(1940, 10, 10)) # set by update_or_create() + self.assertEqual(updated_person.last_name, 'NotLennon') # set by update() + self.assertGreater(after_update - before_start, timedelta(seconds=1)) + class InvalidCreateArgumentsTests(SimpleTestCase): msg = "Invalid field name(s) for model Thing: 'nonexistent'." |