From cf7be5b48465cdd3a3976eebbcdca76ca35623b0 Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Sat, 26 Sep 2026 09:05:31 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(backend)=20retry=20the=20duplicate?= =?UTF-8?q?=20of=20a=20document=20on=20a=20tree=20path=20collision?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The duplicate was not using create_tree_node_with_retry helper managing duplicate path. We will have collision on this action if we don't use it. --- CHANGELOG.md | 1 + src/backend/core/api/viewsets.py | 37 +++-- ..._api_documents_duplicate_path_collision.py | 146 ++++++++++++++++++ 3 files changed, 170 insertions(+), 14 deletions(-) create mode 100644 src/backend/core/tests/documents/test_api_documents_duplicate_path_collision.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c7653de09..9d6af4441 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,7 @@ and this project adheres to - 🐛(frontend) open search results in a new tab with ctrl/cmd+click #2719 - 🐛(docker) pull minio images from pgsty +- 🐛(backend) retry the duplicate of a document on a tree path collision - 🐛(frontend) clear callout background on Backspace #2052 - 🐛(export) keep image aspect ratio in PDF columns #2670 - 🐛(frontend) fix redirect after deleting a document #2706 diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 6ec050c4f..ca8456ac8 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -1455,7 +1455,9 @@ class DocumentViewSet( extracted_attachments & set(document_to_duplicate.attachments) ) title = capfirst(_("copy of {title}").format(title=document_to_duplicate.title)) - # If parent_duplicate is provided we must add the duplicated document as a child + # If parent_duplicate is provided we must add the duplicated document as a child. + # No retry here: the parent was created by this very transaction, nobody + # else can add children under it, so its child paths cannot collide if new_parent is not None: duplicated_document = new_parent.add_child( title=title, @@ -1487,12 +1489,14 @@ class DocumentViewSet( elif not document_to_duplicate.is_root() and choices.RoleChoices.get_priority( user_role ) < choices.RoleChoices.get_priority(models.RoleChoices.EDITOR): - duplicated_document = models.Document.add_root( - creator=user, - title=title, - attachments=attachments, - duplicated_from=document_to_duplicate, - **link_kwargs, + duplicated_document = create_tree_node_with_retry( + lambda: models.Document.add_root( + creator=user, + title=title, + attachments=attachments, + duplicated_from=document_to_duplicate, + **link_kwargs, + ) ) models.DocumentAccess.objects.create( document=duplicated_document, @@ -1500,13 +1504,18 @@ class DocumentViewSet( role=models.RoleChoices.OWNER, ) else: - duplicated_document = document_to_duplicate.add_sibling( - "last-sibling", - title=title, - attachments=attachments, - duplicated_from=document_to_duplicate, - creator=user, - **link_kwargs, + # Treebeard computes the path of the new sibling from the current + # last one: two requests creating a node at the same level at the + # same time compute the same path, the loser retries with a fresh one + duplicated_document = create_tree_node_with_retry( + lambda: document_to_duplicate.add_sibling( + "last-sibling", + title=title, + attachments=attachments, + duplicated_from=document_to_duplicate, + creator=user, + **link_kwargs, + ) ) # Always add the logged-in user as OWNER for root documents diff --git a/src/backend/core/tests/documents/test_api_documents_duplicate_path_collision.py b/src/backend/core/tests/documents/test_api_documents_duplicate_path_collision.py new file mode 100644 index 000000000..a6b087538 --- /dev/null +++ b/src/backend/core/tests/documents/test_api_documents_duplicate_path_collision.py @@ -0,0 +1,146 @@ +""" +Tests of the retry on a path collision of the duplicate API endpoint. + +Treebeard computes the materialized path of a new node from the current last +sibling (or the last root): two requests adding a node at the same level at the +same time compute the same path, and the loser of the race hits the unique index. +""" + +from unittest import mock + +from django.db import IntegrityError + +import pytest +from rest_framework.test import APIClient + +from core import factories, models +from core.factories import YDOC_HELLO_WORLD_UPDATE + +pytestmark = pytest.mark.django_db + + +@pytest.fixture(autouse=True, name="mock_yhub") +def mock_yhub_fixture(): + """The collaboration server holds the content of every document.""" + with mock.patch("core.api.viewsets.YHubService") as mock_service: + mock_service.return_value.get_ydoc.return_value = YDOC_HELLO_WORLD_UPDATE + yield mock_service + + +PATH_COLLISION = IntegrityError( + 'duplicate key value violates unique constraint "impress_document_path_key"\n' + "DETAIL: Key (path)=(0001dbx) already exists." +) + + +def test_api_documents_duplicate_retries_sibling_path_collision(): + """ + Treebeard computes the path of the duplicate from the current last sibling: + two requests adding a node at the same level at the same time compute the same + path. The loser must retry with a fresh path rather than answer a 500. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + root = factories.DocumentFactory(users=[(user, "owner")], title="Root") + factories.DocumentFactory(parent=root, title="Child") + + original_add_sibling = models.Document.add_sibling + attempts = [] + + def add_sibling(self, pos, **kwargs): + attempts.append(pos) + if len(attempts) == 1: + raise PATH_COLLISION + return original_add_sibling(self, pos, **kwargs) + + with ( + mock.patch.object( + models.Document, "add_sibling", autospec=True, side_effect=add_sibling + ), + mock.patch("core.api.viewsets.posthog_capture"), + ): + response = client.post( + f"/api/v1.0/documents/{root.id!s}/duplicate/", + {"with_descendants": True}, + format="json", + ) + + assert response.status_code == 201 + assert attempts == ["last-sibling", "last-sibling"] + + duplicated = models.Document.objects.get(id=response.json()["id"]) + assert duplicated.is_root() + assert duplicated.get_siblings().count() == 2 + assert [child.title for child in duplicated.get_children()] == ["Copy of Child"] + assert models.Document.objects.count() == 4 + assert duplicated.accesses.filter(user=user, role="owner").exists() + + +def test_api_documents_duplicate_retries_root_path_collision(): + """ + A non-privileged user duplicating a sub-document gets a new root: its path is + computed from the current last root and must be retried on collision as well. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent = factories.DocumentFactory() + child = factories.DocumentFactory( + parent=parent, users=[(user, "reader")], title="Sub Document" + ) + factories.DocumentFactory(parent=child, title="Grandchild") + + original_add_root = models.Document.add_root + attempts = [] + + def add_root(**kwargs): + attempts.append(kwargs["title"]) + if len(attempts) == 1: + raise PATH_COLLISION + return original_add_root(**kwargs) + + with ( + mock.patch.object(models.Document, "add_root", side_effect=add_root), + mock.patch("core.api.viewsets.posthog_capture"), + ): + response = client.post( + f"/api/v1.0/documents/{child.id!s}/duplicate/", + {"with_descendants": True}, + format="json", + ) + + assert response.status_code == 201 + assert len(attempts) == 2 + + duplicated = models.Document.objects.get(id=response.json()["id"]) + assert duplicated.is_root() + assert [child.title for child in duplicated.get_children()] == [ + "Copy of Grandchild" + ] + assert models.Document.objects.count() == 5 + assert duplicated.accesses.filter(user=user, role="owner").exists() + + +def test_api_documents_duplicate_path_collision_exceeding_attempts(settings): + """A collision that persists after the last attempt is not swallowed.""" + settings.TREEBEARD_PATH_COMPUTE_RETRY_MAX_ATTEMPTS = 2 + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + root = factories.DocumentFactory(users=[(user, "owner")]) + + with ( + mock.patch.object( + models.Document, "add_sibling", autospec=True, side_effect=PATH_COLLISION + ) as mock_add_sibling, + mock.patch("core.api.viewsets.posthog_capture") as mock_capture, + pytest.raises(IntegrityError), + ): + client.post(f"/api/v1.0/documents/{root.id!s}/duplicate/", format="json") + + assert mock_add_sibling.call_count == 2 + mock_capture.assert_not_called() + assert models.Document.objects.count() == 1