From 37ec23dc3e3262cfed7e9c969a58319698db7e5d Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Wed, 12 Aug 2026 12:13:35 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20wired=20soft=20deletion=20?= =?UTF-8?q?with=20yhub=20server?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit yhub is the source of truth, when a user delete a document, it should also be deleted in the yhub server. We call the yhub server in the perform_destroy action but also the restore endpoint of yhub when a document is restored. --- CHANGELOG.md | 13 ++ src/backend/core/api/viewsets.py | 17 +++ src/backend/core/services/yhub_services.py | 35 +++++- src/backend/core/tasks/documents.py | 59 +++++++++ .../documents/test_api_documents_delete.py | 29 +++++ .../documents/test_api_documents_restore.py | 33 +++++ .../core/tests/test_services_yhub_services.py | 48 +++++++ .../core/tests/test_tasks_documents.py | 118 ++++++++++++++++++ 8 files changed, 351 insertions(+), 1 deletion(-) create mode 100644 src/backend/core/tasks/documents.py create mode 100644 src/backend/core/tests/test_tasks_documents.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 335a51741..c1ea4ed3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,19 @@ and this project adheres to ### Added +- ✨(collaboration) delete a document on the collaboration server when it is + deleted in Docs, and restore it when it comes back out of the trashbin. The + content lives there now, so until it was told, the clients already editing a + deleted document went on editing it and its content outlived it. Both go + through `sync_service_deletions_in_cascade`, which walks the deleted subtree + and reports what each of its documents is now — a restored document only + brings back the part of its subtree that was deleted with it. Deleting uses + yhub's built-in `DELETE .../ydoc/` (a soft deletion: connected clients are + disconnected with the close code 4404 and every route answers 404, the + content is left untouched), restoring the new backend-internal + `POST /collaboration/restore-ydoc/v1/{org}/{docid}`, since yhub 0.6.0 has no + built-in route for it. Erasing the content for good stays out of reach, as it + is in Docs: a document that is no longer restorable is not erased either - ⬆️(collaboration) upgrade yhub to 0.6.0, which needs a schema change: a `yhub_ydoc_tombstones_v1` table (it adds document deletion) and four `*_is_reference` markers on `yhub_ydoc_v1`. Neither is optional — every diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 88be69242..b8c78c0ba 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -8,6 +8,7 @@ import logging import socket import uuid from collections import defaultdict +from functools import partial from urllib.parse import unquote, urlencode, urlparse from django.conf import settings @@ -69,6 +70,7 @@ from core.services.search_indexers import ( ) from core.services.yhub_services import YHubError, YHubService from core.tasks.access import reset_service_connections_in_cascade +from core.tasks.documents import sync_service_deletions_in_cascade from core.tasks.mail import send_ask_for_access_mail from core.tasks.search import trigger_batch_document_indexer from core.utils.analytics import PosthogEventName, posthog_capture @@ -820,6 +822,14 @@ class DocumentViewSet( """Override to implement a soft delete instead of dumping the record in database.""" instance.soft_delete() + # the collaboration server holds the content: until it is told, it goes + # on serving the document to the clients editing it. On commit, because + # the task reads back what was just written to know what to report — it + # would find the document alive and restore it instead + transaction.on_commit( + partial(sync_service_deletions_in_cascade.delay, str(instance.id)) + ) + posthog_capture( PosthogEventName.DOC_DELETED, self.request.user, {}, document=instance ) @@ -1102,6 +1112,13 @@ class DocumentViewSet( except RuntimeError as err: raise drf.exceptions.ValidationError({"detail": str(err)}) from err + # the counterpart of the deletion: the same walk puts back the content + # of the documents that came back with this one, and it reads the + # restored state back, hence on commit as well + transaction.on_commit( + partial(sync_service_deletions_in_cascade.delay, str(document.id)) + ) + return drf_response.Response( {"detail": "Document has been successfully restored."}, status=status.HTTP_200_OK, diff --git a/src/backend/core/services/yhub_services.py b/src/backend/core/services/yhub_services.py index 8ce04fa27..214decbd1 100644 --- a/src/backend/core/services/yhub_services.py +++ b/src/backend/core/services/yhub_services.py @@ -9,7 +9,8 @@ Every route is mounted under the `apiPrefix` yhub is configured with, and a room is addressed as `/{prefix}/{endpoint}/{version}/{org}/{docid}`, where `org` is the yhub organization Docs runs under and `docid` the document id. The built-in endpoints are `ydoc` (get the state of a document, patch it with a Yjs -update), `rollback`, `prune`, `changeset` and `activity`, all at `v1`. yhub also +update, delete it), `rollback`, `prune`, `changeset` and `activity`, all at +`v1`. yhub also accepts a `branch` query parameter, but our auth plugin only ever grants access to the `main` branch, so this service never sends it. @@ -287,6 +288,38 @@ class YHubService: }, ) + def delete_ydoc(self, document): + """ + Delete a document on the collaboration server. + + This is what stops the clients editing a deleted document: they are + disconnected, and the collaboration server answers 404 for it from then + on. The deletion is a soft one, its content is left untouched and + `restore_ydoc` brings the document back whole. Erasing the content for + good is a separate, irreversible operation that yhub deliberately does + not expose over its REST API. + + Idempotent, and never refused: deleting a document twice keeps the date + of the first deletion, and a document the collaboration server holds + nothing for is recorded as deleted all the same. + """ + return self.request("delete", self.build_url("ydoc", document)) + + def restore_ydoc(self, document): + """ + Undo the deletion of a document on the collaboration server. + + Its content was never touched, so it comes back with its whole history. + A document that is not deleted is left alone rather than refused, which + is what lets a restored subtree be reported without asking what became + of each of its documents. + + yhub answers 409 for a document whose content was erased — there is + nothing left to bring back — reported as an `APIError` carrying the + status. + """ + return self.request("post", self.build_url("restore-ydoc", document)) + def migrate(self, document, force=False): """ Replay the legacy version history of a document into the collaboration server. diff --git a/src/backend/core/tasks/documents.py b/src/backend/core/tasks/documents.py new file mode 100644 index 000000000..0b4297671 --- /dev/null +++ b/src/backend/core/tasks/documents.py @@ -0,0 +1,59 @@ +"""Tasks dedicated to the documents themselves.""" + +from logging import getLogger + +from core import models +from core.services.yhub_services import YHubError, YHubService + +from impress.celery_app import app + +logger = getLogger(__name__) + + +@app.task +def sync_service_deletions_in_cascade(document_id): + """ + Report the deletion of a document and of its descendants to the + collaboration server. + + The content of a document lives there, not here: until it is told, it keeps + serving a deleted document to the clients already editing it, and its + content outlives the document. The endpoint is document scoped, hence the + walk down the tree — deleting a document deletes the subtree under it. + + Restoring goes through the very same walk. A restored document brings back + only the part of its subtree that was deleted with it, the documents deleted + on their own stay deleted, so what each document of the subtree needs is + read from what it is now rather than from what was just done to it. Running + this twice therefore changes nothing, and running it late still lands on the + right answer. + + A document failing is logged and does not stop the ones after it; the + collaboration server keeps serving it until something says so again. + """ + try: + document = models.Document.objects.get(pk=document_id) + except models.Document.DoesNotExist: + logger.error("Document %s does not exists anymore", document_id) + return + + documents = models.Document.objects.filter( + path__startswith=document.path, depth__gte=document.depth + ).order_by("path") + + service = YHubService() + for doc in documents: + # a descendant carries the deletion of its ancestors, never its own + # `deleted_at`, unless it was deleted on its own beforehand + deleted = doc.deleted_at is not None or doc.ancestors_deleted_at is not None + try: + if deleted: + service.delete_ydoc(doc) + else: + service.restore_ydoc(doc) + except YHubError: + logger.exception( + "impossible to %s document %s on the collaboration server", + "delete" if deleted else "restore", + doc.id, + ) diff --git a/src/backend/core/tests/documents/test_api_documents_delete.py b/src/backend/core/tests/documents/test_api_documents_delete.py index f89503eb7..6672b1376 100644 --- a/src/backend/core/tests/documents/test_api_documents_delete.py +++ b/src/backend/core/tests/documents/test_api_documents_delete.py @@ -148,3 +148,32 @@ def test_api_documents_delete_authenticated_owner(via, mock_user_teams): {}, document=document, ) + + +def test_api_documents_delete_reports_the_deletion_to_the_collaboration_server( + django_capture_on_commit_callbacks, +): + """ + Deleting a document should tell the collaboration server, which holds its + content and would otherwise go on serving it to the clients editing it. + """ + user = factories.UserFactory() + document = factories.DocumentFactory(users=[(user, "owner")]) + child = factories.DocumentFactory(parent=document) + + client = APIClient() + client.force_login(user) + + # the report is made once the deletion is committed: the task reads it back + with ( + mock.patch("core.tasks.documents.YHubService") as mock_service, + django_capture_on_commit_callbacks(execute=True), + ): + response = client.delete(f"/api/v1.0/documents/{document.id!s}/") + + assert response.status_code == 204 + # the subtree goes with it + assert mock_service.return_value.delete_ydoc.call_args_list == [ + mock.call(document), + mock.call(child), + ] diff --git a/src/backend/core/tests/documents/test_api_documents_restore.py b/src/backend/core/tests/documents/test_api_documents_restore.py index a1343d7c7..850a0d309 100644 --- a/src/backend/core/tests/documents/test_api_documents_restore.py +++ b/src/backend/core/tests/documents/test_api_documents_restore.py @@ -3,6 +3,7 @@ Test restoring documents after a soft delete via the detail action API endpoint. """ from datetime import timedelta +from unittest import mock from django.utils import timezone @@ -145,3 +146,35 @@ def test_api_documents_restore_authenticated_owner_not_deleted(): document.refresh_from_db() assert document.deleted_at is None assert document.ancestors_deleted_at is None + + +def test_api_documents_restore_reports_the_restoration_to_the_collaboration_server( + django_capture_on_commit_callbacks, +): + """ + Restoring a document should tell the collaboration server, which answers + 404 for it as long as it believes it deleted. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory() + child = factories.DocumentFactory(parent=document) + factories.UserDocumentAccessFactory(document=document, user=user, role="owner") + document.soft_delete() + + # the report is made once the restoration is committed: the task reads it back + with ( + mock.patch("core.tasks.documents.YHubService") as mock_service, + django_capture_on_commit_callbacks(execute=True), + ): + response = client.post(f"/api/v1.0/documents/{document.id!s}/restore/") + + assert response.status_code == 200 + # the subtree comes back with it + assert mock_service.return_value.restore_ydoc.call_args_list == [ + mock.call(document), + mock.call(child), + ] + mock_service.return_value.delete_ydoc.assert_not_called() diff --git a/src/backend/core/tests/test_services_yhub_services.py b/src/backend/core/tests/test_services_yhub_services.py index 9379188c3..e39219252 100644 --- a/src/backend/core/tests/test_services_yhub_services.py +++ b/src/backend/core/tests/test_services_yhub_services.py @@ -242,6 +242,54 @@ def test_migrate_forced(mock_request): assert args[1].endswith("?force=true") +@patch("requests.request") +def test_delete_ydoc(mock_request): + """Should ask yhub to delete the document, on its built-in endpoint.""" + mock_request.return_value.ok = True + + response = YHubService().delete_ydoc(DOCUMENT) + + assert response is mock_request.return_value + args, _kwargs = mock_request.call_args + assert args == ( + "delete", + f"http://yhub:3002/collaboration/ydoc/v1/docs/{DOCUMENT.id!s}", + ) + + +@patch("requests.request") +def test_restore_ydoc(mock_request): + """Should ask yhub to bring the document back, on our own endpoint.""" + mock_request.return_value.ok = True + + response = YHubService().restore_ydoc(DOCUMENT) + + assert response is mock_request.return_value + args, _kwargs = mock_request.call_args + # yhub has a built-in route to delete a document but none to restore one + assert args == ( + "post", + f"http://yhub:3002/collaboration/restore-ydoc/v1/docs/{DOCUMENT.id!s}", + ) + + +@patch("requests.request") +def test_restore_ydoc_erased_content(mock_request): + """A document whose content was erased should report the conflict it is.""" + mock_request.return_value.ok = False + mock_request.return_value.status_code = 409 + mock_request.return_value.text = '{"error": "Document content was erased"}' + mock_request.return_value.json.return_value = { + "error": "Document content was erased" + } + + with pytest.raises(APIError) as excinfo: + YHubService().restore_ydoc(DOCUMENT) + + assert excinfo.value.status_code == 409 + assert "Document content was erased" in str(excinfo.value) + + @patch("requests.request") def test_reset_connections(mock_request): """Should ask yhub to re-check every connection of the document.""" diff --git a/src/backend/core/tests/test_tasks_documents.py b/src/backend/core/tests/test_tasks_documents.py new file mode 100644 index 000000000..47840b212 --- /dev/null +++ b/src/backend/core/tests/test_tasks_documents.py @@ -0,0 +1,118 @@ +""" +Tests for the `sync_service_deletions_in_cascade` Celery task in the +core.tasks.documents module. +""" + +from unittest import mock + +import pytest + +from core import factories +from core.services.yhub_services import ServiceUnavailableError +from core.tasks.documents import sync_service_deletions_in_cascade + +pytestmark = pytest.mark.django_db + + +@mock.patch("core.tasks.documents.YHubService") +def test_sync_service_deletions_deletes_the_document(mock_service): + """A deleted document should be deleted on the collaboration server.""" + document = factories.DocumentFactory() + document.soft_delete() + + sync_service_deletions_in_cascade(str(document.id)) + + mock_service.return_value.delete_ydoc.assert_called_once_with(document) + mock_service.return_value.restore_ydoc.assert_not_called() + + +@mock.patch("core.tasks.documents.YHubService") +def test_sync_service_deletions_in_cascade(mock_service): + """ + Deleting a document deletes the subtree under it, so the whole subtree + should be deleted, the document itself included and its ancestors left out. + """ + parent = factories.DocumentFactory() + document = factories.DocumentFactory(parent=parent) + child = factories.DocumentFactory(parent=document) + grand_child = factories.DocumentFactory(parent=child) + factories.DocumentFactory() # a document of another tree + document.soft_delete() + + sync_service_deletions_in_cascade(str(document.id)) + + assert mock_service.return_value.delete_ydoc.call_args_list == [ + mock.call(document), + mock.call(child), + mock.call(grand_child), + ] + + +@mock.patch("core.tasks.documents.YHubService") +def test_sync_service_deletions_restores_the_document(mock_service): + """A document that is back should be restored on the collaboration server.""" + document = factories.DocumentFactory() + child = factories.DocumentFactory(parent=document) + document.soft_delete() + document.restore() + + sync_service_deletions_in_cascade(str(document.id)) + + assert mock_service.return_value.restore_ydoc.call_args_list == [ + mock.call(document), + mock.call(child), + ] + mock_service.return_value.delete_ydoc.assert_not_called() + + +@mock.patch("core.tasks.documents.YHubService") +def test_sync_service_deletions_restore_leaves_out_what_stays_deleted(mock_service): + """ + A document deleted on its own before its ancestor was stays deleted when + the ancestor comes back, and so should its content. + """ + document = factories.DocumentFactory() + child = factories.DocumentFactory(parent=document) + grand_child = factories.DocumentFactory(parent=child) + child.soft_delete() + document.soft_delete() + document.restore() + + sync_service_deletions_in_cascade(str(document.id)) + + # the subtree of the child was deleted on its own and is still deleted + assert mock_service.return_value.restore_ydoc.call_args_list == [ + mock.call(document) + ] + assert mock_service.return_value.delete_ydoc.call_args_list == [ + mock.call(child), + mock.call(grand_child), + ] + + +@mock.patch("core.tasks.documents.YHubService") +def test_sync_service_deletions_unknown_document(mock_service): + """A document deleted for good in the meantime should not reach the service.""" + sync_service_deletions_in_cascade("d43ea3c5-b8ee-4a4a-9c60-2ad7a1d9e6cf") + + mock_service.return_value.delete_ydoc.assert_not_called() + mock_service.return_value.restore_ydoc.assert_not_called() + + +@mock.patch("core.tasks.documents.YHubService") +def test_sync_service_deletions_keeps_going_on_failure(mock_service): + """A document failing should not deprive the ones after it of their deletion.""" + document = factories.DocumentFactory() + child = factories.DocumentFactory(parent=document) + document.soft_delete() + mock_service.return_value.delete_ydoc.side_effect = [ + ServiceUnavailableError("yhub is down"), + None, + ] + + sync_service_deletions_in_cascade(str(document.id)) + + assert mock_service.return_value.delete_ydoc.call_args_list == [ + mock.call(document), + mock.call(child), + ]