2023-07-18 18:17:53 +01:00
|
|
|
|
from rest_framework import serializers
|
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
```
2024-07-19 08:56:26 -03:00
|
|
|
|
from rest_framework.utils import model_meta
|
2023-07-18 18:17:53 +01:00
|
|
|
|
|
|
|
|
|
|
from common.api_helpers.exceptions import BadRequest
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
class OrderedModelSerializer(serializers.ModelSerializer):
|
|
|
|
|
|
"""Ordered model serializer to be used in public API."""
|
|
|
|
|
|
|
|
|
|
|
|
position = serializers.IntegerField(required=False, source="order")
|
|
|
|
|
|
# manual_order=True is intended for use by Terraform provider only, and is not a documented feature.
|
|
|
|
|
|
manual_order = serializers.BooleanField(default=False, write_only=True)
|
|
|
|
|
|
|
|
|
|
|
|
class Meta:
|
|
|
|
|
|
fields = ["position", "manual_order"]
|
|
|
|
|
|
|
|
|
|
|
|
def create(self, validated_data):
|
|
|
|
|
|
# Remove "manual_order" and "order" fields from validated_data, so they are not passed to create method.
|
2023-07-18 20:29:04 +01:00
|
|
|
|
manual_order = validated_data.pop("manual_order", False)
|
2023-07-18 18:17:53 +01:00
|
|
|
|
order = validated_data.pop("order", None)
|
|
|
|
|
|
|
|
|
|
|
|
# Create the instance.
|
|
|
|
|
|
# Instances are always created at the end of the list, and then moved to the desired position by _adjust_order.
|
|
|
|
|
|
instance = super().create(validated_data)
|
|
|
|
|
|
|
|
|
|
|
|
# Adjust order of the instance if necessary.
|
|
|
|
|
|
if order is not None:
|
|
|
|
|
|
self._adjust_order(instance, manual_order, order, created=True)
|
|
|
|
|
|
|
|
|
|
|
|
return instance
|
|
|
|
|
|
|
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
```
2024-07-19 08:56:26 -03:00
|
|
|
|
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
|
|
|
|
|
|
|
2023-07-18 18:17:53 +01:00
|
|
|
|
def update(self, instance, validated_data):
|
|
|
|
|
|
# Remove "manual_order" and "order" fields from validated_data, so they are not passed to update method.
|
2023-07-18 20:29:04 +01:00
|
|
|
|
manual_order = validated_data.pop("manual_order", False)
|
2023-07-18 18:17:53 +01:00
|
|
|
|
order = validated_data.pop("order", None)
|
|
|
|
|
|
|
|
|
|
|
|
# Adjust order of the instance if necessary.
|
|
|
|
|
|
if order is not None:
|
|
|
|
|
|
self._adjust_order(instance, manual_order, order, created=False)
|
|
|
|
|
|
|
|
|
|
|
|
# Proceed with the update.
|
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
```
2024-07-19 08:56:26 -03:00
|
|
|
|
return self._update(instance, validated_data)
|
2023-07-18 18:17:53 +01:00
|
|
|
|
|
|
|
|
|
|
@staticmethod
|
|
|
|
|
|
def _adjust_order(instance, manual_order, order, created):
|
|
|
|
|
|
# Passing order=-1 means that the policy should be moved to the end of the list.
|
|
|
|
|
|
# Works only for public API but not for Terraform provider.
|
|
|
|
|
|
if order == -1 and not manual_order:
|
|
|
|
|
|
if created:
|
|
|
|
|
|
# The policy was just created, so it is already at the end of the list.
|
|
|
|
|
|
return
|
|
|
|
|
|
|
|
|
|
|
|
order = instance.max_order()
|
|
|
|
|
|
# max_order() can't be None here because at least one instance exists – the one we are moving.
|
|
|
|
|
|
assert order is not None
|
|
|
|
|
|
|
|
|
|
|
|
# Check the order is in the valid range.
|
|
|
|
|
|
# https://docs.djangoproject.com/en/4.1/ref/models/fields/#positiveintegerfield
|
|
|
|
|
|
if order < 0 or order > 2147483647:
|
|
|
|
|
|
raise BadRequest(detail="Invalid value for position field")
|
|
|
|
|
|
|
|
|
|
|
|
# Orders are swapped instead of moved when using Terraform, because Terraform may issue concurrent requests
|
|
|
|
|
|
# to create / update / delete multiple policies. "Move to" operation is not deterministic in this case, and
|
|
|
|
|
|
# final order of policies may be different depending on the order in which requests are processed. On the other
|
|
|
|
|
|
# hand, the result of concurrent "swap" operations is deterministic and does not depend on the order in
|
|
|
|
|
|
# which requests are processed.
|
|
|
|
|
|
if manual_order:
|
|
|
|
|
|
instance.swap(order)
|
|
|
|
|
|
else:
|
|
|
|
|
|
instance.to(order)
|