WIP ✨(backend) fixup & improve tests

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

Author: joehybird

src/backend/core/api/viewsets.py

@@ -50,7 +50,7 @@
     YdocConverter,
 )
 from core.services.search_indexers import (
-    default_document_indexer,
+    get_document_indexer,
     get_visited_document_ids_of,
 )
 from core.tasks.mail import send_ask_for_access_mail
@@ -1042,21 +1042,30 @@ def duplicate(self, request, *args, **kwargs):
     def search(self, request, *args, **kwargs):
         """
         Returns a DRF response containing the filtered, annotated and ordered document list.
-        The filtering allows full text search through the opensearch indexation app "find".
+
+        Applies filtering based on request parameter 'q' from `FindDocumentSerializer`.
+        Depending of the configuration it can be:
+         - A fulltext search through the opensearch indexation app "find" if the backend is
+           enabled (see SEARCH_BACKEND_CLASS)
+         - A filtering by the model field 'title'.
+
+        The ordering is always by the most recent first.
         """
         access_token = request.session.get("oidc_access_token")
         user = request.user
 
         serializer = serializers.FindDocumentSerializer(data=request.query_params)
         serializer.is_valid(raise_exception=True)
 
-        indexer = default_document_indexer()
+        indexer = get_document_indexer()
+        text = serializer.validated_data["q"]
 
+        # The indexer is not configured, so we fallback on a simple filter on the
+        # model field 'title'.
         if not indexer:
+            # As the 'list' view we get a prefiltered queryset (deleted docs are excluded)
             queryset = self.get_queryset()
-            filterset = DocumentFilter(
-                {"title": serializer.validated_data.get("q", "")}, queryset=queryset
-            )
+            filterset = DocumentFilter({"title": text}, queryset=queryset)
 
             if not filterset.is_valid():
                 raise drf.exceptions.ValidationError(filterset.errors)
@@ -1071,15 +1080,17 @@ def search(self, request, *args, **kwargs):
             )
 
         queryset = models.Document.objects.all()
+
+        # Retrieve the documents ids from Find.
         results = indexer.search(
-            text=serializer.validated_data.get("q", ""),
+            text=text,
             token=access_token,
             visited=get_visited_document_ids_of(queryset, user),
             page=serializer.validated_data.get("page", 1),
             page_size=serializer.validated_data.get("page_size", 20),
         )
 
-        queryset = queryset.filter(pk__in=results)
+        queryset = queryset.filter(pk__in=results).order_by("-updated_at")
 
         return self.get_response_for_queryset(
             queryset,

src/backend/core/services/search_indexers.py

@@ -19,37 +19,24 @@
 
 
 @cache
-def default_document_indexer():
-    """Returns default indexer service is enabled and properly configured."""
+def get_document_indexer():
+    """Returns an instance of indexer service if enabled and properly configured."""
+    classpath = settings.SEARCH_INDEXER_CLASS
 
     # For this usecase an empty indexer class is not an issue but a feature.
-    if not getattr(settings, "SEARCH_INDEXER_CLASS", None):
+    if not classpath:
         logger.info("Document indexer is not configured (see SEARCH_INDEXER_CLASS)")
         return None
 
     try:
-        return get_document_indexer_class()()
+        indexer_class = import_string(settings.SEARCH_INDEXER_CLASS)
+        return indexer_class()
+    except ImportError as err:
+        logger.error("SEARCH_INDEXER_CLASS setting is not valid : %s", err)
     except ImproperlyConfigured as err:
         logger.error("Document indexer is not properly configured : %s", err)
-        return None
 
-
-@cache
-def get_document_indexer_class():
-    """Return the indexer backend class based on the settings."""
-    classpath = settings.SEARCH_INDEXER_CLASS
-
-    if not classpath:
-        raise ImproperlyConfigured(
-            "SEARCH_INDEXER_CLASS must be set in Django settings."
-        )
-
-    try:
-        return import_string(settings.SEARCH_INDEXER_CLASS)
-    except ImportError as err:
-        raise ImproperlyConfigured(
-            f"SEARCH_INDEXER_CLASS setting is not valid : {err}"
-        ) from err
+    return None
 
 
 def get_batch_accesses_by_users_and_teams(paths):
@@ -100,9 +87,11 @@ def get_visited_document_ids_of(queryset, user):
             ancestors_deleted_at__isnull=True,
         )
         .filter(pk__in=Subquery(qs.values("document_id")))
+        .order_by("pk")
+        .distinct("pk")
     )
 
-    return list({str(id) for id in docs.values_list("pk", flat=True)})
+    return [str(id) for id in docs.values_list("pk", flat=True)]
 
 
 class BaseDocumentIndexer(ABC):

src/backend/core/signals.py

@@ -9,7 +9,6 @@
 from django.dispatch import receiver
 
 from . import models
-from .services.search_indexers import default_document_indexer
 from .tasks.find import trigger_document_indexer
 
 
@@ -20,14 +19,13 @@ 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.
     """
-    if default_document_indexer() is not None:
-        transaction.on_commit(partial(trigger_document_indexer, instance))
+    transaction.on_commit(partial(trigger_document_indexer, instance))
 
 
 @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 and default_document_indexer() is not None:
+    if not created:
         transaction.on_commit(partial(trigger_document_indexer, instance.document))

src/backend/core/tasks/find.py

@@ -10,22 +10,21 @@
 logger = getLogger(__file__)
 
 
-def document_indexer_debounce_key(document_id):
-    """Returns debounce cache key"""
-    return f"doc-indexer-debounce-{document_id}"
-
-
-def incr_counter(key):
+def indexer_debounce_lock(document_id):
     """Increase or reset counter"""
+    key = f"doc-indexer-debounce-{document_id}"
+
     try:
         return cache.incr(key)
     except ValueError:
         cache.set(key, 1)
         return 1
 
 
-def decr_counter(key):
+def indexer_debounce_release(document_id):
     """Decrease or reset counter"""
+    key = f"doc-indexer-debounce-{document_id}"
+
     try:
         return cache.decr(key)
     except ValueError:
@@ -36,24 +35,26 @@ def decr_counter(key):
 @app.task
 def document_indexer_task(document_id):
     """Celery Task : Sends indexation query for a document."""
-    key = document_indexer_debounce_key(document_id)
+    # Prevents some circular imports
+    # pylint: disable=import-outside-toplevel
+    from core import models  # noqa : PLC0415
+    from core.services.search_indexers import (  # noqa : PLC0415
+        get_batch_accesses_by_users_and_teams,
+        get_document_indexer,
+    )
 
     # check if the counter : if still up, skip the task. only the last one
     # within the countdown delay will do the query.
-    if decr_counter(key) > 0:
+    if indexer_debounce_release(document_id) > 0:
         logger.info("Skip document %s indexation", document_id)
         return
 
-    # Prevents some circular imports
-    # pylint: disable=import-outside-toplevel
-    from core import models  # noqa: PLC0415
-    from core.services.search_indexers import (  # noqa: PLC0415
-        get_batch_accesses_by_users_and_teams,
-        get_document_indexer_class,
-    )
+    indexer = get_document_indexer()
+
+    if indexer is None:
+        return
 
     doc = models.Document.objects.get(pk=document_id)
-    indexer = get_document_indexer_class()()
     accesses = get_batch_accesses_by_users_and_teams((doc.path,))
 
     data = indexer.serialize_document(document=doc, accesses=accesses)
@@ -69,11 +70,7 @@ def trigger_document_indexer(document):
     Args:
         document (Document): The document instance.
     """
-    if document.deleted_at or document.ancestors_deleted_at:
-        return
-
-    key = document_indexer_debounce_key(document.pk)
-    countdown = getattr(settings, "SEARCH_INDEXER_COUNTDOWN", 1)
+    countdown = settings.SEARCH_INDEXER_COUNTDOWN
 
     logger.info(
         "Add task for document %s indexation in %.2f seconds",
@@ -83,6 +80,6 @@ def trigger_document_indexer(document):
 
     # Each time this method is called during the countdown, we increment the
     # counter and each task decrease it, so the index be run only once.
-    incr_counter(key)
+    indexer_debounce_lock(document.pk)
 
     document_indexer_task.apply_async(args=[document.pk], countdown=countdown)

src/backend/core/tests/conftest.py

@@ -34,12 +34,10 @@ def indexer_settings_fixture(settings):
 
     # pylint: disable-next=import-outside-toplevel
     from core.services.search_indexers import (  # noqa: PLC0415
-        default_document_indexer,
-        get_document_indexer_class,
+        get_document_indexer,
     )
 
-    default_document_indexer.cache_clear()
-    get_document_indexer_class.cache_clear()
+    get_document_indexer.cache_clear()
 
     settings.SEARCH_INDEXER_CLASS = "core.services.search_indexers.FindDocumentIndexer"
     settings.SEARCH_INDEXER_SECRET = "ThisIsAKeyForTest"
@@ -51,5 +49,4 @@ def indexer_settings_fixture(settings):
     yield settings
 
     # clear cache to prevent issues with other tests
-    default_document_indexer.cache_clear()
-    get_document_indexer_class.cache_clear()
+    get_document_indexer.cache_clear()

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

@@ -10,7 +10,7 @@
 from rest_framework.test import APIClient
 
 from core import factories, models
-from core.services.search_indexers import default_document_indexer
+from core.services.search_indexers import get_document_indexer
 
 fake = Faker()
 pytestmark = pytest.mark.django_db
@@ -54,7 +54,7 @@ def test_api_documents_search_endpoint_is_none(indexer_settings):
     """
     indexer_settings.SEARCH_INDEXER_QUERY_URL = None
 
-    assert default_document_indexer() is None
+    assert get_document_indexer() is None
 
     user = factories.UserFactory()
     document = factories.DocumentFactory(title="alpha")
@@ -130,7 +130,7 @@ def test_api_documents_search_format(indexer_settings):
     """Validate the format of documents as returned by the search view."""
     indexer_settings.SEARCH_INDEXER_QUERY_URL = "http://find/api/v1.0/search"
 
-    assert default_document_indexer() is not None
+    assert get_document_indexer() is not None
 
     user = factories.UserFactory()
 
@@ -193,7 +193,7 @@ def test_api_documents_search_pagination(indexer_settings):
     """Documents should be ordered by descending "updated_at" by default"""
     indexer_settings.SEARCH_INDEXER_QUERY_URL = "http://find/api/v1.0/search"
 
-    assert default_document_indexer() is not None
+    assert get_document_indexer() is not None
 
     user = factories.UserFactory()