✨(backend) wired soft deletion with yhub server

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

@@ -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

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,

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.

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,
+            )

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),
+    ]

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()

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."""

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),
+    ]