🐛(backend) race condition create doc
When 2 docs are created almost at the same time,
the second one will fail because the first one.
We get a unicity error on the path key already
used ("impress_document_path_key").
To fix this issue, we will lock the table the
time to create the document, the next query will
wait for the lock to be released.
This commit is contained in:
@@ -145,6 +145,10 @@ and this project adheres to
|
|||||||
- 🐛(email) invitation emails in receivers language
|
- 🐛(email) invitation emails in receivers language
|
||||||
|
|
||||||
|
|
||||||
|
## Fixed
|
||||||
|
|
||||||
|
- 🐛(backend) race condition create doc #633
|
||||||
|
|
||||||
## [2.2.0] - 2025-02-10
|
## [2.2.0] - 2025-02-10
|
||||||
|
|
||||||
## Added
|
## Added
|
||||||
|
|||||||
@@ -11,8 +11,8 @@ from django.contrib.postgres.fields import ArrayField
|
|||||||
from django.contrib.postgres.search import TrigramSimilarity
|
from django.contrib.postgres.search import TrigramSimilarity
|
||||||
from django.core.exceptions import ValidationError
|
from django.core.exceptions import ValidationError
|
||||||
from django.core.files.storage import default_storage
|
from django.core.files.storage import default_storage
|
||||||
|
from django.db import connection, transaction
|
||||||
from django.db import models as db
|
from django.db import models as db
|
||||||
from django.db import transaction
|
|
||||||
from django.db.models.expressions import RawSQL
|
from django.db.models.expressions import RawSQL
|
||||||
from django.db.models.functions import Left, Length
|
from django.db.models.functions import Left, Length
|
||||||
from django.http import Http404, StreamingHttpResponse
|
from django.http import Http404, StreamingHttpResponse
|
||||||
@@ -607,6 +607,14 @@ class DocumentViewSet(
|
|||||||
@transaction.atomic
|
@transaction.atomic
|
||||||
def perform_create(self, serializer):
|
def perform_create(self, serializer):
|
||||||
"""Set the current user as creator and owner of the newly created object."""
|
"""Set the current user as creator and owner of the newly created object."""
|
||||||
|
|
||||||
|
# locks the table to ensure safe concurrent access
|
||||||
|
with connection.cursor() as cursor:
|
||||||
|
cursor.execute(
|
||||||
|
f'LOCK TABLE "{models.Document._meta.db_table}" ' # noqa: SLF001
|
||||||
|
"IN SHARE ROW EXCLUSIVE MODE;"
|
||||||
|
)
|
||||||
|
|
||||||
obj = models.Document.add_root(
|
obj = models.Document.add_root(
|
||||||
creator=self.request.user,
|
creator=self.request.user,
|
||||||
**serializer.validated_data,
|
**serializer.validated_data,
|
||||||
@@ -666,10 +674,19 @@ class DocumentViewSet(
|
|||||||
permission_classes=[],
|
permission_classes=[],
|
||||||
url_path="create-for-owner",
|
url_path="create-for-owner",
|
||||||
)
|
)
|
||||||
|
@transaction.atomic
|
||||||
def create_for_owner(self, request):
|
def create_for_owner(self, request):
|
||||||
"""
|
"""
|
||||||
Create a document on behalf of a specified owner (pre-existing user or invited).
|
Create a document on behalf of a specified owner (pre-existing user or invited).
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
# locks the table to ensure safe concurrent access
|
||||||
|
with connection.cursor() as cursor:
|
||||||
|
cursor.execute(
|
||||||
|
f'LOCK TABLE "{models.Document._meta.db_table}" ' # noqa: SLF001
|
||||||
|
"IN SHARE ROW EXCLUSIVE MODE;"
|
||||||
|
)
|
||||||
|
|
||||||
# Deserialize and validate the data
|
# Deserialize and validate the data
|
||||||
serializer = serializers.ServerCreateDocumentSerializer(data=request.data)
|
serializer = serializers.ServerCreateDocumentSerializer(data=request.data)
|
||||||
if not serializer.is_valid():
|
if not serializer.is_valid():
|
||||||
@@ -775,7 +792,12 @@ class DocumentViewSet(
|
|||||||
serializer.is_valid(raise_exception=True)
|
serializer.is_valid(raise_exception=True)
|
||||||
|
|
||||||
with transaction.atomic():
|
with transaction.atomic():
|
||||||
child_document = document.add_child(
|
# "select_for_update" locks the table to ensure safe concurrent access
|
||||||
|
locked_parent = models.Document.objects.select_for_update().get(
|
||||||
|
pk=document.pk
|
||||||
|
)
|
||||||
|
|
||||||
|
child_document = locked_parent.add_child(
|
||||||
creator=request.user,
|
creator=request.user,
|
||||||
**serializer.validated_data,
|
**serializer.validated_data,
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -2,6 +2,7 @@
|
|||||||
Tests for Documents API endpoint in impress's core app: children create
|
Tests for Documents API endpoint in impress's core app: children create
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
from concurrent.futures import ThreadPoolExecutor
|
||||||
from uuid import uuid4
|
from uuid import uuid4
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
@@ -249,3 +250,41 @@ def test_api_documents_children_create_force_id_existing():
|
|||||||
assert response.json() == {
|
assert response.json() == {
|
||||||
"id": ["A document with this ID already exists. You cannot override it."]
|
"id": ["A document with this ID already exists. You cannot override it."]
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db(transaction=True)
|
||||||
|
def test_api_documents_create_document_children_race_condition():
|
||||||
|
"""
|
||||||
|
It should be possible to create several documents at the same time
|
||||||
|
without causing any race conditions or data integrity issues.
|
||||||
|
"""
|
||||||
|
|
||||||
|
user = factories.UserFactory()
|
||||||
|
|
||||||
|
client = APIClient()
|
||||||
|
client.force_login(user)
|
||||||
|
|
||||||
|
document = factories.DocumentFactory()
|
||||||
|
|
||||||
|
factories.UserDocumentAccessFactory(user=user, document=document, role="owner")
|
||||||
|
|
||||||
|
def create_document():
|
||||||
|
return client.post(
|
||||||
|
f"/api/v1.0/documents/{document.id}/children/",
|
||||||
|
{
|
||||||
|
"title": "my child",
|
||||||
|
},
|
||||||
|
)
|
||||||
|
|
||||||
|
with ThreadPoolExecutor(max_workers=2) as executor:
|
||||||
|
future1 = executor.submit(create_document)
|
||||||
|
future2 = executor.submit(create_document)
|
||||||
|
|
||||||
|
response1 = future1.result()
|
||||||
|
response2 = future2.result()
|
||||||
|
|
||||||
|
assert response1.status_code == 201
|
||||||
|
assert response2.status_code == 201
|
||||||
|
|
||||||
|
document.refresh_from_db()
|
||||||
|
assert document.numchild == 2
|
||||||
|
|||||||
@@ -2,6 +2,7 @@
|
|||||||
Tests for Documents API endpoint in impress's core app: create
|
Tests for Documents API endpoint in impress's core app: create
|
||||||
"""
|
"""
|
||||||
|
|
||||||
|
from concurrent.futures import ThreadPoolExecutor
|
||||||
from uuid import uuid4
|
from uuid import uuid4
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
@@ -51,6 +52,36 @@ def test_api_documents_create_authenticated_success():
|
|||||||
assert document.accesses.filter(role="owner", user=user).exists()
|
assert document.accesses.filter(role="owner", user=user).exists()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db(transaction=True)
|
||||||
|
def test_api_documents_create_document_race_condition():
|
||||||
|
"""
|
||||||
|
It should be possible to create several documents at the same time
|
||||||
|
without causing any race conditions or data integrity issues.
|
||||||
|
"""
|
||||||
|
|
||||||
|
def create_document(title):
|
||||||
|
user = factories.UserFactory()
|
||||||
|
client = APIClient()
|
||||||
|
client.force_login(user)
|
||||||
|
return client.post(
|
||||||
|
"/api/v1.0/documents/",
|
||||||
|
{
|
||||||
|
"title": title,
|
||||||
|
},
|
||||||
|
format="json",
|
||||||
|
)
|
||||||
|
|
||||||
|
with ThreadPoolExecutor(max_workers=2) as executor:
|
||||||
|
future1 = executor.submit(create_document, "my document 1")
|
||||||
|
future2 = executor.submit(create_document, "my document 2")
|
||||||
|
|
||||||
|
response1 = future1.result()
|
||||||
|
response2 = future2.result()
|
||||||
|
|
||||||
|
assert response1.status_code == 201
|
||||||
|
assert response2.status_code == 201
|
||||||
|
|
||||||
|
|
||||||
def test_api_documents_create_authenticated_title_null():
|
def test_api_documents_create_authenticated_title_null():
|
||||||
"""It should be possible to create several documents with a null title."""
|
"""It should be possible to create several documents with a null title."""
|
||||||
user = factories.UserFactory()
|
user = factories.UserFactory()
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ Tests for Documents API endpoint in impress's core app: create
|
|||||||
|
|
||||||
# pylint: disable=W0621
|
# pylint: disable=W0621
|
||||||
|
|
||||||
|
from concurrent.futures import ThreadPoolExecutor
|
||||||
from unittest.mock import patch
|
from unittest.mock import patch
|
||||||
|
|
||||||
from django.core import mail
|
from django.core import mail
|
||||||
@@ -425,6 +426,36 @@ def test_api_documents_create_for_owner_new_user_no_sub_no_fallback_allow_duplic
|
|||||||
assert document.creator == user
|
assert document.creator == user
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.django_db(transaction=True)
|
||||||
|
def test_api_documents_create_document_race_condition():
|
||||||
|
"""
|
||||||
|
It should be possible to create several documents at the same time
|
||||||
|
without causing any race conditions or data integrity issues.
|
||||||
|
"""
|
||||||
|
|
||||||
|
def create_document(title):
|
||||||
|
user = factories.UserFactory()
|
||||||
|
client = APIClient()
|
||||||
|
client.force_login(user)
|
||||||
|
return client.post(
|
||||||
|
"/api/v1.0/documents/",
|
||||||
|
{
|
||||||
|
"title": title,
|
||||||
|
},
|
||||||
|
format="json",
|
||||||
|
)
|
||||||
|
|
||||||
|
with ThreadPoolExecutor(max_workers=2) as executor:
|
||||||
|
future1 = executor.submit(create_document, "my document 1")
|
||||||
|
future2 = executor.submit(create_document, "my document 2")
|
||||||
|
|
||||||
|
response1 = future1.result()
|
||||||
|
response2 = future2.result()
|
||||||
|
|
||||||
|
assert response1.status_code == 201
|
||||||
|
assert response2.status_code == 201
|
||||||
|
|
||||||
|
|
||||||
@patch.object(ServerCreateDocumentSerializer, "_send_email_notification")
|
@patch.object(ServerCreateDocumentSerializer, "_send_email_notification")
|
||||||
@override_settings(SERVER_TO_SERVER_API_TOKENS=["DummyToken"], LANGUAGE_CODE="de-de")
|
@override_settings(SERVER_TO_SERVER_API_TOKENS=["DummyToken"], LANGUAGE_CODE="de-de")
|
||||||
def test_api_documents_create_for_owner_with_default_language(
|
def test_api_documents_create_for_owner_with_default_language(
|
||||||
|
|||||||
Reference in New Issue
Block a user