diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 6d6ce6148cd2..da68dcacc297 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -21,7 +21,7 @@ from meilisearch.errors import MeilisearchApiError, MeilisearchError from meilisearch.models.task import TaskInfo from opaque_keys import OpaqueKey -from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.keys import CourseKey, LearningContextKey, UsageKey from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2 from openedx_content import api as content_api from openedx_content import models_api as content_models @@ -44,6 +44,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError from .documents import ( + DocType, Fields, meili_id_from_opaque_key, searchable_doc_collections, @@ -60,14 +61,19 @@ User = get_user_model() -STUDIO_INDEX_SUFFIX = "studio_content" +# Course content and library content live in separate indexes. Library authoring waits synchronously on +# Meilisearch, and indexing cost grows with index size, so library content gets a small index of its own. +# The course index keeps the original "studio_content" name so existing course documents don't need reindexing. +STUDIO_COURSE_INDEX_SUFFIX = "studio_content" +STUDIO_LIBRARY_INDEX_SUFFIX = "studio_library_content" Filter = str | list[str | list[str]] -if hasattr(settings, "MEILISEARCH_INDEX_PREFIX"): - STUDIO_INDEX_NAME = settings.MEILISEARCH_INDEX_PREFIX + STUDIO_INDEX_SUFFIX -else: - STUDIO_INDEX_NAME = STUDIO_INDEX_SUFFIX +_INDEX_PREFIX = getattr(settings, "MEILISEARCH_INDEX_PREFIX", "") +# Holds DocType.course_block documents (courses and legacy modulestore libraries). +STUDIO_COURSE_INDEX_NAME = _INDEX_PREFIX + STUDIO_COURSE_INDEX_SUFFIX +# Holds DocType.library_block, DocType.library_container and DocType.collection documents (Libraries V2). +STUDIO_LIBRARY_INDEX_NAME = _INDEX_PREFIX + STUDIO_LIBRARY_INDEX_SUFFIX _MEILI_CLIENT = None @@ -81,13 +87,25 @@ EXCLUDED_XBLOCK_TYPES = ["course", "course_info"] +def _index_name_for_key(key: OpaqueKey) -> str: + """ + Return the name of the index that holds the document(s) for the given key. + + Accepts a learning context key (course/library) or any key inside one (block, collection, container). + """ + context_key = key if isinstance(key, LearningContextKey) else key.context_key + if isinstance(context_key, LibraryLocatorV2): + return STUDIO_LIBRARY_INDEX_NAME + return STUDIO_COURSE_INDEX_NAME + + @contextmanager -def _index_rebuild_lock() -> Generator[str, None, None]: +def _index_rebuild_lock(index_name: str) -> Generator[str, None, None]: """ - Lock to prevent that more than one rebuild is running at the same time + Lock to prevent that more than one rebuild of the given index is running at the same time """ - lock_id = f"lock-meilisearch-index-{STUDIO_INDEX_NAME}" - new_index_name = STUDIO_INDEX_NAME + "_new" + lock_id = f"lock-meilisearch-index-{index_name}" + new_index_name = index_name + "_new" status = cache.add(lock_id, new_index_name, LOCK_EXPIRE) @@ -103,8 +121,8 @@ def _index_rebuild_lock() -> Generator[str, None, None]: cache.delete(lock_id) -def _get_running_rebuild_index_name() -> str | None: - lock_id = f"lock-meilisearch-index-{STUDIO_INDEX_NAME}" +def _get_running_rebuild_index_name(index_name: str) -> str | None: + lock_id = f"lock-meilisearch-index-{index_name}" return cache.get(lock_id) @@ -192,20 +210,24 @@ def _index_exists(index_name: str) -> bool: @contextmanager -def _using_temp_index(status_cb: Callable[[str], None] | None = None) -> Generator[str, None, None]: +def _using_temp_index( + index_name: str, + 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. Args: + index_name (str): The index that the temporary index will replace status_cb (Callable): A callback function to report status messages """ if status_cb is None: status_cb = log.info client = _get_meilisearch_client() - status_cb("Checking index...") - with _index_rebuild_lock() as temp_index_name: + status_cb(f"Checking index '{index_name}'...") + with _index_rebuild_lock(index_name) 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)) @@ -216,17 +238,17 @@ def _using_temp_index(status_cb: Callable[[str], None] | None = None) -> Generat yield temp_index_name - if not _index_exists(STUDIO_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(STUDIO_INDEX_NAME)) + _wait_for_meili_task(client.create_index(index_name)) status_cb("Swapping index...") - client.swap_indexes([{"indexes": [temp_index_name, STUDIO_INDEX_NAME]}]) + 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(STUDIO_INDEX_NAME).created_at != new_index_created: + if client.get_index(index_name).created_at != new_index_created: status_cb("Waiting for swap completion...") else: break @@ -313,22 +335,22 @@ def _recurse_children(block, fn, status_cb: Callable[[str], None] | None = None) fn(child) -def _update_index_docs(docs) -> None: +def _update_index_docs(index_name: str, docs) -> None: """ - Helper function that updates the documents in the search index + Helper function that updates the documents in the given search index - If there is a rebuild in progress, the document will also be added to the new index. + If there is a rebuild of that index 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() + current_rebuild_index_name = _get_running_rebuild_index_name(index_name) if current_rebuild_index_name: # If there is a rebuild in progress, the document will also be added to the new index. client.index(current_rebuild_index_name).update_documents(docs) - _wait_for_meili_task(client.index(STUDIO_INDEX_NAME).update_documents(docs)) + _wait_for_meili_task(client.index(index_name).update_documents(docs)) def only_if_meilisearch_enabled(f): @@ -355,15 +377,15 @@ def is_meilisearch_enabled() -> bool: return False -def reset_index(status_cb: Callable[[str], None] | None = None) -> None: +def reset_index(index_name: str, status_cb: Callable[[str], None] | None = None) -> None: """ - Reset the Meilisearch index, deleting all documents and reconfiguring it + Reset the given Meilisearch index, deleting all documents and reconfiguring it """ if status_cb is None: status_cb = log.info - status_cb("Creating new empty index...") - with _using_temp_index(status_cb) as temp_index_name: + status_cb(f"Creating new empty index '{index_name}'...") + with _using_temp_index(index_name, status_cb) as temp_index_name: _apply_index_settings(temp_index_name, wait=False) status_cb("Index recreated!") status_cb("Index reset complete.") @@ -448,13 +470,13 @@ def _compare_setting(key, expected): ) -def reconcile_index( +def reconcile_indexes( status_cb: Callable[[str], None] | None = None, warn_cb: Callable[[str], None] | None = None ) -> None: # noqa: E501 """ - Reconcile the Meilisearch index state. + Reconcile the state of the Studio course index and library index. - Inspects the current Studio Meilisearch index and takes appropriate action based on its state: + Inspects each Studio Meilisearch index and takes appropriate action based on its state: - Creates the index if missing. - Reconfigures if empty and drifted. - Applies updated settings if populated and drifted. @@ -468,44 +490,66 @@ def reconcile_index( if warn_cb is None: warn_cb = log.warning - drift = _detect_index_drift(STUDIO_INDEX_NAME) + for index_name in (STUDIO_COURSE_INDEX_NAME, STUDIO_LIBRARY_INDEX_NAME): + reconcile_index(index_name, status_cb, warn_cb) + + +def reconcile_index( + index_name: str, + status_cb: Callable[[str], None] | None = None, + warn_cb: Callable[[str], None] | None = None, +) -> None: + """ + Reconcile the state of one Studio Meilisearch index. See reconcile_indexes(). + """ + if status_cb is None: + status_cb = log.info + if warn_cb is None: + warn_cb = log.warning + + if index_name == STUDIO_LIBRARY_INDEX_NAME: + populate_cmd = "./manage.py cms reindex_studio --libraries-only" + else: + populate_cmd = "./manage.py cms reindex_studio" + + drift = _detect_index_drift(index_name) # CASE: Index missing if not drift.exists: - status_cb("Studio search index not found. Creating and configuring...") - reset_index(status_cb) - status_cb("Index created. Run './manage.py cms reindex_studio' to populate.") + status_cb(f"Studio search index '{index_name}' not found. Creating and configuring...") + reset_index(index_name, status_cb) + status_cb(f"Index '{index_name}' created. Run '{populate_cmd}' to populate.") return # CASE: Primary key mismatch (must recreate regardless of population state) if not drift.primary_key_correct: if drift.is_empty: - warn_cb("Primary key mismatch on empty index. Recreating...") + warn_cb(f"Primary key mismatch on empty index '{index_name}'. Recreating...") else: warn_cb( - f"PRIMARY KEY MISMATCH on populated index '{STUDIO_INDEX_NAME}'. " + f"PRIMARY KEY MISMATCH on populated index '{index_name}'. " "Index must be recreated (data loss is unavoidable for primary key changes)." ) - warn_cb("Dropping and recreating index. Repopulate with: './manage.py cms reindex_studio'") - reset_index(status_cb) - warn_cb("Index recreated empty. Run './manage.py cms reindex_studio' to repopulate.") + warn_cb(f"Dropping and recreating index. Repopulate with: '{populate_cmd}'") + reset_index(index_name, status_cb) + warn_cb(f"Index '{index_name}' recreated empty. Run '{populate_cmd}' to repopulate.") return # CASE: Index empty if drift.is_empty: if drift.is_settings_drifted: - status_cb("Empty index has drifted settings. Reconfiguring...") - _apply_index_settings(STUDIO_INDEX_NAME, wait=True, status_cb=status_cb) - status_cb("Reconfigured. Run './manage.py cms reindex_studio' to populate.") + status_cb(f"Empty index '{index_name}' has drifted settings. Reconfiguring...") + _apply_index_settings(index_name, wait=True, status_cb=status_cb) + status_cb(f"Reconfigured. Run '{populate_cmd}' to populate.") else: status_cb( - "Index exists and is correctly configured but empty. Run './manage.py cms reindex_studio' to populate." + f"Index '{index_name}' exists and is correctly configured but empty. Run '{populate_cmd}' to populate." ) return # CASE: Index populated, attribute drifted i.e settings mismatched if drift.is_settings_drifted: - warn_cb(f"Settings drift detected on populated index '{STUDIO_INDEX_NAME}'. Applying updated settings...") + warn_cb(f"Settings drift detected on populated index '{index_name}'. Applying updated settings...") # Log per-setting mismatch details for field_name, match in ( ("distinctAttribute", drift.distinct_attribute_match), @@ -517,14 +561,14 @@ def reconcile_index( if match is False: warn_cb(f" - {field_name}: DRIFTED") - _apply_index_settings(STUDIO_INDEX_NAME, wait=True, status_cb=status_cb) + _apply_index_settings(index_name, wait=True, status_cb=status_cb) warn_cb( "Settings applied. Meilisearch will re-index documents in the background. " - "Consider running './manage.py cms reindex_studio' for a full rebuild " + f"Consider running '{populate_cmd}' for a full rebuild " "if search quality is affected." ) else: - status_cb("Index is populated and correctly configured. No action needed.") + status_cb(f"Index '{index_name}' is populated and correctly configured. No action needed.") def init_index(status_cb: Callable[[str], None] | None = None, warn_cb: Callable[[str], None] | None = None) -> None: @@ -533,10 +577,10 @@ def init_index(status_cb: Callable[[str], None] | None = None, warn_cb: Callable Initialize the Meilisearch index, creating it and configuring it if it doesn't exist. - This is a compatibility wrapper around reconcile_index(). + This is a compatibility wrapper around reconcile_indexes(). """ log.warning("init_index is deprecated as of Verawood and will be removed in the future release.") - reconcile_index(status_cb=status_cb, warn_cb=warn_cb) + reconcile_indexes(status_cb=status_cb, warn_cb=warn_cb) def index_course( @@ -551,7 +595,7 @@ def index_course( client = _get_meilisearch_client() docs = [] if index_name is None: - index_name = STUDIO_INDEX_NAME + index_name = STUDIO_COURSE_INDEX_NAME if status_cb is None: status_cb = log.info @@ -579,10 +623,16 @@ def add_with_children(block): def rebuild_index( # pylint: disable=too-many-statements - status_cb: Callable[[str], None] | None = None, incremental=False + status_cb: Callable[[str], None] | None = None, incremental=False, include_courses=True ) -> None: """ - Rebuild the Meilisearch index from scratch + Rebuild the Meilisearch indexes from scratch + + Libraries go into the library index, courses into the course index. Each index is swapped in + separately, so the library index is live as soon as the (fast) library pass finishes. + + With include_courses=False only the library index is rebuilt. This is how an existing install + populates the library index when upgrading from a single shared index without reindexing courses. """ if status_cb is None: status_cb = log.info @@ -605,7 +655,7 @@ def rebuild_index( # pylint: disable=too-many-statements # Get the list of courses status_cb("Counting courses...") - num_courses = CourseOverview.objects.count() + num_courses = CourseOverview.objects.count() if include_courses else 0 # Some counters so we can track our progress as indexing progresses: num_libs_skipped = len(keys_indexed) @@ -614,7 +664,11 @@ def rebuild_index( # pylint: disable=too-many-statements num_blocks_done = 0 # How many individual components/XBlocks we've indexed status_cb(f"Found {num_courses} courses, {num_libraries} libraries.") - with _using_temp_index(status_cb) if not incremental else nullcontext(STUDIO_INDEX_NAME) as index_name: + library_index_ctx = ( + nullcontext(STUDIO_LIBRARY_INDEX_NAME) if incremental + else _using_temp_index(STUDIO_LIBRARY_INDEX_NAME, status_cb) + ) + with library_index_ctx as index_name: ############## Configure the index ############## # The index settings are best changed on an empty index. @@ -624,6 +678,8 @@ def rebuild_index( # pylint: disable=too-many-statements _apply_index_settings(index_name, wait=False) ############## Libraries ############## + # MeilisearchError from add_documents is not caught below: an incomplete library index must not be + # swapped in or followed by the course index cleanup, and the Celery task needs the error to retry. status_cb("Indexing libraries...") def index_library(lib_key: LibraryLocatorV2) -> list: @@ -643,7 +699,7 @@ def index_library(lib_key: LibraryLocatorV2) -> list: try: # Add all the docs in this library at once (usually faster than adding one at a time): _wait_for_meili_task(client.index(index_name).add_documents(docs)) - except (TypeError, KeyError, MeilisearchError) as err: + except (TypeError, KeyError) as err: status_cb(f"Error indexing library {lib_key}: {err}") return docs @@ -664,7 +720,7 @@ def index_collection_batch(batch, num_done, library_key) -> int: try: # Add docs in batch of 100 at once (usually faster than adding one at a time): _wait_for_meili_task(client.index(index_name).add_documents(docs)) - except (TypeError, KeyError, MeilisearchError) as err: + except (TypeError, KeyError) as err: status_cb(f"Error indexing collection batch {p}: {err}") return num_done @@ -695,7 +751,7 @@ def index_container_batch(batch, num_done, library_key) -> int: try: # Add docs in batch of 100 at once (usually faster than adding one at a time): _wait_for_meili_task(client.index(index_name).add_documents(docs)) - except (TypeError, KeyError, MeilisearchError) as err: + except (TypeError, KeyError) as err: status_cb(f"Error indexing container batch {p}: {err}") return num_done @@ -741,24 +797,37 @@ def index_container_batch(batch, num_done, library_key) -> int: num_contexts_done += 1 - ############## Courses ############## - status_cb("Indexing courses...") - # To reduce memory usage on large instances, split up the CourseOverviews into pages of 1,000 courses: + # Library content used to be stored in the course index. Now that the library index has it, drop the old copies. + delete_library_docs_from_course_index(status_cb) - paginator = Paginator(CourseOverview.objects.only("id", "display_name").order_by("-created", "id"), 1000) - for p in paginator.page_range: - for course in paginator.page(p).object_list: - status_cb( - f"{num_contexts_done + 1}/{num_contexts}. Now indexing course {course.display_name} ({course.id})" - ) - if course.id in keys_indexed: + if include_courses: + course_index_ctx = ( + nullcontext(STUDIO_COURSE_INDEX_NAME) if incremental + else _using_temp_index(STUDIO_COURSE_INDEX_NAME, status_cb) + ) + with course_index_ctx as index_name: + if not incremental: + _apply_index_settings(index_name, wait=False) + + ############## Courses ############## + status_cb("Indexing courses...") + # To reduce memory usage on large instances, split up the CourseOverviews into pages of 1,000 courses: + + paginator = Paginator(CourseOverview.objects.only("id", "display_name").order_by("-created", "id"), 1000) + for p in paginator.page_range: + for course in paginator.page(p).object_list: + status_cb( + f"{num_contexts_done + 1}/{num_contexts}. " + f"Now indexing course {course.display_name} ({course.id})" + ) + if course.id in keys_indexed: + num_contexts_done += 1 + continue + course_docs = index_course(course.id, index_name, status_cb) + if incremental: + IncrementalIndexCompleted.objects.get_or_create(context_key=course.id) num_contexts_done += 1 - continue - course_docs = index_course(course.id, index_name, status_cb) - if incremental: - IncrementalIndexCompleted.objects.get_or_create(context_key=course.id) - num_contexts_done += 1 - num_blocks_done += len(course_docs) + num_blocks_done += len(course_docs) IncrementalIndexCompleted.objects.all().delete() status_cb(f"Done! {num_blocks_done} blocks indexed across {num_contexts_done} courses, collections and libraries.") @@ -790,7 +859,7 @@ def add_with_children(block): add_with_children(xblock) - _update_index_docs(docs) + _update_index_docs(STUDIO_COURSE_INDEX_NAME, docs) def delete_index_doc(key: OpaqueKey, *, delete_children: bool = False) -> None: @@ -800,55 +869,77 @@ def delete_index_doc(key: OpaqueKey, *, delete_children: bool = False) -> None: Args: key (OpaqueKey): The opaque key of the XBlock/Container to be removed from the index """ + index_name = _index_name_for_key(key) doc = searchable_doc_for_key(key) - _delete_index_doc(doc[Fields.id]) + _delete_index_doc(index_name, doc[Fields.id]) if delete_children: - _delete_documents(f'{Fields.breadcrumbs}.{Fields.usage_key} = "{key}"') + _delete_documents(index_name, f'{Fields.breadcrumbs}.{Fields.usage_key} = "{key}"') def delete_docs_with_context_key(key: OpaqueKey) -> None: """ Delete all docs for given context key """ - _delete_documents(f'{Fields.context_key} = "{key}"') + _delete_documents(_index_name_for_key(key), f'{Fields.context_key} = "{key}"') + + +def delete_library_docs_from_course_index(status_cb: Callable[[str], None] | None = None) -> None: + """ + Delete every non-course document from the course index. + + Before the course/library index split, library blocks, containers and collections were stored in the + course index. This removes those leftovers. It is a no-op once they are gone. + """ + if status_cb is None: + status_cb = log.info + + if not _index_exists(STUDIO_COURSE_INDEX_NAME): + return + status_cb(f"Removing library documents from course index '{STUDIO_COURSE_INDEX_NAME}'...") + _wait_for_meili_task( + _get_meilisearch_client().index(STUDIO_COURSE_INDEX_NAME).delete_documents( + filter=f'{Fields.type} != "{DocType.course_block}"' + ) + ) -def _delete_documents(filter_query: str) -> None: +def _delete_documents(index_name: str, filter_query: str) -> None: """ - Deletes all documents from the search index that match the given filter + Deletes all documents from the given search index that match the given filter Args: - filter (str): The query to use when filtering documents + index_name (str): The index to delete the documents from + filter_query (str): The query to use when filtering documents """ if not filter_query: return client = _get_meilisearch_client() - current_rebuild_index_name = _get_running_rebuild_index_name() + current_rebuild_index_name = _get_running_rebuild_index_name(index_name) if current_rebuild_index_name: # If there is a rebuild in progress, the document will also be removed from the new index. client.index(current_rebuild_index_name).delete_documents(filter=filter_query) - _wait_for_meili_task(client.index(STUDIO_INDEX_NAME).delete_documents(filter=filter_query)) + _wait_for_meili_task(client.index(index_name).delete_documents(filter=filter_query)) -def _delete_index_doc(doc_id) -> None: +def _delete_index_doc(index_name: str, doc_id) -> None: """ - Helper function that deletes the document with the given ID from the search index + Helper function that deletes the document with the given ID from the given search index - If there is a rebuild in progress, the document will also be removed from the new index. + If there is a rebuild of that index in progress, the document will also be removed from the new index. """ if not doc_id: return client = _get_meilisearch_client() - current_rebuild_index_name = _get_running_rebuild_index_name() + current_rebuild_index_name = _get_running_rebuild_index_name(index_name) if current_rebuild_index_name: # If there is a rebuild in progress, the document will also be removed from the new index. client.index(current_rebuild_index_name).delete_document(doc_id) - _wait_for_meili_task(client.index(STUDIO_INDEX_NAME).delete_document(doc_id)) + _wait_for_meili_task(client.index(index_name).delete_document(doc_id)) def upsert_library_block_index_doc(usage_key: UsageKey) -> None: @@ -861,10 +952,10 @@ def upsert_library_block_index_doc(usage_key: UsageKey) -> None: docs = [searchable_doc_for_library_block(library_block_metadata)] - _update_index_docs(docs) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, docs) -def _get_document_from_index(document_id: str) -> dict: +def _get_document_from_index(index_name: str, document_id: str) -> dict: """ Returns the Document identified by the given ID, from the given index. @@ -872,7 +963,6 @@ def _get_document_from_index(document_id: str) -> dict: """ client = _get_meilisearch_client() document = None - index_name = STUDIO_INDEX_NAME try: index = client.get_index(index_name) document = index.get_document(document_id) @@ -894,11 +984,11 @@ def upsert_library_collection_index_doc(collection_key: LibraryCollectionLocator # (If the collection is soft-deleted, searchable_doc_for_collection() sets `_disabled: True`) # (If the collection is hard-deleted, searchable_doc_for_collection() leaves all fields other than ID empty) if doc.get("_disabled") or not doc.get(Fields.type): - _delete_index_doc(doc[Fields.id]) + _delete_index_doc(STUDIO_LIBRARY_INDEX_NAME, doc[Fields.id]) return # Normal case - update the collection doc. - _update_index_docs([doc]) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, [doc]) # We do NOT update the individual entities (components/containers) in the collection here. # This event can be called if a single entity is added or removed from the collection (to update the "# of items in @@ -945,7 +1035,7 @@ def update_library_components_collections( log.info( f"Updating document.collections for library {library_key} components page {page} / {paginator.num_pages}" ) - _update_index_docs(docs) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, docs) def update_library_containers_collections( @@ -984,7 +1074,7 @@ def update_library_containers_collections( log.info( f"Updating document.collections for library {library_key} containers page {page} / {paginator.num_pages}" ) - _update_index_docs(docs) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, docs) def upsert_library_container_index_doc(container_key: LibraryContainerLocator) -> None: @@ -998,15 +1088,15 @@ def upsert_library_container_index_doc(container_key: LibraryContainerLocator) - # Soft-deleted/disabled containers are removed from the index # and their components updated. if doc.get("_disabled"): - _delete_index_doc(doc[Fields.id]) + _delete_index_doc(STUDIO_LIBRARY_INDEX_NAME, doc[Fields.id]) # Hard-deleted containers are also deleted from the index elif not doc.get(Fields.type): - _delete_index_doc(doc[Fields.id]) + _delete_index_doc(STUDIO_LIBRARY_INDEX_NAME, doc[Fields.id]) # Otherwise, upsert the container. else: - _update_index_docs([doc]) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, [doc]) def upsert_content_library_index_docs(library_key: LibraryLocatorV2, full_index: bool = False) -> None: @@ -1034,7 +1124,7 @@ def upsert_content_library_index_docs(library_key: LibraryLocatorV2, full_index: doc = searchable_doc_for_collection(collection_key, collection=collection) docs.append(doc) - _update_index_docs(docs) + _update_index_docs(STUDIO_LIBRARY_INDEX_NAME, docs) def upsert_content_object_tags_index_doc(key: OpaqueKey): @@ -1043,7 +1133,7 @@ def upsert_content_object_tags_index_doc(key: OpaqueKey): """ doc = {Fields.id: meili_id_from_opaque_key(key)} doc.update(searchable_doc_tags(key)) - _update_index_docs([doc]) + _update_index_docs(_index_name_for_key(key), [doc]) def upsert_item_collections_index_docs(opaque_key: OpaqueKey): @@ -1052,7 +1142,7 @@ def upsert_item_collections_index_docs(opaque_key: OpaqueKey): """ doc = {Fields.id: meili_id_from_opaque_key(opaque_key)} doc.update(searchable_doc_collections(opaque_key)) - _update_index_docs([doc]) + _update_index_docs(_index_name_for_key(opaque_key), [doc]) def upsert_item_containers_index_docs(opaque_key: OpaqueKey, container_type: str): @@ -1061,7 +1151,7 @@ def upsert_item_containers_index_docs(opaque_key: OpaqueKey, container_type: str """ doc = {Fields.id: meili_id_from_opaque_key(opaque_key)} doc.update(searchable_doc_containers(opaque_key, container_type)) - _update_index_docs([doc]) + _update_index_docs(_index_name_for_key(opaque_key), [doc]) def _get_user_orgs(request: Request) -> list[str]: @@ -1100,8 +1190,10 @@ def generate_user_token_for_studio_search(request): """ expires_at = datetime.now(tz=timezone.utc) + timedelta(days=7) # noqa: UP017 + access_filter = _get_meili_access_filter(request) search_rules = { - STUDIO_INDEX_NAME: _get_meili_access_filter(request), + STUDIO_COURSE_INDEX_NAME: access_filter, + STUDIO_LIBRARY_INDEX_NAME: access_filter, } # 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( @@ -1112,7 +1204,11 @@ def generate_user_token_for_studio_search(request): return { "url": settings.MEILISEARCH_PUBLIC_URL, - "index_name": STUDIO_INDEX_NAME, + "course_index_name": STUDIO_COURSE_INDEX_NAME, + "library_index_name": STUDIO_LIBRARY_INDEX_NAME, + # Deprecated: use course_index_name / library_index_name. Kept for one release so frontends that predate + # the course/library index split keep working for course search. Remove in the following release. + "index_name": STUDIO_COURSE_INDEX_NAME, "api_key": restricted_api_key, } @@ -1158,7 +1254,8 @@ def fetch_block_types(extra_filter: Filter | None = None): extra_filter_formatted = force_array(extra_filter) client = _get_meilisearch_client() - index = client.get_index(STUDIO_INDEX_NAME) + # Only used for modulestore content (courses and legacy libraries), which lives in the course index. + index = client.get_index(STUDIO_COURSE_INDEX_NAME) response = index.search( "", @@ -1189,7 +1286,8 @@ def get_all_blocks_from_context( offset = 0 client = _get_meilisearch_client() - index = client.get_index(STUDIO_INDEX_NAME) + # Only used for modulestore content (courses and legacy libraries), which lives in the course index. + index = client.get_index(STUDIO_COURSE_INDEX_NAME) while True: response = index.search( diff --git a/openedx/core/djangoapps/content/search/docs/decisions/0002-separate-course-and-library-indexes.rst b/openedx/core/djangoapps/content/search/docs/decisions/0002-separate-course-and-library-indexes.rst new file mode 100644 index 000000000000..1fa2b849edf2 --- /dev/null +++ b/openedx/core/djangoapps/content/search/docs/decisions/0002-separate-course-and-library-indexes.rst @@ -0,0 +1,84 @@ +Separate Meilisearch Indexes for Course and Library Content +############################################################ + +Status +****** + +Accepted + + +Context +******* + +`0001-meilisearch.rst`_ put all Studio content into one Meilisearch index, +``studio_content``: course blocks (``type = course_block``) plus Libraries V2 +components, containers and collections. + +Library authoring updates that index synchronously so the frontend sees the +change on its next query. Meilisearch's per-document indexing cost grows with +index size, and course blocks dominate the index on large instances. One +instance reported 1.3 million documents (11.9 GiB) of which 99.93% were course +blocks, and saw Studio time out when creating library components +(`openedx-platform#38993`_). + +Course content is indexed asynchronously by Celery tasks, so it does not need to +share an index with library content. + +.. _0001-meilisearch.rst: ./0001-meilisearch.rst +.. _openedx-platform#38993: https://github.com/openedx/openedx-platform/issues/38993 + + +Decision +******** + +Studio content is stored in two indexes: + +* ``studio_content``: course blocks + (``DocType.course_block``). This is the original index name, so upgrading does + not require reindexing course content. +* ``studio_library_content``: library blocks, library + containers and collections. + +Both indexes use the same settings. Every write is routed by document type or by +the learning context of its key. Rebuild locks and temporary ``_new`` indexes +are per index. + +Every modulestore block is indexed as a ``course_block`` document, including +course blocks that link to upstream library content, so those stay in the +course index. + +``GET /api/content_search/v2/studio/`` returns ``course_index_name`` and +``library_index_name``, and one tenant token whose search rules cover both +indexes with the same access filter. Each Studio search surface already +searches either course content or library content, never both, so each one +picks the matching index. ``index_name`` is still returned (equal to +``course_index_name``) for one release, for frontends that predate the split. + + +Upgrading +********* + +On an instance that already has a populated ``studio_content`` index: + +#. ``./manage.py cms migrate``. The ``post_migrate`` reconciliation creates and + configures the empty library index. +#. ``./manage.py cms reindex_studio --libraries-only``. This enqueues a Celery + task that rebuilds the library index, then deletes every document with + ``type != "course_block"`` from the course index. Courses are not reindexed. + +Deploy a frontend that reads ``library_index_name`` at the same time. A frontend +that only knows ``index_name`` searches the course index, so it stops finding +library content once step 2 has run. + +A full ``./manage.py cms reindex_studio`` also populates both indexes and runs +the same cleanup, but it reindexes every course. + + +Consequences +************ + +* Library writes no longer pay for the size of the course index. +* A future search across courses and libraries together would need a + multi-index search request. +* The Meilisearch API key used by Studio must be allowed to manage both index + names (and their ``_new`` temporary indexes). diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 0fe93292d8fa..c60c67eb7115 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -49,7 +49,7 @@ from .api import ( is_meilisearch_enabled, only_if_meilisearch_enabled, - reconcile_index, + reconcile_indexes, upsert_content_object_tags_index_doc, upsert_item_collections_index_docs, upsert_item_containers_index_docs, @@ -86,7 +86,7 @@ def handle_post_migrate(sender, **kwargs): return try: - reconcile_index(status_cb=log.info, warn_cb=log.warning) + reconcile_indexes(status_cb=log.info, warn_cb=log.warning) except ConnectionError as exc: log.warning( "Meilisearch reconciliation skipped during post_migrate: %s. " 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 a1d7318f16c0..10d9b60dc61b 100644 --- a/openedx/core/djangoapps/content/search/management/commands/reindex_studio.py +++ b/openedx/core/djangoapps/content/search/management/commands/reindex_studio.py @@ -15,7 +15,7 @@ from django.core.management import BaseCommand, CommandError from ... import api -from ...tasks import rebuild_index_incremental +from ...tasks import rebuild_index_incremental, rebuild_library_index log = logging.getLogger(__name__) @@ -37,11 +37,24 @@ class Command(BaseCommand): ./manage.py cms shell -c 'IncrementalIndexCompleted.objects.all().delete()' This will delete all the IncrementalIndexCompleted records and will help in restarting the index population. + + When upgrading from the single shared Studio index to separate course and library indexes, run + `./manage.py cms reindex_studio --libraries-only` once instead. It rebuilds only the library index and + deletes library documents from the course index, without reindexing courses. """ help = "Add all course and library content to the Studio search index." def add_arguments(self, parser): + parser.add_argument( + "--libraries-only", + action="store_true", + default=False, + help=( + "Rebuild only the library index, then delete library documents from the course index. " + "Run this once when upgrading from the single shared Studio index; courses are not reindexed." + ), + ) # Removed flags — provide clear error messages for operators with old automation. parser.add_argument( "--experimental", @@ -97,6 +110,17 @@ def handle(self, *args, **options): "reindex_studio is now a stable command, so the flag is no longer necessary." ) + if options["libraries_only"]: + result = rebuild_library_index.delay() + if settings.CELERY_ALWAYS_EAGER: + self.stdout.write("Library indexing complete!") + else: + self.stdout.write( + f"Studio library index rebuild has been queued (task_id={result.id}). " + "Monitor progress in Celery worker logs." + ) + return + result = rebuild_index_incremental.delay() if settings.CELERY_ALWAYS_EAGER: diff --git a/openedx/core/djangoapps/content/search/migrations/0003_clear_library_incremental_index_checkpoints.py b/openedx/core/djangoapps/content/search/migrations/0003_clear_library_incremental_index_checkpoints.py new file mode 100644 index 000000000000..95e9a1e28e58 --- /dev/null +++ b/openedx/core/djangoapps/content/search/migrations/0003_clear_library_incremental_index_checkpoints.py @@ -0,0 +1,26 @@ +from django.db import migrations +from opaque_keys.edx.locator import LibraryLocatorV2 + + +def clear_library_checkpoints(apps, schema_editor): + """ + Checkpoints written before the course/library index split record libraries indexed into the course index. + An incremental rebuild would skip those libraries and never add them to the new library index. + """ + IncrementalIndexCompleted = apps.get_model("search", "IncrementalIndexCompleted") + library_ids = [ + checkpoint.id + for checkpoint in IncrementalIndexCompleted.objects.all() + if isinstance(checkpoint.context_key, LibraryLocatorV2) + ] + IncrementalIndexCompleted.objects.filter(id__in=library_ids).delete() + + +class Migration(migrations.Migration): + dependencies = [ + ("search", "0002_incrementalindexcompleted"), + ] + + operations = [ + migrations.RunPython(clear_library_checkpoints, migrations.RunPython.noop), + ] diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py index 2245cc85fa76..3a94bd362129 100644 --- a/openedx/core/djangoapps/content/search/tasks.py +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -203,3 +203,28 @@ def rebuild_index_incremental() -> None: raise log.info("Incremental Studio search index population complete.") + + +@shared_task( + base=LoggedTask, + autoretry_for=(MeilisearchError, ConnectionError), + max_retries=3, + retry_backoff=True, +) +def rebuild_library_index() -> None: + """ + Celery task to rebuild the Studio library index and remove library documents from the course index. + + Run once when upgrading from the single shared index. Courses are not reindexed. + """ + log.info("Starting Studio library index rebuild...") + + try: + api.rebuild_index(status_cb=log.info, include_courses=False) + except RuntimeError as exc: + if "already in progress" in str(exc).lower(): + log.warning("Studio library index rebuild skipped: a rebuild is already in progress.") + return + raise + + log.info("Studio library index rebuild complete.") diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 3fd6859cafd8..c81a4521faff 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -4,16 +4,19 @@ from __future__ import annotations import copy +import importlib +from collections import defaultdict from datetime import UTC, datetime from unittest.mock import MagicMock, Mock, call, patch import ddt import pytest +from django.apps import apps from django.test import override_settings from freezegun import freeze_time -from meilisearch.errors import MeilisearchApiError -from opaque_keys.edx.keys import UsageKey -from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator +from meilisearch.errors import MeilisearchApiError, MeilisearchError +from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2 from openedx_content import api as content_api from openedx_content import models_api as content_models from organizations.tests.factories import OrganizationFactory @@ -337,6 +340,20 @@ def tearDown(self): content_models.Container.reset_cache() return super().tearDown() + def _mock_indexes(self, mock_meilisearch) -> defaultdict: + """ + Give each index name its own mock, so tests can assert which index a call went to. + """ + indexes = defaultdict(MagicMock) + mock_meilisearch.return_value.index.side_effect = lambda name: indexes[name] + return indexes + + def _indexes_used(self, mock_meilisearch) -> set[str]: + """ + Names of all indexes that were written to via client.index(name). + """ + return {c.args[0] for c in mock_meilisearch.return_value.index.call_args_list} + @override_settings(MEILISEARCH_ENABLED=False) def test_reindex_meilisearch_disabled(self, mock_meilisearch) -> None: with self.assertRaises(RuntimeError): # noqa: PT027 @@ -374,17 +391,79 @@ def test_reindex_meilisearch(self, mock_meilisearch) -> None: doc_section["tags"] = copy.deepcopy(EMPTY_TAGS) doc_section["collections"] = {'display_name': [], 'key': []} + indexes = self._mock_indexes(mock_meilisearch) api.rebuild_index() - assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4 - mock_meilisearch.return_value.index.return_value.add_documents.assert_has_calls( + + # Library content goes to a temporary library index, course content to a temporary course index + library_temp_index = indexes[api.STUDIO_LIBRARY_INDEX_NAME + "_new"] + course_temp_index = indexes[api.STUDIO_COURSE_INDEX_NAME + "_new"] + assert library_temp_index.add_documents.call_count == 3 + library_temp_index.add_documents.assert_has_calls( [ - call([doc_sequential, doc_vertical]), call([doc_problem1, doc_problem2]), call([doc_collection]), call([doc_unit, doc_subsection, doc_section]), ], any_order=True, ) + course_temp_index.add_documents.assert_called_once_with([doc_sequential, doc_vertical]) + library_temp_index.update_filterable_attributes.assert_called_once_with(api.INDEX_FILTERABLE_ATTRIBUTES) + course_temp_index.update_filterable_attributes.assert_called_once_with(api.INDEX_FILTERABLE_ATTRIBUTES) + # Each temporary index is swapped into its own live index + assert mock_meilisearch.return_value.swap_indexes.call_args_list == [ + call([{"indexes": [api.STUDIO_LIBRARY_INDEX_NAME + "_new", api.STUDIO_LIBRARY_INDEX_NAME]}]), + call([{"indexes": [api.STUDIO_COURSE_INDEX_NAME + "_new", api.STUDIO_COURSE_INDEX_NAME]}]), + ] + # Library documents left over from the single shared index are removed from the course index + indexes[api.STUDIO_COURSE_INDEX_NAME].delete_documents.assert_called_once_with( + filter='type != "course_block"' + ) + indexes[api.STUDIO_LIBRARY_INDEX_NAME].delete_documents.assert_not_called() + + @override_settings(MEILISEARCH_ENABLED=True) + def test_reindex_meilisearch_libraries_only(self, mock_meilisearch) -> None: + """ + include_courses=False rebuilds the library index and cleans up the course index without reindexing courses. + """ + indexes = self._mock_indexes(mock_meilisearch) + api.rebuild_index(include_courses=False) + + assert indexes[api.STUDIO_LIBRARY_INDEX_NAME + "_new"].add_documents.call_count == 3 + mock_meilisearch.return_value.swap_indexes.assert_called_once_with( + [{"indexes": [api.STUDIO_LIBRARY_INDEX_NAME + "_new", api.STUDIO_LIBRARY_INDEX_NAME]}] + ) + assert api.STUDIO_COURSE_INDEX_NAME + "_new" not in indexes + course_index = indexes[api.STUDIO_COURSE_INDEX_NAME] + course_index.add_documents.assert_not_called() + course_index.delete_documents.assert_called_once_with(filter='type != "course_block"') + + @override_settings(MEILISEARCH_ENABLED=True) + def test_reindex_library_write_error_aborts_before_swap(self, mock_meilisearch) -> None: + """ + A failed library write must not swap in the partial library index or clean up the course index. + """ + indexes = self._mock_indexes(mock_meilisearch) + indexes[api.STUDIO_LIBRARY_INDEX_NAME + "_new"].add_documents.side_effect = MeilisearchError("write failed") + + with pytest.raises(MeilisearchError): + api.rebuild_index(include_courses=False) + + mock_meilisearch.return_value.swap_indexes.assert_not_called() + indexes[api.STUDIO_COURSE_INDEX_NAME].delete_documents.assert_not_called() + + def test_migration_clears_library_incremental_checkpoints(self, mock_meilisearch) -> None: + """ + Library checkpoints from before the index split are dropped so incremental rebuilds reindex those libraries. + """ + migration = importlib.import_module( + "openedx.core.djangoapps.content.search.migrations.0003_clear_library_incremental_index_checkpoints" + ) + IncrementalIndexCompleted.objects.create(context_key=self.library.key) + IncrementalIndexCompleted.objects.create(context_key=self.course.id) + + migration.clear_library_checkpoints(apps, None) + + assert list(IncrementalIndexCompleted.objects.values_list("context_key", flat=True)) == [self.course.id] @override_settings(MEILISEARCH_ENABLED=True) def test_reindex_meilisearch_incremental(self, mock_meilisearch) -> None: @@ -416,17 +495,24 @@ def test_reindex_meilisearch_incremental(self, mock_meilisearch) -> None: doc_section["tags"] = copy.deepcopy(EMPTY_TAGS) doc_section["collections"] = {'display_name': [], 'key': []} + indexes = self._mock_indexes(mock_meilisearch) + library_index = indexes[api.STUDIO_LIBRARY_INDEX_NAME] + course_index = indexes[api.STUDIO_COURSE_INDEX_NAME] + api.rebuild_index(incremental=True) - assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4 - mock_meilisearch.return_value.index.return_value.add_documents.assert_has_calls( + # Incremental rebuilds write straight into the live indexes + mock_meilisearch.return_value.swap_indexes.assert_not_called() + assert library_index.add_documents.call_count == 3 + library_index.add_documents.assert_has_calls( [ - call([doc_sequential, doc_vertical]), call([doc_problem1, doc_problem2]), call([doc_collection]), call([doc_unit, doc_subsection, doc_section]), ], any_order=True, ) + course_index.add_documents.assert_called_once_with([doc_sequential, doc_vertical]) + course_index.delete_documents.assert_called_once_with(filter='type != "course_block"') # Now we simulate interruption by passing this function to the status_cb argument def simulated_interruption(message): @@ -437,21 +523,27 @@ def simulated_interruption(message): with pytest.raises(Exception, match="Simulated interruption"): api.rebuild_index(simulated_interruption, incremental=True) - # three more calls due to collections and containers - assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 7 + # The library was indexed again (blocks, collections and containers); no courses were + assert library_index.add_documents.call_count == 6 + assert course_index.add_documents.call_count == 1 assert IncrementalIndexCompleted.objects.all().count() == 1 api.rebuild_index(incremental=True) assert IncrementalIndexCompleted.objects.all().count() == 0 - # one missing course indexed - assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 8 + # one missing course indexed, the already-indexed library skipped + assert course_index.add_documents.call_count == 2 + assert library_index.add_documents.call_count == 6 @override_settings(MEILISEARCH_ENABLED=True) def test_reset_meilisearch_index(self, mock_meilisearch) -> None: - api.reset_index() - mock_meilisearch.return_value.swap_indexes.assert_called_once() - mock_meilisearch.return_value.create_index.assert_called_once() + api.reset_index(api.STUDIO_LIBRARY_INDEX_NAME) + mock_meilisearch.return_value.swap_indexes.assert_called_once_with( + [{"indexes": [api.STUDIO_LIBRARY_INDEX_NAME + "_new", api.STUDIO_LIBRARY_INDEX_NAME]}] + ) + mock_meilisearch.return_value.create_index.assert_called_once_with( + api.STUDIO_LIBRARY_INDEX_NAME + "_new", {"primaryKey": "id"} + ) mock_meilisearch.return_value.delete_index.call_count = 2 - api.reset_index() + api.reset_index(api.STUDIO_LIBRARY_INDEX_NAME) mock_meilisearch.return_value.delete_index.call_count = 4 @override_settings(MEILISEARCH_ENABLED=True) @@ -481,18 +573,22 @@ def test_init_meilisearch_index(self, mock_meilisearch) -> None: mock_meilisearch.return_value.create_index.assert_not_called() mock_meilisearch.return_value.delete_index.assert_not_called() - # Test index does not exist — should create it - mock_meilisearch.return_value.get_index.side_effect = [ - MeilisearchApiError("Testing reindex", Mock(text='{"code":"index_not_found"}')), - MeilisearchApiError("Testing reindex", Mock(text='{"code":"index_not_found"}')), - Mock(created_at=1), - Mock(created_at=1), - Mock(created_at=1), - ] + # Test the library index does not exist (upgrade from a single shared index) — should create only that one + created_indexes = set() + + def get_index(name): + if name == api.STUDIO_COURSE_INDEX_NAME or name in created_indexes: + return mock_index + raise MeilisearchApiError("Testing reindex", Mock(text='{"code":"index_not_found"}')) + + mock_meilisearch.return_value.get_index.side_effect = get_index + mock_meilisearch.return_value.create_index.side_effect = lambda name, *args: created_indexes.add(name) api.init_index() - mock_meilisearch.return_value.swap_indexes.assert_called_once() - mock_meilisearch.return_value.create_index.assert_called_once() - mock_meilisearch.return_value.delete_index.call_count = 2 + mock_meilisearch.return_value.swap_indexes.assert_called_once_with( + [{"indexes": [api.STUDIO_LIBRARY_INDEX_NAME + "_new", api.STUDIO_LIBRARY_INDEX_NAME]}] + ) + assert created_indexes == {api.STUDIO_LIBRARY_INDEX_NAME + "_new", api.STUDIO_LIBRARY_INDEX_NAME} + mock_meilisearch.return_value.delete_index.assert_called_once_with(api.STUDIO_LIBRARY_INDEX_NAME + "_new") @override_settings(MEILISEARCH_ENABLED=True) @patch( @@ -597,6 +693,7 @@ def test_index_xblock_metadata(self, recursive, mock_meilisearch) -> None: expected_docs = [self.doc_sequential] mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with(expected_docs) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_COURSE_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_no_index_excluded_xblocks(self, mock_meilisearch) -> None: @@ -643,6 +740,7 @@ def test_index_xblock_tags(self, mock_meilisearch) -> None: ], any_order=True, ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_COURSE_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_remove_xblock_tag_clears_index_tags(self, mock_meilisearch) -> None: @@ -693,6 +791,7 @@ def test_delete_index_xblock(self, mock_meilisearch) -> None: mock_meilisearch.return_value.index.return_value.delete_documents.assert_called_once_with( filter=f'breadcrumbs.usage_key = "{self.sequential.usage_key}"' ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_COURSE_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_index_library_block_metadata(self, mock_meilisearch) -> None: @@ -702,6 +801,7 @@ def test_index_library_block_metadata(self, mock_meilisearch) -> None: api.upsert_library_block_index_doc(self.problem1.usage_key) mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([self.doc_problem1]) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_index_library_block_tags(self, mock_meilisearch) -> None: @@ -743,6 +843,7 @@ def test_index_library_block_tags(self, mock_meilisearch) -> None: ], any_order=True, ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_index_library_block_and_collections(self, mock_meilisearch) -> None: @@ -885,6 +986,7 @@ def test_index_library_block_and_collections(self, mock_meilisearch) -> None: ], any_order=True, ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_delete_index_library_block(self, mock_meilisearch) -> None: @@ -896,17 +998,24 @@ def test_delete_index_library_block(self, mock_meilisearch) -> None: mock_meilisearch.return_value.index.return_value.delete_document.assert_called_once_with( self.doc_problem1['id'] ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_delete_docs_with_context_key(self, mock_meilisearch) -> None: """ Test deleting a all Block docs from the index using context_key. """ + indexes = self._mock_indexes(mock_meilisearch) + api.delete_docs_with_context_key(self.course.id) + api.delete_docs_with_context_key(self.library.key) - mock_meilisearch.return_value.index.return_value.delete_documents.assert_called_once_with( + indexes[api.STUDIO_COURSE_INDEX_NAME].delete_documents.assert_called_once_with( filter=f'context_key = "{self.course.id}"' ) + indexes[api.STUDIO_LIBRARY_INDEX_NAME].delete_documents.assert_called_once_with( + filter=f'context_key = "{self.library.key}"' + ) @override_settings(MEILISEARCH_ENABLED=True) def test_index_content_library_metadata(self, mock_meilisearch) -> None: @@ -918,6 +1027,7 @@ def test_index_content_library_metadata(self, mock_meilisearch) -> None: mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with( [self.doc_problem1, self.doc_problem2] ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_index_tags_in_collections(self, mock_meilisearch) -> None: @@ -1083,6 +1193,7 @@ def test_delete_collection(self, mock_meilisearch) -> None: ], any_order=True, ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @ddt.data( "unit", @@ -1188,6 +1299,7 @@ def test_index_library_container_metadata(self, container_type, mock_meilisearch api.upsert_library_container_index_doc(container.container_key) mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([container_dict]) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @ddt.data( ("unit", "lctorg1libunitunit-1-e4527f7c"), @@ -1276,6 +1388,7 @@ def test_block_in_units(self, mock_meilisearch) -> None: ], any_order=True, ) + assert self._indexes_used(mock_meilisearch) == {api.STUDIO_LIBRARY_INDEX_NAME} @override_settings(MEILISEARCH_ENABLED=True) def test_units_in_subsection(self, mock_meilisearch) -> None: @@ -1379,6 +1492,7 @@ def test_fetch_block_types(self, mock_meilisearch): mock_index = mock_meilisearch.return_value.get_index.return_value fetch_block_types('context_key = test') + mock_meilisearch.return_value.get_index.assert_called_once_with(api.STUDIO_COURSE_INDEX_NAME) mock_index.search.assert_called_once_with( "", { @@ -1436,3 +1550,52 @@ def test_get_all_blocks_from_context(self, mock_meilisearch): "attributesToRetrieve": ["usage_key", "display_name"], } ) + mock_meilisearch.return_value.get_index.assert_called_once_with(api.STUDIO_COURSE_INDEX_NAME) + + @ddt.data( + (CourseKey, "course-v1:org1+test_course+test_run", "course"), + (UsageKey, "block-v1:org1+test_course+test_run+type@sequential+block@test_sequential", "course"), + (CourseKey, "library-v1:org1+legacy_lib", "course"), + (LibraryLocatorV2, "lib:org1:lib", "library"), + (UsageKey, "lb:org1:lib:problem:p1", "library"), + (LibraryCollectionLocator, "lib-collection:org1:lib:MYCOL", "library"), + (LibraryContainerLocator, "lct:org1:lib:unit:unit-1", "library"), + ) + @ddt.unpack + def test_index_name_for_key(self, key_type, key_str, expected, mock_meilisearch) -> None: + """ + Modulestore content (courses, legacy libraries) routes to the course index, Libraries V2 content to the + library index. + """ + expected_index = {"course": api.STUDIO_COURSE_INDEX_NAME, "library": api.STUDIO_LIBRARY_INDEX_NAME}[expected] + assert api._index_name_for_key(key_type.from_string(key_str)) == expected_index # pylint: disable=protected-access + + @override_settings(MEILISEARCH_ENABLED=True) + def test_update_during_rebuild_uses_that_index_temp_index(self, mock_meilisearch) -> None: + """ + While the library index is being rebuilt, library writes also go to its temporary index. + Course writes are unaffected by the library rebuild lock. + """ + indexes = self._mock_indexes(mock_meilisearch) + + with api._index_rebuild_lock(api.STUDIO_LIBRARY_INDEX_NAME): # pylint: disable=protected-access + api.upsert_library_block_index_doc(self.problem1.usage_key) + api.upsert_xblock_index_doc(self.sequential.usage_key, recursive=False) + + indexes[api.STUDIO_LIBRARY_INDEX_NAME + "_new"].update_documents.assert_called_once_with([self.doc_problem1]) + indexes[api.STUDIO_LIBRARY_INDEX_NAME].update_documents.assert_called_once_with([self.doc_problem1]) + indexes[api.STUDIO_COURSE_INDEX_NAME].update_documents.assert_called_once_with([self.doc_sequential]) + assert api.STUDIO_COURSE_INDEX_NAME + "_new" not in indexes + + @override_settings(MEILISEARCH_ENABLED=True) + def test_delete_library_docs_from_missing_course_index(self, mock_meilisearch) -> None: + """ + Cleaning up the course index does nothing if there is no course index yet. + """ + mock_meilisearch.return_value.get_index.side_effect = MeilisearchApiError( + "Not found", Mock(text='{"code":"index_not_found"}') + ) + + api.delete_library_docs_from_course_index() + + mock_meilisearch.return_value.index.assert_not_called() diff --git a/openedx/core/djangoapps/content/search/tests/test_handlers.py b/openedx/core/djangoapps/content/search/tests/test_handlers.py index e2215c7f3c56..9220acdf8d5f 100644 --- a/openedx/core/djangoapps/content/search/tests/test_handlers.py +++ b/openedx/core/djangoapps/content/search/tests/test_handlers.py @@ -82,6 +82,7 @@ def test_create_delete_xblock(self, meilisearch_client): } meilisearch_client.return_value.index.return_value.update_documents.assert_called_with([doc_sequential]) + meilisearch_client.return_value.index.assert_called_with(api.STUDIO_COURSE_INDEX_NAME) with freeze_time(created_date), self.captureOnCommitCallbacks(execute=True): vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") @@ -134,6 +135,9 @@ def test_create_delete_xblock(self, meilisearch_client): meilisearch_client.return_value.index.return_value.delete_document.assert_called_with( "block-v1orgatest_coursetest_runtypeverticalblocktest_vertical-011f143b" ) + assert {c.args[0] for c in meilisearch_client.return_value.index.call_args_list} == { + api.STUDIO_COURSE_INDEX_NAME + } def test_library_creation_creates_search_access(self, meilisearch_client): """ @@ -225,3 +229,6 @@ def test_create_delete_library_block(self, meilisearch_client): with self.captureOnCommitCallbacks(execute=True): library_api.restore_library_block(problem.usage_key) meilisearch_client.return_value.index.return_value.update_documents.assert_any_call([doc_problem]) + assert {c.args[0] for c in meilisearch_client.return_value.index.call_args_list} == { + api.STUDIO_LIBRARY_INDEX_NAME + } diff --git a/openedx/core/djangoapps/content/search/tests/test_reconcile.py b/openedx/core/djangoapps/content/search/tests/test_reconcile.py index 6904f555ebb4..1b44b8c6acb0 100644 --- a/openedx/core/djangoapps/content/search/tests/test_reconcile.py +++ b/openedx/core/djangoapps/content/search/tests/test_reconcile.py @@ -1,10 +1,11 @@ """ Tests for the Meilisearch index reconciliation logic. """ +# pylint: disable=protected-access from __future__ import annotations -from unittest.mock import MagicMock, Mock, patch +from unittest.mock import MagicMock, Mock, call, patch import pytest from django.test import TestCase, override_settings @@ -18,7 +19,7 @@ IndexDrift, _apply_index_settings, _detect_index_drift, - reconcile_index, + reconcile_indexes, ) from ..apps import ContentSearchConfig from ..handlers import handle_post_migrate @@ -349,12 +350,55 @@ def test_raises_on_task_failure(self, mock_wait, mock_meilisearch): @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 TestReconcileIndex(TestCase): - """Tests for reconcile_index().""" + """Tests for reconcile_indexes().""" def setUp(self): super().setUp() api.clear_meilisearch_client() + @patch("openedx.core.djangoapps.content.search.api.reconcile_index") + def test_reconciles_both_indexes(self, mock_reconcile_single, mock_meilisearch): + """Both the course index and the library index are reconciled.""" + status_cb = Mock() + warn_cb = Mock() + + reconcile_indexes(status_cb=status_cb, warn_cb=warn_cb) + + assert mock_reconcile_single.call_args_list == [ + call(api.STUDIO_COURSE_INDEX_NAME, status_cb, warn_cb), + call(api.STUDIO_LIBRARY_INDEX_NAME, status_cb, warn_cb), + ] + + @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") + @patch("openedx.core.djangoapps.content.search.api.reset_index") + def test_only_library_index_missing(self, mock_reset, mock_drift, mock_meilisearch): + """ + Upgrading from a single shared index: the populated course index is left alone and the library index is + created, with a hint to populate it without reindexing courses. + """ + populated = IndexDrift( + exists=True, + is_empty=False, + primary_key_correct=True, + distinct_attribute_match=True, + filterable_attributes_match=True, + searchable_attributes_match=True, + sortable_attributes_match=True, + ranking_rules_match=True, + ) + mock_drift.side_effect = lambda name: ( + populated if name == api.STUDIO_COURSE_INDEX_NAME else IndexDrift(exists=False) + ) + status_cb = Mock() + + reconcile_indexes(status_cb=status_cb) + + mock_reset.assert_called_once_with(api.STUDIO_LIBRARY_INDEX_NAME, status_cb) + status_cb.assert_any_call( + f"Index '{api.STUDIO_LIBRARY_INDEX_NAME}' created. " + "Run './manage.py cms reindex_studio --libraries-only' to populate." + ) + @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") @patch("openedx.core.djangoapps.content.search.api.reset_index") def test_index_missing(self, mock_reset, mock_drift, mock_meilisearch): @@ -362,10 +406,12 @@ def test_index_missing(self, mock_reset, mock_drift, mock_meilisearch): mock_drift.return_value = IndexDrift(exists=False) status_cb = Mock() - reconcile_index(status_cb=status_cb) + api.reconcile_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) - mock_reset.assert_called_once() - status_cb.assert_any_call("Studio search index not found. Creating and configuring...") + mock_reset.assert_called_once_with(api.STUDIO_COURSE_INDEX_NAME, status_cb) + status_cb.assert_any_call( + f"Studio search index '{api.STUDIO_COURSE_INDEX_NAME}' not found. Creating and configuring..." + ) @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") def test_index_empty_configured(self, mock_drift, mock_meilisearch): @@ -382,10 +428,11 @@ def test_index_empty_configured(self, mock_drift, mock_meilisearch): ) status_cb = Mock() - reconcile_index(status_cb=status_cb) + api.reconcile_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) status_cb.assert_any_call( - "Index exists and is correctly configured but empty. Run './manage.py cms reindex_studio' to populate." + f"Index '{api.STUDIO_COURSE_INDEX_NAME}' exists and is correctly configured but empty. " + "Run './manage.py cms reindex_studio' to populate." ) @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") @@ -404,10 +451,12 @@ def test_index_empty_drifted_settings(self, mock_apply, mock_drift, mock_meilise ) status_cb = Mock() - reconcile_index(status_cb=status_cb) + api.reconcile_index(api.STUDIO_LIBRARY_INDEX_NAME, status_cb, Mock()) - mock_apply.assert_called_once_with(api.STUDIO_INDEX_NAME, wait=True, status_cb=status_cb) - status_cb.assert_any_call("Empty index has drifted settings. Reconfiguring...") + mock_apply.assert_called_once_with(api.STUDIO_LIBRARY_INDEX_NAME, wait=True, status_cb=status_cb) + status_cb.assert_any_call( + f"Empty index '{api.STUDIO_LIBRARY_INDEX_NAME}' has drifted settings. Reconfiguring..." + ) @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") @patch("openedx.core.djangoapps.content.search.api.reset_index") @@ -425,10 +474,12 @@ def test_index_empty_wrong_pk(self, mock_reset, mock_drift, mock_meilisearch): ) warn_cb = Mock() - reconcile_index(warn_cb=warn_cb) + api.reconcile_index(api.STUDIO_COURSE_INDEX_NAME, Mock(), warn_cb) mock_reset.assert_called_once() - warn_cb.assert_any_call("Primary key mismatch on empty index. Recreating...") + warn_cb.assert_any_call( + f"Primary key mismatch on empty index '{api.STUDIO_COURSE_INDEX_NAME}'. Recreating..." + ) @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") def test_index_populated_configured(self, mock_drift, mock_meilisearch): @@ -445,9 +496,11 @@ def test_index_populated_configured(self, mock_drift, mock_meilisearch): ) status_cb = Mock() - reconcile_index(status_cb=status_cb) + api.reconcile_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, Mock()) - status_cb.assert_any_call("Index is populated and correctly configured. No action needed.") + status_cb.assert_any_call( + f"Index '{api.STUDIO_COURSE_INDEX_NAME}' is populated and correctly configured. No action needed." + ) @patch("openedx.core.djangoapps.content.search.api._detect_index_drift") @patch("openedx.core.djangoapps.content.search.api._apply_index_settings") @@ -466,9 +519,9 @@ def test_index_populated_drifted_settings(self, mock_apply, mock_drift, mock_mei status_cb = Mock() warn_cb = Mock() - reconcile_index(status_cb=status_cb, warn_cb=warn_cb) + api.reconcile_index(api.STUDIO_COURSE_INDEX_NAME, status_cb, warn_cb) - mock_apply.assert_called_once_with(api.STUDIO_INDEX_NAME, wait=True, status_cb=status_cb) + mock_apply.assert_called_once_with(api.STUDIO_COURSE_INDEX_NAME, wait=True, status_cb=status_cb) # Check that drifted fields are logged warn_cb.assert_any_call(" - filterableAttributes: DRIFTED") warn_cb.assert_any_call(" - searchableAttributes: DRIFTED") @@ -492,18 +545,21 @@ def test_index_populated_wrong_pk(self, mock_reset, mock_drift, mock_meilisearch ) warn_cb = Mock() - reconcile_index(warn_cb=warn_cb) + api.reconcile_index(api.STUDIO_LIBRARY_INDEX_NAME, Mock(), warn_cb) mock_reset.assert_called_once() # Should warn about data loss - warn_cb.assert_any_call("Index recreated empty. Run './manage.py cms reindex_studio' to repopulate.") + warn_cb.assert_any_call( + f"Index '{api.STUDIO_LIBRARY_INDEX_NAME}' recreated empty. " + "Run './manage.py cms reindex_studio --libraries-only' to repopulate." + ) @override_settings(MEILISEARCH_ENABLED=False) def test_meilisearch_disabled(self, mock_meilisearch): - """When Meilisearch is disabled, reconcile_index raises RuntimeError (from client).""" + """When Meilisearch is disabled, reconcile_indexes raises RuntimeError (from client).""" api.clear_meilisearch_client() with pytest.raises(RuntimeError): - reconcile_index() + reconcile_indexes() @skip_unless_cms @@ -517,9 +573,9 @@ def setUp(self): super().setUp() api.clear_meilisearch_client() - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_calls_reconcile_for_search_app(self, mock_reconcile, mock_meilisearch): - """Handler calls reconcile_index when sender is the search app.""" + """Handler calls reconcile_indexes when sender is the search app.""" sender = Mock() sender.label = ContentSearchConfig.label @@ -527,7 +583,7 @@ def test_calls_reconcile_for_search_app(self, mock_reconcile, mock_meilisearch): mock_reconcile.assert_called_once() - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_skips_wrong_sender(self, mock_reconcile, mock_meilisearch): """Handler does nothing when sender is a different app.""" sender = Mock() @@ -538,7 +594,7 @@ def test_skips_wrong_sender(self, mock_reconcile, mock_meilisearch): mock_reconcile.assert_not_called() @override_settings(MEILISEARCH_ENABLED=False) - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_skips_when_disabled(self, mock_reconcile, mock_meilisearch): """Handler does nothing when Meilisearch is disabled.""" sender = Mock() @@ -548,7 +604,7 @@ def test_skips_when_disabled(self, mock_reconcile, mock_meilisearch): mock_reconcile.assert_not_called() - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_catches_connection_error(self, mock_reconcile, mock_meilisearch): """Handler catches ConnectionError and logs warning.""" sender = Mock() @@ -558,7 +614,7 @@ def test_catches_connection_error(self, mock_reconcile, mock_meilisearch): # Should not raise handle_post_migrate(sender=sender) - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_catches_meilisearch_error(self, mock_reconcile, mock_meilisearch): """Handler catches MeilisearchError and logs warning.""" sender = Mock() @@ -568,7 +624,7 @@ def test_catches_meilisearch_error(self, mock_reconcile, mock_meilisearch): # Should not raise handle_post_migrate(sender=sender) - @patch("openedx.core.djangoapps.content.search.handlers.reconcile_index") + @patch("openedx.core.djangoapps.content.search.handlers.reconcile_indexes") def test_catches_generic_exception(self, mock_reconcile, mock_meilisearch): """Handler catches unexpected exceptions and logs warning.""" sender = Mock() @@ -600,9 +656,9 @@ def setUp(self): super().setUp() api.clear_meilisearch_client() - @patch("openedx.core.djangoapps.content.search.api.reconcile_index") + @patch("openedx.core.djangoapps.content.search.api.reconcile_indexes") def test_init_index_delegates_to_reconcile(self, mock_reconcile, mock_meilisearch): - """init_index() should delegate to reconcile_index().""" + """init_index() should delegate to reconcile_indexes().""" status_cb = Mock() warn_cb = Mock() diff --git a/openedx/core/djangoapps/content/search/tests/test_reindex_cmd.py b/openedx/core/djangoapps/content/search/tests/test_reindex_cmd.py index 45a89d7d9e04..30e7b8b49681 100644 --- a/openedx/core/djangoapps/content/search/tests/test_reindex_cmd.py +++ b/openedx/core/djangoapps/content/search/tests/test_reindex_cmd.py @@ -14,7 +14,7 @@ try: from .. import api - from ..tasks import rebuild_index_incremental + from ..tasks import rebuild_index_incremental, rebuild_library_index except RuntimeError: pass @@ -50,6 +50,17 @@ def test_incremental_flag_accepted_with_warning(self, mock_log, mock_delay): assert mock_log.warning.call_count == 4 mock_delay.assert_called_once_with() + @patch("openedx.core.djangoapps.content.search.tasks.rebuild_index_incremental.delay") + @patch("openedx.core.djangoapps.content.search.tasks.rebuild_library_index.delay") + def test_libraries_only(self, mock_library_delay, mock_incremental_delay): + """--libraries-only enqueues the library index rebuild and not the full incremental rebuild.""" + mock_library_delay.return_value = Mock(id="fake-task-id") + + call_command("reindex_studio", "--libraries-only") + + mock_library_delay.assert_called_once_with() + mock_incremental_delay.assert_not_called() + @skip_unless_cms @override_settings(MEILISEARCH_ENABLED=True) @@ -94,3 +105,41 @@ def test_idempotent(self, mock_rebuild, mock_meilisearch): rebuild_index_incremental() assert mock_rebuild.call_count == 2 + + +@skip_unless_cms +@override_settings(MEILISEARCH_ENABLED=True) +@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 TestRebuildLibraryIndexTask(TestCase): + """Tests for the rebuild_library_index Celery task.""" + + def setUp(self): + super().setUp() + api.clear_meilisearch_client() + + @patch("openedx.core.djangoapps.content.search.api.rebuild_index") + def test_rebuilds_libraries_only(self, mock_rebuild, mock_meilisearch): + """Task rebuilds the library index without reindexing courses.""" + rebuild_library_index() + + mock_rebuild.assert_called_once() + _, kwargs = mock_rebuild.call_args + assert kwargs["include_courses"] is False + assert kwargs.get("incremental", False) is False + + @patch("openedx.core.djangoapps.content.search.api.rebuild_index") + def test_rebuild_already_in_progress(self, mock_rebuild, mock_meilisearch): + """Task exits gracefully if the library index rebuild lock is already held.""" + mock_rebuild.side_effect = RuntimeError("Rebuild already in progress") + + # Should not raise + rebuild_library_index() + + @patch("openedx.core.djangoapps.content.search.api.rebuild_index") + def test_other_runtime_error_raised(self, mock_rebuild, mock_meilisearch): + """Task re-raises RuntimeError if it's not about lock contention.""" + mock_rebuild.side_effect = RuntimeError("Something else went wrong") + + with pytest.raises(RuntimeError, match="Something else went wrong"): + rebuild_library_index() diff --git a/openedx/core/djangoapps/content/search/tests/test_views.py b/openedx/core/djangoapps/content/search/tests/test_views.py index 3d056b2b4327..8ab573ff50c5 100644 --- a/openedx/core/djangoapps/content/search/tests/test_views.py +++ b/openedx/core/djangoapps/content/search/tests/test_views.py @@ -53,6 +53,16 @@ def wrapper(*args, **kwargs): return decorator +def search_rules_for_both_indexes(rule: dict) -> dict: + """ + The tenant token applies the same access rule to the course index and the library index. + """ + return { + "studio_content": rule, + "studio_library_content": rule, + } + + @ddt.ddt @skip_unless_cms @patch("openedx.core.djangoapps.content.search.api._wait_for_meili_task", new=MagicMock(return_value=None)) @@ -113,6 +123,9 @@ def test_studio_search_enabled(self, mock_search_client): mock_generate_tenant_token = self._mock_generate_tenant_token(mock_search_client) # noqa: F841 result = self.client.get(STUDIO_SEARCH_ENDPOINT_URL) assert result.status_code == 200 + assert result.data["course_index_name"] == "studio_content" + assert result.data["library_index_name"] == "studio_library_content" + # Deprecated key, kept for frontends that predate the index split 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) # noqa: PT018 @@ -129,11 +142,9 @@ def test_studio_search_student_no_access(self, mock_search_client): assert result.status_code == 200 mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": { - "filter": "org IN [] OR access_id IN []", - } - }, + search_rules=search_rules_for_both_indexes({ + "filter": "org IN [] OR access_id IN []", + }), expires_at=ANY, ) @@ -149,9 +160,7 @@ def test_studio_search_staff(self, mock_search_client): assert result.status_code == 200 mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": {} - }, + search_rules=search_rules_for_both_indexes({}), expires_at=ANY, ) @@ -173,11 +182,9 @@ def test_studio_search_course_staff_access(self, mock_search_client): mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": { - "filter": f"org IN [] OR access_id IN {expected_access_ids}", - } - }, + search_rules=search_rules_for_both_indexes({ + "filter": f"org IN [] OR access_id IN {expected_access_ids}", + }), expires_at=ANY, ) @@ -197,11 +204,9 @@ def test_studio_search_org_access(self, username, mock_search_client): assert result.status_code == 200 mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": { - "filter": "org IN ['org1'] OR access_id IN []", - } - }, + search_rules=search_rules_for_both_indexes({ + "filter": "org IN ['org1'] OR access_id IN []", + }), expires_at=ANY, ) @@ -224,11 +229,9 @@ def test_studio_search_omit_orgs(self, mock_search_client): mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": { - "filter": f"org IN ['org1'] OR access_id IN {expected_access_ids}", - } - }, + search_rules=search_rules_for_both_indexes({ + "filter": f"org IN ['org1'] OR access_id IN {expected_access_ids}", + }), expires_at=ANY, ) @@ -259,10 +262,8 @@ def test_studio_search_limits(self, mock_search_client, mock_get_access_ids, moc mock_get_access_ids.assert_called_once() mock_generate_tenant_token.assert_called_once_with( api_key_uid=MOCK_API_KEY_UID, - search_rules={ - "studio_content": { - "filter": f"org IN {expected_user_orgs} OR access_id IN {expected_access_ids}", - } - }, + search_rules=search_rules_for_both_indexes({ + "filter": f"org IN {expected_user_orgs} OR access_id IN {expected_access_ids}", + }), expires_at=ANY, )