diff --git a/src/forum/search/typesense.py b/src/forum/search/typesense.py index 0c10223b..3ebcd614 100644 --- a/src/forum/search/typesense.py +++ b/src/forum/search/typesense.py @@ -2,6 +2,7 @@ Typesense backend for searching comments and threads. """ +import math from typing import Any, Optional, cast from bs4 import BeautifulSoup @@ -23,6 +24,11 @@ _TYPESENSE_CLIENT: Client | None = None +# Typesense rejects any search with per_page above 250 (HTTP 422), so a deep +# search for FORUM_MAX_DEEP_SEARCH_COMMENT_COUNT hits has to be paginated. +# https://typesense.org/docs/30.2/api/search.html#pagination-parameters +TYPESENSE_MAX_PER_PAGE = 250 + def get_typesense_client() -> Client: """ @@ -169,6 +175,7 @@ def build_search_parameters( course_id: str | None, context: str, commentable_ids: list[str] | None, + page: int = 1, ) -> SearchParameters: """ Build Typesense search parameters for searching the index. @@ -180,7 +187,8 @@ def build_search_parameters( if commentable_ids: safe_ids = ", ".join(quote_filter_value(value) for value in commentable_ids) - filters.append(f"commentable_ids:[{safe_ids}]") + # The field is `commentable_id`, singular, as declared in collection_schema(). + filters.append(f"commentable_id:[{safe_ids}]") if course_id: filters.append(f"course_id:={quote_filter_value(course_id)}") @@ -189,7 +197,8 @@ def build_search_parameters( "q": search_text, "query_by": "text", "filter_by": " && ".join(filters), - "per_page": FORUM_MAX_DEEP_SEARCH_COMMENT_COUNT, + "per_page": TYPESENSE_MAX_PER_PAGE, + "page": page, } @@ -408,18 +417,26 @@ def get_thread_ids( Retrieve thread IDs based on search criteria. """ client = get_typesense_client() + collection = client.collections[collection_name()] - params = build_search_parameters( - search_text=search_text, - course_id=course_id, - context=context, - commentable_ids=commentable_ids, + thread_ids: set[str] = set() + page_count = math.ceil( + FORUM_MAX_DEEP_SEARCH_COMMENT_COUNT / TYPESENSE_MAX_PER_PAGE ) + for page in range(1, page_count + 1): + params = build_search_parameters( + search_text=search_text, + course_id=course_id, + context=context, + commentable_ids=commentable_ids, + page=page, + ) + results = collection.documents.search(params) + hits = cast(list[dict[str, Any]], results.get("hits", [])) + thread_ids.update(hit["document"]["thread_id"] for hit in hits) + if len(hits) < TYPESENSE_MAX_PER_PAGE: + break - results = client.collections[collection_name()].documents.search(params) - thread_ids: set[str] = { - hit["document"]["thread_id"] for hit in results.get("hits", []) # type: ignore - } return list(thread_ids) def get_suggested_text(self, search_text: str) -> Optional[str]: diff --git a/src/forum/settings/test.py b/src/forum/settings/test.py index 7e8f1631..4edeeb41 100644 --- a/src/forum/settings/test.py +++ b/src/forum/settings/test.py @@ -82,4 +82,4 @@ def root(*args: str) -> str: TYPESENSE_COLLECTION_PREFIX = "forum_unittest_prefix_" TYPESENSE_API_KEY = "example-typesense-api-key" -TYPESENSE_URLS = ["http://0.0.0.0:8108"] +TYPESENSE_URLS = ["http://localhost:5108"] diff --git a/tests/e2e/docker-compose.yml b/tests/e2e/docker-compose.yml index 14cca4b8..aba6f44c 100644 --- a/tests/e2e/docker-compose.yml +++ b/tests/e2e/docker-compose.yml @@ -17,3 +17,10 @@ services: - 127.0.0.1:5700:7700 environment: MEILI_MASTER_KEY: "MEILISEARCH_MASTER_KEY" + + typesense: + # https://hub.docker.com/r/typesense/typesense/tags + image: docker.io/typesense/typesense:30.2 + ports: + - 127.0.0.1:5108:8108 + command: "--data-dir /tmp --api-key=example-typesense-api-key" diff --git a/tests/e2e/test_search_typesense.py b/tests/e2e/test_search_typesense.py new file mode 100644 index 00000000..d70cf4fd --- /dev/null +++ b/tests/e2e/test_search_typesense.py @@ -0,0 +1,134 @@ +""" +Typesense end-to-end tests. + +These run against a real Typesense server, which the mocked unit tests in +tests/test_typesense.py cannot substitute for: both of the bugs these tests +cover were rejections from the server, invisible to a mock. +""" + +import typing as t + +from django.test import override_settings +import pytest + +from forum.search import typesense + +pytestmark = pytest.mark.django_db + +COURSE_ID = "course-v1:Arbisoft+SE002+2024_S2" + + +@pytest.fixture(autouse=True) +def configure_typesense_search_backend() -> t.Generator[t.Any, t.Any, t.Any]: + """Configure Django to use Typesense as a search backend.""" + with override_settings( + FORUM_SEARCH_BACKEND="forum.search.typesense.TypesenseBackend" + ): + yield + + +@pytest.fixture(autouse=True) +def typesense_cleanup() -> None: + """Start each test from an empty collection.""" + typesense.TypesenseIndexBackend().initialize_indices(force_new_index=True) + + +def index_threads(threads: list[dict[str, t.Any]]) -> None: + """Bulk-index thread documents, then confirm Typesense accepted every one.""" + client = typesense.get_typesense_client() + response = client.collections[typesense.collection_name()].documents.import_( + [typesense.document_from_thread(thread["id"], thread) for thread in threads], + {"action": "upsert"}, + ) + assert all(result["success"] for result in response), response + + +def test_initialize_indices() -> None: + index_backend = typesense.TypesenseIndexBackend() + index_backend.initialize_indices() + # raises AssertionError if the collection on the server does not match the schema + index_backend.validate_indices() + + +def test_insert_document( + patched_get_backend: t.Any, user_data: tuple[str, str] +) -> None: + index_backend = typesense.TypesenseIndexBackend() + index_backend.initialize_indices() + + backend = patched_get_backend() + user_id, _ = user_data + comment_thread_id = backend.create_thread( + { + "title": "title", + "body": "Hello World!", + "pinned": False, + "author_id": user_id, + "course_id": COURSE_ID, + "commentable_id": "66b4e0440dead7001deb948b", + "author_username": "Faraz", + } + ) + + index_backend.refresh_indices() + thread_backend = typesense.TypesenseThreadSearchBackend() + assert thread_backend.get_thread_ids("course", [], "hello") == [comment_thread_id] + + +def test_search_filtered_by_commentable_id() -> None: + """ + Scoping a search to discussion topics has to filter on the field name the + collection actually declares, or Typesense rejects the search with HTTP 400. + """ + index_threads( + [ + { + "id": index, + "course_id": COURSE_ID, + "commentable_id": commentable_id, + "context": "course", + "title": "searchable thread", + "body": "
Hello World!
", + } + for index, commentable_id in enumerate(["week-1", "week-1", "week-2"]) + ] + ) + + thread_backend = typesense.TypesenseThreadSearchBackend() + assert sorted( + thread_backend.get_thread_ids( + "course", + [], + "searchable", + commentable_ids=["week-1"], + course_id=COURSE_ID, + ) + ) == ["0", "1"] + + +def test_deep_search_past_the_per_page_limit() -> None: + """ + A deep search asks for FORUM_MAX_DEEP_SEARCH_COMMENT_COUNT hits, which is far + more than Typesense will return in one page — it rejects any per_page above + 250 with HTTP 422. All of the matches still have to come back. + """ + thread_count = typesense.TYPESENSE_MAX_PER_PAGE * 2 + 1 + index_threads( + [ + { + "id": index, + "course_id": COURSE_ID, + "commentable_id": "week-1", + "context": "course", + "title": "searchable thread", + "body": f"Hello World {index}!
", + } + for index in range(thread_count) + ] + ) + + thread_backend = typesense.TypesenseThreadSearchBackend() + thread_ids = thread_backend.get_thread_ids( + "course", [], "searchable", commentable_ids=["week-1"], course_id=COURSE_ID + ) + assert len(thread_ids) == thread_count diff --git a/tests/test_typesense.py b/tests/test_typesense.py index 16da561e..1c21d039 100644 --- a/tests/test_typesense.py +++ b/tests/test_typesense.py @@ -100,16 +100,90 @@ def test_search_threads(mock_get_client: Mock) -> None: expected_params = { "q": "thoughts", "query_by": "text", - "filter_by": "context:`course` && commentable_ids:[`4`, `7[||`] " + # `commentable_id` is singular: it must match the field name in collection_schema() + "filter_by": "context:`course` && commentable_id:[`4`, `7[||`] " "&& course_id:=`course-v1:OpenedX+DemoX+DemoCourse`", - "per_page": constants.FORUM_MAX_DEEP_SEARCH_COMMENT_COUNT, + "per_page": typesense.TYPESENSE_MAX_PER_PAGE, + "page": 1, } + # a short page means the results are exhausted, so only one request is made mock_search.assert_called_once_with(expected_params) # suggested text is not supported; always returns None assert backend.get_suggested_text("foo") is None +def test_per_page_within_typesense_limit() -> None: + """Typesense returns HTTP 422 for per_page above 250.""" + assert typesense.TYPESENSE_MAX_PER_PAGE <= 250 + + +@patch("forum.search.typesense.get_typesense_client") +def test_search_threads_paginates(mock_get_client: Mock) -> None: + """ + A full page means there may be more, so the backend walks pages until it has + covered FORUM_MAX_DEEP_SEARCH_COMMENT_COUNT hits or a page comes back short. + """ + mock_client = MagicMock() + mock_get_client.return_value = mock_client + mock_search = mock_client.collections[ + "forum_unittest_prefix_forum" + ].documents.search + + full_page = { + "hits": [ + {"document": {"thread_id": f"T{index}"}} + for index in range(typesense.TYPESENSE_MAX_PER_PAGE) + ] + } + short_page = {"hits": [{"document": {"thread_id": "LAST"}}]} + mock_search.side_effect = [full_page, short_page] + + backend = typesense.TypesenseThreadSearchBackend() + thread_ids = backend.get_thread_ids( + context="course", + group_ids=[], + search_text="thoughts", + commentable_ids=None, + course_id=None, + ) + + assert len(thread_ids) == typesense.TYPESENSE_MAX_PER_PAGE + 1 + assert "LAST" in thread_ids + assert [call.args[0]["page"] for call in mock_search.call_args_list] == [1, 2] + + +@patch("forum.search.typesense.get_typesense_client") +def test_search_threads_stops_at_deep_search_budget(mock_get_client: Mock) -> None: + """Full pages all the way down still stop at FORUM_MAX_DEEP_SEARCH_COMMENT_COUNT.""" + mock_client = MagicMock() + mock_get_client.return_value = mock_client + mock_search = mock_client.collections[ + "forum_unittest_prefix_forum" + ].documents.search + mock_search.return_value = { + "hits": [ + {"document": {"thread_id": f"T{index}"}} + for index in range(typesense.TYPESENSE_MAX_PER_PAGE) + ] + } + + backend = typesense.TypesenseThreadSearchBackend() + backend.get_thread_ids( + context="course", + group_ids=[], + search_text="thoughts", + commentable_ids=None, + course_id=None, + ) + + expected_pages = ( + constants.FORUM_MAX_DEEP_SEARCH_COMMENT_COUNT + // typesense.TYPESENSE_MAX_PER_PAGE + ) + assert mock_search.call_count == expected_pages + + @patch("forum.search.typesense.get_typesense_client") def test_index_comment_document(mock_get_client: Mock) -> None: mock_client = MagicMock()