diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 7628812206f5..46eb96a2399a 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -9,10 +9,10 @@ from datetime import datetime, timedelta, timezone from typing import Callable, Generator -import meilisearch from django.conf import settings from django.contrib.auth import get_user_model from django.core.cache import cache +from meilisearch import Client as MeilisearchClient from meilisearch.errors import MeilisearchError from meilisearch.models.task import TaskInfo from opaque_keys.edx.keys import UsageKey @@ -35,7 +35,12 @@ User = get_user_model() STUDIO_INDEX_NAME = "studio_content" -INDEX_NAME = settings.MEILISEARCH_INDEX_PREFIX + STUDIO_INDEX_NAME + +if hasattr(settings, "MEILISEARCH_INDEX_PREFIX"): + INDEX_NAME = settings.MEILISEARCH_INDEX_PREFIX + STUDIO_INDEX_NAME +else: + INDEX_NAME = STUDIO_INDEX_NAME + _MEILI_CLIENT = None _MEILI_API_KEY_UID = None @@ -44,13 +49,13 @@ @contextmanager -def _index_rebuild_lock(index_name: str) -> Generator[str, None, None]: +def _index_rebuild_lock() -> Generator[str, None, None]: """ Lock to prevent that more than one rebuild is running at the same time """ timeout_at = time.monotonic() + LOCK_EXPIRE - lock_id = f"lock-meilisearch-index-{index_name}" - new_index_name = index_name + "_new" + lock_id = f"lock-meilisearch-index-{INDEX_NAME}" + new_index_name = INDEX_NAME + "_new" while True: status = cache.add(lock_id, new_index_name, LOCK_EXPIRE) @@ -58,10 +63,10 @@ def _index_rebuild_lock(index_name: str) -> Generator[str, None, None]: # Lock acquired try: yield new_index_name + break finally: # Release the lock cache.delete(lock_id) - return if time.monotonic() > timeout_at: raise TimeoutError("Timeout acquiring lock") @@ -69,8 +74,8 @@ def _index_rebuild_lock(index_name: str) -> Generator[str, None, None]: time.sleep(1) -def _get_running_rebuild_index_name(index_name: str) -> str | None: - lock_id = f"lock-meilisearch-index-{index_name}" +def _get_running_rebuild_index_name() -> str | None: + lock_id = f"lock-meilisearch-index-{INDEX_NAME}" return cache.get(lock_id) @@ -82,13 +87,13 @@ def _get_meilisearch_client(): global _MEILI_CLIENT # pylint: disable=global-statement # Connect to Meilisearch - if not settings.MEILISEARCH_ENABLED: + if not is_meilisearch_enabled(): raise RuntimeError("MEILISEARCH_ENABLED is not set - search functionality disabled.") - + if _MEILI_CLIENT is not None: return _MEILI_CLIENT - _MEILI_CLIENT = meilisearch.Client(settings.MEILISEARCH_URL, settings.MEILISEARCH_API_KEY) + _MEILI_CLIENT = MeilisearchClient(settings.MEILISEARCH_URL, settings.MEILISEARCH_API_KEY) try: _MEILI_CLIENT.health() except MeilisearchError as err: @@ -126,6 +131,15 @@ def _wait_for_meili_task(info: TaskInfo) -> None: raise MeilisearchError(err_reason) +def _wait_for_meili_tasks(info_list: list[TaskInfo]) -> None: + """ + Simple helper method to wait for multiple Meilisearch tasks to complete + """ + while info_list: + info = info_list.pop() + _wait_for_meili_task(info) + + def _index_exists(index_name: str) -> bool: """ Check if an index exists @@ -142,7 +156,7 @@ def _index_exists(index_name: str) -> bool: @contextmanager -def _using_temp_index(target_index, status_cb: Callable[[str], None] | None = None) -> Generator[str, None, None]: +def _using_temp_index(status_cb: Callable[[str], None] | None = None) -> Generator[str, None, None]: """ Create a new temporary Meilisearch index, populate it, then swap it to become the active index. @@ -155,7 +169,7 @@ def nop(_): client = _get_meilisearch_client() status_cb("Checking index...") - with _index_rebuild_lock(target_index) as temp_index_name: + with _index_rebuild_lock() as temp_index_name: if _index_exists(temp_index_name): status_cb("Temporary index already exists. Deleting it...") _wait_for_meili_task(client.delete_index(temp_index_name)) @@ -168,17 +182,17 @@ def nop(_): yield temp_index_name - if not _index_exists(target_index): + if not _index_exists(INDEX_NAME): # We have to create the "target" index before we can successfully swap the new one into it: status_cb("Preparing to swap into index (first time)...") - _wait_for_meili_task(client.create_index(target_index)) + _wait_for_meili_task(client.create_index(INDEX_NAME)) status_cb("Swapping index...") - client.swap_indexes([{'indexes': [temp_index_name, target_index]}]) + client.swap_indexes([{'indexes': [temp_index_name, INDEX_NAME]}]) # If we're using an API key that's restricted to certain index prefix(es), we won't be able to get the status # of this request unfortunately. https://github.com/meilisearch/meilisearch/issues/4103 while True: time.sleep(1) - if client.get_index(target_index).created_at != new_index_created: + if client.get_index(INDEX_NAME).created_at != new_index_created: status_cb("Waiting for swap completion...") else: break @@ -205,7 +219,17 @@ def _recurse_children(block, fn, status_cb: Callable[[str], None] | None = None) fn(child) -def rebuild_index(status_cb: Callable[[str], None] | None) -> None: +def is_meilisearch_enabled() -> bool: + """ + Returns whether Meilisearch is enabled + """ + if hasattr(settings, "MEILISEARCH_INDEX_PREFIX"): + + return settings.MEILISEARCH_ENABLED + return False + + +def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: """ Rebuild the Meilisearch index from scratch """ @@ -235,7 +259,7 @@ def nop(_message): num_blocks_done = 0 # How many individual components/XBlocks we've indexed status_cb(f"Found {num_courses} courses and {num_libraries} libraries.") - with _using_temp_index(INDEX_NAME, status_cb) as temp_index_name: + with _using_temp_index(status_cb) as temp_index_name: ############## Configure the index ############## # Mark usage_key as unique (it's not the primary key for the index, but nevertheless must be unique): client.index(temp_index_name).update_distinct_attribute(Fields.usage_key) @@ -294,15 +318,14 @@ def upsert_xblock_index_doc( """ Creates or updates the document for the given XBlock in the search index + Args: usage_key (UsageKey): The usage key of the XBlock to index recursive (bool): If True, also index all children of the XBlock update_metadata (bool): If True, update the metadata of the XBlock update_tags (bool): If True, update the tags of the XBlock """ - # If there is a rebuild in progress, wait for it to finish - # If a rebuild starts right after this check, it will already have the updated data, so this is not a problem. - current_rebuild_index_name = _get_running_rebuild_index_name(INDEX_NAME) + current_rebuild_index_name = _get_running_rebuild_index_name() course = modulestore().get_item(usage_key) client = _get_meilisearch_client() @@ -318,30 +341,33 @@ def add_with_children(block): add_with_children(course) - # FixMe: Create function to wait for multiple tasks to run this in parallel - _wait_for_meili_task(client.index(INDEX_NAME).update_documents(docs)) + tasks = [] if current_rebuild_index_name: - _wait_for_meili_task(client.index(current_rebuild_index_name).update_documents(docs)) + # If there is a rebuild in progress, the document will also be added to the new index. + tasks.append(client.index(current_rebuild_index_name).update_documents(docs)) + tasks.append(client.index(INDEX_NAME).update_documents(docs)) + + _wait_for_meili_tasks(tasks) def delete_xblock_index_doc(usage_key: UsageKey) -> None: """ Deletes the document for the given XBlock from the search index + + Args: + usage_key (UsageKey): The usage key of the XBlock to be removed from the index """ - # If there is a rebuild in progress, wait for it to finish - # If a rebuild starts right after this check, it will already have the updated data, so this is not a problem. - current_rebuild_index_name = _get_running_rebuild_index_name(INDEX_NAME) + current_rebuild_index_name = _get_running_rebuild_index_name() client = _get_meilisearch_client() - # FixMe: Create function to wait for multiple tasks to run this in parallel + tasks = [] if current_rebuild_index_name: - # Updates the new index - _wait_for_meili_task(client.index(current_rebuild_index_name).delete_document( - meili_id_from_opaque_key(usage_key) - )) + # If there is a rebuild in progress, the document will also be added to the new index. + tasks.append(client.index(current_rebuild_index_name).delete_document(meili_id_from_opaque_key(usage_key))) + tasks.append(client.index(INDEX_NAME).delete_document(meili_id_from_opaque_key(usage_key))) - _wait_for_meili_task(client.index(INDEX_NAME).delete_document(meili_id_from_opaque_key(usage_key))) + _wait_for_meili_tasks(tasks) def generate_user_token(user): @@ -357,7 +383,7 @@ def generate_user_token(user): } } # Note: the following is just generating a JWT. It doesn't actually make an API call to Meilisearch. - restricted_api_key = _get_meilisearch_client.generate_tenant_token( + restricted_api_key = _get_meilisearch_client().generate_tenant_token( api_key_uid=_get_meili_api_key_uid(), search_rules=search_rules, expires_at=expires_at, diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 1f6c0d39080e..99fd7cefdf16 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -14,6 +14,7 @@ ) from .tasks import delete_xblock_index_doc, upsert_xblock_index_doc +from .api import is_meilisearch_enabled log = logging.getLogger(__name__) @@ -23,7 +24,7 @@ def xblock_created_handler(**kwargs) -> None: """ Create the index for the XBlock """ - if not settings.MELISEARCH_ENABLED: + if not is_meilisearch_enabled(): return xblock_info = kwargs.get("xblock_info", None) @@ -44,7 +45,7 @@ def xblock_updated_handler(**kwargs) -> None: """ Update the index for the XBlock and its children """ - if not settings.MELISEARCH_ENABLED: + if not is_meilisearch_enabled(): return xblock_info = kwargs.get("xblock_info", None) @@ -65,7 +66,7 @@ def xblock_deleted_handler(**kwargs) -> None: """ Delete the index for the XBlock """ - if not settings.MELISEARCH_ENABLED: + if not is_meilisearch_enabled(): return xblock_info = kwargs.get("xblock_info", None) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py new file mode 100644 index 000000000000..f3df8a10302f --- /dev/null +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -0,0 +1,140 @@ +""" +Tests for the Studio content search API. +""" +from __future__ import annotations + +import types +from unittest.mock import MagicMock, patch + +from django.test import override_settings + +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangolib.testing.utils import skip_unless_cms +from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase + +from .. import api + +STUDIO_SEARCH_ENDPOINT_URL = "/api/content_search/v2/studio/" + + +class MockMeilisearchClient(MagicMock): + """ + Mock Meilisearch client. + """ + mock_index = MagicMock() + + def health(self): + return True + + def get_key(self, *_, **_kwargs): + key = types.SimpleNamespace() + key.uid = "3203d764-370f-4e99-a917-d47ab7f29739" + return key + + def generate_tenant_token(self, *_args, **_kwargs): + return "token" + + def get_index(self, *_, **_kwargs): + index = types.SimpleNamespace() + index.created_at = "2024-01-01T00:00:00.000Z" + return index + + def delete_index(self, *_args, **_kwargs): + pass + + def swap_indexes(self, *_args, **_kwargs): + pass + + def create_index(self, *_args, **_kwargs): + pass + + def index(self, index_name): + return self.mock_index + + +class MeilisearchTestMixin: + """ + Mixin for tests that use Meilisearch. + """ + + def setUp(self): + super().setUp() + patcher = patch('openedx.core.djangoapps.content.search.api.MeilisearchClient', new=MockMeilisearchClient) + self.mock_meilisearch = patcher.start() + self.addCleanup(patcher.stop) + + +@skip_unless_cms +class TestSearchApi(MeilisearchTestMixin, ModuleStoreTestCase): + """ + Tests for the Studio content search and index API. + """ + + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.user_id = self.user.id + + self.modulestore_patcher = patch( + "openedx.core.djangoapps.content.search.api.modulestore", return_value=self.store + ) + self.addCleanup(self.modulestore_patcher.stop) + self.modulestore_patcher.start() + + self.api_patcher = patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", return_value=None) + self.addCleanup(self.api_patcher.stop) + self.api_patcher.start() + + @override_settings(MEILISEARCH_ENABLED=False) + def test_reindex_meilisearch_disabled(self): + with self.assertRaises(RuntimeError): + api.rebuild_index() + + def test_reindex_meilisearch(self): + # Create course + course = self.store.create_course( + "orgA", + "test_course", + "test_run", + self.user_id, + fields={"display_name": "Test Course"}, + ) + + # Create XBlocks + sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") + vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") + + with override_settings(MEILISEARCH_ENABLED=True): + api.rebuild_index() + self.mock_meilisearch.mock_index.add_documents.assert_called_once_with([ + { + 'id': 'block-v1orgatest_coursetest_runtypesequentialblocktest_sequential-a32ce3b', + 'type': 'course_block', + 'usage_key': 'block-v1:orgA+test_course+test_run+type@sequential+block@test_sequential', + 'block_id': 'test_sequential', + 'display_name': 'sequential', + 'block_type': 'sequential', + 'context_key': 'course-v1:orgA+test_course+test_run', + 'org': 'orgA', + 'breadcrumbs': [{'display_name': 'Test Course'}], + 'content': {} + }, + { + 'id': 'block-v1orgatest_coursetest_runtypeverticalblocktest_vertical-f4cb441', + 'type': 'course_block', + 'usage_key': 'block-v1:orgA+test_course+test_run+type@vertical+block@test_vertical', + 'block_id': 'test_vertical', + 'display_name': 'vertical', + 'block_type': 'vertical', + 'context_key': 'course-v1:orgA+test_course+test_run', + 'org': 'orgA', + 'breadcrumbs': [ + {'display_name': 'Test Course'}, + {'display_name': 'sequential'} + ], + 'content': {} + } + ]) + api.rebuild_index() diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 78ed68039c51..c023d9939cd1 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -51,7 +51,7 @@ def setUpClass(cls): tagging_api.add_tag_to_taxonomy(cls.subject_tags, tag="Jump Links", parent_tag_value="Hypertext") # Tag stuff: - tagging_api.tag_object(cls.problem_block.usage_key, cls.difficulty_tags, tags=["Easy"]) + tagging_api.tag_object(cls.problem_block.usage_key, cls.difficulty_tags, ["Easy"]) tagging_api.tag_object(cls.html_block_key, cls.subject_tags, tags=["Chinese", "Jump Links"]) tagging_api.tag_object(cls.html_block_key, cls.difficulty_tags, tags=["Normal"]) diff --git a/openedx/core/djangoapps/content/search/tests/test_views.py b/openedx/core/djangoapps/content/search/tests/test_views.py index e117cf1b969a..8116e5088219 100644 --- a/openedx/core/djangoapps/content/search/tests/test_views.py +++ b/openedx/core/djangoapps/content/search/tests/test_views.py @@ -1,18 +1,21 @@ """ Tests for the Studio content search REST API. """ +from __future__ import annotations + from django.test import override_settings -from rest_framework.test import APITestCase, APIClient -from unittest import mock +from rest_framework.test import APIClient, APITestCase from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangolib.testing.utils import skip_unless_cms +from .test_api import MeilisearchTestMixin + STUDIO_SEARCH_ENDPOINT_URL = "/api/content_search/v2/studio/" @skip_unless_cms -class StudioSearchViewTest(APITestCase): +class StudioSearchViewTest(MeilisearchTestMixin, APITestCase): """ General tests for the Studio search REST API. """ @@ -67,14 +70,12 @@ def test_studio_search_student_forbidden(self): assert result.status_code == 403 @override_settings(MEILISEARCH_ENABLED=True) - @mock.patch('openedx.core.djangoapps.content.search.views._get_meili_api_key_uid') - def test_studio_search_staff(self, mock_get_api_key_uid): + def test_studio_search_staff(self): """ Global staff can get a restricted API key for Meilisearch using the REST API. """ self.client.login(username='staff', password='staff_pass') - mock_get_api_key_uid.return_value = "3203d764-370f-4e99-a917-d47ab7f29739" result = self.client.get(STUDIO_SEARCH_ENDPOINT_URL) assert result.status_code == 200 assert result.data["index_name"] == "studio_content" diff --git a/openedx/core/djangoapps/content/search/views.py b/openedx/core/djangoapps/content/search/views.py index e25a6fe4a87f..5ec893bccd94 100644 --- a/openedx/core/djangoapps/content/search/views.py +++ b/openedx/core/djangoapps/content/search/views.py @@ -28,7 +28,7 @@ def get(self, request): """ Give user details on how they can search studio content """ - if not settings.MEILISEARCH_ENABLED: + if not api.is_meilisearch_enabled(): raise NotFound("Meilisearch features are not enabled.") if not GlobalStaff().has_user(request.user): # Until we enforce permissions properly (see below), this endpoint is restricted to global staff,