Fix duplicate orders for user notification policies (#2278)
# What this PR does
Fixes an issue when multiple user notification policies have duplicated
order values, leading to the following unexpected behaviours:
1. Not possible to rearrange notification policies that have duplicated
orders.
2. The notification system only executes the first policy from each
order group. For example, if there are policies with orders `[0, 0, 0,
0]`, only the first policy will be executed, and all others will be
skipped. So the user will see four policies in the UI, while only one of
them will be actually executed.
This PR fixes the issue by adding a unique index on `(user_id,
important, order)` for `UserNotificationPolicy` model. However, it's not
possible to add that unique index using the ordering library that we use
due to it's implementation details.
I added a new abstract Django model `OrderedModel` that's able to work
with such unique indices + under concurrent load.
Important info on this new `OrderedModel` abstract model:
- Orders are unique on the DB level
- Orders are allowed to be non-consecutive, for example order sequence
`[100, 150, 400]` is valid
- When deleting an instance, orders of other instances don't change.
This is a notable difference from the library we use. I think it's
better to only delete the instance without changing any other orders,
because it reduces the number of dependencies between instances (e.g.
Terraform drift will be much smaller this way if a policy is deleted via
the web UI).
## Which issue(s) this PR fixes
Related to https://github.com/grafana/oncall-private/issues/1680
## Checklist
- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
2023-06-21 12:13:56 +01:00
|
|
|
import random
|
|
|
|
|
import threading
|
|
|
|
|
|
|
|
|
|
import pytest
|
|
|
|
|
from django.db import models
|
|
|
|
|
|
2023-07-18 18:17:53 +01:00
|
|
|
from common.ordered_model.ordered_model import OrderedModel
|
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 common.ordered_model.serializer import OrderedModelSerializer
|
Fix duplicate orders for user notification policies (#2278)
# What this PR does
Fixes an issue when multiple user notification policies have duplicated
order values, leading to the following unexpected behaviours:
1. Not possible to rearrange notification policies that have duplicated
orders.
2. The notification system only executes the first policy from each
order group. For example, if there are policies with orders `[0, 0, 0,
0]`, only the first policy will be executed, and all others will be
skipped. So the user will see four policies in the UI, while only one of
them will be actually executed.
This PR fixes the issue by adding a unique index on `(user_id,
important, order)` for `UserNotificationPolicy` model. However, it's not
possible to add that unique index using the ordering library that we use
due to it's implementation details.
I added a new abstract Django model `OrderedModel` that's able to work
with such unique indices + under concurrent load.
Important info on this new `OrderedModel` abstract model:
- Orders are unique on the DB level
- Orders are allowed to be non-consecutive, for example order sequence
`[100, 150, 400]` is valid
- When deleting an instance, orders of other instances don't change.
This is a notable difference from the library we use. I think it's
better to only delete the instance without changing any other orders,
because it reduces the number of dependencies between instances (e.g.
Terraform drift will be much smaller this way if a policy is deleted via
the web UI).
## Which issue(s) this PR fixes
Related to https://github.com/grafana/oncall-private/issues/1680
## Checklist
- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
2023-06-21 12:13:56 +01:00
|
|
|
|
|
|
|
|
|
|
|
|
|
class TestOrderedModel(OrderedModel):
|
2023-07-21 21:35:19 +02:00
|
|
|
__test__ = False
|
|
|
|
|
|
Fix duplicate orders for user notification policies (#2278)
# What this PR does
Fixes an issue when multiple user notification policies have duplicated
order values, leading to the following unexpected behaviours:
1. Not possible to rearrange notification policies that have duplicated
orders.
2. The notification system only executes the first policy from each
order group. For example, if there are policies with orders `[0, 0, 0,
0]`, only the first policy will be executed, and all others will be
skipped. So the user will see four policies in the UI, while only one of
them will be actually executed.
This PR fixes the issue by adding a unique index on `(user_id,
important, order)` for `UserNotificationPolicy` model. However, it's not
possible to add that unique index using the ordering library that we use
due to it's implementation details.
I added a new abstract Django model `OrderedModel` that's able to work
with such unique indices + under concurrent load.
Important info on this new `OrderedModel` abstract model:
- Orders are unique on the DB level
- Orders are allowed to be non-consecutive, for example order sequence
`[100, 150, 400]` is valid
- When deleting an instance, orders of other instances don't change.
This is a notable difference from the library we use. I think it's
better to only delete the instance without changing any other orders,
because it reduces the number of dependencies between instances (e.g.
Terraform drift will be much smaller this way if a policy is deleted via
the web UI).
## Which issue(s) this PR fixes
Related to https://github.com/grafana/oncall-private/issues/1680
## Checklist
- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
2023-06-21 12:13:56 +01:00
|
|
|
test_field = models.CharField(max_length=255)
|
|
|
|
|
extra_field = models.IntegerField(null=True, default=None)
|
|
|
|
|
order_with_respect_to = ["test_field"]
|
|
|
|
|
|
|
|
|
|
class Meta:
|
|
|
|
|
app_label = "base"
|
|
|
|
|
ordering = ["order"]
|
|
|
|
|
constraints = [
|
|
|
|
|
models.UniqueConstraint(fields=["test_field", "order"], name="unique_test_field_order"),
|
|
|
|
|
]
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def _get_ids():
|
|
|
|
|
return list(TestOrderedModel.objects.filter(test_field="test").values_list("id", flat=True))
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def _get_orders():
|
|
|
|
|
return list(TestOrderedModel.objects.filter(test_field="test").values_list("order", flat=True))
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def _orders_are_sequential():
|
|
|
|
|
orders = _get_orders()
|
|
|
|
|
return orders == list(range(len(orders)))
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@pytest.mark.django_db
|
|
|
|
|
def test_ordered_model_create():
|
|
|
|
|
first = TestOrderedModel.objects.create(test_field="test")
|
|
|
|
|
second = TestOrderedModel.objects.create(test_field="test")
|
|
|
|
|
|
|
|
|
|
assert first.order == 0
|
|
|
|
|
assert second.order == 1
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@pytest.mark.django_db
|
|
|
|
|
def test_ordered_model_delete():
|
|
|
|
|
instances = [TestOrderedModel.objects.create(test_field="test") for _ in range(3)]
|
|
|
|
|
|
|
|
|
|
instances[1].delete()
|
|
|
|
|
assert instances[1].pk is None
|
|
|
|
|
assert _get_ids() == [instances[0].id, instances[2].id]
|
|
|
|
|
assert _get_orders() == [0, 2]
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@pytest.mark.django_db
|
|
|
|
|
def test_ordered_model_to():
|
|
|
|
|
instances = [TestOrderedModel.objects.create(test_field="test") for _ in range(5)]
|
|
|
|
|
|
|
|
|
|
def _ids(indices):
|
|
|
|
|
return [instances[i].id for i in indices]
|
|
|
|
|
|
|
|
|
|
# move to the end
|
|
|
|
|
instances[0].to(4)
|
|
|
|
|
assert instances[0].order == 4
|
|
|
|
|
assert _get_ids() == _ids([1, 2, 3, 4, 0])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# move to the beginning
|
|
|
|
|
instances[0].to(0)
|
|
|
|
|
assert instances[0].order == 0
|
|
|
|
|
assert _get_ids() == _ids([0, 1, 2, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# move to the middle
|
|
|
|
|
instances[0].to(2)
|
|
|
|
|
assert instances[0].order == 2
|
|
|
|
|
assert _get_ids() == _ids([1, 2, 0, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# move from the middle to the end
|
|
|
|
|
instances[0].to(4)
|
|
|
|
|
assert instances[0].order == 4
|
|
|
|
|
assert _get_ids() == _ids([1, 2, 3, 4, 0])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# move from the end to the second position
|
|
|
|
|
instances[0].to(1)
|
|
|
|
|
assert instances[0].order == 1
|
|
|
|
|
assert _get_ids() == _ids([1, 0, 2, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# move from the second position to the beginning
|
|
|
|
|
instances[0].to(0)
|
|
|
|
|
assert instances[0].order == 0
|
|
|
|
|
assert _get_ids() == _ids([0, 1, 2, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# don't move if the order is the same
|
|
|
|
|
for instance in instances:
|
|
|
|
|
instance.to(instance.order)
|
|
|
|
|
assert instance.order == instance.order
|
|
|
|
|
assert _get_ids() == _ids([0, 1, 2, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@pytest.mark.django_db
|
|
|
|
|
def test_ordered_model_to_index():
|
|
|
|
|
instances = [TestOrderedModel.objects.create(test_field="test") for _ in range(5)]
|
|
|
|
|
|
|
|
|
|
def _ids(indices):
|
|
|
|
|
return [instances[i].id for i in indices]
|
|
|
|
|
|
|
|
|
|
# move to the end
|
|
|
|
|
instances[0].to_index(4)
|
|
|
|
|
assert instances[0].order == 4
|
|
|
|
|
assert _get_ids() == _ids([1, 2, 3, 4, 0])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# move to the beginning
|
|
|
|
|
instances[0].to_index(0)
|
|
|
|
|
assert instances[0].order == 0
|
|
|
|
|
assert _get_ids() == _ids([0, 1, 2, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# move to the middle
|
|
|
|
|
instances[0].to_index(2)
|
|
|
|
|
assert instances[0].order == 2
|
|
|
|
|
assert _get_ids() == _ids([1, 2, 0, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# move from the middle to the end
|
|
|
|
|
instances[0].to_index(4)
|
|
|
|
|
assert instances[0].order == 4
|
|
|
|
|
assert _get_ids() == _ids([1, 2, 3, 4, 0])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# move from the end to the second position
|
|
|
|
|
instances[0].to_index(1)
|
|
|
|
|
assert instances[0].order == 1
|
|
|
|
|
assert _get_ids() == _ids([1, 0, 2, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# move from the second position to the beginning
|
|
|
|
|
instances[0].to_index(0)
|
|
|
|
|
assert instances[0].order == 0
|
|
|
|
|
assert _get_ids() == _ids([0, 1, 2, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# don't move if the order is the same
|
|
|
|
|
for instance in instances:
|
|
|
|
|
instance.to_index(instance.order)
|
|
|
|
|
assert instance.order == instance.order
|
|
|
|
|
assert _get_ids() == _ids([0, 1, 2, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@pytest.mark.django_db
|
|
|
|
|
def test_ordered_model_swap():
|
|
|
|
|
instances = [TestOrderedModel.objects.create(test_field="test") for _ in range(5)]
|
|
|
|
|
|
|
|
|
|
def _ids(indices):
|
|
|
|
|
return [instances[i].id for i in indices]
|
|
|
|
|
|
|
|
|
|
# swap with last
|
|
|
|
|
instances[0].swap(4)
|
|
|
|
|
assert instances[0].order == 4
|
|
|
|
|
assert _get_ids() == _ids([4, 1, 2, 3, 0])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# swap with first
|
|
|
|
|
instances[0].swap(0)
|
|
|
|
|
assert instances[0].order == 0
|
|
|
|
|
assert _get_ids() == _ids([0, 1, 2, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# swap with middle
|
|
|
|
|
instances[0].swap(2)
|
|
|
|
|
assert instances[0].order == 2
|
|
|
|
|
assert _get_ids() == _ids([2, 1, 0, 3, 4])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# swap from the middle to the end
|
|
|
|
|
instances[0].swap(4)
|
|
|
|
|
assert instances[0].order == 4
|
|
|
|
|
assert _get_ids() == _ids([2, 1, 4, 3, 0])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# swap from the end to the second position
|
|
|
|
|
instances[0].swap(1)
|
|
|
|
|
assert instances[0].order == 1
|
|
|
|
|
assert _get_ids() == _ids([2, 0, 4, 3, 1])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# swap from the second position to the beginning
|
|
|
|
|
instances[0].swap(0)
|
|
|
|
|
assert instances[0].order == 0
|
|
|
|
|
assert _get_ids() == _ids([0, 2, 4, 3, 1])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# swap with itself
|
|
|
|
|
for instance in instances:
|
|
|
|
|
instance.refresh_from_db(fields=["order"])
|
|
|
|
|
instance.swap(instance.order)
|
|
|
|
|
assert instance.order == instance.order
|
|
|
|
|
assert _get_ids() == _ids([0, 2, 4, 3, 1])
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@pytest.mark.django_db
|
|
|
|
|
def test_order_with_respect_to_isolation():
|
|
|
|
|
instances = [TestOrderedModel.objects.create(test_field="test") for _ in range(5)]
|
|
|
|
|
other_instances = [TestOrderedModel.objects.create(test_field="test1") for _ in range(5)]
|
|
|
|
|
|
|
|
|
|
assert [i.order for i in instances] == [0, 1, 2, 3, 4]
|
|
|
|
|
assert [i.order for i in other_instances] == [0, 1, 2, 3, 4]
|
|
|
|
|
|
|
|
|
|
assert instances[-1].next() is None
|
|
|
|
|
assert instances[-1].max_order() == 4
|
|
|
|
|
|
|
|
|
|
instances[0].to(8)
|
|
|
|
|
instances[1].swap(7)
|
|
|
|
|
|
|
|
|
|
for idx, instance in enumerate(other_instances):
|
|
|
|
|
instance.refresh_from_db()
|
|
|
|
|
assert instance.order == idx
|
|
|
|
|
|
|
|
|
|
with pytest.raises(IndexError):
|
|
|
|
|
instances[0].to_index(6)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
# Tests below are for checking that concurrent operations are performed correctly.
|
|
|
|
|
# They are skipped by default because they might take a lot of time to run.
|
|
|
|
|
# It could be useful to run them manually when making changes to the code, making sure
|
|
|
|
|
# that the changes don't break concurrent operations. To run the tests, set SKIP_CONCURRENT to False.
|
|
|
|
|
SKIP_CONCURRENT = True
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@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_create_concurrent():
|
|
|
|
|
LOOPS = 30
|
|
|
|
|
THREADS = 10
|
|
|
|
|
exceptions = []
|
|
|
|
|
|
|
|
|
|
def create():
|
2023-11-29 12:04:48 -03:00
|
|
|
for _ in range(LOOPS):
|
Fix duplicate orders for user notification policies (#2278)
# What this PR does
Fixes an issue when multiple user notification policies have duplicated
order values, leading to the following unexpected behaviours:
1. Not possible to rearrange notification policies that have duplicated
orders.
2. The notification system only executes the first policy from each
order group. For example, if there are policies with orders `[0, 0, 0,
0]`, only the first policy will be executed, and all others will be
skipped. So the user will see four policies in the UI, while only one of
them will be actually executed.
This PR fixes the issue by adding a unique index on `(user_id,
important, order)` for `UserNotificationPolicy` model. However, it's not
possible to add that unique index using the ordering library that we use
due to it's implementation details.
I added a new abstract Django model `OrderedModel` that's able to work
with such unique indices + under concurrent load.
Important info on this new `OrderedModel` abstract model:
- Orders are unique on the DB level
- Orders are allowed to be non-consecutive, for example order sequence
`[100, 150, 400]` is valid
- When deleting an instance, orders of other instances don't change.
This is a notable difference from the library we use. I think it's
better to only delete the instance without changing any other orders,
because it reduces the number of dependencies between instances (e.g.
Terraform drift will be much smaller this way if a policy is deleted via
the web UI).
## Which issue(s) this PR fixes
Related to https://github.com/grafana/oncall-private/issues/1680
## Checklist
- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
2023-06-21 12:13:56 +01:00
|
|
|
try:
|
|
|
|
|
TestOrderedModel.objects.create(test_field="test")
|
|
|
|
|
except Exception as e:
|
|
|
|
|
exceptions.append(e)
|
|
|
|
|
|
|
|
|
|
threads = [threading.Thread(target=create) for _ in range(THREADS)]
|
|
|
|
|
for thread in threads:
|
|
|
|
|
thread.start()
|
|
|
|
|
for thread in threads:
|
|
|
|
|
thread.join()
|
|
|
|
|
|
|
|
|
|
assert not exceptions
|
|
|
|
|
assert TestOrderedModel.objects.count() == LOOPS * THREADS
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@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_to_concurrent():
|
|
|
|
|
THREADS = 300
|
|
|
|
|
exceptions = []
|
|
|
|
|
|
|
|
|
|
TestOrderedModel.objects.all().delete() # clear table
|
|
|
|
|
instances = [TestOrderedModel.objects.create(test_field="test") for _ in range(THREADS)]
|
|
|
|
|
|
|
|
|
|
random.seed(42)
|
|
|
|
|
positions = [random.randint(0, THREADS - 1) for _ in range(THREADS)]
|
|
|
|
|
|
|
|
|
|
def to(idx):
|
|
|
|
|
try:
|
|
|
|
|
instance = instances[idx]
|
|
|
|
|
instance.to(positions[idx]) # swap with next
|
|
|
|
|
except Exception as e:
|
|
|
|
|
exceptions.append(e)
|
|
|
|
|
|
|
|
|
|
threads = [threading.Thread(target=to, args=(idx,)) for idx in range(THREADS - 1)]
|
|
|
|
|
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()
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@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_concurrent():
|
|
|
|
|
THREADS = 300
|
|
|
|
|
exceptions = []
|
|
|
|
|
|
|
|
|
|
TestOrderedModel.objects.all().delete() # clear table
|
|
|
|
|
instances = [TestOrderedModel.objects.create(test_field="test") for _ in range(THREADS)]
|
|
|
|
|
|
|
|
|
|
# generate random unique orders
|
|
|
|
|
random.seed(42)
|
|
|
|
|
unique_orders = list(range(THREADS))
|
|
|
|
|
random.shuffle(unique_orders)
|
|
|
|
|
|
|
|
|
|
def swap(idx):
|
|
|
|
|
try:
|
|
|
|
|
instance = instances[idx]
|
|
|
|
|
instance.swap(unique_orders[idx])
|
|
|
|
|
except Exception as e:
|
|
|
|
|
exceptions.append(e)
|
|
|
|
|
|
|
|
|
|
threads = [threading.Thread(target=swap, args=(idx,)) for idx in range(THREADS)]
|
|
|
|
|
for thread in threads:
|
|
|
|
|
thread.start()
|
|
|
|
|
for thread in threads:
|
|
|
|
|
thread.join()
|
|
|
|
|
|
|
|
|
|
assert not exceptions
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
|
|
|
|
|
# in case of unique orders, the final order is deterministic
|
|
|
|
|
assert list(TestOrderedModel.objects.order_by("id").values_list("order", flat=True)) == unique_orders
|
|
|
|
|
|
|
|
|
|
|
Update OrderedModel.swap to retry on IntegrityError (#3940)
Related to https://github.com/grafana/oncall-private/issues/2386.
Issue seems related to [multiple concurrent Terraform-triggered
updates](https://ops.grafana-ops.net/explore?schemaVersion=1&panes=%7B%22j0s%22:%7B%22datasource%22:%22000000193%22,%22queries%22:%5B%7B%22refId%22:%22B%22,%22expr%22:%22%7Bcluster%3D%5C%22prod-eu-west-0%5C%22,%20namespace%3D%5C%22amixr-prod%5C%22%7D%20%7C%3D%20%5C%22logger%3Dinsight_logger%20tenant_id%3D735393%5C%22%20%7C%3D%20%5C%22resource_type%3Droute%5C%22%22,%22queryType%22:%22range%22,%22datasource%22:%7B%22type%22:%22loki%22,%22uid%22:%22000000193%22%7D,%22editorMode%22:%22code%22%7D%5D,%22range%22:%7B%22from%22:%221707813893526%22,%22to%22:%221707815856774%22%7D%7D%7D&orgId=1
) changing the route order to zero (and eventually succeeding)
Sample test run with the failing error:
```
========================================== short test summary info ===========================================
FAILED common/tests/test_ordered_model.py::test_ordered_model_swap_all_to_zero - assert not [DoesNotExist(), IntegrityError(1062, "Duplicate entry 'test-0' for key 'base_testorderedmodel...
======================================= 1 failed, 5 warnings in 34.39s =======================================
make: *** [Makefile:283: run-backend-test] Error 1
```
2024-02-22 17:51:05 -03:00
|
|
|
@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():
|
|
|
|
|
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 swap(idx):
|
|
|
|
|
try:
|
|
|
|
|
instance = instances[idx]
|
|
|
|
|
instance.swap(positions[idx])
|
|
|
|
|
except Exception as e:
|
|
|
|
|
exceptions.append(e)
|
|
|
|
|
|
|
|
|
|
threads = [threading.Thread(target=swap, 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()
|
|
|
|
|
|
|
|
|
|
|
Fix duplicate orders for user notification policies (#2278)
# What this PR does
Fixes an issue when multiple user notification policies have duplicated
order values, leading to the following unexpected behaviours:
1. Not possible to rearrange notification policies that have duplicated
orders.
2. The notification system only executes the first policy from each
order group. For example, if there are policies with orders `[0, 0, 0,
0]`, only the first policy will be executed, and all others will be
skipped. So the user will see four policies in the UI, while only one of
them will be actually executed.
This PR fixes the issue by adding a unique index on `(user_id,
important, order)` for `UserNotificationPolicy` model. However, it's not
possible to add that unique index using the ordering library that we use
due to it's implementation details.
I added a new abstract Django model `OrderedModel` that's able to work
with such unique indices + under concurrent load.
Important info on this new `OrderedModel` abstract model:
- Orders are unique on the DB level
- Orders are allowed to be non-consecutive, for example order sequence
`[100, 150, 400]` is valid
- When deleting an instance, orders of other instances don't change.
This is a notable difference from the library we use. I think it's
better to only delete the instance without changing any other orders,
because it reduces the number of dependencies between instances (e.g.
Terraform drift will be much smaller this way if a policy is deleted via
the web UI).
## Which issue(s) this PR fixes
Related to https://github.com/grafana/oncall-private/issues/1680
## Checklist
- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
2023-06-21 12:13:56 +01:00
|
|
|
@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_non_unique_orders_concurrent():
|
|
|
|
|
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 swap(idx):
|
|
|
|
|
try:
|
|
|
|
|
instance = instances[idx]
|
|
|
|
|
instance.swap(positions[idx])
|
|
|
|
|
except Exception as e:
|
|
|
|
|
exceptions.append(e)
|
|
|
|
|
|
|
|
|
|
threads = [threading.Thread(target=swap, args=(idx,)) for idx 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()
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
@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_create_swap_and_delete_concurrent():
|
|
|
|
|
"""Check that create+swap, swap and delete operations are performed correctly when run concurrently."""
|
|
|
|
|
|
|
|
|
|
THREADS = 100
|
|
|
|
|
exceptions = []
|
|
|
|
|
|
|
|
|
|
instances = [TestOrderedModel.objects.create(test_field="test", extra_field=idx) for idx in range(THREADS * 3)]
|
|
|
|
|
|
|
|
|
|
def create_swap(idx):
|
|
|
|
|
try:
|
|
|
|
|
instance = TestOrderedModel.objects.create(test_field="test", extra_field=idx + 1000)
|
|
|
|
|
instance.swap(idx)
|
|
|
|
|
except Exception as e:
|
|
|
|
|
exceptions.append(("create_swap", e))
|
|
|
|
|
|
|
|
|
|
def swap(idx):
|
|
|
|
|
try:
|
|
|
|
|
instances[idx].swap(idx + 1)
|
|
|
|
|
except Exception as e:
|
|
|
|
|
exceptions.append(("swap", e))
|
|
|
|
|
|
|
|
|
|
def delete(idx):
|
|
|
|
|
try:
|
|
|
|
|
instances[idx].delete()
|
|
|
|
|
except Exception as e:
|
|
|
|
|
exceptions.append(("delete", e))
|
|
|
|
|
|
|
|
|
|
threads = [threading.Thread(target=create_swap, args=(idx,)) for idx in list(range(THREADS))]
|
|
|
|
|
threads += [threading.Thread(target=delete, args=(idx,)) for idx in range(THREADS)]
|
|
|
|
|
threads += [threading.Thread(target=swap, args=(idx,)) for idx in range(THREADS, THREADS * 2 - 1)]
|
|
|
|
|
|
|
|
|
|
for thread in threads:
|
|
|
|
|
thread.start()
|
|
|
|
|
for thread in threads:
|
|
|
|
|
thread.join()
|
|
|
|
|
|
|
|
|
|
expected_extra_field_values = list(range(1000, 1000 + THREADS))
|
|
|
|
|
expected_extra_field_values += [THREADS * 2 - 1] + list(range(THREADS, THREADS * 2 - 1))
|
|
|
|
|
expected_extra_field_values += [instance.extra_field for instance in instances[THREADS * 2 : THREADS * 3]]
|
|
|
|
|
|
|
|
|
|
assert not exceptions
|
|
|
|
|
assert _orders_are_sequential()
|
|
|
|
|
assert list(TestOrderedModel.objects.values_list("extra_field", flat=True)) == expected_extra_field_values
|
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
|
|
|
|
|
|
|
|
|
2024-08-13 17:51:18 -03:00
|
|
|
class OrderedModelSerializerForTests(OrderedModelSerializer):
|
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
|
|
|
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]
|
2024-08-13 17:51:18 -03:00
|
|
|
serializer = OrderedModelSerializerForTests(instance, data={"order": 0, "extra_field": idx}, partial=True)
|
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
|
|
|
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()
|