✨(backend) implement Find feature flags
For beta testing purposes we need to be able to activate Find hybrid search to some users, Find full-text search to some others and leave remaining users on basic DRF title search. Signed-off-by: charles <[email protected]>
This commit is contained in:
@@ -32,6 +32,7 @@ from django.utils.translation import gettext_lazy as _
|
||||
|
||||
import requests
|
||||
import rest_framework as drf
|
||||
import waffle
|
||||
from botocore.exceptions import ClientError
|
||||
from csp.constants import NONE
|
||||
from csp.decorators import csp_update
|
||||
@@ -63,6 +64,7 @@ from core.services.search_indexers import (
|
||||
from core.tasks.mail import send_ask_for_access_mail
|
||||
from core.utils import extract_attachments, filter_descendants
|
||||
|
||||
from ..enums import SearchType
|
||||
from . import permissions, serializers, utils
|
||||
from .filters import (
|
||||
DocumentFilter,
|
||||
@@ -1181,6 +1183,10 @@ class DocumentViewSet(
|
||||
"""
|
||||
params = serializers.SearchDocumentSerializer(data=request.query_params)
|
||||
params.is_valid(raise_exception=True)
|
||||
search_type = self._get_search_type()
|
||||
|
||||
if search_type == SearchType.TITLE:
|
||||
return self._title_search(request, params.validated_data, *args, **kwargs)
|
||||
|
||||
indexer = get_document_indexer()
|
||||
if indexer is None:
|
||||
@@ -1188,17 +1194,29 @@ class DocumentViewSet(
|
||||
return self._title_search(request, params.validated_data, *args, **kwargs)
|
||||
|
||||
try:
|
||||
return self._search_with_indexer(indexer, request, params=params)
|
||||
return self._search_with_indexer(
|
||||
indexer, request, params=params, search_type=search_type
|
||||
)
|
||||
except requests.exceptions.RequestException as e:
|
||||
logger.error("Error while searching documents with indexer: %s", e)
|
||||
# fallback on title search if the indexer is not reached
|
||||
return self._title_search(
|
||||
request, params.validated_data, *args, **kwargs
|
||||
)
|
||||
return self._title_search(request, params.validated_data, *args, **kwargs)
|
||||
|
||||
def _get_search_type(self) -> SearchType:
|
||||
"""
|
||||
Returns the search type to use for the search endpoint based on feature flags.
|
||||
If a user has both flags activated the most advanced search is used
|
||||
(HYBRID > FULL_TEXT > TITLE).
|
||||
A user with no flag will default to the basic title search.
|
||||
"""
|
||||
if waffle.flag_is_active(self.request, "flag_find_hybrid_search"):
|
||||
return SearchType.HYBRID
|
||||
if waffle.flag_is_active(self.request, "flag_find_full_text_search"):
|
||||
return SearchType.FULL_TEXT
|
||||
return SearchType.TITLE
|
||||
|
||||
@staticmethod
|
||||
def _search_with_indexer(indexer, request, params):
|
||||
def _search_with_indexer(indexer, request, params, search_type):
|
||||
"""
|
||||
Returns a list of documents matching the query (q) according to the configured indexer.
|
||||
"""
|
||||
@@ -1206,6 +1224,7 @@ class DocumentViewSet(
|
||||
|
||||
results = indexer.search(
|
||||
q=params.validated_data["q"],
|
||||
search_type=search_type,
|
||||
token=request.session.get("oidc_access_token"),
|
||||
path=(
|
||||
params.validated_data["path"]
|
||||
|
||||
@@ -3,7 +3,7 @@ Core application enums declaration
|
||||
"""
|
||||
|
||||
import re
|
||||
from enum import StrEnum
|
||||
from enum import Enum, StrEnum
|
||||
|
||||
from django.conf import global_settings, settings
|
||||
from django.db import models
|
||||
@@ -46,3 +46,15 @@ class DocumentAttachmentStatus(StrEnum):
|
||||
|
||||
PROCESSING = "processing"
|
||||
READY = "ready"
|
||||
|
||||
|
||||
class SearchType(str, Enum):
|
||||
"""
|
||||
Defines the possible search types for a document search query.
|
||||
- TITLE: DRF based search in the title of the documents only.
|
||||
- HYBRID and FULL_TEXT: more advanced search based on Find indexer.
|
||||
"""
|
||||
|
||||
TITLE = "title"
|
||||
HYBRID = "hybrid"
|
||||
FULL_TEXT = "full-text"
|
||||
|
||||
@@ -13,6 +13,7 @@ from django.utils.module_loading import import_string
|
||||
import requests
|
||||
|
||||
from core import models, utils
|
||||
from core.enums import SearchType
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -68,7 +69,7 @@ def get_batch_accesses_by_users_and_teams(paths):
|
||||
return dict(access_by_document_path)
|
||||
|
||||
|
||||
def get_visited_document_ids_of(queryset, user):
|
||||
def get_visited_document_ids_of(queryset, user) -> tuple[str, ...]:
|
||||
"""
|
||||
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
|
||||
@@ -92,7 +93,7 @@ def get_visited_document_ids_of(queryset, user):
|
||||
.distinct("pk")
|
||||
)
|
||||
|
||||
return [str(id) for id in docs.values_list("pk", flat=True)]
|
||||
return tuple(str(id) for id in docs.values_list("pk", flat=True))
|
||||
|
||||
|
||||
class BaseDocumentIndexer(ABC):
|
||||
@@ -181,8 +182,16 @@ class BaseDocumentIndexer(ABC):
|
||||
Must be implemented by subclasses.
|
||||
"""
|
||||
|
||||
# pylint: disable-next=too-many-arguments,too-many-positional-arguments
|
||||
def search(self, q, token, visited=(), nb_results=None, path=None):
|
||||
# pylint: disable=too-many-arguments, too-many-positional-arguments
|
||||
def search( # noqa : PLR0913
|
||||
self,
|
||||
q: str,
|
||||
token: str,
|
||||
visited: tuple[str, ...] = (),
|
||||
nb_results: int = None,
|
||||
path: str = None,
|
||||
search_type: SearchType = None,
|
||||
):
|
||||
"""
|
||||
Search for documents in Find app.
|
||||
Ensure the same default ordering as "Docs" list : -updated_at
|
||||
@@ -200,6 +209,8 @@ class BaseDocumentIndexer(ABC):
|
||||
Defaults to 50 if not specified.
|
||||
path (str, optional):
|
||||
The parent path to search descendants of.
|
||||
search_type:
|
||||
hybrid or full-text search
|
||||
"""
|
||||
nb_results = nb_results or self.search_limit
|
||||
results = self.search_query(
|
||||
@@ -211,6 +222,7 @@ class BaseDocumentIndexer(ABC):
|
||||
"order_by": "updated_at",
|
||||
"order_direction": "desc",
|
||||
"path": path,
|
||||
"search_type": search_type,
|
||||
},
|
||||
token=token,
|
||||
)
|
||||
@@ -231,10 +243,29 @@ class FindDocumentIndexer(BaseDocumentIndexer):
|
||||
Document indexer that indexes and searches documents with La Suite Find app.
|
||||
"""
|
||||
|
||||
# pylint: disable=too-many-arguments,too-many-positional-arguments
|
||||
def search(self, q, token, visited=(), nb_results=None, path=None):
|
||||
# pylint: disable=too-many-arguments, too-many-positional-arguments
|
||||
def search( # noqa : PLR0913
|
||||
self,
|
||||
q: str,
|
||||
token: str,
|
||||
visited: tuple[()] = (),
|
||||
nb_results: int = None,
|
||||
path: str = None,
|
||||
search_type: SearchType = None,
|
||||
):
|
||||
"""format Find search results"""
|
||||
search_results = super().search(q, token, visited, nb_results, path)
|
||||
search_results = (
|
||||
super()
|
||||
.search(
|
||||
q=q,
|
||||
token=token,
|
||||
visited=visited,
|
||||
nb_results=nb_results,
|
||||
path=path,
|
||||
search_type=search_type,
|
||||
)
|
||||
.get("results", [])
|
||||
)
|
||||
return [
|
||||
{
|
||||
**hit["_source"],
|
||||
|
||||
@@ -9,14 +9,23 @@ import responses
|
||||
from faker import Faker
|
||||
from rest_framework import response as drf_response
|
||||
from rest_framework.test import APIClient
|
||||
from waffle.testutils import override_flag
|
||||
|
||||
from core import factories
|
||||
from core.enums import SearchType
|
||||
from core.services.search_indexers import get_document_indexer
|
||||
|
||||
fake = Faker()
|
||||
pytestmark = pytest.mark.django_db
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def enable_flag_find_hybrid_search():
|
||||
"""Enable flag_find_hybrid_search for all tests in this module."""
|
||||
with override_flag("flag_find_hybrid_search", active=True):
|
||||
yield
|
||||
|
||||
|
||||
@mock.patch("core.services.search_indexers.FindDocumentIndexer.search_query")
|
||||
@responses.activate
|
||||
def test_api_documents_search_anonymous(search_query, indexer_settings):
|
||||
@@ -46,6 +55,7 @@ def test_api_documents_search_anonymous(search_query, indexer_settings):
|
||||
"order_by": "updated_at",
|
||||
"order_direction": "desc",
|
||||
"path": None,
|
||||
"search_type": SearchType.HYBRID,
|
||||
},
|
||||
"token": None,
|
||||
}
|
||||
|
||||
@@ -12,6 +12,7 @@ from django.db import transaction
|
||||
import pytest
|
||||
|
||||
from core import factories, models
|
||||
from core.enums import SearchType
|
||||
from core.services.search_indexers import FindDocumentIndexer
|
||||
|
||||
pytestmark = pytest.mark.django_db
|
||||
@@ -473,8 +474,14 @@ def test_find_document_indexer_search(mock_search_query):
|
||||
nb_results = 10
|
||||
path = "/some/path/"
|
||||
visited = ["doc-123"]
|
||||
search_type = SearchType.HYBRID
|
||||
results = FindDocumentIndexer().search(
|
||||
q=q, token=token, nb_results=nb_results, path=path, visited=visited
|
||||
q=q,
|
||||
token=token,
|
||||
nb_results=nb_results,
|
||||
path=path,
|
||||
visited=visited,
|
||||
search_type=search_type,
|
||||
)
|
||||
|
||||
mock_search_query.assert_called_once()
|
||||
@@ -487,6 +494,7 @@ def test_find_document_indexer_search(mock_search_query):
|
||||
"order_by": "updated_at",
|
||||
"order_direction": "desc",
|
||||
"path": path,
|
||||
"search_type": search_type,
|
||||
}
|
||||
|
||||
assert len(results) == 2
|
||||
|
||||
@@ -494,7 +494,7 @@ def test_get_visited_document_ids_of():
|
||||
factories.UserDocumentAccessFactory(user=user, document=doc2)
|
||||
|
||||
# The second document have an access for the user
|
||||
assert get_visited_document_ids_of(queryset, user) == [str(doc1.pk)]
|
||||
assert get_visited_document_ids_of(queryset, user) == (str(doc1.pk),)
|
||||
|
||||
|
||||
@pytest.mark.usefixtures("indexer_settings")
|
||||
@@ -528,7 +528,7 @@ def test_get_visited_document_ids_of_deleted():
|
||||
doc_deleted.soft_delete()
|
||||
|
||||
# Only the first document is not deleted
|
||||
assert get_visited_document_ids_of(queryset, user) == [str(doc.pk)]
|
||||
assert get_visited_document_ids_of(queryset, user) == (str(doc.pk),)
|
||||
|
||||
|
||||
@responses.activate
|
||||
@@ -548,7 +548,7 @@ def test_services_search_indexers_search_errors(indexer_settings):
|
||||
)
|
||||
|
||||
with pytest.raises(HTTPError):
|
||||
FindDocumentIndexer().search("alpha", token="mytoken")
|
||||
FindDocumentIndexer().search(q="alpha", token="mytoken")
|
||||
|
||||
|
||||
@patch("requests.post")
|
||||
@@ -572,7 +572,7 @@ def test_services_search_indexers_search(mock_post, indexer_settings):
|
||||
|
||||
visited = get_visited_document_ids_of(models.Document.objects.all(), user)
|
||||
|
||||
indexer.search("alpha", visited=visited, token="mytoken")
|
||||
indexer.search(q="alpha", visited=visited, token="mytoken")
|
||||
|
||||
args, kwargs = mock_post.call_args
|
||||
|
||||
@@ -613,7 +613,7 @@ def test_services_search_indexers_search_nb_results(mock_post, indexer_settings)
|
||||
|
||||
visited = get_visited_document_ids_of(models.Document.objects.all(), user)
|
||||
|
||||
indexer.search("alpha", visited=visited, token="mytoken")
|
||||
indexer.search(q="alpha", visited=visited, token="mytoken")
|
||||
|
||||
args, kwargs = mock_post.call_args
|
||||
|
||||
@@ -621,7 +621,7 @@ def test_services_search_indexers_search_nb_results(mock_post, indexer_settings)
|
||||
assert kwargs.get("json")["nb_results"] == 25
|
||||
|
||||
# The argument overrides the setting value
|
||||
indexer.search("alpha", visited=visited, token="mytoken", nb_results=109)
|
||||
indexer.search(q="alpha", visited=visited, token="mytoken", nb_results=109)
|
||||
|
||||
args, kwargs = mock_post.call_args
|
||||
|
||||
|
||||
Reference in New Issue
Block a user