♻️(backend) index the content of a document from updated_content endpoint

the search indexer reads it with `YHubService`, and the indexation of an
edited document is triggered by the `content-updated` call the collaboration
server makes — nothing else sees the content change anymore. It is queued as
a celery task, throttled like the other updates, so no indexation ever runs
in the process serving the request. A document whose content cannot be read
is left out of the batch rather than indexed empty, which would have erased
it from the search backend

CHANGELOG.md

@@ -133,6 +133,14 @@ and this project adheres to
   endpoint added above is its server-side replacement, backend wiring
   pending). The get-connections API is dropped for good: its only consumer
   was the removed can-edit mechanism, so it is not needed anymore
+- ♻️(backend) index the content of a document as the collaboration server has
+  it: the search indexer reads it with `YHubService`, and the indexation of an
+  edited document is triggered by the `content-updated` call the collaboration
+  server makes — nothing else sees the content change anymore. It is queued as
+  a celery task, throttled like the other updates, so no indexation ever runs
+  in the process serving the request. A document whose content cannot be read
+  is left out of the batch rather than indexed empty, which would have erased
+  it from the search backend
 - 🔥(backend) remove the unused `CollaborationService`
 - 💥(backend) remove the `documents/{id}/content/` endpoint
 - 💥(backend) remove the `documents/{id}/can-edit/` endpoint

src/backend/core/api/viewsets.py

@@ -70,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.mail import send_ask_for_access_mail
+from core.tasks.search import trigger_batch_document_indexer
 from core.utils.analytics import PosthogEventName, posthog_capture
 from core.utils.paths import filter_descendants
 from core.utils.treebeard import create_tree_node_with_retry
@@ -948,19 +949,28 @@ class DocumentViewSet(
         it would freeze. The collaboration server calls this once it persisted
         the changes of a document, at most once per debounce window.
 
-        The update is written without going through the model, saving it would
-        trigger a re-indexing of a content Django did not see change.
+        The new content is then read back from the collaboration server to
+        refresh the search index, in a task: this call is on the path of a
+        worker persisting a document, it only records what happened.
+
+        The update is written without going through the model: saving it would
+        index the document a second time, through the post_save signal.
         """
         try:
             document_id = uuid.UUID(kwargs["pk"])
         except ValueError as err:
             raise Http404 from err
 
+        updated_at = timezone.now()
         if not models.Document.objects.filter(pk=document_id).update(
-            updated_at=timezone.now()
+            updated_at=updated_at
         ):
             raise Http404
 
+        # Throttled like any other change: the collaboration server calls this
+        # once per debounce window, for as long as a document is being edited.
+        trigger_batch_document_indexer(document_id, updated_at)
+
         return drf_response.Response(status=status.HTTP_204_NO_CONTENT)
 
     @drf.decorators.action(detail=True, methods=["post"])

src/backend/core/services/search_indexers.py

@@ -14,9 +14,10 @@ import requests
 
 from core import models
 from core.enums import SearchType
+from core.services.yhub_services import YHubError, YHubService
 from core.utils.dicts import get_value_by_pattern
 from core.utils.paths import get_ancestor_to_descendants_map
-from core.utils.yjs import base64_yjs_to_text
+from core.utils.yjs import yjs_to_text
 
 logger = logging.getLogger(__name__)
 
@@ -157,11 +158,27 @@ class BaseDocumentIndexer(ABC):
             last_id = documents_batch[-1].id
             accesses_by_document_path = get_batch_accesses_by_users_and_teams(doc_paths)
 
-            serialized_batch = [
-                self.serialize_document(document, accesses_by_document_path)
-                for document in documents_batch
-                if document.content or document.title
-            ]
+            serialized_batch = []
+            for document in documents_batch:
+                try:
+                    content = self.get_document_content(document)
+                except YHubError:
+                    # A document whose content we could not read is left alone:
+                    # pushing it with an empty content would erase what the
+                    # search backend knows of it over a transient failure.
+                    logger.exception(
+                        "Document %s was not indexed, its content could not be "
+                        "read from the collaboration server",
+                        document.pk,
+                    )
+                    continue
+
+                if content or document.title:
+                    serialized_batch.append(
+                        self.serialize_document(
+                            document, content, accesses_by_document_path
+                        )
+                    )
 
             if serialized_batch:
                 self.push(serialized_batch)
@@ -169,11 +186,27 @@ class BaseDocumentIndexer(ABC):
 
         return count
 
+    @staticmethod
+    def get_document_content(document):
+        """
+        Return the text of a document, as the collaboration server has it.
+
+        The collaboration server owns the content of the documents, so it is
+        read from there and never from the database. A document it holds no
+        content for has none, and is indexed on its metadata alone.
+        """
+        update = YHubService().get_ydoc(document)
+
+        return yjs_to_text(update) if update else ""
+
     @abstractmethod
-    def serialize_document(self, document, accesses):
+    def serialize_document(self, document, content, accesses):
         """
         Convert a Document instance to a JSON-serializable format for indexing.
 
+        The content is passed in rather than read from the document: it is
+        fetched once, from the collaboration server, by `index`.
+
         Must be implemented by subclasses.
         """
 
@@ -308,25 +341,25 @@ class FindDocumentIndexer(BaseDocumentIndexer):
             return source["title"]
         return ""
 
-    def serialize_document(self, document, accesses):
+    def serialize_document(self, document, content, accesses):
         """
         Convert a Document to the JSON format expected by La Suite Find.
 
         Args:
             document (Document): The document instance.
+            content (str): The text of the document, as read from the
+                collaboration server.
             accesses (dict): Mapping of document ID to user/team access.
 
         Returns:
             dict: A JSON-serializable dictionary.
         """
         doc_path = document.path
-        doc_content = document.content
-        text_content = base64_yjs_to_text(doc_content) if doc_content else ""
 
         return {
             "id": str(document.id),
             "title": document.title or "",
-            "content": text_content,
+            "content": content,
             "depth": document.depth,
             "path": document.path,
             "numchild": document.numchild,
@@ -335,7 +368,7 @@ class FindDocumentIndexer(BaseDocumentIndexer):
             "users": list(accesses.get(doc_path, {}).get("users", set())),
             "groups": list(accesses.get(doc_path, {}).get("teams", set())),
             "reach": document.computed_link_reach,
-            "size": len(text_content.encode("utf-8")),
+            "size": len(content.encode("utf-8")),
             "is_active": not bool(document.ancestors_deleted_at),
         }
 

src/backend/core/signals.py

@@ -21,7 +21,9 @@ def document_post_save(sender, instance, **kwargs):  # pylint: disable=unused-ar
     Note : Within the transaction we can have an empty content and a serialization
     error.
     """
-    transaction.on_commit(partial(trigger_batch_document_indexer, instance))
+    transaction.on_commit(
+        partial(trigger_batch_document_indexer, instance.pk, instance.updated_at)
+    )
 
 
 @receiver(signals.post_save, sender=models.DocumentAccess)
@@ -31,8 +33,9 @@ def document_access_post_save(sender, instance, created, **kwargs):  # pylint: d
     Clear cache for the affected user.
     """
     if not created:
+        document = instance.document
         transaction.on_commit(
-            partial(trigger_batch_document_indexer, instance.document)
+            partial(trigger_batch_document_indexer, document.pk, document.updated_at)
         )
 
     # Invalidate cache for the user

src/backend/core/tasks/search.py

@@ -63,12 +63,13 @@ def batch_document_indexer_task(timestamp):
         logger.info("Indexed %d documents", count)
 
 
-def trigger_batch_document_indexer(document):
+def trigger_batch_document_indexer(document_id, updated_at):
     """
     Trigger indexation task with debounce a delay set by the SEARCH_INDEXER_COUNTDOWN setting.
 
     Args:
-        document (Document): The document instance.
+        document_id (UUID): The id of the document that changed.
+        updated_at (datetime): When it changed, the horizon of the batch.
     """
     countdown = int(settings.SEARCH_INDEXER_COUNTDOWN)
 
@@ -82,14 +83,17 @@ def trigger_batch_document_indexer(document):
         if batch_indexer_throttle_acquire(timeout=countdown):
             logger.info(
                 "Add task for batch document indexation from updated_at=%s in %d seconds",
-                document.updated_at.isoformat(),
+                updated_at.isoformat(),
                 countdown,
             )
 
             batch_document_indexer_task.apply_async(
-                args=[document.updated_at], countdown=countdown
+                args=[updated_at], countdown=countdown
             )
         else:
-            logger.info("Skip task for batch document %s indexation", document.pk)
+            logger.info("Skip task for batch document %s indexation", document_id)
     else:
-        document_indexer_task.apply(args=[document.pk])
+        # Indexing reads the content of the document from the collaboration
+        # server and pushes it to the search backend: never in the process
+        # asking for it.
+        document_indexer_task.delay(document_id)

src/backend/core/tests/commands/test_index.py

@@ -12,6 +12,11 @@ import pytest
 
 from core import factories
 from core.services.search_indexers import FindDocumentIndexer
+from core.utils.yjs import base64_yjs_to_text
+
+# what the fake collaboration server of the indexer_settings fixture serves
+# for a document created with the content of the factory
+CONTENT = base64_yjs_to_text(factories.YDOC_HELLO_WORLD_BASE64)
 
 
 @pytest.mark.django_db
@@ -46,8 +51,8 @@ def test_index():
 
         assert sorted(push_call_args[0], key=itemgetter("id")) == sorted(
             [
-                indexer.serialize_document(doc, accesses),
-                indexer.serialize_document(no_title_doc, accesses),
+                indexer.serialize_document(doc, CONTENT, accesses),
+                indexer.serialize_document(no_title_doc, CONTENT, accesses),
             ],
             key=itemgetter("id"),
         )

src/backend/core/tests/conftest.py

@@ -9,6 +9,7 @@ import pytest
 import responses
 
 from core import factories
+from core.services.yhub_services import YHubService
 from core.tests.utils.urls import reload_urls
 
 USER = "user"
@@ -35,6 +36,11 @@ def mock_user_teams():
 def indexer_settings_fixture(settings):
     """
     Setup valid settings for the document indexer. Clear the indexer cache.
+
+    The indexer reads the content of a document from the collaboration server,
+    which is faked here: it serves what the factories wrote in the database, so
+    a document built with `content=""` is one the collaboration server holds no
+    content for.
     """
 
     # pylint: disable-next=import-outside-toplevel
@@ -50,7 +56,14 @@ def indexer_settings_fixture(settings):
     settings.SEARCH_URL = "http://localhost:8081/api/v1.0/documents/search/"
     settings.SEARCH_INDEXER_COUNTDOWN = 1
 
-    yield settings
+    def get_ydoc(_service, document):
+        """Answer the raw update the collaboration server would serve."""
+        return base64.b64decode(document.content) if document.content else None
+
+    with mock.patch.object(
+        YHubService, "get_ydoc", autospec=True, side_effect=get_ydoc
+    ):
+        yield settings
 
     # clear cache to prevent issues with other tests
     get_document_indexer.cache_clear()

src/backend/core/tests/documents/test_api_documents_content_updated.py

@@ -4,6 +4,7 @@ Tests for Documents API endpoint in impress's core app: content updated
 
 from datetime import datetime, timedelta
 from datetime import timezone as tz
+from unittest import mock
 from uuid import uuid4
 
 import jwt
@@ -15,7 +16,9 @@ from rest_framework.test import APIClient
 from core import factories
 from core.authentication import CollaborationServerAuthentication
 from core.models import Document
+from core.services.search_indexers import FindDocumentIndexer
 from core.tests.utils.jwt_helper import build_jwks, generate_key_pair, key_id
+from core.utils.yjs import base64_yjs_to_text
 
 pytestmark = pytest.mark.django_db
 
@@ -34,9 +37,9 @@ def yhub_jwks_fixture(settings):
     """
     settings.YHUB_API_BASE_URL = "http://yhub:3002"
 
-    with responses.RequestsMock(assert_all_requests_are_fired=False) as mock:
-        mock.get(JWKS_URL, json=build_jwks(PUBLIC_KEY))
-        yield mock
+    with responses.RequestsMock(assert_all_requests_are_fired=False) as jwks:
+        jwks.get(JWKS_URL, json=build_jwks(PUBLIC_KEY))
+        yield jwks
 
 
 def collaboration_token(private_key=PRIVATE_KEY, public_key=PUBLIC_KEY, **claims):
@@ -237,6 +240,42 @@ def test_api_documents_content_updated():
     assert document.title == "my document"
 
 
+@pytest.mark.usefixtures("indexer_settings")
+def test_api_documents_content_updated_indexes_the_document():
+    """
+    The content changed on the collaboration server: the search index follows.
+
+    Nothing else refreshes it anymore, the content does not go through Django.
+    """
+    document = factories.DocumentFactory(title="my document")
+
+    with mock.patch.object(FindDocumentIndexer, "push") as mock_push:
+        response = APIClient().post(
+            f"/api/v1.0/documents/{document.id!s}/content-updated/",
+            HTTP_AUTHORIZATION=f"Bearer {collaboration_token()}",
+        )
+
+    assert response.status_code == 204
+    # the task reads the content back from the collaboration server
+    indexed = {doc["id"]: doc for doc in mock_push.call_args[0][0]}
+    assert indexed[str(document.id)]["content"] == base64_yjs_to_text(
+        factories.YDOC_HELLO_WORLD_BASE64
+    )
+
+
+@pytest.mark.usefixtures("indexer_settings")
+def test_api_documents_content_updated_does_not_index_when_the_document_is_unknown():
+    """A document that does not exist is not worth an indexation task."""
+    with mock.patch("core.api.viewsets.trigger_batch_document_indexer") as mock_trigger:
+        response = APIClient().post(
+            f"/api/v1.0/documents/{uuid4()!s}/content-updated/",
+            HTTP_AUTHORIZATION=f"Bearer {collaboration_token()}",
+        )
+
+    assert response.status_code == 404
+    mock_trigger.assert_not_called()
+
+
 def test_api_documents_content_updated_restricted_document():
     """
     The collaboration server acts for whoever is editing, the access of a

src/backend/core/tests/test_services_find_document_indexer.py

@@ -14,9 +14,14 @@ import pytest
 from core import factories, models
 from core.enums import SearchType
 from core.services.search_indexers import FindDocumentIndexer
+from core.utils.yjs import base64_yjs_to_text
 
 pytestmark = pytest.mark.django_db
 
+# The documents of this module all carry the content of the factory, which the
+# fake collaboration server of the indexer_settings fixture serves back.
+CONTENT = base64_yjs_to_text(factories.YDOC_HELLO_WORLD_BASE64)
+
 
 def reset_batch_indexer_throttle():
     """Reset throttle flag"""
@@ -49,9 +54,9 @@ def test_models_documents_post_save_indexer(mock_push):
     # One call
     assert sorted(data[0], key=itemgetter("id")) == sorted(
         [
-            indexer.serialize_document(doc1, accesses),
-            indexer.serialize_document(doc2, accesses),
-            indexer.serialize_document(doc3, accesses),
+            indexer.serialize_document(doc1, CONTENT, accesses),
+            indexer.serialize_document(doc2, CONTENT, accesses),
+            indexer.serialize_document(doc3, CONTENT, accesses),
         ],
         key=itemgetter("id"),
     )
@@ -81,9 +86,9 @@ def test_models_documents_post_save_indexer_no_batches(indexer_settings):
     # all documents are indexed
     assert sorted([d[0] for d in data], key=itemgetter("id")) == sorted(
         [
-            indexer.serialize_document(doc1, accesses),
-            indexer.serialize_document(doc2, accesses),
-            indexer.serialize_document(doc3, accesses),
+            indexer.serialize_document(doc1, CONTENT, accesses),
+            indexer.serialize_document(doc2, CONTENT, accesses),
+            indexer.serialize_document(doc3, CONTENT, accesses),
         ],
         key=itemgetter("id"),
     )
@@ -151,9 +156,9 @@ def test_models_documents_post_save_indexer_with_accesses(mock_push):
     assert len(data) == 1
     assert sorted(data[0], key=itemgetter("id")) == sorted(
         [
-            indexer.serialize_document(doc1, accesses),
-            indexer.serialize_document(doc2, accesses),
-            indexer.serialize_document(doc3, accesses),
+            indexer.serialize_document(doc1, CONTENT, accesses),
+            indexer.serialize_document(doc2, CONTENT, accesses),
+            indexer.serialize_document(doc3, CONTENT, accesses),
         ],
         key=itemgetter("id"),
     )
@@ -215,9 +220,9 @@ def test_models_documents_post_save_indexer_deleted(mock_push):
     # First indexation on document creation
     assert sorted(data[0], key=itemgetter("id")) == sorted(
         [
-            indexer.serialize_document(doc, accesses),
-            indexer.serialize_document(main_doc, accesses),
-            indexer.serialize_document(child_doc, accesses),
+            indexer.serialize_document(doc, CONTENT, accesses),
+            indexer.serialize_document(main_doc, CONTENT, accesses),
+            indexer.serialize_document(child_doc, CONTENT, accesses),
         ],
         key=itemgetter("id"),
     )
@@ -225,8 +230,10 @@ def test_models_documents_post_save_indexer_deleted(mock_push):
     # Even deleted items are re-indexed : only update their status in the future
     assert sorted(data[1], key=itemgetter("id")) == sorted(
         [
-            indexer.serialize_document(main_doc_deleted, accesses),  # soft_delete()
-            indexer.serialize_document(child_doc_deleted, accesses),
+            indexer.serialize_document(
+                main_doc_deleted, CONTENT, accesses
+            ),  # soft_delete()
+            indexer.serialize_document(child_doc_deleted, CONTENT, accesses),
         ],
         key=itemgetter("id"),
     )
@@ -317,9 +324,9 @@ def test_models_documents_post_save_indexer_restored(mock_push):
     # First indexation on items creation & soft delete (in the same transaction)
     assert sorted(data[0], key=itemgetter("id")) == sorted(
         [
-            indexer.serialize_document(doc, accesses),
-            indexer.serialize_document(doc_deleted, accesses),
-            indexer.serialize_document(doc_ancestor_deleted, accesses),
+            indexer.serialize_document(doc, CONTENT, accesses),
+            indexer.serialize_document(doc_deleted, CONTENT, accesses),
+            indexer.serialize_document(doc_ancestor_deleted, CONTENT, accesses),
         ],
         key=itemgetter("id"),
     )
@@ -327,8 +334,8 @@ def test_models_documents_post_save_indexer_restored(mock_push):
     # Restored items are re-indexed : only update their status in the future
     assert sorted(data[1], key=itemgetter("id")) == sorted(
         [
-            indexer.serialize_document(doc_restored, accesses),  # restore()
-            indexer.serialize_document(doc_ancestor_restored, accesses),
+            indexer.serialize_document(doc_restored, CONTENT, accesses),  # restore()
+            indexer.serialize_document(doc_ancestor_restored, CONTENT, accesses),
         ],
         key=itemgetter("id"),
     )
@@ -376,9 +383,9 @@ def test_models_documents_post_save_indexer_throttle():
 
         assert sorted(data[0], key=itemgetter("id")) == sorted(
             [
-                indexer.serialize_document(docs[0], accesses),
-                indexer.serialize_document(docs[2], accesses),
-                indexer.serialize_document(docs[3], accesses),
+                indexer.serialize_document(docs[0], CONTENT, accesses),
+                indexer.serialize_document(docs[2], CONTENT, accesses),
+                indexer.serialize_document(docs[3], CONTENT, accesses),
             ],
             key=itemgetter("id"),
         )

src/backend/core/tests/test_services_search_indexers.py

@@ -1,5 +1,6 @@
 """Tests for Documents search indexers"""
 
+from base64 import b64decode
 from functools import partial
 from json import dumps as json_dumps
 from unittest.mock import patch
@@ -19,6 +20,7 @@ from core.services.search_indexers import (
     get_document_indexer,
     get_visited_document_ids_of,
 )
+from core.services.yhub_services import ServiceUnavailableError, YHubService
 from core.utils.yjs import base64_yjs_to_text
 
 pytestmark = pytest.mark.django_db
@@ -27,7 +29,7 @@ pytestmark = pytest.mark.django_db
 class FakeDocumentIndexer(BaseDocumentIndexer):
     """Fake indexer for test purpose"""
 
-    def serialize_document(self, document, accesses):
+    def serialize_document(self, document, content, accesses):
         return {}
 
     def push(self, data):
@@ -190,7 +192,9 @@ def test_services_search_indexers_serialize_document_returns_expected_json():
     }
 
     indexer = FindDocumentIndexer()
-    result = indexer.serialize_document(document, accesses)
+    # the content is read from the collaboration server and passed in, the
+    # serialization never reaches for it itself
+    result = indexer.serialize_document(document, "Hello world", accesses)
 
     assert set(result.pop("users")) == {str(user_a.sub), str(user_b.sub)}
     assert set(result.pop("groups")) == {"team1", "team2"}
@@ -200,11 +204,11 @@ def test_services_search_indexers_serialize_document_returns_expected_json():
         "depth": 1,
         "path": document.path,
         "numchild": 1,
-        "content": base64_yjs_to_text(document.content),
+        "content": "Hello world",
         "created_at": document.created_at.isoformat(),
         "updated_at": document.updated_at.isoformat(),
         "reach": document.link_reach,
-        "size": 13,
+        "size": 11,
         "is_active": True,
     }
 
@@ -219,7 +223,7 @@ def test_services_search_indexers_serialize_document_deleted():
     document.refresh_from_db()
 
     indexer = FindDocumentIndexer()
-    result = indexer.serialize_document(document, {})
+    result = indexer.serialize_document(document, "", {})
 
     assert result["is_active"] is False
 
@@ -230,7 +234,7 @@ def test_services_search_indexers_serialize_document_empty():
     document = factories.DocumentFactory(content="", title=None)
 
     indexer = FindDocumentIndexer()
-    result = indexer.serialize_document(document, {})
+    result = indexer.serialize_document(document, "", {})
 
     assert result["content"] == ""
     assert result["title"] == ""
@@ -334,6 +338,48 @@ def test_services_search_indexers_batch_size_argument(mock_push):
     assert seen_doc_ids == {str(d.id) for d in documents}
 
 
+@patch.object(FindDocumentIndexer, "push")
+@pytest.mark.usefixtures("indexer_settings")
+def test_services_search_indexers_index_the_content_of_the_collaboration_server(
+    mock_push,
+):
+    """The indexed content is the one the collaboration server holds."""
+    document = factories.DocumentFactory()
+
+    assert FindDocumentIndexer().index() == 1
+
+    indexed = mock_push.call_args[0][0][0]
+    assert indexed["id"] == str(document.id)
+    assert indexed["content"] == base64_yjs_to_text(factories.YDOC_HELLO_WORLD_BASE64)
+
+
+@patch.object(FindDocumentIndexer, "push")
+@pytest.mark.usefixtures("indexer_settings")
+def test_services_search_indexers_skip_documents_the_content_of_which_is_unreadable(
+    mock_push,
+):
+    """
+    A document whose content cannot be read is left out of the batch.
+
+    Indexing it with an empty content would erase what the search backend knows
+    of it, on nothing more than a collaboration server hiccup.
+    """
+    unreadable, readable = factories.DocumentFactory.create_batch(2)
+
+    def get_ydoc(_service, document):
+        if document.pk == unreadable.pk:
+            raise ServiceUnavailableError("yhub is unreachable")
+        return b64decode(document.content)
+
+    # a plain replacement: the indexer_settings fixture already serves the
+    # content of the documents, this test needs one of them to fail
+    with patch.object(YHubService, "get_ydoc", get_ydoc):
+        assert FindDocumentIndexer().index() == 1
+
+    results = {doc["id"] for doc in mock_push.call_args[0][0]}
+    assert results == {str(readable.id)}
+
+
 @patch.object(FindDocumentIndexer, "push")
 @pytest.mark.usefixtures("indexer_settings")
 def test_services_search_indexers_ignore_empty_documents(mock_push):

src/backend/core/utils/yjs.py

@@ -23,12 +23,17 @@ def base64_yjs_to_xml(base64_string):
     return yjs_to_xml(base64.b64decode(base64_string))
 
 
+def yjs_to_text(update):
+    """Extract text from a raw yjs update."""
+
+    soup = BeautifulSoup(yjs_to_xml(update), "lxml-xml")
+    return soup.get_text(separator=" ", strip=True)
+
+
 def base64_yjs_to_text(base64_string):
     """Extract text from base64 yjs document."""
 
-    blocknote_structure = base64_yjs_to_xml(base64_string)
-    soup = BeautifulSoup(blocknote_structure, "lxml-xml")
-    return soup.get_text(separator=" ", strip=True)
+    return yjs_to_text(base64.b64decode(base64_string))
 
 
 def extract_attachments(content):