diff --git a/CHANGELOG.md b/CHANGELOG.md index a7819716..7ac31a5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -145,6 +145,10 @@ and this project adheres to - 🐛(email) invitation emails in receivers language +## Fixed + +- 🐛(backend) race condition create doc #633 + ## [2.2.0] - 2025-02-10 ## Added diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 685cfb83..544fdb2a 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -11,8 +11,8 @@ from django.contrib.postgres.fields import ArrayField from django.contrib.postgres.search import TrigramSimilarity from django.core.exceptions import ValidationError 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 transaction from django.db.models.expressions import RawSQL from django.db.models.functions import Left, Length from django.http import Http404, StreamingHttpResponse @@ -607,6 +607,14 @@ class DocumentViewSet( @transaction.atomic def perform_create(self, serializer): """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( creator=self.request.user, **serializer.validated_data, @@ -666,10 +674,19 @@ class DocumentViewSet( permission_classes=[], url_path="create-for-owner", ) + @transaction.atomic def create_for_owner(self, request): """ 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 serializer = serializers.ServerCreateDocumentSerializer(data=request.data) if not serializer.is_valid(): @@ -775,7 +792,12 @@ class DocumentViewSet( serializer.is_valid(raise_exception=True) 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, **serializer.validated_data, ) diff --git a/src/backend/core/tests/documents/test_api_documents_children_create.py b/src/backend/core/tests/documents/test_api_documents_children_create.py index 3a3c3ff9..5aea1b60 100644 --- a/src/backend/core/tests/documents/test_api_documents_children_create.py +++ b/src/backend/core/tests/documents/test_api_documents_children_create.py @@ -2,6 +2,7 @@ Tests for Documents API endpoint in impress's core app: children create """ +from concurrent.futures import ThreadPoolExecutor from uuid import uuid4 import pytest @@ -249,3 +250,41 @@ def test_api_documents_children_create_force_id_existing(): assert response.json() == { "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 diff --git a/src/backend/core/tests/documents/test_api_documents_create.py b/src/backend/core/tests/documents/test_api_documents_create.py index 151724e0..2b6c404d 100644 --- a/src/backend/core/tests/documents/test_api_documents_create.py +++ b/src/backend/core/tests/documents/test_api_documents_create.py @@ -2,6 +2,7 @@ Tests for Documents API endpoint in impress's core app: create """ +from concurrent.futures import ThreadPoolExecutor from uuid import uuid4 import pytest @@ -51,6 +52,36 @@ def test_api_documents_create_authenticated_success(): 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(): """It should be possible to create several documents with a null title.""" user = factories.UserFactory() diff --git a/src/backend/core/tests/documents/test_api_documents_create_for_owner.py b/src/backend/core/tests/documents/test_api_documents_create_for_owner.py index 084aaec1..c2e878d1 100644 --- a/src/backend/core/tests/documents/test_api_documents_create_for_owner.py +++ b/src/backend/core/tests/documents/test_api_documents_create_for_owner.py @@ -4,6 +4,7 @@ Tests for Documents API endpoint in impress's core app: create # pylint: disable=W0621 +from concurrent.futures import ThreadPoolExecutor from unittest.mock import patch 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 +@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") @override_settings(SERVER_TO_SERVER_API_TOKENS=["DummyToken"], LANGUAGE_CODE="de-de") def test_api_documents_create_for_owner_with_default_language(