From f514257f2eb6e0da0b4366f0f6b7143e86797903 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 29 Mar 2024 10:25:31 -0300 Subject: [PATCH 01/18] feat: update search index when course content is updated --- openedx/core/djangoapps/content/search/api.py | 486 ++++++++++++++++++ .../docs/decisions/0001-meilisearch.rst | 2 + .../djangoapps/content/search/documents.py | 65 ++- .../djangoapps/content/search/handlers.py | 121 +++++ .../search/management/commands/meili_mixin.py | 105 ---- .../management/commands/reindex_studio.py | 134 +---- .../core/djangoapps/content/search/tasks.py | 112 ++++ .../content/search/tests/test_api.py | 193 +++++++ .../content/search/tests/test_documents.py | 1 + .../content/search/tests/test_handlers.py | 144 ++++++ .../content/search/tests/test_views.py | 27 +- .../core/djangoapps/content/search/views.py | 42 +- 12 files changed, 1133 insertions(+), 299 deletions(-) create mode 100644 openedx/core/djangoapps/content/search/api.py delete mode 100644 openedx/core/djangoapps/content/search/management/commands/meili_mixin.py create mode 100644 openedx/core/djangoapps/content/search/tasks.py create mode 100644 openedx/core/djangoapps/content/search/tests/test_api.py create mode 100644 openedx/core/djangoapps/content/search/tests/test_handlers.py diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py new file mode 100644 index 000000000000..c808ce07fabb --- /dev/null +++ b/openedx/core/djangoapps/content/search/api.py @@ -0,0 +1,486 @@ +""" +Content index and search API using Meilisearch +""" +from __future__ import annotations + +import logging +import time +from contextlib import contextmanager +from datetime import datetime, timedelta, timezone +from functools import wraps +from typing import Callable, Generator + +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 +from opaque_keys.edx.locator import LibraryLocatorV2 + +from openedx.core.djangoapps.content.search.documents import ( + STUDIO_INDEX_NAME, + Fields, + meili_id_from_opaque_key, + searchable_doc_for_course_block, + searchable_doc_for_library_block +) +from openedx.core.djangoapps.content_libraries import api as lib_api +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore + +from .documents import Fields, searchable_doc_for_course_block, searchable_doc_for_library_block + +log = logging.getLogger(__name__) + +User = get_user_model() + +STUDIO_INDEX_NAME = "studio_content" + +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 + +LOCK_EXPIRE = 5 * 60 # Lock expires in 5 minutes + + +@contextmanager +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" + + while True: + status = cache.add(lock_id, new_index_name, LOCK_EXPIRE) + if status: + # Lock acquired + try: + yield new_index_name + break + finally: + # Release the lock + cache.delete(lock_id) + + if time.monotonic() > timeout_at: + raise TimeoutError("Timeout acquiring lock") + + time.sleep(1) + + +def _get_running_rebuild_index_name() -> str | None: + lock_id = f"lock-meilisearch-index-{INDEX_NAME}" + + return cache.get(lock_id) + + +def _get_meilisearch_client(): + """ + Get the Meiliesearch client + """ + global _MEILI_CLIENT # pylint: disable=global-statement + + # Connect to Meilisearch + 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 = MeilisearchClient(settings.MEILISEARCH_URL, settings.MEILISEARCH_API_KEY) + try: + _MEILI_CLIENT.health() + except MeilisearchError as err: + _MEILI_CLIENT = None + raise ConnectionError("Unable to connect to Meilisearch") from err + return _MEILI_CLIENT + + +def clear_meilisearch_client(): + global _MEILI_CLIENT # pylint: disable=global-statement + + _MEILI_CLIENT = None + + +def _get_meili_api_key_uid(): + """ + Helper method to get the UID of the API key we're using for Meilisearch + """ + global _MEILI_API_KEY_UID # pylint: disable=global-statement + + if _MEILI_API_KEY_UID is not None: + return _MEILI_API_KEY_UID + + _MEILI_API_KEY_UID = _get_meilisearch_client().get_key(settings.MEILISEARCH_API_KEY).uid + + +def _wait_for_meili_task(info: TaskInfo) -> None: + """ + Simple helper method to wait for a Meilisearch task to complete + """ + client = _get_meilisearch_client() + current_status = client.get_task(info.task_uid) + while current_status.status in ("enqueued", "processing"): + time.sleep(1) + current_status = client.get_task(info.task_uid) + if current_status.status != "succeeded": + try: + err_reason = current_status.error['message'] + except (TypeError, KeyError): + err_reason = "Unknown error" + 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 + """ + client = _get_meilisearch_client() + try: + client.get_index(index_name) + except MeilisearchError as err: + if err.code == "index_not_found": + return False + else: + raise err + return True + + +@contextmanager +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. + """ + def nop(_): + pass + + if status_cb is None: + status_cb = nop + + client = _get_meilisearch_client() + status_cb("Checking index...") + 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)) + + status_cb("Creating new index...") + _wait_for_meili_task( + client.create_index(temp_index_name, {'primaryKey': 'id'}) + ) + new_index_created = client.get_index(temp_index_name).created_at + + yield temp_index_name + + 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(INDEX_NAME)) + status_cb("Swapping 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(INDEX_NAME).created_at != new_index_created: + status_cb("Waiting for swap completion...") + else: + break + status_cb("Deleting old index...") + _wait_for_meili_task(client.delete_index(temp_index_name)) + + +def _recurse_children(block, fn, status_cb: Callable[[str], None] | None = None) -> None: + """ + Recurse the children of an XBlock and call the given function for each + + The main purpose of this is just to wrap the loading of each child in + try...except. Otherwise block.get_children() would do what we need. + """ + if block.has_children: + for child_id in block.children: + try: + child = block.get_child(child_id) + except Exception as err: # pylint: disable=broad-except + log.exception(err) + if status_cb is not None: + status_cb(f"Unable to load block {child_id}") + else: + fn(child) + + +def only_if_meilisearch_enabled(f): + """ + Only call `f` if meilisearch is enabled + """ + @wraps(f) + def wrapper(*args, **kwargs): + """Wraps the decorated function.""" + if is_meilisearch_enabled(): + return f(*args, **kwargs) + return wrapper + + +def is_meilisearch_enabled() -> bool: + """ + Returns whether Meilisearch is enabled + """ + if hasattr(settings, "MEILISEARCH_ENABLED"): + return settings.MEILISEARCH_ENABLED + + return False + + +def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: + """ + Rebuild the Meilisearch index from scratch + """ + def nop(_message): + pass + + if status_cb is None: + status_cb = nop + + client = _get_meilisearch_client() + store = modulestore() + + # Get the lists of libraries + status_cb("Counting libraries...") + lib_keys = [lib.library_key for lib in lib_api.ContentLibrary.objects.select_related('org').only('org', 'slug')] + num_libraries = len(lib_keys) + + # Get the list of courses + status_cb("Counting courses...") + with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): + all_courses = store.get_courses() + num_courses = len(all_courses) + + # Some counters so we can track our progress as indexing progresses: + num_contexts = num_courses + num_libraries + num_contexts_done = 0 # How many courses/libraries we've indexed + 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(status_cb) as temp_index_name: + ############## Configure the index ############## + + # The following index settings are best changed on an empty index. + # Changing them on a populated index will "re-index all documents in the index, which can take some time" + # and use more RAM. Instead, we configure an empty index then populate it one course/library at a time. + + # 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) + # Mark which attributes can be used for filtering/faceted search: + client.index(temp_index_name).update_filterable_attributes([ + Fields.block_type, + Fields.context_key, + Fields.org, + Fields.tags, + Fields.type, + ]) + + ############## Libraries ############## + status_cb("Indexing libraries...") + for lib_key in lib_keys: + status_cb(f"{num_contexts_done + 1}/{num_contexts}. Now indexing library {lib_key}") + docs = [] + for component in lib_api.get_library_components(lib_key): + metadata = lib_api.LibraryXBlockMetadata.from_component(lib_key, component) + doc = searchable_doc_for_library_block(metadata) + docs.append(doc) + num_blocks_done += 1 + if docs: + # Add all the docs in this library at once (usually faster than adding one at a time): + _wait_for_meili_task(client.index(temp_index_name).add_documents(docs)) + num_contexts_done += 1 + + ############## Courses ############## + status_cb("Indexing courses...") + for course in all_courses: + status_cb( + f"{num_contexts_done + 1}/{num_contexts}. Now indexing course {course.display_name} ({course.id})" + ) + docs = [] + + # Pre-fetch the course with all of its children: + course = store.get_course(course.id, depth=None) + + def add_with_children(block): + """ Recursively index the given XBlock/component """ + doc = searchable_doc_for_course_block(block) + docs.append(doc) # pylint: disable=cell-var-from-loop + _recurse_children(block, add_with_children) # pylint: disable=cell-var-from-loop + + _recurse_children(course, add_with_children) + + if docs: + # Add all the docs in this course at once (usually faster than adding one at a time): + _wait_for_meili_task(client.index(temp_index_name).add_documents(docs)) + num_contexts_done += 1 + num_blocks_done += len(docs) + + status_cb(f"Done! {num_blocks_done} blocks indexed across {num_contexts_done} courses and libraries.") + + +def upsert_xblock_index_doc( + usage_key: UsageKey, recursive: bool = True, update_metadata: bool = True, update_tags: bool = True +) -> None: + """ + 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 + """ + current_rebuild_index_name = _get_running_rebuild_index_name() + + xblock = modulestore().get_item(usage_key) + client = _get_meilisearch_client() + + docs = [] + + def add_with_children(block): + """ Recursively index the given XBlock/component """ + doc = searchable_doc_for_course_block(block, include_metadata=update_metadata, include_tags=update_tags) + docs.append(doc) + if recursive: + _recurse_children(block, add_with_children) + + add_with_children(xblock) + + if not docs: + return + + tasks = [] + if current_rebuild_index_name: + # 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_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 + """ + current_rebuild_index_name = _get_running_rebuild_index_name() + + client = _get_meilisearch_client() + + tasks = [] + if current_rebuild_index_name: + # 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_tasks(tasks) + + +def upsert_library_block_index_doc( + usage_key: UsageKey, update_metadata: bool = True, update_tags: bool = True +) -> None: + """ + Creates or updates the document for the given Library Block in the search index + + + Args: + usage_key (UsageKey): The usage key of the Library Block to index + update_metadata (bool): If True, update the metadata of the Library Block + update_tags (bool): If True, update the tags of the Library Block + """ + current_rebuild_index_name = _get_running_rebuild_index_name() + + library_block = lib_api.get_component_from_usage_key(usage_key) + library_block_metadata = lib_api.LibraryXBlockMetadata.from_component(usage_key.context_key, library_block) + client = _get_meilisearch_client() + + docs = [ + searchable_doc_for_library_block( + library_block_metadata, include_metadata=update_metadata, include_tags=update_tags + ) + ] + + tasks = [] + if current_rebuild_index_name: + # 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 upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None: + """ + Creates or updates the documents for the given Content Library in the search index + """ + current_rebuild_index_name = _get_running_rebuild_index_name() + client = _get_meilisearch_client() + + docs = [] + for component in lib_api.get_library_components(library_key): + metadata = lib_api.LibraryXBlockMetadata.from_component(library_key, component) + doc = searchable_doc_for_library_block(metadata, include_metadata=True, include_tags=False) + docs.append(doc) + + tasks = [] + + if not docs: + return + + if current_rebuild_index_name: + # 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 generate_user_token(user): + """ + Returns a Meilisearch API key that only allows the user to search content that they have permission to view + """ + expires_at = datetime.now(tz=timezone.utc) + timedelta(days=7) + search_rules = { + INDEX_NAME: { + # TODO: Apply filters here based on the user's permissions, so they can only search for content + # that they have permission to view. Example: + # 'filter': 'org = BradenX' + } + } + # 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( + api_key_uid=_get_meili_api_key_uid(), + search_rules=search_rules, + expires_at=expires_at, + ) + + return { + "url": settings.MEILISEARCH_PUBLIC_URL, + "index_name": INDEX_NAME, + "api_key": restricted_api_key, + } diff --git a/openedx/core/djangoapps/content/search/docs/decisions/0001-meilisearch.rst b/openedx/core/djangoapps/content/search/docs/decisions/0001-meilisearch.rst index 1f355409da0e..f7430121605d 100644 --- a/openedx/core/djangoapps/content/search/docs/decisions/0001-meilisearch.rst +++ b/openedx/core/djangoapps/content/search/docs/decisions/0001-meilisearch.rst @@ -104,6 +104,8 @@ Decision new ``content/search`` Django app, so it's relatively easy to swap out later if this experiment doesn't pan out. 4. We will not use ``edx-search`` for the new search functionality. +5. For the experiment, we won't use Meilisearch during tests, but we expect to + add that in the future if we move forward with replacing Elasticsearch completely. Consequences diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index fc1388f82455..2d76a6eaf3d8 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -2,11 +2,12 @@ Utilities related to indexing content for search """ from __future__ import annotations -from hashlib import blake2b + import logging +from hashlib import blake2b from django.utils.text import slugify -from opaque_keys.edx.keys import UsageKey, LearningContextKey +from opaque_keys.edx.keys import LearningContextKey, UsageKey from openedx.core.djangoapps.content_libraries import api as lib_api from openedx.core.djangoapps.content_tagging import api as tagging_api @@ -62,7 +63,7 @@ class DocType: library_block = "library_block" -def _meili_id_from_opaque_key(usage_key: UsageKey) -> str: +def meili_id_from_opaque_key(usage_key: UsageKey) -> str: """ Meilisearch requires each document to have a primary key that's either an integer or a string composed of alphanumeric characters (a-z A-Z 0-9), @@ -88,7 +89,6 @@ class implementation returns only: {"content": {"display_name": "..."}, "content_type": "..."} """ block_data = { - Fields.id: _meili_id_from_opaque_key(block.usage_key), Fields.usage_key: str(block.usage_key), Fields.block_id: str(block.usage_key.block_id), Fields.display_name: xblock_api.get_block_display_name(block), @@ -196,33 +196,58 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: return {Fields.tags: result} -def searchable_doc_for_library_block(metadata: lib_api.LibraryXBlockMetadata) -> dict: +def searchable_doc_for_library_block( + xblock_metadata: lib_api.LibraryXBlockMetadata, include_metadata: bool = True, include_tags: bool = True +) -> dict: """ Generate a dictionary document suitable for ingestion into a search engine like Meilisearch or Elasticsearch, so that the given library block can be found using faceted search. + + Args: + xblock_metadata: The XBlock component metadata to index + include_metadata: If True, include the block's metadata in the doc + include_tags: If True, include the block's tags in the doc """ - library_name = lib_api.get_library(metadata.usage_key.context_key).title - doc = {} - try: - block = xblock_api.load_block(metadata.usage_key, user=None) - except Exception as err: # pylint: disable=broad-except - log.exception(f"Failed to load XBlock {metadata.usage_key}: {err}") - doc.update(_fields_from_block(block)) - doc.update(_tags_for_content_object(metadata.usage_key)) - doc[Fields.type] = DocType.library_block - # Add the breadcrumbs. In v2 libraries, the library itself is not a "parent" of the XBlocks so we add it here: - doc[Fields.breadcrumbs] = [{"display_name": library_name}] + library_name = lib_api.get_library(xblock_metadata.usage_key.context_key).title + block = xblock_api.load_block(xblock_metadata.usage_key, user=None) + + doc = { + Fields.id: meili_id_from_opaque_key(xblock_metadata.usage_key), + Fields.type: DocType.library_block, + } + + if include_metadata: + doc.update(_fields_from_block(block)) + # Add the breadcrumbs. In v2 libraries, the library itself is not a "parent" of the XBlocks so we add it here: + doc[Fields.breadcrumbs] = [{"display_name": library_name}] + + if include_tags: + doc.update(_tags_for_content_object(xblock_metadata.usage_key)) + return doc -def searchable_doc_for_course_block(block) -> dict: +def searchable_doc_for_course_block(block, include_metadata: bool = True, include_tags: bool = True) -> dict: """ Generate a dictionary document suitable for ingestion into a search engine like Meilisearch or Elasticsearch, so that the given course block can be found using faceted search. + + Args: + block: The XBlock instance to index + include_metadata: If True, include the block's metadata in the doc + include_tags: If True, include the block's tags in the doc """ - doc = _fields_from_block(block) - doc.update(_tags_for_content_object(block.usage_key)) - doc[Fields.type] = DocType.course_block + doc = { + Fields.id: meili_id_from_opaque_key(block.usage_key), + Fields.type: DocType.course_block, + } + + if include_metadata: + doc.update(_fields_from_block(block)) + + if include_tags: + doc.update(_tags_for_content_object(block.usage_key)) + return doc diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index e69de29bb2d1..6a7bd152fef7 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -0,0 +1,121 @@ +""" +Handlers for content indexing +""" + +import logging + +from django.dispatch import receiver +from openedx_events.content_authoring.data import ContentLibraryData, LibraryBlockData, XBlockData +from openedx_events.content_authoring.signals import ( + CONTENT_LIBRARY_UPDATED, + LIBRARY_BLOCK_CREATED, + LIBRARY_BLOCK_DELETED, + XBLOCK_CREATED, + XBLOCK_DELETED, + XBLOCK_UPDATED +) + +from .api import only_if_meilisearch_enabled +from .tasks import ( + delete_library_block_index_doc, + delete_xblock_index_doc, + update_content_library_index_docs, + upsert_library_block_index_doc, + upsert_xblock_index_doc +) + +log = logging.getLogger(__name__) + + +@receiver(XBLOCK_CREATED) +@only_if_meilisearch_enabled +def xblock_created_handler(**kwargs) -> None: + """ + Create the index for the XBlock + """ + xblock_info = kwargs.get("xblock_info", None) + if not xblock_info or not isinstance(xblock_info, XBlockData): # pragma: no cover + log.error("Received null or incorrect data for event") + return + + upsert_xblock_index_doc.delay( + str(xblock_info.usage_key), + recursive=False, + update_metadata=True, + update_tags=False, + ) + + +@receiver(XBLOCK_UPDATED) +@only_if_meilisearch_enabled +def xblock_updated_handler(**kwargs) -> None: + """ + Update the index for the XBlock and its children + """ + xblock_info = kwargs.get("xblock_info", None) + if not xblock_info or not isinstance(xblock_info, XBlockData): # pragma: no cover + log.error("Received null or incorrect data for event") + return + + upsert_xblock_index_doc.delay( + str(xblock_info.usage_key), + recursive=True, # Update all children because the breadcrumb may have changed + update_metadata=True, + update_tags=False, + ) + + +@receiver(XBLOCK_DELETED) +@only_if_meilisearch_enabled +def xblock_deleted_handler(**kwargs) -> None: + """ + Delete the index for the XBlock + """ + xblock_info = kwargs.get("xblock_info", None) + if not xblock_info or not isinstance(xblock_info, XBlockData): # pragma: no cover + log.error("Received null or incorrect data for event") + return + + delete_xblock_index_doc.delay(str(xblock_info.usage_key)) + + +@receiver(LIBRARY_BLOCK_CREATED) +@only_if_meilisearch_enabled +def library_block_updated_handler(**kwargs) -> None: + """ + Create or update the index for the content library block + """ + library_block_data = kwargs.get("library_block", None) + if not library_block_data or not isinstance(library_block_data, LibraryBlockData): # pragma: no cover + log.error("Received null or incorrect data for event") + return + + upsert_library_block_index_doc.delay(str(library_block_data.usage_key), update_metadata=True, update_tags=False) + + +@receiver(LIBRARY_BLOCK_DELETED) +@only_if_meilisearch_enabled +def library_block_deleted(**kwargs) -> None: + """ + Delete the index for the content library block + """ + library_block_data = kwargs.get("library_block", None) + if not library_block_data or not isinstance(library_block_data, LibraryBlockData): # pragma: no cover + log.error("Received null or incorrect data for event") + return + + delete_library_block_index_doc.delay(str(library_block_data.usage_key)) + + +@receiver(CONTENT_LIBRARY_UPDATED) +@only_if_meilisearch_enabled +def content_library_updated_handler(**kwargs) -> None: + """ + Update the index for the content library + """ + content_library_data = kwargs.get("content_library", None) + if not content_library_data or not isinstance(content_library_data, ContentLibraryData): # pragma: no cover + log.error("Received null or incorrect data for event") + return + + update_content_library_index_docs.delay(str(content_library_data.library_key)) diff --git a/openedx/core/djangoapps/content/search/management/commands/meili_mixin.py b/openedx/core/djangoapps/content/search/management/commands/meili_mixin.py deleted file mode 100644 index 18c7681879cb..000000000000 --- a/openedx/core/djangoapps/content/search/management/commands/meili_mixin.py +++ /dev/null @@ -1,105 +0,0 @@ -""" -Mixin for Django management commands that interact with Meilisearch -""" -from contextlib import contextmanager -import time - -from django.conf import settings -from django.core.management import CommandError -import meilisearch -from meilisearch.errors import MeilisearchError -from meilisearch.models.task import TaskInfo - - -class MeiliCommandMixin: - """ - Mixin for Django management commands that interact with Meilisearch - """ - def get_meilisearch_client(self): - """ - Get the Meiliesearch client - """ - if hasattr(self, "_meili_client"): - return self._meili_client - # Connect to Meilisearch - if not settings.MEILISEARCH_ENABLED: - raise CommandError("MEILISEARCH_ENABLED is not set - search functionality disabled.") - - self._meili_client = meilisearch.Client(settings.MEILISEARCH_URL, settings.MEILISEARCH_API_KEY) - try: - self._meili_client.health() - except MeilisearchError as err: - self.stderr.write(err.message) # print this because 'raise...from...' doesn't print the details - raise CommandError("Unable to connect to Meilisearch") from err - return self._meili_client - - def wait_for_meili_task(self, info: TaskInfo): - """ - Simple helper method to wait for a Meilisearch task to complete - """ - client = self.get_meilisearch_client() - current_status = client.get_task(info.task_uid) - while current_status.status in ("enqueued", "processing"): - self.stdout.write("...") - time.sleep(1) - current_status = client.get_task(info.task_uid) - if current_status.status != "succeeded": - self.stderr.write(f"Task has status: {current_status.status}") - self.stderr.write(str(current_status.error)) - try: - err_reason = current_status.error['message'] - except (TypeError, KeyError): - err_reason = "Unknown error" - raise MeilisearchError(err_reason) - - def index_exists(self, index_name: str) -> bool: - """ - Check if an index exists - """ - client = self.get_meilisearch_client() - try: - client.get_index(index_name) - except MeilisearchError as err: - if err.code == "index_not_found": - return False - else: - raise err - return True - - @contextmanager - def using_temp_index(self, target_index): - """ - Create a new temporary Meilisearch index, populate it, then swap it to - become the active index. - """ - client = self.get_meilisearch_client() - self.stdout.write("Checking index...") - temp_index_name = target_index + "_new" - if self.index_exists(temp_index_name): - self.stdout.write("Temporary index already exists. Deleting it...") - self.wait_for_meili_task(client.delete_index(temp_index_name)) - - self.stdout.write("Creating new index...") - self.wait_for_meili_task( - client.create_index(temp_index_name, {'primaryKey': 'id'}) - ) - new_index_created = client.get_index(temp_index_name).created_at - - yield temp_index_name - - if not self.index_exists(target_index): - # We have to create the "target" index before we can successfully swap the new one into it: - self.stdout.write("Preparing to swap into index (first time)...") - self.wait_for_meili_task(client.create_index(target_index)) - self.stdout.write("Swapping index...") - client.swap_indexes([{'indexes': [temp_index_name, target_index]}]) - # 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: - self.stdout.write("Waiting for swap completion...") - else: - break - self.stdout.write("Deleting old index...") - self.wait_for_meili_task(client.delete_index(temp_index_name)) diff --git a/openedx/core/djangoapps/content/search/management/commands/reindex_studio.py b/openedx/core/djangoapps/content/search/management/commands/reindex_studio.py index 0f52eea51d4d..3767ebcba6c9 100644 --- a/openedx/core/djangoapps/content/search/management/commands/reindex_studio.py +++ b/openedx/core/djangoapps/content/search/management/commands/reindex_studio.py @@ -5,28 +5,12 @@ See also cms/djangoapps/contentstore/management/commands/reindex_course.py which indexes LMS (published) courses in ElasticSearch. """ -import logging -import time - -from django.conf import settings from django.core.management import BaseCommand, CommandError -from openedx.core.djangoapps.content_libraries import api as lib_api -from openedx.core.djangoapps.content.search.documents import ( - Fields, - searchable_doc_for_course_block, - searchable_doc_for_library_block, - STUDIO_INDEX_NAME, -) -from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.django import modulestore -from .meili_mixin import MeiliCommandMixin - - -log = logging.getLogger(__name__) +from ... import api -class Command(MeiliCommandMixin, BaseCommand): +class Command(BaseCommand): """ Build or re-build the search index for courses and libraries (in Studio, i.e. Draft mode) @@ -41,119 +25,13 @@ def handle(self, *args, **options): """ Build a new search index for Studio, containing content from courses and libraries """ + if not api.is_meilisearch_enabled(): + raise CommandError("Meilisearch is not enabled. Please set MEILISEARCH_ENABLED to True in your settings.") + if not options["experimental"]: raise CommandError( "This command is experimental and not recommended for production. " "Use the --experimental argument to acknowledge and run it." ) - start_time = time.perf_counter() - client = self.get_meilisearch_client() - store = modulestore() - - # Get the lists of libraries - self.stdout.write("Counting libraries...") - lib_keys = [lib.library_key for lib in lib_api.ContentLibrary.objects.select_related('org').only('org', 'slug')] - num_libraries = len(lib_keys) - - # Get the list of courses - self.stdout.write("Counting courses...") - with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): - all_courses = store.get_courses() - num_courses = len(all_courses) - - # Some counters so we can track our progress as indexing progresses: - num_contexts = num_courses + num_libraries - num_contexts_done = 0 # How many courses/libraries we've indexed - num_blocks_done = 0 # How many individual components/XBlocks we've indexed - - self.stdout.write(f"Found {num_courses} courses and {num_libraries} libraries.") - index_name = settings.MEILISEARCH_INDEX_PREFIX + STUDIO_INDEX_NAME - with self.using_temp_index(index_name) as temp_index_name: - ############## Configure the index ############## - - # The following index settings are best changed on an empty index. - # Changing them on a populated index will "re-index all documents in the index, which can take some time" - # and use more RAM. Instead, we configure an empty index then populate it one course/library at a time. - - # 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) - # Mark which attributes can be used for filtering/faceted search: - client.index(temp_index_name).update_filterable_attributes([ - Fields.block_type, - Fields.context_key, - Fields.org, - Fields.tags, - Fields.type, - ]) - # Mark which attributes are used for keyword search, in order of importance: - client.index(temp_index_name).update_searchable_attributes([ - Fields.display_name, - Fields.block_id, - Fields.content, - Fields.tags, - # Keyword search does _not_ search the course name, course ID, breadcrumbs, block type, or other fields. - ]) - ############## Libraries ############## - self.stdout.write("Indexing libraries...") - for lib_key in lib_keys: - self.stdout.write(f"{num_contexts_done + 1}/{num_contexts}. Now indexing library {lib_key}") - docs = [] - for component in lib_api.get_library_components(lib_key): - metadata = lib_api.LibraryXBlockMetadata.from_component(lib_key, component) - doc = searchable_doc_for_library_block(metadata) - docs.append(doc) - num_blocks_done += 1 - if docs: - # Add all the docs in this library at once (usually faster than adding one at a time): - self.wait_for_meili_task(client.index(temp_index_name).add_documents(docs)) - self.wait_for_meili_task(client.index(temp_index_name).add_documents(docs)) - num_contexts_done += 1 - - ############## Courses ############## - self.stdout.write("Indexing courses...") - for course in all_courses: - self.stdout.write( - f"{num_contexts_done + 1}/{num_contexts}. Now indexing course {course.display_name} ({course.id})" - ) - docs = [] - - # Pre-fetch the course with all of its children: - course = store.get_course(course.id, depth=None) - - def add_with_children(block): - """ Recursively index the given XBlock/component """ - doc = searchable_doc_for_course_block(block) - docs.append(doc) # pylint: disable=cell-var-from-loop - self.recurse_children(block, add_with_children) # pylint: disable=cell-var-from-loop - - self.recurse_children(course, add_with_children) - - if docs: - # Add all the docs in this course at once (usually faster than adding one at a time): - self.wait_for_meili_task(client.index(temp_index_name).add_documents(docs)) - num_contexts_done += 1 - num_blocks_done += len(docs) - - elapsed_time = time.perf_counter() - start_time - self.stdout.write( - f"Done! {num_blocks_done} blocks indexed across {num_contexts_done} courses " - f"and libraries in {elapsed_time:.0f}s." - ) - - def recurse_children(self, block, fn): - """ - Recurse the children of an XBlock and call the given function for each - - The main purpose of this is just to wrap the loading of each child in - try...except. Otherwise block.get_children() would do what we need. - """ - if block.has_children: - for child_id in block.children: - try: - child = block.get_child(child_id) - except Exception as err: # pylint: disable=broad-except - log.exception(err) - self.stderr.write(f"Unable to load block {child_id}") - else: - fn(child) + api.rebuild_index(self.stdout.write) diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py new file mode 100644 index 000000000000..18ab232119d8 --- /dev/null +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -0,0 +1,112 @@ +""" +Defines asynchronous celery task for content indexing +""" + +from __future__ import annotations + +import logging + +from celery import shared_task +from celery_utils.logged_task import LoggedTask +from edx_django_utils.monitoring import set_code_owner_attribute +from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 + +from . import api + +log = logging.getLogger(__name__) + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def upsert_xblock_index_doc(usage_key_str: str, recursive: bool, update_metadata: bool, update_tags: bool) -> bool: + """ + Celery task to update the content index document for an XBlock + """ + try: + usage_key = UsageKey.from_string(usage_key_str) + + log.info("Updating content index document for XBlock with id: %s", usage_key) + + api.upsert_xblock_index_doc(usage_key, recursive, update_metadata, update_tags) + + return True + except Exception as e: # pragma: no cover # pylint: disable=broad-except + log.error("Error updating content index document for XBlock with id: %s. %s", usage_key_str, e) + return False + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def delete_xblock_index_doc(usage_key_str: str) -> bool: + """ + Celery task to delete the content index document for an XBlock + """ + try: + usage_key = UsageKey.from_string(usage_key_str) + + log.info("Updating content index document for XBlock with id: %s", usage_key) + + api.delete_index_doc(usage_key) + + return True + except Exception as e: # pragma: no cover # pylint: disable=broad-except + log.error("Error deleting content index document for XBlock with id: %s. %s", usage_key_str, e) + return False + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def upsert_library_block_index_doc(usage_key_str: str, update_metadata: bool, update_tags: bool) -> bool: + """ + Celery task to update the content index document for a library block + """ + try: + usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) + + log.info("Updating content index document for library block with id: %s", usage_key) + + api.upsert_library_block_index_doc(usage_key, update_metadata, update_tags) + + return True + except Exception as e: # pragma: no cover # pylint: disable=broad-except + log.error("Error updating content index document for libray block with id: %s. %s", usage_key_str, e) + return False + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def delete_library_block_index_doc(usage_key_str: str) -> bool: + """ + Celery task to delete the content index document for a library block + """ + try: + usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) + + log.info("Deleting content index document for library block with id: %s", usage_key) + + api.delete_index_doc(usage_key) + + return True + except Exception as e: # pragma: no cover # pylint: disable=broad-except + log.error("Error deleting content index document for library block with id: %s. %s", usage_key_str, e) + return False + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def update_content_library_index_docs(library_key_str: str) -> bool: + """ + Celery task to update the content index documents for all library blocks in a library + """ + try: + library_key = LibraryLocatorV2.from_string(library_key_str) + + log.info("Updating content index documents for library with id: %s", library_key) + + api.upsert_content_library_index_docs(library_key) + + return True + except Exception as e: # pragma: no cover # pylint: disable=broad-except + log.error("Error updating content index documents for library with id: %s. %s", library_key, e) + return False 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..50d361ff6b16 --- /dev/null +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -0,0 +1,193 @@ +""" +Tests for the Studio content search API. +""" +from __future__ import annotations + +from unittest.mock import MagicMock, call, patch + +import ddt +from django.test import override_settings +from organizations.tests.factories import OrganizationFactory + +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content_libraries import api as library_api +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/" + + +@ddt.ddt +@skip_unless_cms +@patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) +@patch("openedx.core.djangoapps.content.search.api.MeilisearchClient") +class TestSearchApi(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() + + # Clear the Meilisearch client to avoid side effects from other tests + api.clear_meilisearch_client() + + # Create course + self.course = self.store.create_course( + "org1", + "test_course", + "test_run", + self.user_id, + fields={"display_name": "Test Course"}, + ) + + # Create XBlocks + self.sequential = self.store.create_child(self.user_id, self.course.location, "sequential", "test_sequential") + self.doc_sequential = { + 'id': 'block-v1org1test_coursetest_runtypesequentialblocktest_sequential-f702c144', + 'type': 'course_block', + 'usage_key': 'block-v1:org1+test_course+test_run+type@sequential+block@test_sequential', + 'block_id': 'test_sequential', + 'display_name': 'sequential', + 'block_type': 'sequential', + 'context_key': 'course-v1:org1+test_course+test_run', + 'org': 'org1', + 'breadcrumbs': [{'display_name': 'Test Course'}], + 'content': {} + } + self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical") + self.doc_vertical = { + 'id': 'block-v1org1test_coursetest_runtypeverticalblocktest_vertical-e76a10a4', + 'type': 'course_block', + 'usage_key': 'block-v1:org1+test_course+test_run+type@vertical+block@test_vertical', + 'block_id': 'test_vertical', + 'display_name': 'vertical', + 'block_type': 'vertical', + 'context_key': 'course-v1:org1+test_course+test_run', + 'org': 'org1', + 'breadcrumbs': [ + {'display_name': 'Test Course'}, + {'display_name': 'sequential'} + ], + 'content': {} + } + + # Create a content library: + self.library = library_api.create_library( + library_type=library_api.COMPLEX, + org=OrganizationFactory.create(short_name="org1"), + slug="lib", + title="Library", + ) + # Populate it with a problem: + self.problem = library_api.create_library_block(self.library.key, "problem", "p1") + self.doc_problem = { + "id": "lborg1libproblemp1-a698218e", + "usage_key": "lb:org1:lib:problem:p1", + "block_id": "p1", + "display_name": "Blank Problem", + "block_type": "problem", + "context_key": "lib:org1:lib", + "org": "org1", + "breadcrumbs": [{"display_name": "Library"}], + "content": {"problem_types": [], "capa_content": " "}, + "type": "library_block", + } + + @override_settings(MEILISEARCH_ENABLED=False) + def test_reindex_meilisearch_disabled(self, mock_meilisearch): + with self.assertRaises(RuntimeError): + api.rebuild_index() + + mock_meilisearch.return_value.swap_indexes.assert_not_called() + + @override_settings(MEILISEARCH_ENABLED=True) + def test_reindex_meilisearch(self, mock_meilisearch): + + api.rebuild_index() + mock_meilisearch.return_value.index.return_value.add_documents.assert_has_calls( + [ + call([self.doc_sequential, self.doc_vertical]), + call([self.doc_problem]), + ], + any_order=True, + ) + + @ddt.data( + True, + False + ) + @override_settings(MEILISEARCH_ENABLED=True) + def test_index_xblock_metadata(self, recursive, mock_meilisearch): + """ + Test indexing an XBlock. + """ + api.upsert_xblock_index_doc( + self.sequential.usage_key, + recursive=recursive, + update_metadata=True, + update_tags=False, + ) + + if recursive: + expected_docs = [self.doc_sequential, self.doc_vertical] + else: + expected_docs = [self.doc_sequential] + + mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with(expected_docs) + + @override_settings(MEILISEARCH_ENABLED=True) + def test_delete_index_xblock(self, mock_meilisearch): + """ + Test deleting an XBlock doc from the index. + """ + api.delete_index_doc(self.sequential.usage_key) + + mock_meilisearch.return_value.index.return_value.delete_document.assert_called_once_with( + self.doc_sequential['id'] + ) + + @override_settings(MEILISEARCH_ENABLED=True) + def test_index_library_block_metadata(self, mock_meilisearch): + """ + Test indexing a Library Block. + """ + api.upsert_library_block_index_doc( + self.problem.usage_key, + update_metadata=True, + update_tags=False, + ) + + mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([self.doc_problem]) + + @override_settings(MEILISEARCH_ENABLED=True) + def test_delete_index_library_block(self, mock_meilisearch): + """ + Test deleting a Library Block doc from the index. + """ + api.delete_index_doc(self.problem.usage_key) + + mock_meilisearch.return_value.index.return_value.delete_document.assert_called_once_with( + self.doc_problem['id'] + ) + + @override_settings(MEILISEARCH_ENABLED=True) + def test_index_content_library_metadata(self, mock_meilisearch): + """ + Test indexing a whole content library. + """ + api.upsert_content_library_index_docs(self.library.key) + + mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([self.doc_problem]) diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index 97b2fb5d033f..f653491e4efa 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -2,6 +2,7 @@ Tests for the Studio content search documents (what gets stored in the index) """ from organizations.models import Organization + from openedx.core.djangoapps.content_tagging import api as tagging_api from openedx.core.djangolib.testing.utils import skip_unless_cms from xmodule.modulestore.django import modulestore diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py new file mode 100644 index 000000000000..d6ccdd5a343f --- /dev/null +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -0,0 +1,144 @@ +""" +Tests for the search index update handlers +""" +from unittest.mock import MagicMock, patch + +from django.test import LiveServerTestCase, override_settings +from organizations.tests.factories import OrganizationFactory + +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.content_libraries import api as library_api +from openedx.core.djangolib.testing.utils import skip_unless_cms +from openedx.core.lib.blockstore_api.tests.base import BlockstoreAppTestMixin +from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase + +from .. import api + + +@patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) +@patch("openedx.core.djangoapps.content.search.api.MeilisearchClient") +@override_settings(MEILISEARCH_ENABLED=True) +@skip_unless_cms +class TestUpdateIndexHandlers( + ModuleStoreTestCase, + BlockstoreAppTestMixin, + LiveServerTestCase, +): + """ + Test that the search index is updated when XBlocks and Library Blocks are modified + """ + + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + + def setUp(self): + super().setUp() + # Create user + self.user = UserFactory.create() + self.user_id = self.user.id + + self.orgA = OrganizationFactory.create(short_name="orgA") + + self.patcher = patch("openedx.core.djangoapps.content_tagging.tasks.modulestore", return_value=self.store) + self.addCleanup(self.patcher.stop) + self.patcher.start() + + api.clear_meilisearch_client() # Clear the Meilisearch client to avoid leaking state from other tests + + def test_create_delete_xblock(self, meilisearch_client): + # Create course + course = self.store.create_course( + self.orgA.short_name, + "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") + doc_sequential = { + "id": "block-v1orgatest_coursetest_runtypesequentialblocktest_sequential-0cdb9395", + "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": {} + } + meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_sequential]) + vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") + doc_vertical = { + "id": "block-v1orgatest_coursetest_runtypeverticalblocktest_vertical-011f143b", + "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": {} + } + + meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_vertical]) + + # Update the XBlock + sequential = self.store.get_item(sequential.location, self.user_id) # Refresh the XBlock + sequential.display_name = "Updated Sequential" + self.store.update_item(sequential, self.user_id) + + # The display name and the child's breadcrumbs should be updated + doc_sequential["display_name"] = "Updated Sequential" + doc_vertical["breadcrumbs"][1]["display_name"] = "Updated Sequential" + meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([ + doc_sequential, + doc_vertical, + ]) + + # Delete the XBlock + self.store.delete_item(vertical.location, self.user_id) + + meilisearch_client.return_value.index.return_value.delete_document.assert_called_with( + "block-v1orgatest_coursetest_runtypeverticalblocktest_vertical-011f143b" + ) + + def test_create_delete_library_block(self, meilisearch_client): + # Create library + library = library_api.create_library( + org=self.orgA, + slug="lib_a", + title="Library Org A", + description="This is a library from Org A", + ) + + problem = library_api.create_library_block(library.key, "problem", "Problem1") + doc_problem = { + "id": "lborgalib_aproblemproblem1-ca3186e9", + "type": "library_block", + "usage_key": "lb:orgA:lib_a:problem:Problem1", + "block_id": "Problem1", + "display_name": "Blank Problem", + "block_type": "problem", + "context_key": "lib:orgA:lib_a", + "org": "orgA", + "breadcrumbs": [{"display_name": "Library Org A"}], + "content": {"problem_types": [], "capa_content": " "} + } + + meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_problem]) + + # Rename the content library + library_api.update_library(library.key, title="Updated Library Org A") + + # The breadcrumbs should be updated + doc_problem["breadcrumbs"][0]["display_name"] = "Updated Library Org A" + meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_problem]) + + # Delete the Library Block + library_api.delete_library_block(problem.usage_key) + + meilisearch_client.return_value.index.return_value.delete_document.assert_called_with( + "lborgalib_aproblemproblem1-ca3186e9" + ) diff --git a/openedx/core/djangoapps/content/search/tests/test_views.py b/openedx/core/djangoapps/content/search/tests/test_views.py index e117cf1b969a..f90b1e7970ef 100644 --- a/openedx/core/djangoapps/content/search/tests/test_views.py +++ b/openedx/core/djangoapps/content/search/tests/test_views.py @@ -1,17 +1,24 @@ """ Tests for the Studio content search REST API. """ +from __future__ import annotations + +from unittest.mock import MagicMock, patch + 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 .. import api + STUDIO_SEARCH_ENDPOINT_URL = "/api/content_search/v2/studio/" @skip_unless_cms +@patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) +@patch("openedx.core.djangoapps.content.search.api.MeilisearchClient") class StudioSearchViewTest(APITestCase): """ General tests for the Studio search REST API. @@ -31,8 +38,11 @@ def setUp(self): super().setUp() self.client = APIClient() + # Clear the Meilisearch client to avoid side effects from other tests + api.clear_meilisearch_client() + @override_settings(MEILISEARCH_ENABLED=False) - def test_studio_search_unathenticated_disabled(self): + def test_studio_search_unathenticated_disabled(self, _meilisearch_client): """ Whether or not Meilisearch is enabled, the API endpoint requires authentication. """ @@ -40,7 +50,7 @@ def test_studio_search_unathenticated_disabled(self): assert result.status_code == 401 @override_settings(MEILISEARCH_ENABLED=True) - def test_studio_search_unathenticated_enabled(self): + def test_studio_search_unathenticated_enabled(self, _meilisearch_client): """ Whether or not Meilisearch is enabled, the API endpoint requires authentication. """ @@ -48,7 +58,7 @@ def test_studio_search_unathenticated_enabled(self): assert result.status_code == 401 @override_settings(MEILISEARCH_ENABLED=False) - def test_studio_search_disabled(self): + def test_studio_search_disabled(self, _meilisearch_client): """ When Meilisearch is disabled, the Studio search endpoint gives a 404 """ @@ -57,7 +67,7 @@ def test_studio_search_disabled(self): assert result.status_code == 404 @override_settings(MEILISEARCH_ENABLED=True) - def test_studio_search_student_forbidden(self): + def test_studio_search_student_forbidden(self, _meilisearch_client): """ Until we implement fine-grained permissions, only global staff can use the Studio search endpoint. @@ -67,14 +77,13 @@ 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, _meilisearch_client): """ Global staff can get a restricted API key for Meilisearch using the REST API. """ + _meilisearch_client.return_value.generate_tenant_token.return_value = "api_key" 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 69b686a3434f..b6be2cfb0c79 100644 --- a/openedx/core/djangoapps/content/search/views.py +++ b/openedx/core/djangoapps/content/search/views.py @@ -1,34 +1,22 @@ """ REST API for content search """ -from datetime import datetime, timedelta, timezone import logging -from django.conf import settings from django.contrib.auth import get_user_model -import meilisearch from rest_framework.exceptions import NotFound, PermissionDenied from rest_framework.response import Response from rest_framework.views import APIView from common.djangoapps.student.roles import GlobalStaff from openedx.core.lib.api.view_utils import view_auth_classes -from openedx.core.djangoapps.content.search.documents import STUDIO_INDEX_NAME + +from . import api User = get_user_model() log = logging.getLogger(__name__) -def _get_meili_api_key_uid(): - """ - Helper method to get the UID of the API key we're using for Meilisearch - """ - if not hasattr(_get_meili_api_key_uid, "uid"): - client = meilisearch.Client(settings.MEILISEARCH_URL, settings.MEILISEARCH_API_KEY) - _get_meili_api_key_uid.uid = client.get_key(settings.MEILISEARCH_API_KEY).uid - return _get_meili_api_key_uid.uid - - @view_auth_classes(is_authenticated=True) class StudioSearchView(APIView): """ @@ -39,33 +27,13 @@ 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, # because it lets you search data from any course/library. raise PermissionDenied("For the moment, use of this search preview is restricted to global staff.") - client = meilisearch.Client(settings.MEILISEARCH_URL, settings.MEILISEARCH_API_KEY) - index_name = settings.MEILISEARCH_INDEX_PREFIX + STUDIO_INDEX_NAME - # Create an API key that only allows the user to search content that they have permission to view: - expires_at = datetime.now(tz=timezone.utc) + timedelta(days=7) - search_rules = { - index_name: { - # TODO: Apply filters here based on the user's permissions, so they can only search for content - # that they have permission to view. Example: - # 'filter': 'org = BradenX' - } - } - # Note: the following is just generating a JWT. It doesn't actually make an API call to Meilisearch. - restricted_api_key = client.generate_tenant_token( - api_key_uid=_get_meili_api_key_uid(), - search_rules=search_rules, - expires_at=expires_at, - ) + response_data = api.generate_user_token(request.user) - return Response({ - "url": settings.MEILISEARCH_PUBLIC_URL, - "index_name": index_name, - "api_key": restricted_api_key, - }) + return Response(response_data) From b834d398c727cd7d6c34258942c089a4217f623e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 3 Apr 2024 18:55:27 -0300 Subject: [PATCH 02/18] feat: add celery retry to search tasks --- .../core/djangoapps/content/search/tasks.py | 81 ++++++------------- 1 file changed, 26 insertions(+), 55 deletions(-) diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py index 18ab232119d8..6a551bd63536 100644 --- a/openedx/core/djangoapps/content/search/tasks.py +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -11,102 +11,73 @@ from edx_django_utils.monitoring import set_code_owner_attribute from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 +from meilisearch.errors import MeilisearchError from . import api log = logging.getLogger(__name__) -@shared_task(base=LoggedTask) +@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute -def upsert_xblock_index_doc(usage_key_str: str, recursive: bool, update_metadata: bool, update_tags: bool) -> bool: +def upsert_xblock_index_doc(usage_key_str: str, recursive: bool, update_metadata: bool, update_tags: bool) -> None: """ Celery task to update the content index document for an XBlock """ - try: - usage_key = UsageKey.from_string(usage_key_str) + usage_key = UsageKey.from_string(usage_key_str) - log.info("Updating content index document for XBlock with id: %s", usage_key) + log.info("Updating content index document for XBlock with id: %s", usage_key) - api.upsert_xblock_index_doc(usage_key, recursive, update_metadata, update_tags) + api.upsert_xblock_index_doc(usage_key, recursive, update_metadata, update_tags) - return True - except Exception as e: # pragma: no cover # pylint: disable=broad-except - log.error("Error updating content index document for XBlock with id: %s. %s", usage_key_str, e) - return False - -@shared_task(base=LoggedTask) +@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute -def delete_xblock_index_doc(usage_key_str: str) -> bool: +def delete_xblock_index_doc(usage_key_str: str) -> None: """ Celery task to delete the content index document for an XBlock """ - try: - usage_key = UsageKey.from_string(usage_key_str) - - log.info("Updating content index document for XBlock with id: %s", usage_key) + usage_key = UsageKey.from_string(usage_key_str) - api.delete_index_doc(usage_key) + log.info("Updating content index document for XBlock with id: %s", usage_key) - return True - except Exception as e: # pragma: no cover # pylint: disable=broad-except - log.error("Error deleting content index document for XBlock with id: %s. %s", usage_key_str, e) - return False + api.delete_index_doc(usage_key) -@shared_task(base=LoggedTask) +@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute -def upsert_library_block_index_doc(usage_key_str: str, update_metadata: bool, update_tags: bool) -> bool: +def upsert_library_block_index_doc(usage_key_str: str, update_metadata: bool, update_tags: bool) -> None: """ Celery task to update the content index document for a library block """ - try: - usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) - - log.info("Updating content index document for library block with id: %s", usage_key) + usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) - api.upsert_library_block_index_doc(usage_key, update_metadata, update_tags) + log.info("Updating content index document for library block with id: %s", usage_key) - return True - except Exception as e: # pragma: no cover # pylint: disable=broad-except - log.error("Error updating content index document for libray block with id: %s. %s", usage_key_str, e) - return False + api.upsert_library_block_index_doc(usage_key, update_metadata, update_tags) -@shared_task(base=LoggedTask) +@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute -def delete_library_block_index_doc(usage_key_str: str) -> bool: +def delete_library_block_index_doc(usage_key_str: str) -> None: """ Celery task to delete the content index document for a library block """ - try: - usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) + usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) - log.info("Deleting content index document for library block with id: %s", usage_key) + log.info("Deleting content index document for library block with id: %s", usage_key) - api.delete_index_doc(usage_key) + api.delete_index_doc(usage_key) - return True - except Exception as e: # pragma: no cover # pylint: disable=broad-except - log.error("Error deleting content index document for library block with id: %s. %s", usage_key_str, e) - return False - -@shared_task(base=LoggedTask) +@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute -def update_content_library_index_docs(library_key_str: str) -> bool: +def update_content_library_index_docs(library_key_str: str) -> None: """ Celery task to update the content index documents for all library blocks in a library """ - try: - library_key = LibraryLocatorV2.from_string(library_key_str) - - log.info("Updating content index documents for library with id: %s", library_key) + library_key = LibraryLocatorV2.from_string(library_key_str) - api.upsert_content_library_index_docs(library_key) + log.info("Updating content index documents for library with id: %s", library_key) - return True - except Exception as e: # pragma: no cover # pylint: disable=broad-except - log.error("Error updating content index documents for library with id: %s. %s", library_key, e) - return False + api.upsert_content_library_index_docs(library_key) From 814d0a6680c02789ddf2d8d0b8d42bc5f693ea3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 5 Apr 2024 18:04:39 -0300 Subject: [PATCH 03/18] fix: use log.info if status callback not defined --- openedx/core/djangoapps/content/search/api.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index c808ce07fabb..c4e9bd19c71e 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -169,11 +169,8 @@ def _using_temp_index(status_cb: Callable[[str], None] | None = None) -> Generat Create a new temporary Meilisearch index, populate it, then swap it to become the active index. """ - def nop(_): - pass - if status_cb is None: - status_cb = nop + status_cb = log.info client = _get_meilisearch_client() status_cb("Checking index...") @@ -253,11 +250,8 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: """ Rebuild the Meilisearch index from scratch """ - def nop(_message): - pass - if status_cb is None: - status_cb = nop + status_cb = log.info client = _get_meilisearch_client() store = modulestore() From bb7ac372660fd3a22bde3e592d426a092a332584 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 5 Apr 2024 18:08:03 -0300 Subject: [PATCH 04/18] fix: update docstring and reduce sleep time --- openedx/core/djangoapps/content/search/api.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index c4e9bd19c71e..9cb773269a87 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -125,11 +125,13 @@ def _get_meili_api_key_uid(): def _wait_for_meili_task(info: TaskInfo) -> None: """ Simple helper method to wait for a Meilisearch task to complete + This method will block until the task is completed, so it should only be used in celery tasks + or management commands. """ client = _get_meilisearch_client() current_status = client.get_task(info.task_uid) while current_status.status in ("enqueued", "processing"): - time.sleep(1) + time.sleep(0.5) current_status = client.get_task(info.task_uid) if current_status.status != "succeeded": try: From d2c3c539b6c615c1b18120b0cf55a703dbd84d83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 5 Apr 2024 18:24:46 -0300 Subject: [PATCH 05/18] refactor: remove update_metadata and update_tags parameters --- openedx/core/djangoapps/content/search/api.py | 43 ++++++--------- .../djangoapps/content/search/documents.py | 52 ++++++++----------- .../djangoapps/content/search/handlers.py | 6 +-- .../core/djangoapps/content/search/tasks.py | 8 +-- .../content/search/tests/test_api.py | 13 +---- .../content/search/tests/test_documents.py | 11 ++-- 6 files changed, 54 insertions(+), 79 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 9cb773269a87..38000cd88fb8 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -19,18 +19,18 @@ from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import LibraryLocatorV2 -from openedx.core.djangoapps.content.search.documents import ( +from openedx.core.djangoapps.content_libraries import api as lib_api +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore + +from .documents import ( STUDIO_INDEX_NAME, Fields, meili_id_from_opaque_key, searchable_doc_for_course_block, - searchable_doc_for_library_block + searchable_doc_for_library_block, + searchable_doc_tags ) -from openedx.core.djangoapps.content_libraries import api as lib_api -from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.django import modulestore - -from .documents import Fields, searchable_doc_for_course_block, searchable_doc_for_library_block log = logging.getLogger(__name__) @@ -300,8 +300,11 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: docs = [] for component in lib_api.get_library_components(lib_key): metadata = lib_api.LibraryXBlockMetadata.from_component(lib_key, component) - doc = searchable_doc_for_library_block(metadata) + doc = {} + doc.update(searchable_doc_for_library_block(metadata)) + doc.update(searchable_doc_tags(metadata.usage_key)) docs.append(doc) + num_blocks_done += 1 if docs: # Add all the docs in this library at once (usually faster than adding one at a time): @@ -336,9 +339,7 @@ def add_with_children(block): status_cb(f"Done! {num_blocks_done} blocks indexed across {num_contexts_done} courses and libraries.") -def upsert_xblock_index_doc( - usage_key: UsageKey, recursive: bool = True, update_metadata: bool = True, update_tags: bool = True -) -> None: +def upsert_xblock_index_doc(usage_key: UsageKey, recursive: bool = True) -> None: """ Creates or updates the document for the given XBlock in the search index @@ -346,8 +347,6 @@ def upsert_xblock_index_doc( 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 """ current_rebuild_index_name = _get_running_rebuild_index_name() @@ -358,7 +357,7 @@ def upsert_xblock_index_doc( def add_with_children(block): """ Recursively index the given XBlock/component """ - doc = searchable_doc_for_course_block(block, include_metadata=update_metadata, include_tags=update_tags) + doc = searchable_doc_for_course_block(block) docs.append(doc) if recursive: _recurse_children(block, add_with_children) @@ -397,17 +396,9 @@ def delete_index_doc(usage_key: UsageKey) -> None: _wait_for_meili_tasks(tasks) -def upsert_library_block_index_doc( - usage_key: UsageKey, update_metadata: bool = True, update_tags: bool = True -) -> None: +def upsert_library_block_index_doc(usage_key: UsageKey) -> None: """ Creates or updates the document for the given Library Block in the search index - - - Args: - usage_key (UsageKey): The usage key of the Library Block to index - update_metadata (bool): If True, update the metadata of the Library Block - update_tags (bool): If True, update the tags of the Library Block """ current_rebuild_index_name = _get_running_rebuild_index_name() @@ -416,9 +407,7 @@ def upsert_library_block_index_doc( client = _get_meilisearch_client() docs = [ - searchable_doc_for_library_block( - library_block_metadata, include_metadata=update_metadata, include_tags=update_tags - ) + searchable_doc_for_library_block(library_block_metadata) ] tasks = [] @@ -440,7 +429,7 @@ def upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None: docs = [] for component in lib_api.get_library_components(library_key): metadata = lib_api.LibraryXBlockMetadata.from_component(library_key, component) - doc = searchable_doc_for_library_block(metadata, include_metadata=True, include_tags=False) + doc = searchable_doc_for_library_block(metadata) docs.append(doc) tasks = [] diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 2d76a6eaf3d8..7573c1bab026 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -160,7 +160,7 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: # Note that we could improve performance for indexing many components from the same library/course, # if we used get_all_object_tags() to load all the tags for the library in a single query rather than loading the # tags for each component separately. - all_tags = tagging_api.get_object_tags(object_id).all() + all_tags = tagging_api.get_object_tags(str(object_id)).all() if not all_tags: return {} result = { @@ -170,10 +170,10 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: } for obj_tag in all_tags: # Add the taxonomy name: - if obj_tag.name not in result[Fields.tags_taxonomy]: - result[Fields.tags_taxonomy].append(obj_tag.name) + if obj_tag.taxonomy.name not in result[Fields.tags_taxonomy]: + result[Fields.tags_taxonomy].append(obj_tag.taxonomy.name) # Taxonomy name plus each level of tags, in a list: - parts = [obj_tag.name] + obj_tag.get_lineage() # e.g. ["Location", "North America", "Canada", "Vancouver"] + parts = [obj_tag.taxonomy.name] + obj_tag.get_lineage() # e.g. ["Location", "North America", "Canada", "Vancouver"] parts = [part.replace(" > ", " _ ") for part in parts] # Escape our separator. # Now we build each level (tags.level0, tags.level1, etc.) as applicable. # We have a hard-coded limit of 4 levels of tags for now (see Fields.tags above). @@ -196,18 +196,11 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: return {Fields.tags: result} -def searchable_doc_for_library_block( - xblock_metadata: lib_api.LibraryXBlockMetadata, include_metadata: bool = True, include_tags: bool = True -) -> dict: +def searchable_doc_for_library_block(xblock_metadata: lib_api.LibraryXBlockMetadata) -> dict: """ Generate a dictionary document suitable for ingestion into a search engine like Meilisearch or Elasticsearch, so that the given library block can be found using faceted search. - - Args: - xblock_metadata: The XBlock component metadata to index - include_metadata: If True, include the block's metadata in the doc - include_tags: If True, include the block's tags in the doc """ library_name = lib_api.get_library(xblock_metadata.usage_key.context_key).title block = xblock_api.load_block(xblock_metadata.usage_key, user=None) @@ -217,37 +210,38 @@ def searchable_doc_for_library_block( Fields.type: DocType.library_block, } - if include_metadata: - doc.update(_fields_from_block(block)) - # Add the breadcrumbs. In v2 libraries, the library itself is not a "parent" of the XBlocks so we add it here: - doc[Fields.breadcrumbs] = [{"display_name": library_name}] + doc.update(_fields_from_block(block)) + + # Add the breadcrumbs. In v2 libraries, the library itself is not a "parent" of the XBlocks so we add it here: + doc[Fields.breadcrumbs] = [{"display_name": library_name}] + + return doc + - if include_tags: - doc.update(_tags_for_content_object(xblock_metadata.usage_key)) +def searchable_doc_tags(usage_key: UsageKey) -> dict: + """ + Generate a dictionary document suitable for ingestion into a search engine + like Meilisearch or Elasticsearch, with the tags data for the given content object. + """ + doc = { + Fields.id: meili_id_from_opaque_key(usage_key), + } + doc.update(_tags_for_content_object(usage_key)) return doc -def searchable_doc_for_course_block(block, include_metadata: bool = True, include_tags: bool = True) -> dict: +def searchable_doc_for_course_block(block) -> dict: """ Generate a dictionary document suitable for ingestion into a search engine like Meilisearch or Elasticsearch, so that the given course block can be found using faceted search. - - Args: - block: The XBlock instance to index - include_metadata: If True, include the block's metadata in the doc - include_tags: If True, include the block's tags in the doc """ doc = { Fields.id: meili_id_from_opaque_key(block.usage_key), Fields.type: DocType.course_block, } - if include_metadata: - doc.update(_fields_from_block(block)) - - if include_tags: - doc.update(_tags_for_content_object(block.usage_key)) + doc.update(_fields_from_block(block)) return doc diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 6a7bd152fef7..fdc59c16bd28 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -41,8 +41,6 @@ def xblock_created_handler(**kwargs) -> None: upsert_xblock_index_doc.delay( str(xblock_info.usage_key), recursive=False, - update_metadata=True, - update_tags=False, ) @@ -60,8 +58,6 @@ def xblock_updated_handler(**kwargs) -> None: upsert_xblock_index_doc.delay( str(xblock_info.usage_key), recursive=True, # Update all children because the breadcrumb may have changed - update_metadata=True, - update_tags=False, ) @@ -90,7 +86,7 @@ def library_block_updated_handler(**kwargs) -> None: log.error("Received null or incorrect data for event") return - upsert_library_block_index_doc.delay(str(library_block_data.usage_key), update_metadata=True, update_tags=False) + upsert_library_block_index_doc.delay(str(library_block_data.usage_key)) @receiver(LIBRARY_BLOCK_DELETED) diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py index 6a551bd63536..06ea3e777c61 100644 --- a/openedx/core/djangoapps/content/search/tasks.py +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -20,7 +20,7 @@ @shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute -def upsert_xblock_index_doc(usage_key_str: str, recursive: bool, update_metadata: bool, update_tags: bool) -> None: +def upsert_xblock_index_doc(usage_key_str: str, recursive: bool) -> None: """ Celery task to update the content index document for an XBlock """ @@ -28,7 +28,7 @@ def upsert_xblock_index_doc(usage_key_str: str, recursive: bool, update_metadata log.info("Updating content index document for XBlock with id: %s", usage_key) - api.upsert_xblock_index_doc(usage_key, recursive, update_metadata, update_tags) + api.upsert_xblock_index_doc(usage_key, recursive) @shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @@ -46,7 +46,7 @@ def delete_xblock_index_doc(usage_key_str: str) -> None: @shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute -def upsert_library_block_index_doc(usage_key_str: str, update_metadata: bool, update_tags: bool) -> None: +def upsert_library_block_index_doc(usage_key_str: str) -> None: """ Celery task to update the content index document for a library block """ @@ -54,7 +54,7 @@ def upsert_library_block_index_doc(usage_key_str: str, update_metadata: bool, up log.info("Updating content index document for library block with id: %s", usage_key) - api.upsert_library_block_index_doc(usage_key, update_metadata, update_tags) + api.upsert_library_block_index_doc(usage_key) @shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 50d361ff6b16..aa425c06e7c6 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -134,12 +134,7 @@ def test_index_xblock_metadata(self, recursive, mock_meilisearch): """ Test indexing an XBlock. """ - api.upsert_xblock_index_doc( - self.sequential.usage_key, - recursive=recursive, - update_metadata=True, - update_tags=False, - ) + api.upsert_xblock_index_doc(self.sequential.usage_key, recursive=recursive) if recursive: expected_docs = [self.doc_sequential, self.doc_vertical] @@ -164,11 +159,7 @@ def test_index_library_block_metadata(self, mock_meilisearch): """ Test indexing a Library Block. """ - api.upsert_library_block_index_doc( - self.problem.usage_key, - update_metadata=True, - update_tags=False, - ) + api.upsert_library_block_index_doc(self.problem.usage_key) mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([self.doc_problem]) diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index f653491e4efa..63bc5f49e568 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -9,7 +9,7 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, ToyCourseFactory -from ..documents import searchable_doc_for_course_block +from ..documents import searchable_doc_for_course_block, searchable_doc_tags STUDIO_SEARCH_ENDPOINT_URL = "/api/content_search/v2/studio/" @@ -61,7 +61,10 @@ def test_problem_block(self): Test how a problem block gets represented in the search index """ block = self.store.get_item(self.problem_block.usage_key) - doc = searchable_doc_for_course_block(block) + doc = {} + doc.update(searchable_doc_for_course_block(block)) + doc.update(searchable_doc_tags(block.usage_key)) + assert doc == { # Note the 'id' has been stripped of special characters to meet Meilisearch requirements. # The '-8516ed8' suffix is deterministic based on the original usage key. @@ -97,7 +100,9 @@ def test_html_block(self): Test how an HTML block gets represented in the search index """ block = self.store.get_item(self.html_block_key) - doc = searchable_doc_for_course_block(block) + doc = {} + doc.update(searchable_doc_for_course_block(block)) + doc.update(searchable_doc_tags(block.usage_key)) assert doc == { "id": "block-v1edxtoy2012_falltypehtmlblocktoyjumpto-efb9c601", "type": "course_block", From 1c498b8b14cc4f27e43d7d23a83cd075c29656db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 5 Apr 2024 18:28:00 -0300 Subject: [PATCH 06/18] refactor: rename generate_user_token to generate_user_token_for_studio_search --- openedx/core/djangoapps/content/search/api.py | 2 +- openedx/core/djangoapps/content/search/views.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 38000cd88fb8..59717a0a8ab5 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -445,7 +445,7 @@ def upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None: _wait_for_meili_tasks(tasks) -def generate_user_token(user): +def generate_user_token_for_studio_search(user): """ Returns a Meilisearch API key that only allows the user to search content that they have permission to view """ diff --git a/openedx/core/djangoapps/content/search/views.py b/openedx/core/djangoapps/content/search/views.py index b6be2cfb0c79..70b88285c8cf 100644 --- a/openedx/core/djangoapps/content/search/views.py +++ b/openedx/core/djangoapps/content/search/views.py @@ -34,6 +34,6 @@ def get(self, request): # because it lets you search data from any course/library. raise PermissionDenied("For the moment, use of this search preview is restricted to global staff.") - response_data = api.generate_user_token(request.user) + response_data = api.generate_user_token_for_studio_search(request.user) return Response(response_data) From 8cd8f6554864514a7127ac87c27d60c71cba5d36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 5 Apr 2024 18:29:32 -0300 Subject: [PATCH 07/18] docs: fix docstring Co-authored-by: Braden MacDonald --- openedx/core/djangoapps/content/search/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 59717a0a8ab5..da3c41829067 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -389,7 +389,7 @@ def delete_index_doc(usage_key: UsageKey) -> None: tasks = [] if current_rebuild_index_name: - # If there is a rebuild in progress, the document will also be added to the new index. + # If there is a rebuild in progress, the document will also be deleted from 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))) From e3bc29d31f59e86f4f4b632babb05ea2a42ac24f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 5 Apr 2024 18:39:50 -0300 Subject: [PATCH 08/18] refactor: rename index name constants --- openedx/core/djangoapps/content/search/api.py | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index da3c41829067..7b1b0e88cb51 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -24,7 +24,6 @@ from xmodule.modulestore.django import modulestore from .documents import ( - STUDIO_INDEX_NAME, Fields, meili_id_from_opaque_key, searchable_doc_for_course_block, @@ -36,12 +35,12 @@ User = get_user_model() -STUDIO_INDEX_NAME = "studio_content" +STUDIO_INDEX_SUFFIX = "studio_content" if hasattr(settings, "MEILISEARCH_INDEX_PREFIX"): - INDEX_NAME = settings.MEILISEARCH_INDEX_PREFIX + STUDIO_INDEX_NAME + STUDIO_INDEX_NAME = settings.MEILISEARCH_INDEX_PREFIX + STUDIO_INDEX_SUFFIX else: - INDEX_NAME = STUDIO_INDEX_NAME + STUDIO_INDEX_NAME = STUDIO_INDEX_SUFFIX _MEILI_CLIENT = None @@ -56,8 +55,8 @@ 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-{STUDIO_INDEX_NAME}" + new_index_name = STUDIO_INDEX_NAME + "_new" while True: status = cache.add(lock_id, new_index_name, LOCK_EXPIRE) @@ -77,7 +76,7 @@ def _index_rebuild_lock() -> Generator[str, None, None]: def _get_running_rebuild_index_name() -> str | None: - lock_id = f"lock-meilisearch-index-{INDEX_NAME}" + lock_id = f"lock-meilisearch-index-{STUDIO_INDEX_NAME}" return cache.get(lock_id) @@ -189,17 +188,17 @@ def _using_temp_index(status_cb: Callable[[str], None] | None = None) -> Generat yield temp_index_name - if not _index_exists(INDEX_NAME): + if not _index_exists(STUDIO_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(INDEX_NAME)) + _wait_for_meili_task(client.create_index(STUDIO_INDEX_NAME)) status_cb("Swapping index...") - client.swap_indexes([{'indexes': [temp_index_name, INDEX_NAME]}]) + client.swap_indexes([{'indexes': [temp_index_name, STUDIO_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(INDEX_NAME).created_at != new_index_created: + if client.get_index(STUDIO_INDEX_NAME).created_at != new_index_created: status_cb("Waiting for swap completion...") else: break @@ -371,7 +370,7 @@ def add_with_children(block): if current_rebuild_index_name: # 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)) + tasks.append(client.index(STUDIO_INDEX_NAME).update_documents(docs)) _wait_for_meili_tasks(tasks) @@ -391,7 +390,7 @@ def delete_index_doc(usage_key: UsageKey) -> None: if current_rebuild_index_name: # If there is a rebuild in progress, the document will also be deleted from 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))) + tasks.append(client.index(STUDIO_INDEX_NAME).delete_document(meili_id_from_opaque_key(usage_key))) _wait_for_meili_tasks(tasks) @@ -414,7 +413,7 @@ def upsert_library_block_index_doc(usage_key: UsageKey) -> None: if current_rebuild_index_name: # 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)) + tasks.append(client.index(STUDIO_INDEX_NAME).update_documents(docs)) _wait_for_meili_tasks(tasks) @@ -440,7 +439,7 @@ def upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None: if current_rebuild_index_name: # 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)) + tasks.append(client.index(STUDIO_INDEX_NAME).update_documents(docs)) _wait_for_meili_tasks(tasks) @@ -451,7 +450,7 @@ def generate_user_token_for_studio_search(user): """ expires_at = datetime.now(tz=timezone.utc) + timedelta(days=7) search_rules = { - INDEX_NAME: { + STUDIO_INDEX_NAME: { # TODO: Apply filters here based on the user's permissions, so they can only search for content # that they have permission to view. Example: # 'filter': 'org = BradenX' @@ -466,6 +465,6 @@ def generate_user_token_for_studio_search(user): return { "url": settings.MEILISEARCH_PUBLIC_URL, - "index_name": INDEX_NAME, + "index_name": STUDIO_INDEX_NAME, "api_key": restricted_api_key, } From 0c20792af0f4afe7c916675c00f01d39672fd242 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 5 Apr 2024 18:49:18 -0300 Subject: [PATCH 09/18] refactor: create _update_index_docs helper --- openedx/core/djangoapps/content/search/api.py | 60 ++++++++----------- 1 file changed, 24 insertions(+), 36 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 7b1b0e88cb51..1543093ae339 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -225,6 +225,27 @@ def _recurse_children(block, fn, status_cb: Callable[[str], None] | None = None) fn(child) +def _update_index_docs(docs)-> None: + """ + Helper function that updates the documents in the search index + + If there is a rebuild in progress, the document will also be added to the new index. + """ + if not docs: + return + + client = _get_meilisearch_client() + current_rebuild_index_name = _get_running_rebuild_index_name() + + tasks = [] + if current_rebuild_index_name: + # 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(STUDIO_INDEX_NAME).update_documents(docs)) + + _wait_for_meili_tasks(tasks) + + def only_if_meilisearch_enabled(f): """ Only call `f` if meilisearch is enabled @@ -347,10 +368,7 @@ def upsert_xblock_index_doc(usage_key: UsageKey, recursive: bool = True) -> None usage_key (UsageKey): The usage key of the XBlock to index recursive (bool): If True, also index all children of the XBlock """ - current_rebuild_index_name = _get_running_rebuild_index_name() - xblock = modulestore().get_item(usage_key) - client = _get_meilisearch_client() docs = [] @@ -363,16 +381,7 @@ def add_with_children(block): add_with_children(xblock) - if not docs: - return - - tasks = [] - if current_rebuild_index_name: - # 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(STUDIO_INDEX_NAME).update_documents(docs)) - - _wait_for_meili_tasks(tasks) + _update_index_docs(docs) def delete_index_doc(usage_key: UsageKey) -> None: @@ -399,49 +408,28 @@ def upsert_library_block_index_doc(usage_key: UsageKey) -> None: """ Creates or updates the document for the given Library Block in the search index """ - current_rebuild_index_name = _get_running_rebuild_index_name() library_block = lib_api.get_component_from_usage_key(usage_key) library_block_metadata = lib_api.LibraryXBlockMetadata.from_component(usage_key.context_key, library_block) - client = _get_meilisearch_client() docs = [ searchable_doc_for_library_block(library_block_metadata) ] - tasks = [] - if current_rebuild_index_name: - # 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(STUDIO_INDEX_NAME).update_documents(docs)) - - _wait_for_meili_tasks(tasks) + _update_index_docs(docs) def upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None: """ Creates or updates the documents for the given Content Library in the search index """ - current_rebuild_index_name = _get_running_rebuild_index_name() - client = _get_meilisearch_client() - docs = [] for component in lib_api.get_library_components(library_key): metadata = lib_api.LibraryXBlockMetadata.from_component(library_key, component) doc = searchable_doc_for_library_block(metadata) docs.append(doc) - tasks = [] - - if not docs: - return - - if current_rebuild_index_name: - # 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(STUDIO_INDEX_NAME).update_documents(docs)) - - _wait_for_meili_tasks(tasks) + _update_index_docs(docs) def generate_user_token_for_studio_search(user): From c8d265165772bf474d4d8f3feac405f8e5fb4580 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 5 Apr 2024 19:00:01 -0300 Subject: [PATCH 10/18] fix: increase rebuild lock time and throws if rebuild already in progress --- openedx/core/djangoapps/content/search/api.py | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 1543093ae339..6bad09b06579 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -46,7 +46,7 @@ _MEILI_CLIENT = None _MEILI_API_KEY_UID = None -LOCK_EXPIRE = 5 * 60 # Lock expires in 5 minutes +LOCK_EXPIRE = 24 * 60 * 60 # Lock expires in 24 hours @contextmanager @@ -54,25 +54,21 @@ 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-{STUDIO_INDEX_NAME}" new_index_name = STUDIO_INDEX_NAME + "_new" - while True: - status = cache.add(lock_id, new_index_name, LOCK_EXPIRE) - if status: - # Lock acquired - try: - yield new_index_name - break - finally: - # Release the lock - cache.delete(lock_id) + status = cache.add(lock_id, new_index_name, LOCK_EXPIRE) - if time.monotonic() > timeout_at: - raise TimeoutError("Timeout acquiring lock") + if not status: + # Lock already acquired + raise RuntimeError("Rebuild already in progress") - time.sleep(1) + # Lock acquired + try: + yield new_index_name + finally: + # Release the lock + cache.delete(lock_id) def _get_running_rebuild_index_name() -> str | None: From 8e64b137df3c03cd092b7b39ad93197f2123ad31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 5 Apr 2024 19:01:37 -0300 Subject: [PATCH 11/18] docs: add docstring for status_cb --- openedx/core/djangoapps/content/search/api.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 6bad09b06579..e75cb9e346a9 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -165,6 +165,9 @@ def _using_temp_index(status_cb: Callable[[str], None] | None = None) -> Generat """ Create a new temporary Meilisearch index, populate it, then swap it to become the active index. + + Args: + status_cb (Callable): A callback function to report status messages """ if status_cb is None: status_cb = log.info From 3515a2b36d700f203b1560a1afe27e5186bbb34c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 5 Apr 2024 19:49:39 -0300 Subject: [PATCH 12/18] style: fix pylint --- openedx/core/djangoapps/content/search/api.py | 2 +- openedx/core/djangoapps/content/search/documents.py | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index e75cb9e346a9..9d3ef1e30f26 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -224,7 +224,7 @@ def _recurse_children(block, fn, status_cb: Callable[[str], None] | None = None) fn(child) -def _update_index_docs(docs)-> None: +def _update_index_docs(docs) -> None: """ Helper function that updates the documents in the search index diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 7573c1bab026..c0e7654144e1 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -14,7 +14,6 @@ from openedx.core.djangoapps.xblock import api as xblock_api log = logging.getLogger(__name__) -STUDIO_INDEX_NAME = "studio_content" class Fields: @@ -173,7 +172,7 @@ def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: if obj_tag.taxonomy.name not in result[Fields.tags_taxonomy]: result[Fields.tags_taxonomy].append(obj_tag.taxonomy.name) # Taxonomy name plus each level of tags, in a list: - parts = [obj_tag.taxonomy.name] + obj_tag.get_lineage() # e.g. ["Location", "North America", "Canada", "Vancouver"] + parts = [obj_tag.taxonomy.name] + obj_tag.get_lineage() # e.g. ["Location", "North America", "Canada"] parts = [part.replace(" > ", " _ ") for part in parts] # Escape our separator. # Now we build each level (tags.level0, tags.level1, etc.) as applicable. # We have a hard-coded limit of 4 levels of tags for now (see Fields.tags above). From 612f32ff6355fa48f5af2c8fae50503706b70dc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 16 Apr 2024 19:26:00 -0300 Subject: [PATCH 13/18] fix: error in library crash reindex ```File"/openedx/edx-platform/openedx/core/djangoapps/content_libraries/api.py",line 617, in get_library_componentslearning_package.id,AttributeError: 'NoneType' object has no attribute'id'``` --- openedx/core/djangoapps/content/search/api.py | 30 +++++++++++-------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 9d3ef1e30f26..3e1e12dcdfc8 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -316,19 +316,23 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: status_cb("Indexing libraries...") for lib_key in lib_keys: status_cb(f"{num_contexts_done + 1}/{num_contexts}. Now indexing library {lib_key}") - docs = [] - for component in lib_api.get_library_components(lib_key): - metadata = lib_api.LibraryXBlockMetadata.from_component(lib_key, component) - doc = {} - doc.update(searchable_doc_for_library_block(metadata)) - doc.update(searchable_doc_tags(metadata.usage_key)) - docs.append(doc) - - num_blocks_done += 1 - if docs: - # Add all the docs in this library at once (usually faster than adding one at a time): - _wait_for_meili_task(client.index(temp_index_name).add_documents(docs)) - num_contexts_done += 1 + try: + docs = [] + for component in lib_api.get_library_components(lib_key): + metadata = lib_api.LibraryXBlockMetadata.from_component(lib_key, component) + doc = {} + doc.update(searchable_doc_for_library_block(metadata)) + doc.update(searchable_doc_tags(metadata.usage_key)) + docs.append(doc) + + num_blocks_done += 1 + if docs: + # Add all the docs in this library at once (usually faster than adding one at a time): + _wait_for_meili_task(client.index(temp_index_name).add_documents(docs)) + except Exception as err: # pylint: disable=broad-except + status_cb(f"Error indexing library {lib_key}: {err}") + finally: + num_contexts_done += 1 ############## Courses ############## status_cb("Indexing courses...") From 670c04ad95f2ea54f69ff92de1c135b11e88bd45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 17 Apr 2024 17:20:11 -0300 Subject: [PATCH 14/18] fix: fix code and tests after merge --- openedx/core/djangoapps/content/search/api.py | 57 ++++++++++++-- .../content/search/tests/test_api.py | 15 +++- .../content/search/tests/test_handlers.py | 19 ++++- .../content/search/tests/test_views.py | 76 ++++++++----------- .../core/djangoapps/content/search/views.py | 1 + 5 files changed, 114 insertions(+), 54 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 3e1e12dcdfc8..fb76148bf65b 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -18,6 +18,10 @@ from meilisearch.models.task import TaskInfo from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import LibraryLocatorV2 +from common.djangoapps.student.roles import GlobalStaff +from rest_framework.request import Request +from common.djangoapps.student.role_helpers import get_course_roles +from openedx.core.djangoapps.content.search.models import get_access_ids_for_request from openedx.core.djangoapps.content_libraries import api as lib_api from xmodule.modulestore import ModuleStoreEnum @@ -48,6 +52,9 @@ LOCK_EXPIRE = 24 * 60 * 60 # Lock expires in 24 hours +MAX_ACCESS_IDS_IN_FILTER = 1_000 +MAX_ORGS_IN_FILTER = 1_000 + @contextmanager def _index_rebuild_lock() -> Generator[str, None, None]: @@ -310,6 +317,15 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.org, Fields.tags, Fields.type, + Fields.access_id, + ]) + # Mark which attributes are used for keyword search, in order of importance: + client.index(temp_index_name).update_searchable_attributes([ + Fields.display_name, + Fields.block_id, + Fields.content, + Fields.tags, + # Keyword search does _not_ search the course name, course ID, breadcrumbs, block type, or other fields. ]) ############## Libraries ############## @@ -435,17 +451,46 @@ def upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None: _update_index_docs(docs) -def generate_user_token_for_studio_search(user): +def _get_user_orgs(request: Request) -> list[str]: + """ + Get the org.short_names for the organizations that the requesting user has OrgStaffRole or OrgInstructorRole. + + Note: org-level roles have course_id=None to distinguish them from course-level roles. + """ + course_roles = get_course_roles(request.user) + return list(set( + role.org + for role in course_roles + if role.course_id is None and role.role in ['staff', 'instructor'] + )) + + +def _get_meili_access_filter(request: Request) -> dict: + """ + Return meilisearch filter based on the requesting user's permissions. + """ + # Global staff can see anything, so no filters required. + if GlobalStaff().has_user(request.user): + return {} + + # Everyone else is limited to their org staff roles... + user_orgs = _get_user_orgs(request)[:MAX_ORGS_IN_FILTER] + + # ...or the N most recent courses and libraries they can access. + access_ids = get_access_ids_for_request(request, omit_orgs=user_orgs)[:MAX_ACCESS_IDS_IN_FILTER] + return { + "filter": f"org IN {user_orgs} OR access_id IN {access_ids}", + } + + +def generate_user_token_for_studio_search(request): """ Returns a Meilisearch API key that only allows the user to search content that they have permission to view """ expires_at = datetime.now(tz=timezone.utc) + timedelta(days=7) + search_rules = { - STUDIO_INDEX_NAME: { - # TODO: Apply filters here based on the user's permissions, so they can only search for content - # that they have permission to view. Example: - # 'filter': 'org = BradenX' - } + STUDIO_INDEX_NAME: _get_meili_access_filter(request), } # 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( diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index aa425c06e7c6..01929c049a67 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -16,6 +16,12 @@ from .. import api +try: + # This import errors in the lms because content.search is not an installed app there. + from openedx.core.djangoapps.content.search.models import SearchAccess +except RuntimeError: + SearchAccess = {} + STUDIO_SEARCH_ENDPOINT_URL = "/api/content_search/v2/studio/" @@ -52,6 +58,7 @@ def setUp(self): self.user_id, fields={"display_name": "Test Course"}, ) + course_access, _ = SearchAccess.objects.get_or_create(context_key=self.course.id) # Create XBlocks self.sequential = self.store.create_child(self.user_id, self.course.location, "sequential", "test_sequential") @@ -65,7 +72,8 @@ def setUp(self): 'context_key': 'course-v1:org1+test_course+test_run', 'org': 'org1', 'breadcrumbs': [{'display_name': 'Test Course'}], - 'content': {} + 'content': {}, + 'access_id': course_access.id, } self.store.create_child(self.user_id, self.sequential.location, "vertical", "test_vertical") self.doc_vertical = { @@ -81,7 +89,8 @@ def setUp(self): {'display_name': 'Test Course'}, {'display_name': 'sequential'} ], - 'content': {} + 'content': {}, + 'access_id': course_access.id, } # Create a content library: @@ -91,6 +100,7 @@ def setUp(self): slug="lib", title="Library", ) + lib_access, _ = SearchAccess.objects.get_or_create(context_key=self.library.key) # Populate it with a problem: self.problem = library_api.create_library_block(self.library.key, "problem", "p1") self.doc_problem = { @@ -104,6 +114,7 @@ def setUp(self): "breadcrumbs": [{"display_name": "Library"}], "content": {"problem_types": [], "capa_content": " "}, "type": "library_block", + "access_id": lib_access.id, } @override_settings(MEILISEARCH_ENABLED=False) diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index d6ccdd5a343f..22beb6551f1f 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -14,6 +14,13 @@ from .. import api +try: + # This import errors in the lms because content.search is not an installed app there. + from openedx.core.djangoapps.content.search.models import SearchAccess +except RuntimeError: + SearchAccess = {} + + @patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) @patch("openedx.core.djangoapps.content.search.api.MeilisearchClient") @@ -53,6 +60,7 @@ def test_create_delete_xblock(self, meilisearch_client): self.user_id, fields={"display_name": "Test Course"}, ) + course_access, _ = SearchAccess.objects.get_or_create(context_key=course.id) # Create XBlocks sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") @@ -65,7 +73,9 @@ def test_create_delete_xblock(self, meilisearch_client): "block_type": "sequential", "context_key": "course-v1:orgA+test_course+test_run", "org": "orgA", - "breadcrumbs": [{"display_name": "Test Course"}], "content": {} + "breadcrumbs": [{"display_name": "Test Course"}], "content": {}, + "access_id": course_access.id, + } meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_sequential]) vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") @@ -79,7 +89,8 @@ def test_create_delete_xblock(self, meilisearch_client): "context_key": "course-v1:orgA+test_course+test_run", "org": "orgA", "breadcrumbs": [{"display_name": "Test Course"}, {"display_name": "sequential"}], - "content": {} + "content": {}, + "access_id": course_access.id, } meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_vertical]) @@ -112,6 +123,7 @@ def test_create_delete_library_block(self, meilisearch_client): title="Library Org A", description="This is a library from Org A", ) + lib_access, _ = SearchAccess.objects.get_or_create(context_key=library.key) problem = library_api.create_library_block(library.key, "problem", "Problem1") doc_problem = { @@ -124,7 +136,8 @@ def test_create_delete_library_block(self, meilisearch_client): "context_key": "lib:orgA:lib_a", "org": "orgA", "breadcrumbs": [{"display_name": "Library Org A"}], - "content": {"problem_types": [], "capa_content": " "} + "content": {"problem_types": [], "capa_content": " "}, + "access_id": lib_access.id, } meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_problem]) diff --git a/openedx/core/djangoapps/content/search/tests/test_views.py b/openedx/core/djangoapps/content/search/tests/test_views.py index 83751eef37c3..9b32bdf327a8 100644 --- a/openedx/core/djangoapps/content/search/tests/test_views.py +++ b/openedx/core/djangoapps/content/search/tests/test_views.py @@ -4,7 +4,7 @@ from __future__ import annotations import functools -from unittest import mock +from unittest.mock import ANY, MagicMock, Mock, patch import ddt from django.test import override_settings @@ -32,7 +32,7 @@ def mock_meilisearch(enabled=True): """ - Decorator that mocks the required parts of content.search.views to simulate a running Meilisearch instance. + Decorator that mocks the required parts of content.search.api to simulate a running Meilisearch instance. """ def decorator(func): """ @@ -44,17 +44,19 @@ def wrapper(*args, **kwargs): MEILISEARCH_ENABLED=enabled, MEILISEARCH_PUBLIC_URL="http://meilisearch.url", ): - with mock.patch( - 'openedx.core.djangoapps.content.search.views._get_meili_api_key_uid', + with patch( + 'openedx.core.djangoapps.content.search.api._get_meili_api_key_uid', return_value=MOCK_API_KEY_UID, ): return func(*args, **kwargs) + return wrapper return decorator @ddt.ddt @skip_unless_cms +@patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) class StudioSearchViewTest(StudioSearchTestMixin, SharedModuleStoreTestCase): """ General tests for the Studio search REST API. @@ -91,45 +93,33 @@ def test_studio_search_disabled(self): result = self.client.get(STUDIO_SEARCH_ENDPOINT_URL) assert result.status_code == 404 + def _mock_generate_tenant_token(self, mock_search_client): + """ + Return a mocked meilisearch.Client.generate_tenant_token method. + """ + mock_generate_tenant_token = Mock(return_value='restricted_api_key') + mock_search_client.return_value = Mock( + generate_tenant_token=mock_generate_tenant_token, + ) + return mock_generate_tenant_token + @mock_meilisearch(enabled=True) - def test_studio_search_enabled(self): + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') + def test_studio_search_enabled(self, mock_search_client): """ We've implement fine-grained permissions on the meilisearch content, so any logged-in user can get a restricted API key for Meilisearch using the REST API. """ self.client.login(username='student', password='student_pass') + mock_generate_tenant_token = self._mock_generate_tenant_token(mock_search_client) result = self.client.get(STUDIO_SEARCH_ENDPOINT_URL) assert result.status_code == 200 assert result.data["index_name"] == "studio_content" assert result.data["url"] == "http://meilisearch.url" assert result.data["api_key"] and isinstance(result.data["api_key"], str) - @override_settings(MEILISEARCH_ENABLED=True) - def test_studio_search_staff(self, _meilisearch_client): - """ - Global staff can get a restricted API key for Meilisearch using the REST - API. - """ - _meilisearch_client.return_value.generate_tenant_token.return_value = "api_key" - self.client.login(username='staff', password='staff_pass') - result = self.client.get(STUDIO_SEARCH_ENDPOINT_URL) - assert result.status_code == 200 - assert result.data["index_name"] == "studio_content" - assert result.data["url"] == "http://meilisearch.url" - assert result.data["api_key"] and isinstance(result.data["api_key"], str) - - def _mock_generate_tenant_token(self, mock_search_client): - """ - Return a mocked meilisearch.Client.generate_tenant_token method. - """ - mock_generate_tenant_token = mock.Mock(return_value='restricted_api_key') - mock_search_client.return_value = mock.Mock( - generate_tenant_token=mock_generate_tenant_token, - ) - return mock_generate_tenant_token - @mock_meilisearch(enabled=True) - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') def test_studio_search_student_no_access(self, mock_search_client): """ Users without access to any courses or libraries will have all documents filtered out. @@ -145,11 +135,11 @@ def test_studio_search_student_no_access(self, mock_search_client): "filter": "org IN [] OR access_id IN []", } }, - expires_at=mock.ANY, + expires_at=ANY, ) @mock_meilisearch(enabled=True) - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') def test_studio_search_staff(self, mock_search_client): """ Users with global staff access can search any document. @@ -163,11 +153,11 @@ def test_studio_search_staff(self, mock_search_client): search_rules={ "studio_content": {} }, - expires_at=mock.ANY, + expires_at=ANY, ) @mock_meilisearch(enabled=True) - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') def test_studio_search_course_staff_access(self, mock_search_client): """ Users with staff or instructor access to a course or library will be limited to these courses/libraries. @@ -189,7 +179,7 @@ def test_studio_search_course_staff_access(self, mock_search_client): "filter": f"org IN [] OR access_id IN {expected_access_ids}", } }, - expires_at=mock.ANY, + expires_at=ANY, ) @ddt.data( @@ -197,7 +187,7 @@ def test_studio_search_course_staff_access(self, mock_search_client): 'org_instr', ) @mock_meilisearch(enabled=True) - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') def test_studio_search_org_access(self, username, mock_search_client): """ Users with org access to any courses or libraries will use the org filter. @@ -213,11 +203,11 @@ def test_studio_search_org_access(self, username, mock_search_client): "filter": "org IN ['org1'] OR access_id IN []", } }, - expires_at=mock.ANY, + expires_at=ANY, ) @mock_meilisearch(enabled=True) - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') def test_studio_search_omit_orgs(self, mock_search_client): """ Grant org access to our staff user to ensure that org's access_ids are omitted from the search filter. @@ -240,13 +230,13 @@ def test_studio_search_omit_orgs(self, mock_search_client): "filter": f"org IN ['org1'] OR access_id IN {expected_access_ids}", } }, - expires_at=mock.ANY, + expires_at=ANY, ) @mock_meilisearch(enabled=True) - @mock.patch('openedx.core.djangoapps.content.search.views._get_user_orgs') - @mock.patch('openedx.core.djangoapps.content.search.views.get_access_ids_for_request') - @mock.patch('openedx.core.djangoapps.content.search.views.meilisearch.Client') + @patch('openedx.core.djangoapps.content.search.api._get_user_orgs') + @patch('openedx.core.djangoapps.content.search.api.get_access_ids_for_request') + @patch('openedx.core.djangoapps.content.search.api.MeilisearchClient') def test_studio_search_limits(self, mock_search_client, mock_get_access_ids, mock_get_user_orgs): """ Users with access to many courses/libraries or orgs will only be able to search content @@ -275,5 +265,5 @@ def test_studio_search_limits(self, mock_search_client, mock_get_access_ids, moc "filter": f"org IN {expected_user_orgs} OR access_id IN {expected_access_ids}", } }, - expires_at=mock.ANY, + expires_at=ANY, ) diff --git a/openedx/core/djangoapps/content/search/views.py b/openedx/core/djangoapps/content/search/views.py index e6dd172bc006..f747fe77e8b5 100644 --- a/openedx/core/djangoapps/content/search/views.py +++ b/openedx/core/djangoapps/content/search/views.py @@ -2,6 +2,7 @@ REST API for content search """ from __future__ import annotations + import logging from django.contrib.auth import get_user_model From 58f8ff42b0626e5156dceda86906f851eb221ba5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 17 Apr 2024 17:57:51 -0300 Subject: [PATCH 15/18] style: fix pylint --- openedx/core/djangoapps/content/search/tests/test_handlers.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index 22beb6551f1f..f961cc9c779c 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -21,7 +21,6 @@ SearchAccess = {} - @patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) @patch("openedx.core.djangoapps.content.search.api.MeilisearchClient") @override_settings(MEILISEARCH_ENABLED=True) From 66b1b7139f34df938b2380fae4eb932a8cae53c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 17 Apr 2024 18:05:01 -0300 Subject: [PATCH 16/18] feat: update meilisearch configuration needed for tags filter Co-authored-by: Braden MacDonald --- openedx/core/djangoapps/content/search/api.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index fb76148bf65b..382e8f8d759a 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -316,6 +316,11 @@ def rebuild_index(status_cb: Callable[[str], None] | None = None) -> None: Fields.context_key, Fields.org, Fields.tags, + Fields.tags + "." + Fields.tags_taxonomy, + Fields.tags + "." + Fields.tags_level0, + Fields.tags + "." + Fields.tags_level1, + Fields.tags + "." + Fields.tags_level2, + Fields.tags + "." + Fields.tags_level3, Fields.type, Fields.access_id, ]) From 83e1d2751d4c154ee2bbb5a651e73e9a1f5b4bc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 17 Apr 2024 18:11:27 -0300 Subject: [PATCH 17/18] fix: conditional import --- openedx/core/djangoapps/content/search/tests/test_api.py | 4 ++-- openedx/core/djangoapps/content/search/tests/test_handlers.py | 4 ++-- openedx/core/djangoapps/content/search/tests/test_views.py | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 01929c049a67..43e1eb20e68e 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -14,11 +14,11 @@ 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 try: # This import errors in the lms because content.search is not an installed app there. - from openedx.core.djangoapps.content.search.models import SearchAccess + from .. import api + from ..models import SearchAccess except RuntimeError: SearchAccess = {} diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index f961cc9c779c..1a1134547666 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -12,11 +12,11 @@ from openedx.core.lib.blockstore_api.tests.base import BlockstoreAppTestMixin from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, ModuleStoreTestCase -from .. import api try: # This import errors in the lms because content.search is not an installed app there. - from openedx.core.djangoapps.content.search.models import SearchAccess + from .. import api + from ..models import SearchAccess except RuntimeError: SearchAccess = {} diff --git a/openedx/core/djangoapps/content/search/tests/test_views.py b/openedx/core/djangoapps/content/search/tests/test_views.py index 9b32bdf327a8..d97a55749de5 100644 --- a/openedx/core/djangoapps/content/search/tests/test_views.py +++ b/openedx/core/djangoapps/content/search/tests/test_views.py @@ -19,12 +19,12 @@ try: # This import errors in the lms because content.search is not an installed app there. - from openedx.core.djangoapps.content.search.models import SearchAccess + from .. import api + from ..models import SearchAccess except RuntimeError: SearchAccess = {} -from .. import api STUDIO_SEARCH_ENDPOINT_URL = "/api/content_search/v2/studio/" MOCK_API_KEY_UID = "3203d764-370f-4e99-a917-d47ab7f29739" From 17e1e537cbe965e4fe89c7e7c77277209e752955 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 17 Apr 2024 18:26:43 -0300 Subject: [PATCH 18/18] style: fix pylint --- openedx/core/djangoapps/content/search/tests/test_views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx/core/djangoapps/content/search/tests/test_views.py b/openedx/core/djangoapps/content/search/tests/test_views.py index d97a55749de5..7c1b8e99850a 100644 --- a/openedx/core/djangoapps/content/search/tests/test_views.py +++ b/openedx/core/djangoapps/content/search/tests/test_views.py @@ -25,7 +25,6 @@ SearchAccess = {} - STUDIO_SEARCH_ENDPOINT_URL = "/api/content_search/v2/studio/" MOCK_API_KEY_UID = "3203d764-370f-4e99-a917-d47ab7f29739"