django-ordered-model · GeeWee · Mar 12, 2019 · Mar 12, 2019 · Apr 15, 2021 · Apr 15, 2021
diff --git a/ordered_model/models.py b/ordered_model/models.py
@@ -1,7 +1,7 @@
 from functools import partial, reduce
 
 from django.db import models
-from django.db.models import Max, Min, F
+from django.db.models import Max, Min, F, Subquery
 from django.db.models.constants import LOOKUP_SEP
 from django.utils.module_loading import import_string
 from django.utils.translation import gettext_lazy as _
@@ -178,7 +178,17 @@ def get_ordering_queryset(self, qs=None):
                 qs = model._meta.default_manager.all()
             else:
                 qs = self._meta.default_manager.all()
-        return qs.filter_by_order_with_respect_to(self)
+        qs = qs.filter_by_order_with_respect_to(self)
+
+        # We need to lock the rows we're interested in, to ensure that we don't end up with deadlocks
+        # when two processes try to update the same rows concurrently.
+        # See https://github.com/bfirsh/django-ordered-model/issues/184
+        qs = qs.select_for_update()
+        # Django does not generate the SELECT ... FOR UPDATE if the queryset is simply .updated(), we need
+        # to force the evaluation here, to get the lock
+        len(qs)
+
+        return qs
 
     def previous(self):
         """
@@ -253,15 +263,38 @@ def to(self, order, extra_update=None):
             # object is already at desired position
             return
         qs = self.get_ordering_queryset()
-        extra_update = {} if extra_update is None else extra_update
-        if getattr(self, order_field_name) > order:
-            qs.below_instance(self).above(order, inclusive=True).increase_order(
-                **extra_update
-            )
+
+        """
+        Get current value in database of our object. Necessary to prevent race conditions, where our model has been
+        moved in the database since being fetched - while it thinks it's in position 2 it's actually in position 3, 4,
+        etc. See https://github.com/bfirsh/django-ordered-model/issues/184
+        """
+        current_order_value_in_db = (
+            self.get_ordering_queryset()
+            .filter(pk=self.pk)
+            .values(self.order_field_name)
+        )
+
+        if getattr(self, self.order_field_name) > order:
+            update_kwargs = {self.order_field_name: F(self.order_field_name) + 1}
+            if extra_update:
+                update_kwargs.update(extra_update)
+            qs.filter(
+                **{
+                    self.order_field_name + "__lt": Subquery(current_order_value_in_db),
+                    self.order_field_name + "__gte": order,
+                }
+            ).update(**update_kwargs)
         else:
-            qs.above_instance(self).below(order, inclusive=True).decrease_order(
-                **extra_update
-            )
+            update_kwargs = {self.order_field_name: F(self.order_field_name) - 1}
+            if extra_update:
+                update_kwargs.update(extra_update)
+            qs.filter(
+                **{
+                    self.order_field_name + "__gt": Subquery(current_order_value_in_db),
+                    self.order_field_name + "__lte": order,
+                }
+            ).update(**update_kwargs)
         setattr(self, order_field_name, order)
         self.save()
 

diff --git a/tests/tests.py b/tests/tests.py
@@ -1081,3 +1081,33 @@ def test_delete_bypass(self):
         self.assertEquals(
             "changing order of tests.OpenQuestion (4) from 3 to 2\n", out.getvalue()
         )
+
+
+class TestFixInOrdering(TestCase):
+    def test_multiple_models_updated_simultaneously(self):
+        """
+        This is a test that tests the case where:
+        Request 1 and 2 happen simultaneously.
+        They both load a CustomItem into memory from database, and try to move them around.
+        We want to ensure consistency.
+        """
+        # Three items are created here
+        item0 = CustomItem.objects.create(pkid="0", name="0")
+        item1 = CustomItem.objects.create(pkid="1", name="1")
+        item2 = CustomItem.objects.create(pkid="2", name="2")
+
+        # Moving item 0 to position 2. This works fine, and moves item 1 to position 0
+        item0.to(2)
+
+        # Item 1 now is *actually* in position 0, but thinks it's in position 1. As we refresh
+        # the value from the database, this is not a problem
+        item1.to(2)
+
+        item0.refresh_from_db()
+        item1.refresh_from_db()
+        item2.refresh_from_db()
+
+        # All orders are correct now.
+        self.assertEqual(item2.order, 0)
+        self.assertEqual(item0.order, 1)
+        self.assertEqual(item1.order, 2)