Update OrderedModel serializer not to save previously updated order (#…

…4706)

We are noticing some
[issues](https://ops.grafana-ops.net/goto/sQFLv4XSg?orgId=1) when
updating routes via Terraform because when receiving multiple concurrent
requests updating position, the order is updated in a transaction but it
is then tried to save again in the `.save` call, which could lead to
`IntegrityError`s, because the order may have been updated in a
concurrent request meanwhile.

Test run with the reproduced error (before the serializer update):

```
_____________________________ test_ordered_model_swap_all_to_zero_via_serializer _____________________________

    @pytest.mark.django_db(transaction=True)
    def test_ordered_model_swap_all_to_zero_via_serializer():
        THREADS = 300
        exceptions = []
    
        TestOrderedModel.objects.all().delete()  # clear table
        instances = [TestOrderedModel.objects.create(test_field="test") for _ in range(THREADS)]
    
        # generate random non-unique orders
        random.seed(42)
        positions = [random.randint(0, THREADS - 1) for _ in range(THREADS)]
    
        def update_order_to_zero(idx):
            try:
                instance = instances[idx]
                serializer = TestOrderedModelSerializer(
                    instance, data={"order": 0, "extra_field": idx}, partial=True
                )
                serializer.is_valid(raise_exception=True)
                serializer.save()
                instance.swap(positions[idx])
            except Exception as e:
                exceptions.append(e)
    
        threads = [threading.Thread(target=update_order_to_zero, args=(0,)) for _ in range(THREADS)]
        for thread in threads:
            thread.start()
        for thread in threads:
            thread.join()
    
        # can only check that orders are still sequential and that there are no exceptions
        # can't check the exact order because it changes depending on the order of execution
>       assert not exceptions
E       assert not [IntegrityError(1062, "Duplicate entry 'test-0' for key 'base_testorderedmodel.unique_test_field_order'"), IntegrityEr...rder'"), IntegrityError(1062, "Duplicate entry 'test-0' for key 'base_testorderedmodel.unique_test_field_order'"), ...]

THREADS    = 300
exceptions = [IntegrityError(1062, "Duplicate entry 'test-0' for key 'base_testorderedmodel.unique_test_field_order'"), IntegrityEr...rder'"), IntegrityError(1062, "Duplicate entry 'test-0' for key 'base_testorderedmodel.unique_test_field_order'"), ...]
instances  = [<TestOrderedModel: TestOrderedModel object (841)>, <TestOrderedModel: TestOrderedModel object (842)>, <TestOrderedMod...ject (844)>, <TestOrderedModel: TestOrderedModel object (845)>, <TestOrderedModel: TestOrderedModel object (846)>, ...]
positions  = [57, 12, 140, 125, 114, 71, ...]
thread     = <Thread(Thread-302 (update_order_to_zero), stopped 139636013323840)>
threads    = [<Thread(Thread-3 (update_order_to_zero), stopped 139646722418240)>, <Thread(Thread-4 (update_order_to_zero), stopped ...ate_order_to_zero), stopped 139646608590400)>, <Thread(Thread-8 (update_order_to_zero), stopped 139646600197696)>, ...]
update_order_to_zero = <function test_ordered_model_swap_all_to_zero_via_serializer.<locals>.update_order_to_zero at 0x7f02086274c0>

common/tests/test_ordered_model.py:481: AssertionError
```

engine/common/ordered_model/serializer.py

@@ -1,4 +1,5 @@
 from rest_framework import serializers
+from rest_framework.utils import model_meta
 
 from common.api_helpers.exceptions import BadRequest
 
@@ -28,6 +29,38 @@ def create(self, validated_data):
 
         return instance
 
+    def _update(self, instance, validated_data):
+        # customize the update method to make sure order field is not saved again
+        # (which could trigger an integrity error if there was another concurrent update changing order)
+        serializers.raise_errors_on_nested_writes("update", self, validated_data)
+        info = model_meta.get_field_info(instance)
+
+        # Simply set each attribute on the instance, and then save it.
+        # Note that unlike `.create()` we don't need to treat many-to-many
+        # relationships as being a special case. During updates we already
+        # have an instance pk for the relationships to be associated with.
+        m2m_fields = []
+        update_fields = []
+        for attr, value in validated_data.items():
+            if attr in info.relations and info.relations[attr].to_many:
+                m2m_fields.append((attr, value))
+            else:
+                setattr(instance, attr, value)
+                update_fields.append(attr)
+
+        # NOTE: this is the only difference, update changed fields to avoid saving order field again
+        if update_fields:
+            instance.save(update_fields=update_fields)
+
+        # Note that many-to-many fields are set after updating instance.
+        # Setting m2m fields triggers signals which could potentially change
+        # updated instance and we do not want it to collide with .update()
+        for attr, value in m2m_fields:
+            field = getattr(instance, attr)
+            field.set(value)
+
+        return instance
+
     def update(self, instance, validated_data):
         # Remove "manual_order" and "order" fields from validated_data, so they are not passed to update method.
         manual_order = validated_data.pop("manual_order", False)
@@ -38,7 +71,7 @@ def update(self, instance, validated_data):
             self._adjust_order(instance, manual_order, order, created=False)
 
         # Proceed with the update.
-        return super().update(instance, validated_data)
+        return self._update(instance, validated_data)
 
     @staticmethod
     def _adjust_order(instance, manual_order, order, created):

engine/common/tests/test_ordered_model.py

@@ -5,6 +5,7 @@
 from django.db import models
 
 from common.ordered_model.ordered_model import OrderedModel
+from common.ordered_model.serializer import OrderedModelSerializer
 
 
 class TestOrderedModel(OrderedModel):
@@ -436,3 +437,44 @@ def delete(idx):
     assert not exceptions
     assert _orders_are_sequential()
     assert list(TestOrderedModel.objects.values_list("extra_field", flat=True)) == expected_extra_field_values
+
+
+class TestOrderedModelSerializer(OrderedModelSerializer):
+    class Meta:
+        model = TestOrderedModel
+        fields = OrderedModelSerializer.Meta.fields + ["test_field", "extra_field"]
+
+
+@pytest.mark.skipif(SKIP_CONCURRENT, reason="OrderedModel concurrent tests are skipped to speed up tests")
+@pytest.mark.django_db(transaction=True)
+def test_ordered_model_swap_all_to_zero_via_serializer():
+    THREADS = 300
+    exceptions = []
+
+    TestOrderedModel.objects.all().delete()  # clear table
+    instances = [TestOrderedModel.objects.create(test_field="test") for _ in range(THREADS)]
+
+    # generate random non-unique orders
+    random.seed(42)
+    positions = [random.randint(0, THREADS - 1) for _ in range(THREADS)]
+
+    def update_order_to_zero(idx):
+        try:
+            instance = instances[idx]
+            serializer = TestOrderedModelSerializer(instance, data={"order": 0, "extra_field": idx}, partial=True)
+            serializer.is_valid(raise_exception=True)
+            serializer.save()
+            instance.swap(positions[idx])
+        except Exception as e:
+            exceptions.append(e)
+
+    threads = [threading.Thread(target=update_order_to_zero, args=(0,)) for _ in range(THREADS)]
+    for thread in threads:
+        thread.start()
+    for thread in threads:
+        thread.join()
+
+    # can only check that orders are still sequential and that there are no exceptions
+    # can't check the exact order because it changes depending on the order of execution
+    assert not exceptions
+    assert _orders_are_sequential()