diff --git a/learning_resources/etl/constants.py b/learning_resources/etl/constants.py index 7f7ee4cb65..7f6351e55e 100644 --- a/learning_resources/etl/constants.py +++ b/learning_resources/etl/constants.py @@ -95,6 +95,15 @@ class ETLSource(ExtendedEnum): ovs = "ovs" +QDRANT_RETAINED_SOURCES = ( + ETLSource.mit_edx.value, + ETLSource.mitxonline.value, + ETLSource.xpro.value, + ETLSource.oll.value, + ETLSource.canvas.value, +) + + class CourseNumberType(Enum): """Enum of course number types""" diff --git a/learning_resources/etl/edx_shared.py b/learning_resources/etl/edx_shared.py index 060a2493b0..365210123d 100644 --- a/learning_resources/etl/edx_shared.py +++ b/learning_resources/etl/edx_shared.py @@ -77,8 +77,8 @@ def build_run_lookup( Args: etl_source(str): The ETL source ids(list of int): List of LearningResource IDs to filter by. - If empty/falsy, all published/test_mode runs for the source - are included. + If empty/falsy, all runs of every published/test_mode course + for the source are included. Returns: dict: Mapping of normalized run_id -> list of LearningResourceRun @@ -87,7 +87,6 @@ def build_run_lookup( LearningResourceRun.objects.filter( learning_resource__etl_source=etl_source, ) - .filter(Q(published=True) | Q(learning_resource__test_mode=True)) .filter( Q(learning_resource__published=True) | Q(learning_resource__test_mode=True) ) @@ -107,6 +106,9 @@ def build_run_lookup( for run in runs: normalized = normalize_run_id(etl_source, run.run_id) lookup.setdefault(normalized, []).append(run) + if etl_source == ETLSource.oll.name: + # OLL archive filenames omit the MITx+ prefix the run_ids carry + lookup.setdefault(normalized.removeprefix("mitx."), []).append(run) return lookup @@ -256,10 +258,12 @@ def sync_edx_archive( trigger_resource_etl(etl_source) return course = run.learning_resource - if course.published and not course.test_mode and course.best_run != run: - # This is not the best run for the published course, so skip it + if not course.published and not course.test_mode: + # Fully-retired course; skip it log.warning( - "%s not the best run for %s, skipping", run.run_id, course.readable_id + "%s belongs to a retired course %s, skipping", + run.run_id, + course.readable_id, ) return bucket = get_bucket_by_name(settings.COURSE_ARCHIVE_BUCKET_NAME) @@ -278,14 +282,10 @@ def run_for_edx_archive(etl_source: str, archive_filename: str): LearningResourceRun or None: The matching run, or None if not found """ normalized_run_id = extract_run_id_from_key(etl_source, archive_filename) - runs = ( - LearningResourceRun.objects.filter( - learning_resource__etl_source=etl_source, - ) - .filter(Q(published=True) | Q(learning_resource__test_mode=True)) - .filter( - Q(learning_resource__published=True) | Q(learning_resource__test_mode=True) - ) + runs = LearningResourceRun.objects.filter( + learning_resource__etl_source=etl_source, + ).filter( + Q(learning_resource__published=True) | Q(learning_resource__test_mode=True) ) if etl_source in (ETLSource.mit_edx.name, ETLSource.oll.name): runs = runs.filter(run_id__iregex=normalized_run_id) @@ -327,10 +327,4 @@ def sync_edx_course_files( log.warning("There are %d runs for %s", len(matching_runs), key) run = matching_runs[0] - course = run.learning_resource - - if course.published and not course.test_mode and course.best_run != run: - # This is not the best run for the published course, so skip it - log.debug("Not the best run for %s, skipping", run.run_id) - continue process_course_archive(bucket, key, run, overwrite=overwrite) diff --git a/learning_resources/etl/edx_shared_test.py b/learning_resources/etl/edx_shared_test.py index 85244874b1..d702f84861 100644 --- a/learning_resources/etl/edx_shared_test.py +++ b/learning_resources/etl/edx_shared_test.py @@ -36,13 +36,11 @@ (ETLSource.oll.name, PlatformType.edx.name), ], ) -@pytest.mark.parametrize("published", [True, False]) def test_sync_edx_course_files( mock_course_archive_bucket, mocker, source, platform, - published, ): # pylint: disable=too-many-arguments,too-many-locals """Sync edx courses from a tarball stored in S3""" mock_load_content_files = mocker.patch( @@ -61,47 +59,69 @@ def test_sync_edx_course_files( "learning_resources.etl.edx_shared.get_bucket_by_name", return_value=mock_course_archive_bucket.bucket, ) + platform_obj = LearningResourcePlatformFactory.create(code=platform) courses = LearningResourceFactory.create_batch( 2, - platform=LearningResourcePlatformFactory.create(code=platform), + platform=platform_obj, etl_source=source, is_course=True, published=True, create_runs=False, ) + # A fully-retired (unpublished, non-test_mode) course must be excluded + retired_course = LearningResourceFactory.create( + platform=platform_obj, + etl_source=source, + is_course=True, + published=False, + test_mode=False, + create_runs=False, + ) keys = [] s3_prefix = get_s3_prefix_for_source(source) - for course in courses: - runs = LearningResourceRunFactory.create_batch( - 2, - learning_resource=course, - published=published, - ) - course.refresh_from_db() - if published: - assert course.best_run in runs - keys.extend( + + def add_keys_for_runs(runs): + new_keys = ( [f"{s3_prefix}/{run.run_id}/foo.tar.gz" for run in runs] if source != ETLSource.oll.name else [f"{s3_prefix}/{run.run_id}_OLL.tar.gz" for run in runs] ) - for key in keys: + keys.extend(new_keys) + for key in new_keys: with Path.open( Path("test_json/course-v1:MITxT+8.01.3x+3T2022.tar.gz"), "rb" ) as infile: - bucket.put_object( - Key=key, - Body=infile.read(), - ACL="public-read", - ) - sync_edx_course_files(source, [course.id for course in courses], keys) - # Only best runs for published courses are processed, so 2 runs (one per course) not 4 - expected_calls = 2 if published else 0 - assert mock_transform.call_count == expected_calls - assert mock_load_content_files.call_count == expected_calls - if published: - for course in courses: - mock_load_content_files.assert_any_call(course.best_run, fake_data) + bucket.put_object(Key=key, Body=infile.read(), ACL="public-read") + + for course in courses: + runs = LearningResourceRunFactory.create_batch( + 2, + learning_resource=course, + published=True, + ) + course.refresh_from_db() + assert course.best_run in runs + add_keys_for_runs(runs) + + retired_runs = LearningResourceRunFactory.create_batch( + 2, + learning_resource=retired_course, + published=True, + ) + add_keys_for_runs(retired_runs) + + sync_edx_course_files( + source, [course.id for course in [*courses, retired_course]], keys + ) + # All runs of published courses are processed; retired-course runs are excluded + assert mock_transform.call_count == 4 + assert mock_load_content_files.call_count == 4 + loaded_runs = {call.args[0] for call in mock_load_content_files.call_args_list} + for course in courses: + for run in course.runs.all(): + assert run in loaded_runs + for run in retired_runs: + assert run not in loaded_runs mock_log.assert_not_called() @@ -692,8 +712,10 @@ def test_sync_edx_archive_no_run_found(mocker, mock_course_archive_bucket, etl_s @pytest.mark.parametrize("etl_source", [ETLSource.mitxonline.name, ETLSource.xpro.name]) -def test_sync_edx_archive_not_best_run(mocker, mock_course_archive_bucket, etl_source): - """Test sync_edx_archive skips processing when run is not the best run""" +def test_sync_edx_archive_non_best_run_processed( + mocker, mock_course_archive_bucket, etl_source +): + """sync_edx_archive now processes a non-best run of a published course.""" from learning_resources.etl.edx_shared import sync_edx_archive platform = ( @@ -701,15 +723,12 @@ def test_sync_edx_archive_not_best_run(mocker, mock_course_archive_bucket, etl_s if etl_source == ETLSource.mitxonline.name else PlatformType.xpro.name ) - - # Create a course with multiple runs course = LearningResourceFactory.create( platform=LearningResourcePlatformFactory.create(code=platform), etl_source=etl_source, published=True, create_runs=False, ) - # Create older run (not best) with earlier start date from datetime import UTC, datetime old_run = LearningResourceRunFactory.create( @@ -718,7 +737,6 @@ def test_sync_edx_archive_not_best_run(mocker, mock_course_archive_bucket, etl_s run_id="course-v1:Test+Course+R1", start_date=datetime(2022, 1, 1, tzinfo=UTC), ) - # Create newer run (will be best) with later start date LearningResourceRunFactory.create( learning_resource=course, published=True, @@ -726,35 +744,72 @@ def test_sync_edx_archive_not_best_run(mocker, mock_course_archive_bucket, etl_s start_date=datetime(2023, 1, 1, tzinfo=UTC), ) course.refresh_from_db() - - # Verify the newer run is the best run assert course.best_run.run_id == "course-v1:Test+Course+R2" - # Archive is for the old run, not the best run bucket = mock_course_archive_bucket.bucket s3_key = "20220101/courses/course-v1:Test+Course+R1/abcdefghijklmnop.tar.gz" - with Path.open( Path("test_json/course-v1:MITxT+8.01.3x+3T2022.tar.gz"), "rb" ) as infile: bucket.put_object(Key=s3_key, Body=infile.read(), ACL="public-read") + mocker.patch( + "learning_resources.etl.edx_shared.get_bucket_by_name", return_value=bucket + ) + mock_process = mocker.patch( + "learning_resources.etl.edx_shared.process_course_archive" + ) + sync_edx_archive(etl_source, s3_key, overwrite=False) + + mock_process.assert_called_once() + assert mock_process.call_args[0][2] == old_run + + +@pytest.mark.parametrize("etl_source", [ETLSource.mitxonline.name, ETLSource.xpro.name]) +def test_sync_edx_archive_retired_course_skipped( + mocker, mock_course_archive_bucket, etl_source +): + """sync_edx_archive still skips runs of a fully-retired (unpublished, non-test_mode) course.""" + from learning_resources.etl.edx_shared import sync_edx_archive + + platform = ( + PlatformType.mitxonline.name + if etl_source == ETLSource.mitxonline.name + else PlatformType.xpro.name + ) + course = LearningResourceFactory.create( + platform=LearningResourcePlatformFactory.create(code=platform), + etl_source=etl_source, + published=False, + test_mode=False, + create_runs=False, + ) + LearningResourceRunFactory.create( + learning_resource=course, + published=False, + run_id="course-v1:Test+Course+R1", + ) + bucket = mock_course_archive_bucket.bucket + s3_key = "20220101/courses/course-v1:Test+Course+R1/abcdefghijklmnop.tar.gz" + with Path.open( + Path("test_json/course-v1:MITxT+8.01.3x+3T2022.tar.gz"), "rb" + ) as infile: + bucket.put_object(Key=s3_key, Body=infile.read(), ACL="public-read") mocker.patch( - "learning_resources.etl.edx_shared.get_bucket_by_name", - return_value=bucket, + "learning_resources.etl.edx_shared.get_bucket_by_name", return_value=bucket + ) + # No matching run is returned for a retired course, so ETL is triggered instead. + mock_trigger = mocker.patch( + "learning_resources.etl.edx_shared.trigger_resource_etl" ) mock_process = mocker.patch( "learning_resources.etl.edx_shared.process_course_archive" ) - mock_log = mocker.patch("learning_resources.etl.edx_shared.log.warning") sync_edx_archive(etl_source, s3_key, overwrite=False) - # Should log warning and not process - assert mock_log.called - assert "not the best run" in mock_log.call_args[0][0] - assert old_run.run_id in mock_log.call_args[0][1] mock_process.assert_not_called() + mock_trigger.assert_called_once_with(etl_source) @pytest.mark.parametrize("etl_source", [ETLSource.mitxonline.name, ETLSource.xpro.name]) @@ -1063,6 +1118,22 @@ def test_build_run_lookup(source, platform): assert lookup[normalized][0].id == run.id +def test_build_run_lookup_oll_strips_mitx_prefix(): + """OLL runs are also indexed without the MITx prefix the archives omit""" + source = ETLSource.oll.name + course = LearningResourceFactory.create( + etl_source=source, published=True, create_runs=False + ) + run = LearningResourceRunFactory.create( + learning_resource=course, run_id="MITx+0.501x+2T2019", published=True + ) + lookup = build_run_lookup(source, [course.id]) + # archive filenames like 0_501x_2T2019_OLL.tar.gz normalize without the prefix + assert "0.501x.2t2019" in lookup + assert lookup["0.501x.2t2019"][0].id == run.id + assert "mitx.0.501x.2t2019" in lookup + + def test_build_run_lookup_filters_by_ids(): """build_run_lookup only includes runs for the specified course ids""" source = ETLSource.mitxonline.name diff --git a/learning_resources/etl/loaders.py b/learning_resources/etl/loaders.py index 328f5f13de..fedaf7a3d6 100644 --- a/learning_resources/etl/loaders.py +++ b/learning_resources/etl/loaders.py @@ -336,6 +336,13 @@ def load_run( run_data["published"] = True with transaction.atomic(): + previously_published = ( + LearningResourceRun.objects.filter( + learning_resource=learning_resource, run_id=run_id + ) + .values_list("published", flat=True) + .first() + ) ( learning_resource_run, _, @@ -352,42 +359,46 @@ def load_run( if hasattr(learning_resource, "best_run"): del learning_resource.best_run - if ( - ( - learning_resource_run == learning_resource.best_run - or learning_resource.test_mode - ) - and learning_resource.etl_source - in ( - ETLSource.mit_edx.value, - ETLSource.mitxonline.value, - ETLSource.xpro.value, - ) - and learning_resource_run.content_files.count() == 0 - ): - # webhook may have been sent before run was created or was best run, - # so trigger a contentfile ingestion for the course. If already - # ingested & checksums match, no new content files will be created - from learning_resources.tasks import import_content_files - - etl_source = learning_resource.etl_source - resource_id = learning_resource.id - cache_key = f"content_tasks_triggered_{etl_source}_{resource_id}" - - def enqueue_content_tasks(): - redis_cache = caches["redis"] - if not redis_cache.add( - cache_key, - True, # noqa: FBT003 - timeout=CONTENT_TASKS_CACHE_TIMEOUT, - ): - return - import_content_files.delay( - etl_source, - learning_resource_ids=[resource_id], + if previously_published and not learning_resource_run.published: + resource_run_unpublished_actions(learning_resource_run) + elif learning_resource.published or learning_resource.test_mode: + if ( + learning_resource.etl_source + in ( + ETLSource.mit_edx.value, + ETLSource.mitxonline.value, + ETLSource.xpro.value, ) + and learning_resource_run.content_files.count() == 0 + ): + # webhook may have been sent before run was created or was best + # run, so trigger a contentfile ingestion for the course. If + # already ingested & checksums match, no new files are created + from learning_resources.tasks import import_content_files + + etl_source = learning_resource.etl_source + resource_id = learning_resource.id + cache_key = f"content_tasks_triggered_{etl_source}_{resource_id}" + + def enqueue_content_tasks(): + redis_cache = caches["redis"] + if not redis_cache.add( + cache_key, + True, # noqa: FBT003 + timeout=CONTENT_TASKS_CACHE_TIMEOUT, + ): + return + import_content_files.delay( + etl_source, + learning_resource_ids=[resource_id], + ) - transaction.on_commit(enqueue_content_tasks) + transaction.on_commit(enqueue_content_tasks) + elif previously_published is False: + # Run was republished. Its content files are still present and + # published (retained sources keep them in Qdrant), just absent + # from OpenSearch. Re-index them without a full re-ingest. + content_files_loaded_actions(learning_resource_run) return learning_resource_run diff --git a/learning_resources/etl/loaders_test.py b/learning_resources/etl/loaders_test.py index cfade57ed7..c0a195dd43 100644 --- a/learning_resources/etl/loaders_test.py +++ b/learning_resources/etl/loaders_test.py @@ -444,6 +444,33 @@ def test_load_run_sets_test_resource_run_to_published(mocker): assert not result.published +@pytest.mark.django_db(transaction=True) +@pytest.mark.parametrize( + ("test_mode", "new_published", "expect_unpublish"), + [ + (False, False, True), # published->False fires the unpublish hook + (False, True, False), # stays published, no hook + (True, False, False), # test_mode forces published=True, never unpublishes + ], +) +def test_load_run_unpublishes_on_transition( + mocker, test_mode, new_published, expect_unpublish +): + """A run flipped published->False via load_run should trigger unpublish actions""" + mock_unpublish = mocker.patch( + "learning_resources.etl.loaders.resource_run_unpublished_actions", + ) + resource = LearningResourceFactory.create(is_course=True, test_mode=test_mode) + run = LearningResourceRunFactory.create(learning_resource=resource, published=True) + + loaders.load_run(resource, {"run_id": run.run_id, "published": new_published}) + + if expect_unpublish: + mock_unpublish.assert_called_once() + else: + mock_unpublish.assert_not_called() + + def test_load_program_bad_platform(mocker): """A bad platform should log an exception and not create the program""" mock_log = mocker.patch("learning_resources.etl.loaders.log.exception") diff --git a/learning_resources_search/indexing_api.py b/learning_resources_search/indexing_api.py index 48d8ed1b46..6cea4acd45 100644 --- a/learning_resources_search/indexing_api.py +++ b/learning_resources_search/indexing_api.py @@ -514,22 +514,23 @@ def deindex_content_files(content_file_ids, learning_resource_id): ) -def deindex_run_content_files(run_id, unpublished_only): +def deindex_run_content_files(run_id, unpublished_only, *, keep_published=False): """ Deindex a list of content files by run from the index. - Files are soft-deleted (published=False) and will be physically deleted - by the cleanup_deleted_content_files task after the retention period. Args: run_id(int): Course run id unpublished_only(bool): if true only deindex files with published=False - + keep_published(bool): if true, deindex all of the run's current content + files from OpenSearch without flipping ContentFile.published. Used to + remove a run from OpenSearch while keeping it in Qdrant / the REST API. """ run = LearningResourceRun.objects.get(id=run_id) if unpublished_only: content_files = run.content_files.filter(published=False).all() else: - run.content_files.filter(published=True).update(published=False) + if not keep_published: + run.content_files.filter(published=True).update(published=False) content_files = run.content_files.all() if not content_files.exists(): diff --git a/learning_resources_search/indexing_api_test.py b/learning_resources_search/indexing_api_test.py index 22036698e1..fe7f735568 100644 --- a/learning_resources_search/indexing_api_test.py +++ b/learning_resources_search/indexing_api_test.py @@ -545,6 +545,27 @@ def test_deindex_run_content_files(mocker): assert ContentFile.objects.filter(published=False).count() == 3 +@pytest.mark.django_db +def test_deindex_run_content_files_keep_published(mocker): + """keep_published removes all docs from OpenSearch without flipping published.""" + mock_deindex_items = mocker.patch( + "learning_resources_search.indexing_api.deindex_items" + ) + run = LearningResourceRunFactory.create(published=True) + content_files = ContentFileFactory.create_batch(3, run=run, published=True) + + indexing_api.deindex_run_content_files( + run.id, unpublished_only=False, keep_published=True + ) + + # All files were sent for deindexing from OpenSearch... + mock_deindex_items.assert_called_once() + # ...but none had published flipped. + for cf in content_files: + cf.refresh_from_db() + assert cf.published is True + + def test_deindex_run_content_files_unpublished_only_does_not_hard_delete(mocker): """deindex_run_content_files with unpublished_only=True should deindex but not hard-delete""" mock_deindex = mocker.patch("learning_resources_search.indexing_api.deindex_items") diff --git a/learning_resources_search/plugins.py b/learning_resources_search/plugins.py index 34e245592c..0511af2cf9 100644 --- a/learning_resources_search/plugins.py +++ b/learning_resources_search/plugins.py @@ -6,6 +6,7 @@ from django.apps import apps from django.conf import settings as django_settings +from learning_resources.etl.constants import QDRANT_RETAINED_SOURCES from learning_resources_search import tasks from learning_resources_search.api import get_similar_topics_qdrant from learning_resources_search.constants import ( @@ -162,66 +163,90 @@ def resource_before_delete(self, resource): # Ensure test mode is false so the resource is removed from the search index resource.test_mode = False self.resource_unpublished(resource) + # Deletion removes the content files, so retained sources (kept in Qdrant + # by resource_run_unpublished) must be purged here too. + if ( + django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS + and resource.resource_type == COURSE_TYPE + and resource.etl_source in QDRANT_RETAINED_SOURCES + ): + for run in resource.runs.all(): + try_with_retry_as_task( + chain(vector_tasks.remove_run_content_files.si(run.id)) + ) @hookimpl def resource_run_unpublished(self, run): """ - Remove a learning resource run's content files from the search index + Deindex an unpublished run's content files. Retained sources + (edX + Canvas) leave OpenSearch only and stay in Qdrant; + all other sources leave both indexes. Args: run(LearningResourceRun): The Learning Resource run that was removed + """ - if run.learning_resource.test_mode: + resource = run.learning_resource + if resource.test_mode: return if not run.content_files.exists(): return - deindex_tasks = [ - tasks.deindex_run_content_files.si(run.id, unpublished_only=False), - ] - if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS: - deindex_tasks.append(vector_tasks.remove_run_content_files.si(run.id)) + + if resource.etl_source in QDRANT_RETAINED_SOURCES: + deindex_tasks = [ + tasks.deindex_run_content_files.si( + run.id, unpublished_only=False, keep_published=True + ), + ] + else: + deindex_tasks = [ + tasks.deindex_run_content_files.si(run.id, unpublished_only=False), + ] + if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS: + deindex_tasks.append(vector_tasks.remove_run_content_files.si(run.id)) try_with_retry_as_task(chain(*deindex_tasks)) @hookimpl def resource_run_delete(self, run): """ - Remove a learning resource run's content files from the search index - and then delete the object + Remove a learning resource run's content files from BOTH search indexes + and then delete the object. """ - if not run.learning_resource.test_mode: - self.resource_run_unpublished(run) + deindex_tasks = [ + tasks.deindex_run_content_files.si(run.id, unpublished_only=False), + ] + if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS: + deindex_tasks.append(vector_tasks.remove_run_content_files.si(run.id)) + try_with_retry_as_task(chain(*deindex_tasks)) run.delete() @hookimpl def content_files_loaded(self, run): """ - Upsert a created/modified run's content files + Upsert a created/modified run's content files. + + Qdrant: embed every loaded run (all runs of a published/test_mode course) + and drop stale files. OpenSearch: index only the best published run, or + any published run of a test_mode course. Args: run(LearningResourceRun): The LearningResourceRun that was upserted """ if not run.content_files.exists(): return - if run.published: - index_tasks = [] - if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS: - index_tasks.append( - vector_tasks.embed_run_content_files.si(run.id), - ) - index_tasks.append( - tasks.index_run_content_files.si(run.id), - ) + + index_tasks = [] + + resource = run.learning_resource + if resource.published or resource.test_mode: + if run.published and (resource.test_mode or resource.best_run == run): + index_tasks.append(tasks.index_run_content_files.si(run.id)) + if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS: + index_tasks.append(vector_tasks.embed_run_content_files.si(run.id)) index_tasks.append( - vector_tasks.remove_unpublished_run_content_files.si(run.id), + vector_tasks.remove_unpublished_run_content_files.si(run.id) ) + + if index_tasks: try_with_retry_as_task(chain(*index_tasks)) - else: - deindex_tasks = [ - tasks.deindex_run_content_files.si(run.id, unpublished_only=False), - ] - if django_settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS: - deindex_tasks.append( - vector_tasks.remove_run_content_files.si(run.id), - ) - try_with_retry_as_task(chain(*deindex_tasks)) diff --git a/learning_resources_search/plugins_test.py b/learning_resources_search/plugins_test.py index 517614a596..40859f1acc 100644 --- a/learning_resources_search/plugins_test.py +++ b/learning_resources_search/plugins_test.py @@ -4,6 +4,7 @@ import pytest +from learning_resources.etl.constants import ETLSource from learning_resources.factories import ( ContentFileFactory, LearningResourceFactory, @@ -95,7 +96,7 @@ def test_search_index_plugin_resource_unpublished( ): """The plugin function should remove a resource from the search index""" resource = LearningResourceFactory.create( - resource_type=resource_type, test_mode=test_mode + resource_type=resource_type, test_mode=test_mode, published=False ) if resource_type == COURSE_TYPE and has_content_files: for run in resource.runs.all(): @@ -110,6 +111,7 @@ def test_search_index_plugin_resource_unpublished( if resource_type == COURSE_TYPE and has_content_files and not test_mode: assert unpublish_run_mock.call_count == resource.runs.count() for run in resource.runs.all(): + # Default "mock" source is non-retained -> removed from both indexes. unpublish_run_mock.assert_any_call(run.id, unpublished_only=False) else: unpublish_run_mock.assert_not_called() @@ -145,52 +147,171 @@ def test_search_index_plugin_resource_before_delete( ) for run in resource.runs.all(): mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_any_call( - run.id, - unpublished_only=False, + run.id, unpublished_only=False ) else: mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_not_called() +@pytest.mark.django_db +def test_resource_before_delete_retained_source_purges_qdrant( + mock_search_index_helpers, settings +): + """Deleting a retained-source course purges each run's content from Qdrant, + since resource_run_unpublished would otherwise keep retained sources there. + """ + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + resource = LearningResourceFactory.create( + resource_type=COURSE_TYPE, + etl_source=ETLSource.mitxonline.value, + create_runs=False, + ) + runs = LearningResourceRunFactory.create_batch( + 2, learning_resource=resource, published=True + ) + for run in runs: + ContentFileFactory.create(run=run) + + SearchIndexPlugin().resource_before_delete(resource) + + assert ( + mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.call_count + == len(runs) + ) + for run in runs: + mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_any_call( + run.id + ) + + +@pytest.mark.django_db +@pytest.mark.parametrize("course_published", [True, False]) +def test_resource_run_unpublished_retained_source_keeps_qdrant( + mock_search_index_helpers, settings, course_published +): + """edX/Canvas: run leaves OpenSearch (keep_published) but ALWAYS stays in + Qdrant -- even when the whole course is unpublished. + """ + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + run = LearningResourceRunFactory.create( + published=False, + learning_resource__published=course_published, + learning_resource__test_mode=False, + learning_resource__etl_source=ETLSource.mitxonline.value, + ) + ContentFileFactory.create(run=run) + + SearchIndexPlugin().resource_run_unpublished(run) + + mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with( + run.id, unpublished_only=False, keep_published=True + ) + mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_not_called() + + +@pytest.mark.django_db +def test_resource_run_unpublished_non_retained_source_removes_both( + mock_search_index_helpers, settings +): + """OCW (non-retained): run is removed from BOTH indexes (current behavior).""" + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + run = LearningResourceRunFactory.create( + published=False, + learning_resource__published=True, + learning_resource__test_mode=False, + learning_resource__etl_source=ETLSource.ocw.value, + ) + ContentFileFactory.create(run=run) + + SearchIndexPlugin().resource_run_unpublished(run) + + mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with( + run.id, unpublished_only=False + ) + mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_called_once_with( + run.id + ) + + @pytest.mark.django_db @pytest.mark.parametrize("has_content_files", [True, False]) -@pytest.mark.parametrize("test_mode", [True, False]) -def test_search_index_plugin_resource_run_unpublished( - mock_search_index_helpers, has_content_files, test_mode +def test_resource_run_unpublished_test_mode_or_empty_noops( + mock_search_index_helpers, settings, has_content_files ): - """The plugin function should remove a run's contenfiles from the search index""" - run = LearningResourceRunFactory.create(learning_resource__test_mode=test_mode) + """test_mode (any source) or a run with no content files -> no-op.""" + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + run = LearningResourceRunFactory.create( + published=False, + learning_resource__published=True, + learning_resource__test_mode=True, + learning_resource__etl_source=ETLSource.mitxonline.value, + ) if has_content_files: ContentFileFactory.create(run=run) + SearchIndexPlugin().resource_run_unpublished(run) - if has_content_files and not test_mode: - mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with( - run.id, - unpublished_only=False, + + mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_not_called() + mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_not_called() + + +@pytest.mark.django_db +def test_resource_unpublished_course_purges_runs_from_qdrant( + mock_search_index_helpers, settings +): + """Unpublishing a whole course purges each run's content files from Qdrant.""" + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + resource = LearningResourceFactory.create( + resource_type=COURSE_TYPE, published=False, test_mode=False, create_runs=False + ) + runs = LearningResourceRunFactory.create_batch( + 2, learning_resource=resource, published=False + ) + for run in runs: + ContentFileFactory.create(run=run) + + SearchIndexPlugin().resource_unpublished(resource) + + assert ( + mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.call_count + == len(runs) + ) + for run in runs: + mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_any_call( + run.id ) - else: - mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_not_called() @pytest.mark.django_db +@pytest.mark.parametrize( + "etl_source", [ETLSource.mitxonline.value, ETLSource.ocw.value] +) @pytest.mark.parametrize("has_content_files", [True, False]) @pytest.mark.parametrize("test_mode", [True, False]) def test_search_index_plugin_resource_run_delete( - mock_search_index_helpers, has_content_files, test_mode + mock_search_index_helpers, settings, etl_source, has_content_files, test_mode ): - """The plugin function should remove contenfiles from the index and delete the run""" - run = LearningResourceRunFactory.create(learning_resource__test_mode=test_mode) + """Deleting a run always purges BOTH indexes and deletes the object, + regardless of source, test_mode, or whether content files exist. + """ + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + run = LearningResourceRunFactory.create( + learning_resource__published=True, + learning_resource__test_mode=test_mode, + learning_resource__etl_source=etl_source, + ) if has_content_files: ContentFileFactory.create(run=run) run_id = run.id + SearchIndexPlugin().resource_run_delete(run) - if has_content_files and not test_mode: - mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with( - run_id, - unpublished_only=False, - ) - else: - mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_not_called() + + mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with( + run_id, unpublished_only=False + ) + mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_called_once_with( + run_id + ) assert LearningResourceRun.objects.filter(id=run_id).exists() is False @@ -199,7 +320,9 @@ def test_search_index_plugin_content_files_loaded_published_run( mock_search_index_helpers, ): """Published run should index content files and remove unpublished ones.""" - run = LearningResourceRunFactory.create(published=True) + run = LearningResourceRunFactory.create( + published=True, learning_resource__create_runs=False + ) ContentFileFactory.create(run=run) SearchIndexPlugin().content_files_loaded(run) @@ -215,7 +338,9 @@ def test_search_index_plugin_content_files_loaded_published_run_with_qdrant( ): """Published run should schedule Qdrant embed and unpublished cleanup.""" settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True - run = LearningResourceRunFactory.create(published=True) + run = LearningResourceRunFactory.create( + published=True, learning_resource__create_runs=False + ) ContentFileFactory.create(run=run) SearchIndexPlugin().content_files_loaded(run) @@ -232,23 +357,103 @@ def test_search_index_plugin_content_files_loaded_published_run_with_qdrant( @pytest.mark.django_db -def test_search_index_plugin_content_files_loaded_unpublished_run_with_qdrant( +def test_content_files_loaded_unpublished_run_embeds_qdrant_only( mock_search_index_helpers, settings ): - """Unpublished run should deindex and remove all run content files.""" + """An unpublished run is embedded into Qdrant but NOT indexed into OpenSearch.""" settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True - run = LearningResourceRunFactory.create(published=False) + run = LearningResourceRunFactory.create( + published=False, learning_resource__published=True + ) ContentFileFactory.create(run=run) SearchIndexPlugin().content_files_loaded(run) - mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_called_once_with( - run.id, - unpublished_only=False, + mock_search_index_helpers.mock_embed_run_contentfiles_immutable_signature.assert_called_once_with( + run.id ) - mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_called_once_with( + mock_search_index_helpers.mock_remove_unpublished_run_contentfiles_immutable_signature.assert_called_once_with( run.id ) + # OpenSearch indexing is skipped for a non-published run. + mock_search_index_helpers.mock_upsert_contentfiles_immutable_signature.assert_not_called() + # The old behavior (deindex / remove from Qdrant) no longer fires here. + mock_search_index_helpers.mock_remove_contentfiles_immutable_signature.assert_not_called() + mock_search_index_helpers.mock_remove_run_contentfiles_immutable_signature.assert_not_called() + + +@pytest.mark.django_db +def test_content_files_loaded_non_best_published_run_skips_opensearch( + mock_search_index_helpers, settings +): + """A published-but-not-best run embeds to Qdrant only (no OpenSearch index).""" + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + from datetime import UTC, datetime + + course = LearningResourceFactory.create(published=True, create_runs=False) + LearningResourceRunFactory.create( + learning_resource=course, + published=True, + start_date=datetime(2023, 1, 1, tzinfo=UTC), + ) # best run + non_best = LearningResourceRunFactory.create( + learning_resource=course, + published=True, + start_date=datetime(2022, 1, 1, tzinfo=UTC), + ) + ContentFileFactory.create(run=non_best) + + SearchIndexPlugin().content_files_loaded(non_best) + + mock_search_index_helpers.mock_embed_run_contentfiles_immutable_signature.assert_called_once_with( + non_best.id + ) + mock_search_index_helpers.mock_upsert_contentfiles_immutable_signature.assert_not_called() + + +@pytest.mark.django_db +def test_content_files_loaded_test_mode_published_run_indexes_opensearch( + mock_search_index_helpers, settings +): + """Any published run of a test_mode course is indexed into OpenSearch.""" + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = True + course = LearningResourceFactory.create( + published=False, test_mode=True, create_runs=False + ) + run = LearningResourceRunFactory.create(learning_resource=course, published=True) + ContentFileFactory.create(run=run) + + SearchIndexPlugin().content_files_loaded(run) + + mock_search_index_helpers.mock_upsert_contentfiles_immutable_signature.assert_called_once_with( + run.id + ) + mock_search_index_helpers.mock_embed_run_contentfiles_immutable_signature.assert_called_once_with( + run.id + ) + + +@pytest.mark.django_db +@pytest.mark.parametrize("qdrant_enabled", [True, False]) +def test_content_files_loaded_retired_course_does_nothing( + mock_search_index_helpers, settings, qdrant_enabled +): + """A fully-retired course (unpublished, non-test_mode) is neither embedded + into Qdrant nor indexed into OpenSearch. + """ + settings.QDRANT_ENABLE_INDEXING_PLUGIN_HOOKS = qdrant_enabled + run = LearningResourceRunFactory.create( + published=False, + learning_resource__published=False, + learning_resource__test_mode=False, + ) + ContentFileFactory.create(run=run) + + SearchIndexPlugin().content_files_loaded(run) + + mock_search_index_helpers.mock_embed_run_contentfiles_immutable_signature.assert_not_called() + mock_search_index_helpers.mock_remove_unpublished_run_contentfiles_immutable_signature.assert_not_called() + mock_search_index_helpers.mock_upsert_contentfiles_immutable_signature.assert_not_called() @pytest.mark.django_db diff --git a/learning_resources_search/tasks.py b/learning_resources_search/tasks.py index 105ee7aed8..0b3f830d1d 100644 --- a/learning_resources_search/tasks.py +++ b/learning_resources_search/tasks.py @@ -529,18 +529,23 @@ def index_run_content_files(run_id, index_types=IndexestoUpdate.all_indexes.valu retry_backoff=True, rate_limit=settings.CELERY_SEARCH_RATE_LIMIT, ) -def deindex_run_content_files(run_id, unpublished_only): +def deindex_run_content_files(run_id, unpublished_only, keep_published=False): # noqa: FBT002 """ Deindex content files for a LearningResourceRun Args: run_id(int): LearningResourceRun id unpublished_only(bool): Whether to only deindex unpublished content files - + keep_published(bool): Whether to remove from OpenSearch without flipping + ContentFile.published """ try: with wrap_retry_exception(*SEARCH_CONN_EXCEPTIONS): - api.deindex_run_content_files(run_id, unpublished_only=unpublished_only) + api.deindex_run_content_files( + run_id, + unpublished_only=unpublished_only, + keep_published=keep_published, + ) except (RetryError, Ignore): raise except: # noqa: E722 diff --git a/learning_resources_search/tasks_test.py b/learning_resources_search/tasks_test.py index ea0278f52b..23980ccc73 100644 --- a/learning_resources_search/tasks_test.py +++ b/learning_resources_search/tasks_test.py @@ -733,7 +733,7 @@ def test_delete_run_content_files(mocker, with_error, unpublished_only): deindex_run_content_files_mock.side_effect = TabError result = deindex_run_content_files.delay(1, unpublished_only=unpublished_only).get() deindex_run_content_files_mock.assert_called_once_with( - 1, unpublished_only=unpublished_only + 1, unpublished_only=unpublished_only, keep_published=False ) assert result == ( diff --git a/vector_search/tasks.py b/vector_search/tasks.py index 88d3fe81ba..b4457941d0 100644 --- a/vector_search/tasks.py +++ b/vector_search/tasks.py @@ -194,21 +194,13 @@ def start_embed_resources(self, indexes, skip_content_files, overwrite): # noqa .exclude(readable_id=blocklisted_ids) .order_by("id") ): - run = ( - course.best_run - if course.best_run - else course.runs.filter(published=True) - .order_by("-start_date") - .first() - ) + # Embed published content files across all runs of the course + # (Qdrant retains all runs, not just best_run). contentfiles = ( - ContentFile.objects.filter( - Q(run=run, published=True, run__published=True) - | Q( - learning_resource=course, - published=True, - learning_resource__published=True, - ) + ContentFile.objects.filter(published=True) + .filter( + Q(run__learning_resource=course) + | Q(learning_resource=course) ) .order_by("id") .values_list("id", flat=True) @@ -306,21 +298,13 @@ def embed_learning_resources_by_id(self, ids, skip_content_files, overwrite): ) elif not skip_content_files and resource_type == COURSE_TYPE: for course in embed_resources.order_by("id"): - run = ( - course.best_run - if course.best_run - else course.runs.filter(published=True) - .order_by("-start_date") - .first() - ) + # Embed published content files across all runs of the course + # (Qdrant retains all runs, not just best_run). content_ids = ( - ContentFile.objects.filter( - Q(run=run, published=True, run__published=True) - | Q( - learning_resource=course, - published=True, - learning_resource__published=True, - ) + ContentFile.objects.filter(published=True) + .filter( + Q(run__learning_resource=course) + | Q(learning_resource=course) ) .order_by("id") .values_list("id", flat=True) @@ -470,35 +454,34 @@ def embeddings_healthcheck(): ) for lr in all_resources: - run = ( - lr.best_run - if lr.best_run - else lr.runs.filter(published=True).order_by("-start_date").first() - ) serialized = LearningResourceSerializer(lr).data point_id = vector_point_id(vector_point_key(serialized)) resource_point_ids[point_id] = {"resource_id": lr.readable_id, "id": lr.id} content_file_point_ids = {} - if run: - for cf in run.content_files.for_serialization().filter(published=True): - if cf and cf.content: - serialized_cf = ContentFileSerializer(cf).data - point_id = vector_point_id( - vector_point_key( - serialized_cf, chunk_number=0, document_type="content_file" - ) + # All runs are embedded in Qdrant, not just best_run. + content_files = ContentFile.objects.for_serialization().filter( + Q(run__learning_resource=lr) | Q(learning_resource=lr), + published=True, + ) + for cf in content_files: + if cf and cf.content: + serialized_cf = ContentFileSerializer(cf).data + point_id = vector_point_id( + vector_point_key( + serialized_cf, chunk_number=0, document_type="content_file" ) - content_file_point_ids[point_id] = {"key": cf.key, "id": cf.id} - for batch in chunks(content_file_point_ids.keys(), chunk_size=200): - remaining_content_files = filter_existing_qdrant_points_by_ids( - batch, collection_name=CONTENT_FILES_COLLECTION_NAME - ) - remaining_content_file_ids.extend( - [ - content_file_point_ids.get(p, {}).get("id") - for p in remaining_content_files - ] ) + content_file_point_ids[point_id] = {"key": cf.key, "id": cf.id} + for batch in chunks(content_file_point_ids.keys(), chunk_size=200): + remaining_content_files = filter_existing_qdrant_points_by_ids( + batch, collection_name=CONTENT_FILES_COLLECTION_NAME + ) + remaining_content_file_ids.extend( + [ + content_file_point_ids.get(p, {}).get("id") + for p in remaining_content_files + ] + ) for batch in chunks( all_resources.values_list("id", flat=True), diff --git a/vector_search/tasks_test.py b/vector_search/tasks_test.py index 25d9c520cc..42b4ac8f14 100644 --- a/vector_search/tasks_test.py +++ b/vector_search/tasks_test.py @@ -334,32 +334,38 @@ def test_embed_learning_resources_by_id(mocker, mocked_celery): assert sorted(resource_ids) == sorted(embedded_resource_ids) -def test_embedded_content_from_best_run(mocker, mocked_celery): +def _embedded_content_file_ids(generate_embeddings_mock): + """Collect all content file ids passed to generate_embeddings across chunks""" + return { + cid + for call in generate_embeddings_mock.si.call_args_list + if call.args[1] == "content_file" + for cid in call.args[0] + } + + +def test_embedded_content_from_all_runs(mocker, mocked_celery): """ - Content files to embed should come from best course run + Content files from every run of a course should be embedded, not just best_run """ mocker.patch("vector_search.tasks.load_course_blocklist", return_value=[]) course = CourseFactory.create(etl_source=ETLSource.ocw.value) course.runs.all().delete() - other_run = LearningResourceRunFactory.create( + older_run = LearningResourceRunFactory.create( learning_resource=course.learning_resource, start_date=datetime.datetime.now(tz=datetime.UTC) - datetime.timedelta(days=2), ) - LearningResourceRunFactory.create( + newer_run = LearningResourceRunFactory.create( learning_resource=course.learning_resource, start_date=datetime.datetime.now(tz=datetime.UTC) + datetime.timedelta(days=2), ) - - best_run_contentfiles = [ + all_contentfiles = { cf.id - for cf in ContentFileFactory.create_batch( - 3, run=course.learning_resource.best_run - ) - ] - # create contentfiles using the other run - ContentFileFactory.create_batch(3, run=other_run) + for run in (older_run, newer_run) + for cf in ContentFileFactory.create_batch(3, run=run) + } generate_embeddings_mock = mocker.patch( "vector_search.tasks.generate_embeddings", autospec=True @@ -370,43 +376,46 @@ def test_embedded_content_from_best_run(mocker, mocked_celery): ["course"], skip_content_files=False, overwrite=True ) - generate_embeddings_mock.si.assert_called_with( - best_run_contentfiles, - "content_file", - True, # noqa: FBT003 - ) + assert all_contentfiles <= _embedded_content_file_ids(generate_embeddings_mock) -def test_embedded_content_from_latest_run_if_next_missing(mocker, mocked_celery): +def test_embed_by_id_all_runs_excludes_unpublished(mocker, mocked_celery): """ - Content files to embed should come from latest run if the next run is missing + embed_learning_resources_by_id embeds published content files from all runs and + excludes unpublished ones """ mocker.patch("vector_search.tasks.load_course_blocklist", return_value=[]) course = CourseFactory.create(etl_source=ETLSource.ocw.value) course.runs.all().delete() - latest_run = LearningResourceRunFactory.create( - learning_resource=course.learning_resource, - start_date=datetime.datetime.now(tz=datetime.UTC) - datetime.timedelta(hours=1), + run_a = LearningResourceRunFactory.create( + learning_resource=course.learning_resource ) - latest_run_contentfiles = [ - cf.id for cf in ContentFileFactory.create_batch(3, run=latest_run) - ] + run_b = LearningResourceRunFactory.create( + learning_resource=course.learning_resource + ) + published_ids = { + cf.id + for run in (run_a, run_b) + for cf in ContentFileFactory.create_batch(2, run=run, published=True) + } + unpublished_ids = { + cf.id for cf in ContentFileFactory.create_batch(2, run=run_a, published=False) + } + generate_embeddings_mock = mocker.patch( "vector_search.tasks.generate_embeddings", autospec=True ) with pytest.raises(mocked_celery.replace_exception_class): - start_embed_resources.delay( - ["course"], skip_content_files=False, overwrite=True + embed_learning_resources_by_id.delay( + [course.learning_resource.id], skip_content_files=False, overwrite=True ) - generate_embeddings_mock.si.assert_called_with( - latest_run_contentfiles, - "content_file", - True, # noqa: FBT003 - ) + embedded = _embedded_content_file_ids(generate_embeddings_mock) + assert published_ids <= embedded + assert not (unpublished_ids & embedded) def test_embedded_content_file_without_runs(mocker, mocked_celery): @@ -697,6 +706,36 @@ def test_embeddings_healthcheck_missing_both(mocker): assert mock_sentry.call_count == 2 +def test_embeddings_healthcheck_checks_all_runs(mocker): + """ + embeddings_healthcheck should check content files from every run, not just best_run + """ + from vector_search.constants import CONTENT_FILES_COLLECTION_NAME + + lr = LearningResourceFactory.create(published=True, create_runs=False) + run_a = LearningResourceRunFactory.create(published=True, learning_resource=lr) + run_b = LearningResourceRunFactory.create(published=True, learning_resource=lr) + ContentFileFactory.create(run=run_a, content="test", published=True) + ContentFileFactory.create(run=run_b, content="test", published=True) + + def fake_filter(batch, collection_name=None): + # report every content file point as missing, no missing resources + return list(batch) if collection_name == CONTENT_FILES_COLLECTION_NAME else [] + + mocker.patch( + "vector_search.tasks.filter_existing_qdrant_points_by_ids", + side_effect=fake_filter, + ) + mock_sentry = mocker.patch("vector_search.tasks.sentry_sdk.capture_message") + + embeddings_healthcheck() + + assert ( + mock_sentry.mock_calls[0].args[0] + == "Warning: 2 missing content file embeddings detected" + ) + + def test_embeddings_healthcheck_missing_summaries(mocker): """ Test embeddings_healthcheck for missing contentfile summaries/flashcards diff --git a/vector_search/utils.py b/vector_search/utils.py index 489ab3eacb..7f147ea93a 100644 --- a/vector_search/utils.py +++ b/vector_search/utils.py @@ -5,7 +5,7 @@ from functools import cache from django.conf import settings -from django.db.models import Q +from django.db.models import Prefetch, Q from langchain_text_splitters import ( MarkdownHeaderTextSplitter, RecursiveCharacterTextSplitter, @@ -17,6 +17,7 @@ from learning_resources.models import ( ContentFile, LearningResource, + LearningResourceRun, LearningResourceTopic, ) from learning_resources.serializers import ( @@ -1125,6 +1126,43 @@ async def _get_facet(agg_key: str): return dict(results) +def best_run_ids_for_resources(readable_ids): + """ + Resolve the run_id values a resource_readable_id content-file query should + be restricted to. + + Non-test_mode course -> its best run only. + test_mode course -> all its published runs (matches OpenSearch indexing). + Course with no published run -> contributes nothing. + + Args: + readable_ids (list[str]): resource readable_id values from the request + + Returns: + list[str]: LearningResourceRun.run_id values to filter on + """ + # Prefetch published runs into _published_runs so both best_run and the + # test_mode branch resolve without a per-resource query (avoids an N+1). + resources = LearningResource.objects.filter( + readable_id__in=readable_ids + ).prefetch_related( + Prefetch( + "runs", + queryset=LearningResourceRun.objects.filter(published=True).order_by( + "start_date", "enrollment_start", "id" + ), + to_attr="_published_runs", + ) + ) + run_ids = [] + for resource in resources: + if resource.test_mode: + run_ids.extend(run.run_id for run in resource.published_runs) + elif resource.best_run: + run_ids.append(resource.best_run.run_id) + return run_ids + + def qdrant_query_conditions(params, collection_name=RESOURCES_COLLECTION_NAME): """ Return a list of Qdrant FieldCondition objects based on params diff --git a/vector_search/utils_test.py b/vector_search/utils_test.py index 13050f7dcd..0eae55645e 100644 --- a/vector_search/utils_test.py +++ b/vector_search/utils_test.py @@ -1956,3 +1956,113 @@ def test_custom_score_formula_defaults(mocker): assert isinstance(results[0].mult[1], models.Filter) assert isinstance(results[0].mult[2], models.GaussDecayExpression) + + +@pytest.mark.django_db +def test_best_run_ids_for_resources_non_test_mode(): + """A normal course resolves to only its best run's run_id.""" + from datetime import timedelta + + from django.utils import timezone + + from vector_search.utils import best_run_ids_for_resources + + course = LearningResourceFactory.create(is_course=True, test_mode=False) + course.runs.all().delete() + LearningResourceRunFactory.create( + learning_resource=course, + run_id="OLD_RUN", + published=True, + start_date=timezone.now() - timedelta(days=60), + enrollment_start=None, + enrollment_end=None, + end_date=None, + ) + best = LearningResourceRunFactory.create( + learning_resource=course, + run_id="NEW_RUN", + published=True, + start_date=timezone.now() - timedelta(days=10), + enrollment_start=None, + enrollment_end=None, + end_date=None, + ) + # best_run falls back to the latest start_date among published runs + assert course.best_run.run_id == best.run_id + + run_ids = best_run_ids_for_resources([course.readable_id]) + + assert run_ids == [best.run_id] + + +@pytest.mark.django_db +def test_best_run_ids_for_resources_test_mode_returns_all_published(): + """A test_mode course resolves to every published run_id.""" + from vector_search.utils import best_run_ids_for_resources + + course = LearningResourceFactory.create(is_course=True, test_mode=True) + course.runs.all().delete() + run_a = LearningResourceRunFactory.create( + learning_resource=course, run_id="RUN_A", published=True + ) + run_b = LearningResourceRunFactory.create( + learning_resource=course, run_id="RUN_B", published=True + ) + LearningResourceRunFactory.create( + learning_resource=course, run_id="RUN_UNPUB", published=False + ) + + run_ids = best_run_ids_for_resources([course.readable_id]) + + assert set(run_ids) == {run_a.run_id, run_b.run_id} + + +@pytest.mark.django_db +def test_best_run_ids_for_resources_union_across_resources(): + """Multiple resources yield the union of their resolved run_ids.""" + from datetime import timedelta + + from django.utils import timezone + + from vector_search.utils import best_run_ids_for_resources + + course1 = LearningResourceFactory.create(is_course=True, test_mode=False) + course1.runs.all().delete() + best1 = LearningResourceRunFactory.create( + learning_resource=course1, + run_id="C1_BEST", + published=True, + start_date=timezone.now() - timedelta(days=10), + enrollment_start=None, + enrollment_end=None, + end_date=None, + ) + course2 = LearningResourceFactory.create(is_course=True, test_mode=False) + course2.runs.all().delete() + best2 = LearningResourceRunFactory.create( + learning_resource=course2, + run_id="C2_BEST", + published=True, + start_date=timezone.now() - timedelta(days=10), + enrollment_start=None, + enrollment_end=None, + end_date=None, + ) + + run_ids = best_run_ids_for_resources([course1.readable_id, course2.readable_id]) + + assert set(run_ids) == {best1.run_id, best2.run_id} + + +@pytest.mark.django_db +def test_best_run_ids_for_resources_no_published_run(): + """A course with no published run contributes nothing (no error).""" + from vector_search.utils import best_run_ids_for_resources + + course = LearningResourceFactory.create(is_course=True, test_mode=False) + course.runs.all().delete() + LearningResourceRunFactory.create( + learning_resource=course, run_id="UNPUB", published=False + ) + + assert best_run_ids_for_resources([course.readable_id]) == [] diff --git a/vector_search/views.py b/vector_search/views.py index 0be47ae325..84c12fa385 100644 --- a/vector_search/views.py +++ b/vector_search/views.py @@ -38,6 +38,7 @@ _resource_vector_hits, async_qdrant_aggregations, async_qdrant_client, + best_run_ids_for_resources, custom_score_formula, dense_encoder, qdrant_query_conditions, @@ -689,11 +690,26 @@ async def get(self, request): f"{settings.QDRANT_BASE_COLLECTION_NAME}.{collection_name_override}" ) + params = dict(request_data.data) + resource_ids = params.get("resource_readable_id") + has_run_filter = "run_readable_id" in params or "edx_module_id" in params + if resource_ids and not has_run_filter: + # Restrict resource-scoped queries to each resource's best run. + # Replace resource_readable_id with a single run_readable_id filter + # (don't AND them: compound filters break Qdrant's approximate + # count). The resource readable_ids are included to match each + # resource's run-less course-metadata point. + best_run_ids = await sync_to_async(best_run_ids_for_resources)( + resource_ids + ) + del params["resource_readable_id"] + params["run_readable_id"] = best_run_ids + list(resource_ids) + response = await self.async_vector_search( query_text, limit=limit, offset=offset, - params=request_data.data, + params=params, search_collection=collection_name, hybrid_search=hybrid_search, ) diff --git a/vector_search/views_test.py b/vector_search/views_test.py index c40b4567f2..e72f4bd235 100644 --- a/vector_search/views_test.py +++ b/vector_search/views_test.py @@ -1,13 +1,19 @@ import asyncio +from datetime import timedelta import pytest from django.contrib.auth.models import Group from django.urls import reverse +from django.utils import timezone from qdrant_client import models from qdrant_client.http.models.models import CountResult from rest_framework.exceptions import NotAuthenticated, PermissionDenied from learning_resources.constants import GROUP_CONTENT_FILE_CONTENT_VIEWERS +from learning_resources.factories import ( + LearningResourceFactory, + LearningResourceRunFactory, +) from vector_search.encoders.utils import dense_encoder, sparse_encoder from vector_search.views import QdrantView @@ -833,3 +839,200 @@ def test_build_search_params_sort_with_cutoff_score( assert search_params["query"].order_by.direction == models.Direction.DESC else: assert search_params["query"].order_by.direction == models.Direction.ASC + + +@pytest.mark.django_db +def test_content_file_search_restricts_resource_query_to_best_run( + mocker, client, django_user_model +): + """resource_readable_id with no run filter is pinned to the best run.""" + course = LearningResourceFactory.create(is_course=True, test_mode=False) + course.runs.all().delete() + # end_date=None is load-bearing: prevents both runs from looking currently-enrollable, + # which would cause best_run to pick earliest start_date (OLD_RUN) instead of latest. + LearningResourceRunFactory.create( + learning_resource=course, + run_id="OLD_RUN", + published=True, + start_date=timezone.now() - timedelta(days=60), + enrollment_start=None, + enrollment_end=None, + end_date=None, + ) + best = LearningResourceRunFactory.create( + learning_resource=course, + run_id="NEW_RUN", + published=True, + start_date=timezone.now() - timedelta(days=10), + enrollment_start=None, + enrollment_end=None, + end_date=None, + ) + + mock_qdrant = mocker.patch( + "qdrant_client.AsyncQdrantClient", return_value=mocker.AsyncMock() + )() + mock_qdrant.scroll = mocker.AsyncMock(return_value=([], None)) + mock_qdrant.query_points = mocker.AsyncMock() + mock_qdrant.query_points_groups = mocker.AsyncMock() + mock_qdrant.count = mocker.AsyncMock(return_value=CountResult(count=10)) + mocker.patch("vector_search.views.async_qdrant_client", return_value=mock_qdrant) + + admin = django_user_model.objects.create_superuser( + "admin", "admin@example.com", "pass" + ) + client.force_login(admin) + + client.get( + reverse("vector_search:v0:vector_content_files_search"), + data={"q": "topics", "resource_readable_id": [course.readable_id]}, + ) + + must = mock_qdrant.query_points.mock_calls[0].kwargs["query_filter"].must + # Allowed runs are the best run PLUS the resource readable_id itself (which + # matches the run-less course-metadata point). + run_conditions = [c for c in must if getattr(c, "key", None) == "run_readable_id"] + assert len(run_conditions) == 1 + assert set(run_conditions[0].match.any) == {best.run_id, course.readable_id} + # resource_readable_id is replaced by the run filter, not AND-ed with it + # (a single-field filter keeps Qdrant's approximate count accurate). + assert not any(getattr(c, "key", None) == "resource_readable_id" for c in must) + + +@pytest.mark.django_db +def test_content_file_search_test_mode_not_restricted( + mocker, client, django_user_model +): + """A test_mode course allows all its published runs.""" + course = LearningResourceFactory.create(is_course=True, test_mode=True) + course.runs.all().delete() + run_a = LearningResourceRunFactory.create( + learning_resource=course, run_id="RUN_A", published=True + ) + run_b = LearningResourceRunFactory.create( + learning_resource=course, run_id="RUN_B", published=True + ) + + mock_qdrant = mocker.patch( + "qdrant_client.AsyncQdrantClient", return_value=mocker.AsyncMock() + )() + mock_qdrant.scroll = mocker.AsyncMock(return_value=([], None)) + mock_qdrant.query_points = mocker.AsyncMock() + mock_qdrant.query_points_groups = mocker.AsyncMock() + mock_qdrant.count = mocker.AsyncMock(return_value=CountResult(count=10)) + mocker.patch("vector_search.views.async_qdrant_client", return_value=mock_qdrant) + + admin = django_user_model.objects.create_superuser( + "admin", "admin@example.com", "pass" + ) + client.force_login(admin) + + client.get( + reverse("vector_search:v0:vector_content_files_search"), + data={"q": "topics", "resource_readable_id": [course.readable_id]}, + ) + + must = mock_qdrant.query_points.mock_calls[0].kwargs["query_filter"].must + run_conditions = [c for c in must if getattr(c, "key", None) == "run_readable_id"] + assert len(run_conditions) == 1 + # All published runs plus the resource readable_id (for the metadata point). + assert set(run_conditions[0].match.any) == { + run_a.run_id, + run_b.run_id, + course.readable_id, + } + + +@pytest.mark.django_db +def test_content_file_search_explicit_run_not_overridden( + mocker, client, django_user_model +): + """An explicit run_readable_id filter is left untouched.""" + course = LearningResourceFactory.create(is_course=True, test_mode=False) + course.runs.all().delete() + LearningResourceRunFactory.create( + learning_resource=course, run_id="BEST_RUN", published=True + ) + + mock_qdrant = mocker.patch( + "qdrant_client.AsyncQdrantClient", return_value=mocker.AsyncMock() + )() + mock_qdrant.scroll = mocker.AsyncMock(return_value=([], None)) + mock_qdrant.query_points = mocker.AsyncMock() + mock_qdrant.query_points_groups = mocker.AsyncMock() + mock_qdrant.count = mocker.AsyncMock(return_value=CountResult(count=10)) + mocker.patch("vector_search.views.async_qdrant_client", return_value=mock_qdrant) + + admin = django_user_model.objects.create_superuser( + "admin", "admin@example.com", "pass" + ) + client.force_login(admin) + + client.get( + reverse("vector_search:v0:vector_content_files_search"), + data={ + "q": "topics", + "resource_readable_id": [course.readable_id], + "run_readable_id": ["EXPLICIT_RUN"], + }, + ) + + must = mock_qdrant.query_points.mock_calls[0].kwargs["query_filter"].must + assert ( + models.FieldCondition( + key="run_readable_id", match=models.MatchAny(any=["EXPLICIT_RUN"]) + ) + in must + ) + assert not any( + getattr(c, "key", None) == "run_readable_id" and c.match.any == ["BEST_RUN"] + for c in must + ) + + +@pytest.mark.django_db +def test_content_file_search_no_best_run_metadata_only( + mocker, client, django_user_model +): + """A resource with no published run resolves to the resource metadata point only. + + best_run_ids_for_resources returns [], so the allowed run set is just the + resource readable_id (which matches the course-metadata point). No real run's + content files leak in. + """ + course = LearningResourceFactory.create(is_course=True, test_mode=False) + course.runs.all().delete() + # Only an unpublished run, so best_run is None and best_run_ids_for_resources -> []. + LearningResourceRunFactory.create( + learning_resource=course, + run_id="UNPUB_RUN", + published=False, + start_date=timezone.now() - timedelta(days=10), + enrollment_start=None, + enrollment_end=None, + end_date=None, + ) + + mock_qdrant = mocker.patch( + "qdrant_client.AsyncQdrantClient", return_value=mocker.AsyncMock() + )() + mock_qdrant.scroll = mocker.AsyncMock(return_value=([], None)) + mock_qdrant.query_points = mocker.AsyncMock() + mock_qdrant.query_points_groups = mocker.AsyncMock() + mock_qdrant.count = mocker.AsyncMock(return_value=CountResult(count=10)) + mocker.patch("vector_search.views.async_qdrant_client", return_value=mock_qdrant) + + admin = django_user_model.objects.create_superuser( + "admin", "admin@example.com", "pass" + ) + client.force_login(admin) + + client.get( + reverse("vector_search:v0:vector_content_files_search"), + data={"q": "topics", "resource_readable_id": [course.readable_id]}, + ) + + must = mock_qdrant.query_points.mock_calls[0].kwargs["query_filter"].must + run_conditions = [c for c in must if getattr(c, "key", None) == "run_readable_id"] + assert len(run_conditions) == 1 + assert set(run_conditions[0].match.any) == {course.readable_id}