From ab700fd4a18e2acc5157fc2fb889c12909aed6c6 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Fri, 19 Jul 2024 08:56:26 -0300 Subject: [PATCH] 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 = [, , , , , ...] positions = [57, 12, 140, 125, 114, 71, ...] thread = threads = [, , , ...] update_order_to_zero = .update_order_to_zero at 0x7f02086274c0> common/tests/test_ordered_model.py:481: AssertionError ``` --- engine/common/ordered_model/serializer.py | 35 ++++++++++++++++++- engine/common/tests/test_ordered_model.py | 42 +++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/engine/common/ordered_model/serializer.py b/engine/common/ordered_model/serializer.py index d5c36ed162..b8aeaa3872 100644 --- a/engine/common/ordered_model/serializer.py +++ b/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): diff --git a/engine/common/tests/test_ordered_model.py b/engine/common/tests/test_ordered_model.py index b9efd98fea..77a58051b1 100644 --- a/engine/common/tests/test_ordered_model.py +++ b/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()