WIP ✨(backend) improve search filtering & ordering

Filter deleted documents from visited ones.
Add ordering to the Find API search call.
BaseDocumentIndexer.search now returns a list of document ids instead of models.
Do not call the indexer in signals when SEARCH_INDEXER_CLASS is not defined
or properly configured.

Signed-off-by: Fabre Florian <[email protected]>

Author: joehybird

src/backend/core/api/serializers.py

@@ -827,3 +827,7 @@ class FindDocumentSerializer(serializers.Serializer):
     """Serializer for Find search requests"""
 
     q = serializers.CharField(required=True, allow_blank=False, trim_whitespace=True)
+    page_size = serializers.IntegerField(
+        required=False, min_value=1, max_value=50, default=20
+    )
+    page = serializers.IntegerField(required=False, min_value=1, default=1)

src/backend/core/api/viewsets.py

@@ -49,7 +49,10 @@
 from core.services.converter_services import (
     YdocConverter,
 )
-from core.services.search_indexers import get_document_indexer_class
+from core.services.search_indexers import (
+    get_document_indexer_class,
+    get_visited_document_ids_of,
+)
 from core.tasks.mail import send_ask_for_access_mail
 from core.utils import extract_attachments, filter_descendants
 
@@ -1042,23 +1045,37 @@ def search(self, request, *args, **kwargs):
         The filtering allows full text search through the opensearch indexation app "find".
         """
         access_token = request.session.get("oidc_access_token")
+        user = request.user
 
         serializer = serializers.FindDocumentSerializer(data=request.query_params)
         serializer.is_valid(raise_exception=True)
 
+        queryset = self.get_queryset()
+
         try:
             indexer = get_document_indexer_class()()
-            queryset = indexer.search(
+            ids = indexer.search(
                 text=serializer.validated_data.get("q", ""),
-                user=request.user,
+                user=user,
                 token=access_token,
+                visited=get_visited_document_ids_of(queryset, user),
+                page_number=serializer.validated_data.get("page"),
+                page_size=serializer.validated_data.get("page_size"),
             )
         except ImproperlyConfigured:
             return drf.response.Response(
                 {"detail": "The service is not properly configured."},
-                status=status.HTTP_401_UNAUTHORIZED,
+                status=status.HTTP_500_INTERNAL_SERVER_ERROR,
             )
 
+        queryset = queryset.filter(pk__in=ids)
+        queryset = queryset.annotate_user_roles(user)
+
+        # Apply ordering only now that everything is filtered and annotated
+        queryset = filters.OrderingFilter().filter_queryset(
+            self.request, queryset, self
+        )
+
         return self.get_response_for_queryset(
             queryset,
             context={

src/backend/core/services/search_indexers.py

@@ -8,6 +8,7 @@
 from django.conf import settings
 from django.contrib.auth.models import AnonymousUser
 from django.core.exceptions import ImproperlyConfigured
+from django.db.models import Subquery
 from django.utils.module_loading import import_string
 
 import requests
@@ -17,6 +18,17 @@
 logger = logging.getLogger(__name__)
 
 
+@cache
+def is_document_indexer_configured() -> bool:
+    """Returns true if the indexer service is enabled and properly configured."""
+    try:
+        get_document_indexer_class()
+    except ImproperlyConfigured:
+        return False
+
+    return True
+
+
 @cache
 def get_document_indexer_class() -> "BaseDocumentIndexer":
     """Return the indexer backend class based on the settings."""
@@ -65,7 +77,7 @@ def get_batch_accesses_by_users_and_teams(paths):
     return dict(access_by_document_path)
 
 
-def get_visited_document_ids_of(user):
+def get_visited_document_ids_of(queryset, user):
     """
     Returns the ids of the documents that have a linktrace to the user and NOT owned.
     It will be use to limit the opensearch responses to the public documents already
@@ -74,11 +86,18 @@ def get_visited_document_ids_of(user):
     if isinstance(user, AnonymousUser):
         return []
 
-    qs = models.LinkTrace.objects.filter(user=user).exclude(
-        document__accesses__user=user,
+    qs = models.LinkTrace.objects.filter(user=user)
+
+    docs = (
+        queryset.exclude(accesses__user=user)
+        .filter(
+            deleted_at__isnull=True,
+            ancestors_deleted_at__isnull=True,
+        )
+        .filter(pk__in=Subquery(qs.values("document_id")))
     )
 
-    return list({str(id) for id in qs.values_list("document_id", flat=True)})
+    return list({str(id) for id in docs.values_list("pk", flat=True)})
 
 
 class BaseDocumentIndexer(ABC):
@@ -159,22 +178,23 @@ def push(self, data):
         Must be implemented by subclasses.
         """
 
-    def search(self, text, user, token):
+    # pylint: disable-next=too-many-arguments,too-many-positional-arguments
+    def search(self, text, token, visited=(), page=1, page_size=50):
         """
         Search for documents in Find app.
         """
-        visited_ids = get_visited_document_ids_of(user)
-
         response = self.search_query(
             data={
                 "q": text,
-                "visited": visited_ids,
+                "visited": visited,
                 "services": ["docs"],
+                "page_number": page,
+                "page_size": page_size,
             },
             token=token,
         )
 
-        return self.format_response(response)
+        return [d["_id"] for d in response]
 
     @abstractmethod
     def search_query(self, data, token) -> dict:
@@ -184,14 +204,6 @@ def search_query(self, data, token) -> dict:
         Must be implemented by subclasses.
         """
 
-    @abstractmethod
-    def format_response(self, data: dict):
-        """
-        Convert the JSON response from Find app as document queryset.
-
-        Must be implemented by subclasses.
-        """
-
 
 class FindDocumentIndexer(BaseDocumentIndexer):
     """
@@ -253,12 +265,6 @@ def search_query(self, data, token) -> requests.Response:
             logger.error("HTTPError: %s", e)
             raise
 
-    def format_response(self, data: dict):
-        """
-        Retrieve documents ids from Find app response and return a queryset.
-        """
-        return models.Document.objects.filter(pk__in=[d["_id"] for d in data])
-
     def push(self, data):
         """
         Push a batch of documents to the Find backend.

src/backend/core/signals.py

@@ -6,6 +6,7 @@
 from django.dispatch import receiver
 
 from . import models
+from .services.search_indexers import is_document_indexer_configured
 from .tasks.find import trigger_document_indexer
 
 
@@ -16,13 +17,14 @@ 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.
     """
-    trigger_document_indexer(instance, on_commit=True)
+    if is_document_indexer_configured():
+        trigger_document_indexer(instance, on_commit=True)
 
 
 @receiver(signals.post_save, sender=models.DocumentAccess)
 def document_access_post_save(sender, instance, created, **kwargs):  # pylint: disable=unused-argument
     """
     Asynchronous call to the document indexer at the end of the transaction.
     """
-    if not created:
+    if not created and is_document_indexer_configured():
         trigger_document_indexer(instance.document, on_commit=True)

src/backend/core/tasks/find.py

@@ -72,15 +72,14 @@ def trigger_document_indexer(document, on_commit=False):
         on_commit (bool): Wait for the end of the transaction before starting the task
             (some fields may be in wrong state within the transaction)
     """
-
-    if document.deleted_at or document.ancestors_deleted_at:
-        pass
-
     if on_commit:
         transaction.on_commit(
             partial(trigger_document_indexer, document, on_commit=False)
         )
     else:
+        if document.deleted_at or document.ancestors_deleted_at:
+            return
+
         key = document_indexer_debounce_key(document.pk)
         countdown = getattr(settings, "SEARCH_INDEXER_COUNTDOWN", 1)
 

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

@@ -140,3 +140,6 @@ def test_api_documents_search_format(settings):
         "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"),
         "user_role": access.role,
     }
+
+
+# TODO : add pagination tests

src/backend/core/tests/test_models_documents.py

@@ -1460,6 +1460,59 @@ def test_models_documents_post_save_indexer(mock_push, settings):
     assert cache.get(document_indexer_debounce_key(doc3.pk)) == 0
 
 
+@mock.patch.object(FindDocumentIndexer, "push")
+@pytest.mark.django_db(transaction=True)
+def test_models_documents_post_save_indexer_deleted(mock_push, settings):
+    """Skip indexation task on deleted or ancestor_deleted documents"""
+    settings.SEARCH_INDEXER_COUNTDOWN = 0
+
+    user = factories.UserFactory()
+
+    with transaction.atomic():
+        doc = factories.DocumentFactory()
+        doc_deleted = factories.DocumentFactory()
+        doc_ancestor_deleted = factories.DocumentFactory(parent=doc_deleted)
+        doc_deleted.soft_delete()
+        doc_ancestor_deleted.ancestors_deleted_at = doc_deleted.deleted_at
+
+        factories.UserDocumentAccessFactory(document=doc, user=user)
+        factories.UserDocumentAccessFactory(document=doc_deleted, user=user)
+        factories.UserDocumentAccessFactory(document=doc_ancestor_deleted, user=user)
+
+    doc_deleted.refresh_from_db()
+    doc_ancestor_deleted.refresh_from_db()
+
+    assert doc_deleted.deleted_at is not None
+    assert doc_deleted.ancestors_deleted_at is not None
+
+    assert doc_ancestor_deleted.deleted_at is None
+    assert doc_ancestor_deleted.ancestors_deleted_at is not None
+
+    time.sleep(0.2)  # waits for the end of the tasks
+
+    accesses = {
+        str(doc.path): {"users": [user.sub]},
+        str(doc_deleted.path): {"users": [user.sub]},
+        str(doc_ancestor_deleted.path): {"users": [user.sub]},
+    }
+
+    data = [call.args[0] for call in mock_push.call_args_list]
+
+    indexer = FindDocumentIndexer()
+
+    # Only the not deleted document is indexed
+    assert data == [
+        indexer.serialize_document(doc, accesses),
+    ]
+
+    # The debounce counters should be reset
+    assert cache.get(document_indexer_debounce_key(doc.pk)) == 0
+
+    # These caches are not filled
+    assert cache.get(document_indexer_debounce_key(doc_deleted.pk)) is None
+    assert cache.get(document_indexer_debounce_key(doc_ancestor_deleted.pk)) is None
+
+
 @pytest.mark.django_db(transaction=True)
 def test_models_documents_post_save_indexer_debounce(settings):
     """Test indexation task skipping on document update"""