diff --git a/CHANGELOG.md b/CHANGELOG.md index 337d8f4dc..8d5accb78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,7 @@ and this project adheres to instead of giving up. The built-in endpoints can also answer JSON on `Accept: application/json` - ✨(collaboration) add a get-ydoc endpoint on yhub +- ✨(backend) duplicate a document through the collaboration server - ✨(backend) call YHubService to seed initial document content - ✨(backend) reset the yhub connections of a document and its descendants when an access or the link configuration changes diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 22969a509..8cd5b7f32 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -81,7 +81,7 @@ from core.utils.s3 import get_s3_client from core.utils.s3_response_stream import content_stream from core.utils.treebeard import create_tree_node_with_retry from core.utils.users import users_sharing_documents_with -from core.utils.yjs import extract_attachments +from core.utils.yjs import extract_attachments, extract_attachments_from_update from ..enums import FeatureFlag, SearchType from . import permissions, serializers, utils @@ -757,6 +757,45 @@ class DocumentViewSet( {"file": ["Could not save the imported file content"]} ) from err + def _get_collaboration_document(self, document): + """ + Return the content of a document, as held by the collaboration server. + + It is the source of truth for the content, the one Django may still + store is ignored. A document it holds nothing for has no content to + copy, it answers None. + """ + try: + return YHubService(user=self.request.user).get_ydoc(document) + except YHubError as err: + logger.error( + "could not fetch the content of document %s with error: %s", + document.id, + err, + ) + raise drf.exceptions.APIException( + "Failed to fetch the document content" + ) from err + + def _copy_collaboration_document(self, document, update): + """ + Seed a duplicated document with the content of the one it copies. + + The duplicate is worthless without it, so a failure is reported to the + caller rather than leaving an empty copy behind. + """ + try: + YHubService(user=self.request.user).create_ydoc(document, update) + except YHubError as err: + logger.error( + "could not copy the content into document %s with error: %s", + document.id, + err, + ) + raise drf.exceptions.APIException( + "Failed to duplicate the document content" + ) from err + def perform_create(self, serializer): """Set the current user as creator and owner of the newly created object.""" @@ -1286,11 +1325,12 @@ class DocumentViewSet( serializer.is_valid(raise_exception=True) user = request.user - duplicated_document = self._duplicate_document( - document_to_duplicate=document_to_duplicate, - serializer=serializer, - user=user, - ) + with transaction.atomic(): + duplicated_document = self._duplicate_document( + document_to_duplicate=document_to_duplicate, + serializer=serializer, + user=user, + ) posthog_capture( PosthogEventName.DOC_DUPLICATED, @@ -1332,7 +1372,9 @@ class DocumentViewSet( user_role = document_to_duplicate.get_role(user) is_owner_or_admin = user_role in models.PRIVILEGED_ROLES - base64_yjs_content = document_to_duplicate.content or "" + # The collaboration server holds the content, the duplicate is seeded + # with it once it exists + ydoc_update = self._get_collaboration_document(document_to_duplicate) # Duplicate the document instance link_kwargs = ( @@ -1343,7 +1385,7 @@ class DocumentViewSet( if with_accesses else {} ) - extracted_attachments = set(extract_attachments(document_to_duplicate.content)) + extracted_attachments = set(extract_attachments_from_update(ydoc_update)) attachments = list( extracted_attachments & set(document_to_duplicate.attachments) ) @@ -1352,7 +1394,6 @@ class DocumentViewSet( if new_parent is not None: duplicated_document = new_parent.add_child( title=title, - content=base64_yjs_content, attachments=attachments, duplicated_from=document_to_duplicate, creator=user, @@ -1384,7 +1425,6 @@ class DocumentViewSet( duplicated_document = models.Document.add_root( creator=user, title=title, - content=base64_yjs_content, attachments=attachments, duplicated_from=document_to_duplicate, **link_kwargs, @@ -1398,7 +1438,6 @@ class DocumentViewSet( duplicated_document = document_to_duplicate.add_sibling( "last-sibling", title=title, - content=base64_yjs_content, attachments=attachments, duplicated_from=document_to_duplicate, creator=user, @@ -1435,6 +1474,11 @@ class DocumentViewSet( # Bulk create all the duplicated accesses models.DocumentAccess.objects.bulk_create(accesses_to_create) + # the accesses exist by now, so the content is only served to the users + # the duplicate is meant for + if ydoc_update: + self._copy_collaboration_document(duplicated_document, ydoc_update) + if with_descendants: for child in document_to_duplicate.get_children().filter( ancestors_deleted_at__isnull=True diff --git a/src/backend/core/tests/documents/test_api_documents_duplicate.py b/src/backend/core/tests/documents/test_api_documents_duplicate.py index b3c5d509c..95a982779 100644 --- a/src/backend/core/tests/documents/test_api_documents_duplicate.py +++ b/src/backend/core/tests/documents/test_api_documents_duplicate.py @@ -19,9 +19,30 @@ from freezegun import freeze_time from rest_framework.test import APIClient from core import factories, models +from core.services.yhub_services import ( + ServiceUnavailableError as YHubServiceUnavailableError, +) pytestmark = pytest.mark.django_db + +@pytest.fixture(autouse=True, name="mock_yhub") +def mock_yhub_fixture(): + """ + The content of a document is held by the collaboration server. + + It stands for a server holding the very content the factories gave the + documents, which is what an editor connected to it would have saved. + """ + + def get_ydoc(document): + return base64.b64decode(document.content) if document.content else None + + with mock.patch("core.api.viewsets.YHubService") as mock_service: + mock_service.return_value.get_ydoc.side_effect = get_ydoc + yield mock_service + + PIXEL = ( b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01\x08\x06\x00" b"\x00\x00\x1f\x15\xc4\x89\x00\x00\x00\nIDATx\x9cc\xf8\xff\xff?\x00\x05\xfe\x02\xfe" @@ -75,7 +96,7 @@ def test_api_documents_duplicate_anonymous(): @pytest.mark.parametrize("index", range(3)) -def test_api_documents_duplicate_success(index): +def test_api_documents_duplicate_success(index, mock_yhub): """ Anonymous users should be able to retrieve attachments linked to a public document. Accesses should not be duplicated if the user does not request it specifically. @@ -120,7 +141,11 @@ def test_api_documents_duplicate_success(index): duplicated_document = models.Document.objects.get(id=response.json()["id"]) assert duplicated_document.title == "Copy of document with an image" - assert duplicated_document.content == document.content + # the content is copied through the collaboration server + assert duplicated_document.content is None + mock_yhub.return_value.create_ydoc.assert_called_once_with( + duplicated_document, base64.b64decode(document.content) + ) assert duplicated_document.creator == user assert duplicated_document.link_reach == "restricted" assert duplicated_document.link_role == "reader" @@ -185,7 +210,7 @@ def test_api_documents_duplicate_success(index): @pytest.mark.parametrize("role", ["owner", "administrator"]) -def test_api_documents_duplicate_with_accesses_admin(role): +def test_api_documents_duplicate_with_accesses_admin(role, mock_yhub): """ Accesses should be duplicated if the user requests it specifically and is owner or admin. """ @@ -219,7 +244,11 @@ def test_api_documents_duplicate_with_accesses_admin(role): duplicated_document = models.Document.objects.get(id=response.json()["id"]) assert duplicated_document.title == "Copy of document with accesses" - assert duplicated_document.content == document.content + # the content is copied through the collaboration server + assert duplicated_document.content is None + mock_yhub.return_value.create_ydoc.assert_called_once_with( + duplicated_document, base64.b64decode(document.content) + ) assert duplicated_document.link_reach == document.link_reach assert duplicated_document.link_role == document.link_role assert duplicated_document.creator == user @@ -246,7 +275,7 @@ def test_api_documents_duplicate_with_accesses_admin(role): @pytest.mark.parametrize("role", ["editor", "reader"]) -def test_api_documents_duplicate_with_accesses_non_admin(role): +def test_api_documents_duplicate_with_accesses_non_admin(role, mock_yhub): """ Accesses should not be duplicated if the user requests it specifically and is not owner or admin. @@ -274,7 +303,11 @@ def test_api_documents_duplicate_with_accesses_non_admin(role): duplicated_document = models.Document.objects.get(id=response.json()["id"]) assert duplicated_document.title == "Copy of document with accesses" - assert duplicated_document.content == document.content + # the content is copied through the collaboration server + assert duplicated_document.content is None + mock_yhub.return_value.create_ydoc.assert_called_once_with( + duplicated_document, base64.b64decode(document.content) + ) assert duplicated_document.link_reach == document.link_reach assert duplicated_document.link_role == document.link_role assert duplicated_document.creator == user @@ -295,7 +328,7 @@ def test_api_documents_duplicate_with_accesses_non_admin(role): @pytest.mark.parametrize("role", ["editor", "reader"]) -def test_api_documents_duplicate_non_root_document(role): +def test_api_documents_duplicate_non_root_document(role, mock_yhub): """ Non-root documents can be duplicated but without accesses. """ @@ -322,7 +355,11 @@ def test_api_documents_duplicate_non_root_document(role): duplicated_document = models.Document.objects.get(id=response.json()["id"]) assert duplicated_document.title == "Copy of document with accesses" - assert duplicated_document.content == child.content + # the content is copied through the collaboration server + assert duplicated_document.content is None + mock_yhub.return_value.create_ydoc.assert_called_once_with( + duplicated_document, base64.b64decode(child.content) + ) assert duplicated_document.link_reach == child.link_reach assert duplicated_document.link_role == child.link_role assert duplicated_document.creator == user @@ -517,7 +554,7 @@ def test_api_documents_duplicate_with_descendants_multi_level(): # pylint: disable=too-many-locals -def test_api_documents_duplicate_with_descendants_and_attachments(): +def test_api_documents_duplicate_with_descendants_and_attachments(mock_yhub): """ Duplicating with descendants should properly handle attachments in all children. """ @@ -590,14 +627,20 @@ def test_api_documents_duplicate_with_descendants_and_attachments(): # Check root attachments assert duplicated_root.attachments == [image_key_root] - assert duplicated_root.content == root_content # Check child attachments dup_children = duplicated_root.get_children() assert dup_children.count() == 1 dup_child = dup_children.first() assert dup_child.attachments == [image_key_child] - assert dup_child.content == child_content + + # the content of the whole subtree is copied through the collaboration server + assert duplicated_root.content is None + assert dup_child.content is None + assert mock_yhub.return_value.create_ydoc.call_args_list == [ + mock.call(duplicated_root, base64.b64decode(root_content)), + mock.call(dup_child, base64.b64decode(child_content)), + ] def test_api_documents_duplicate_with_descendants_and_accesses(): @@ -864,33 +907,57 @@ def test_api_documents_duplicate_with_descendants_complex_tree(): assert dup_grandchildren2.first().title == "Copy of GrandChild 3" -def test_api_documents_duplicate_document_without_content(): +def test_api_documents_duplicate_content_from_collaboration_server(mock_yhub): """ - A document that was never edited has no content in object storage. Duplicating it - should succeed and produce an empty duplicate. + The content held by the collaboration server is the one duplicated, the + content Django may still store for the document is ignored. """ user = factories.UserFactory() client = APIClient() client.force_login(user) - # Create through the API so nothing is ever written to object storage - with mock.patch("core.api.viewsets.posthog_capture"): - response = client.post( - "/api/v1.0/documents/", {"title": "never edited"}, format="json" - ) + image_key, image_url = get_image_refs(uuid.uuid4()) + + # what the collaboration server holds, an image Django never saw + ydoc = pycrdt.Doc() + ydoc["document-store"] = pycrdt.XmlFragment( + [pycrdt.XmlElement("img", {"src": image_url})] + ) + edited_update = ydoc.get_update() + mock_yhub.return_value.get_ydoc.side_effect = None + mock_yhub.return_value.get_ydoc.return_value = edited_update + + document = factories.DocumentFactory( + users=[(user, "owner")], + title="an edited document", + attachments=[image_key], + ) + + response = client.post(f"/api/v1.0/documents/{document.id!s}/duplicate/") assert response.status_code == 201 - document = models.Document.objects.get(id=response.json()["id"]) - assert document.content is None - - with mock.patch("core.api.viewsets.posthog_capture"): - response = client.post(f"/api/v1.0/documents/{document.id!s}/duplicate/") - - assert response.status_code == 201 - duplicated_document = models.Document.objects.get(id=response.json()["id"]) - assert duplicated_document.title == "Copy of never edited" - assert duplicated_document.content is None - assert duplicated_document.creator == user - assert duplicated_document.duplicated_from == document - assert duplicated_document.attachments == [] + + mock_yhub.return_value.get_ydoc.assert_called_once_with(document) + mock_yhub.return_value.create_ydoc.assert_called_once_with( + duplicated_document, edited_update + ) + # the attachments are the ones of the duplicated state, not of Django's + assert duplicated_document.attachments == [image_key] + + +def test_api_documents_duplicate_collaboration_server_unavailable(mock_yhub): + """A document whose content cannot be copied should not be duplicated.""" + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory(users=[(user, "owner")], title="my document") + mock_yhub.return_value.create_ydoc.side_effect = YHubServiceUnavailableError( + "Failed to connect to the yhub service" + ) + + response = client.post(f"/api/v1.0/documents/{document.id!s}/duplicate/") + + assert response.status_code == 500 + assert models.Document.objects.count() == 1 diff --git a/src/backend/core/tests/external_api/test_external_api_documents.py b/src/backend/core/tests/external_api/test_external_api_documents.py index e03a9286a..b4bb8f1eb 100644 --- a/src/backend/core/tests/external_api/test_external_api_documents.py +++ b/src/backend/core/tests/external_api/test_external_api_documents.py @@ -429,9 +429,12 @@ def test_external_api_documents_duplicate_allowed( role=models.RoleChoices.OWNER, ) - response = client.post( - f"/external_api/v1.0/documents/{document.id!s}/duplicate/", - ) + with patch("core.api.viewsets.YHubService") as mock_yhub: + # the collaboration server holds no content for this document + mock_yhub.return_value.get_ydoc.return_value = None + response = client.post( + f"/external_api/v1.0/documents/{document.id!s}/duplicate/", + ) assert response.status_code == 201 diff --git a/src/backend/core/utils/yjs.py b/src/backend/core/utils/yjs.py index a5f4c8b2e..7856d0636 100644 --- a/src/backend/core/utils/yjs.py +++ b/src/backend/core/utils/yjs.py @@ -9,14 +9,18 @@ from bs4 import BeautifulSoup from core import enums +def yjs_to_xml(update): + """Extract xml from a raw yjs update.""" + + doc = pycrdt.Doc() + doc.apply_update(update) + return str(doc.get("document-store", type=pycrdt.XmlFragment)) + + def base64_yjs_to_xml(base64_string): """Extract xml from base64 yjs document.""" - decoded_bytes = base64.b64decode(base64_string) - - doc = pycrdt.Doc() - doc.apply_update(decoded_bytes) - return str(doc.get("document-store", type=pycrdt.XmlFragment)) + return yjs_to_xml(base64.b64decode(base64_string)) def base64_yjs_to_text(base64_string): @@ -34,3 +38,11 @@ def extract_attachments(content): xml_content = base64_yjs_to_xml(content) return re.findall(enums.MEDIA_STORAGE_URL_EXTRACT, xml_content) + + +def extract_attachments_from_update(update): + """Helper method to extract media paths from a raw yjs update.""" + if not update: + return [] + + return re.findall(enums.MEDIA_STORAGE_URL_EXTRACT, yjs_to_xml(update))