diff --git a/CHANGELOG.md b/CHANGELOG.md index b249b4fd..a577cada 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to ### Fixed - 🐛(backend) create a link_trace record for on-boarding documents +- 🐛(backend) manage race condition when creating sandbox document ## [v4.7.0] - 2026-03-09 diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 322b8c42..99c2ccd4 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -19,7 +19,7 @@ from django.core.cache import cache from django.core.files.base import ContentFile from django.core.files.storage import default_storage from django.core.mail import send_mail -from django.db import models, transaction +from django.db import connection, models, transaction from django.db.models.functions import Left, Length from django.template.loader import render_to_string from django.utils import timezone @@ -265,28 +265,38 @@ class User(AbstractBaseUser, BaseModel, auth_models.PermissionsMixin): duplicate the sandbox document for the user """ if settings.USER_ONBOARDING_SANDBOX_DOCUMENT: - sandbox_id = settings.USER_ONBOARDING_SANDBOX_DOCUMENT - try: - template_document = Document.objects.get(id=sandbox_id) - except Document.DoesNotExist: - logger.warning( - "Onboarding sandbox document with id %s does not exist. Skipping.", - sandbox_id, + # transaction.atomic is used in a context manager to avoid a transaction if + # the settings USER_ONBOARDING_SANDBOX_DOCUMENT is unused + with transaction.atomic(): + # locks the table to ensure safe concurrent access + with connection.cursor() as cursor: + cursor.execute( + f'LOCK TABLE "{Document._meta.db_table}" ' # noqa: SLF001 + "IN SHARE ROW EXCLUSIVE MODE;" + ) + + sandbox_id = settings.USER_ONBOARDING_SANDBOX_DOCUMENT + try: + template_document = Document.objects.get(id=sandbox_id) + except Document.DoesNotExist: + logger.warning( + "Onboarding sandbox document with id %s does not exist. Skipping.", + sandbox_id, + ) + return + + sandbox_document = template_document.add_sibling( + "right", + title=template_document.title, + content=template_document.content, + attachments=template_document.attachments, + duplicated_from=template_document, + creator=self, ) - return - sandbox_document = template_document.add_sibling( - "right", - title=template_document.title, - content=template_document.content, - attachments=template_document.attachments, - duplicated_from=template_document, - creator=self, - ) - - DocumentAccess.objects.create( - user=self, document=sandbox_document, role=RoleChoices.OWNER - ) + DocumentAccess.objects.create( + user=self, document=sandbox_document, role=RoleChoices.OWNER + ) def _convert_valid_invitations(self): """ diff --git a/src/backend/core/tests/test_models_invitations.py b/src/backend/core/tests/test_models_invitations.py index 56cf546c..251fca44 100644 --- a/src/backend/core/tests/test_models_invitations.py +++ b/src/backend/core/tests/test_models_invitations.py @@ -114,7 +114,7 @@ def test_models_invitations_new_user_convert_invitations_to_accesses(): ).exists() # the other invitation remains -def test_models_invitationd_new_user_filter_expired_invitations(): +def test_models_invitations_new_user_filter_expired_invitations(): """ Upon creating a new identity, valid invitations should be converted into accesses and expired invitations should remain unchanged. @@ -145,7 +145,7 @@ def test_models_invitationd_new_user_filter_expired_invitations(): @pytest.mark.parametrize("num_invitations, num_queries", [(0, 3), (1, 7), (20, 7)]) -def test_models_invitationd_new_userd_user_creation_constant_num_queries( +def test_models_invitations_new_userd_user_creation_constant_num_queries( django_assert_num_queries, num_invitations, num_queries ): """ diff --git a/src/backend/core/tests/test_models_users.py b/src/backend/core/tests/test_models_users.py index f0f6b01c..cbfa144e 100644 --- a/src/backend/core/tests/test_models_users.py +++ b/src/backend/core/tests/test_models_users.py @@ -3,6 +3,7 @@ Unit tests for the User model """ import uuid +from concurrent.futures import ThreadPoolExecutor from unittest.mock import patch from django.core.exceptions import ValidationError @@ -188,9 +189,8 @@ def test_models_users_handle_onboarding_documents_on_restricted_document_is_not_ """On-boarding document can be used when restricted""" document = factories.DocumentFactory(link_reach=models.LinkReachChoices.RESTRICTED) - settings.USER_ONBOARDING_DOCUMENTS = [str(document.id)] - - user = factories.UserFactory() + with override_settings(USER_ONBOARDING_DOCUMENTS=[str(document.id)]): + user = factories.UserFactory() assert not models.LinkTrace.objects.filter(user=user, document=document).exists() @@ -302,3 +302,30 @@ def test_models_users_duplicate_onboarding_sandbox_document_integration_with_oth document=sandbox_doc, user=user, role=models.RoleChoices.OWNER ).exists() assert models.LinkTrace.objects.filter(document=onboarding_doc, user=user).exists() + + +@pytest.mark.django_db(transaction=True) +def test_models_users_duplicate_onboarding_sandbox_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_user(): + return factories.UserFactory() + + template_document = factories.DocumentFactory(title="Getting started with Docs") + with ( + override_settings( + USER_ONBOARDING_SANDBOX_DOCUMENT=str(template_document.id), + ), + ThreadPoolExecutor(max_workers=2) as executor, + ): + future1 = executor.submit(create_user) + future2 = executor.submit(create_user) + + user1 = future1.result() + user2 = future2.result() + + assert isinstance(user1, models.User) + assert isinstance(user2, models.User)