From df55a90d2e00563039455a16f66604e6d8164f3b Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 13 Nov 2026 15:34:35 -0700 Subject: [PATCH 1/3] refactor: route contentstore() requests through a RoutingContentStore As a first step towards storing course files in openedx_content (see openedx-core ADR 0016, "Replacing Contentstore"), contentstore() now returns a RoutingContentStore which delegates each request to the backend that stores the relevant course's files. For now, the legacy MongoDB/GridFS store is the only backend, so there is no change in behavior. The ContentStore base class now documents the full interface that the rest of the platform uses, and the generic get_attr()/set_attr() helpers move into it from MongoContentStore. Co-Authored-By: Claude Opus 5.5 --- .../commands/tests/test_cleanup_assets.py | 7 +- mypy.ini | 3 + xmodule/contentstore/content.py | 165 +++++++++++-- xmodule/contentstore/django.py | 37 ++- xmodule/contentstore/mongo.py | 26 +-- xmodule/contentstore/routing.py | 164 +++++++++++++ xmodule/tests/test_contentstore_routing.py | 216 ++++++++++++++++++ 7 files changed, 565 insertions(+), 53 deletions(-) create mode 100644 xmodule/contentstore/routing.py create mode 100644 xmodule/tests/test_contentstore_routing.py diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_cleanup_assets.py b/cms/djangoapps/contentstore/management/commands/tests/test_cleanup_assets.py index d5850c065f71..b501a5d5f8d1 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_cleanup_assets.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_cleanup_assets.py @@ -67,11 +67,12 @@ def test_export_all_courses(self): course_filter = course.id.make_asset_key("asset", None) query = location_to_query(course_filter, wildcard=True, tag=XASSET_LOCATION_TAG) query['_id.name'] = all_assets[0]['_id']['name'] - asset_doc = self.content_store.fs_files.find_one(query) + fs_files = self.content_store.legacy_store.fs_files + asset_doc = fs_files.find_one(query) asset_doc['_id']['name'] = '._example_test.txt' - self.content_store.fs_files.insert_one(asset_doc) + fs_files.insert_one(asset_doc) asset_doc['_id']['name'] = '.DS_Store' - self.content_store.fs_files.insert_one(asset_doc) + fs_files.insert_one(asset_doc) # check that now course has four assets all_assets, count = self.content_store.get_all_content_for_course(course.id) diff --git a/mypy.ini b/mypy.ini index 3318d9c96d93..ea4be77527cd 100644 --- a/mypy.ini +++ b/mypy.ini @@ -32,6 +32,9 @@ files = openedx/core/lib/derived.py, openedx/core/types, openedx/core/djangoapps/content_tagging, + xmodule/contentstore/content.py, + xmodule/contentstore/django.py, + xmodule/contentstore/routing.py, xmodule/util/keys.py, xmodule/item_bank_block.py diff --git a/xmodule/contentstore/content.py b/xmodule/contentstore/content.py index 08a6e0abc034..21c8cc7c072a 100644 --- a/xmodule/contentstore/content.py +++ b/xmodule/contentstore/content.py @@ -4,7 +4,9 @@ import os import re import uuid +from collections.abc import Sequence from io import BytesIO +from typing import Any, Literal, overload from urllib.parse import parse_qsl, quote_plus, urlencode, urlparse, urlunparse from opaque_keys import InvalidKeyError @@ -349,17 +351,109 @@ def copy_to_in_mem(self): # pylint: disable=missing-function-docstring class ContentStore: - ''' - Abstraction for all ContentStore providers (e.g. MongoDB) - ''' - def save(self, content): + """ + Abstraction for all ContentStore providers. + + This is the interface that the rest of the platform uses (via + ``xmodule.contentstore.django.contentstore()``) to store and retrieve + course "Files" (assets). + + Implemented by ``RoutingContentStore``, which in turn sends requests to + either ``MongoContentStore`` (legacy) or the newer shim that wraps + ``openedx_content``. + """ + + # Methods that act on a single asset + + def save(self, content: StaticContent) -> StaticContent: + """ + Save the given ``StaticContent``, replacing any existing asset with the same location. + + Returns the content. + """ raise NotImplementedError - def find(self, filename): + @overload + def find( + self, location: AssetKey, throw_on_not_found: Literal[True] = True, as_stream: Literal[False] = False + ) -> StaticContent: ... + + @overload + def find( + self, location: AssetKey, throw_on_not_found: Literal[True] = True, *, as_stream: Literal[True] + ) -> StaticContentStream: ... + + @overload + def find( + self, location: AssetKey, throw_on_not_found: bool = True, as_stream: bool = False + ) -> StaticContent | None: ... + + def find( + self, location: AssetKey, throw_on_not_found: bool = True, as_stream: bool = False + ) -> StaticContent | None: + """ + Get the asset with the given ``AssetKey``. + + Returns a ``StaticContent``, or a ``StaticContentStream`` if ``as_stream`` is True. If the asset doesn't exist, + raises ``NotFoundError``, or returns ``None`` if ``throw_on_not_found`` is False. + """ raise NotImplementedError - def get_all_content_for_course(self, course_key, start=0, maxresults=-1, sort=None, filter_params=None): - ''' + def delete(self, location: AssetKey) -> None: + """ + Delete the asset with the given ``AssetKey``. Deleting an asset that doesn't exist is not an error. + """ + raise NotImplementedError + + def get_attrs(self, location: AssetKey) -> dict[str, Any]: + """ + Get a dict of all the attributes of the asset with the given ``AssetKey``, including built-in attributes such + as ``length`` and ``uploadDate``. + + Raises ``NotFoundError`` if the asset doesn't exist. + """ + raise NotImplementedError + + def set_attrs(self, location: AssetKey, attr_dict: dict[str, Any]) -> None: + """ + Set several attributes (e.g. ``locked``) of the asset with the given ``AssetKey``. + + Raises ``NotFoundError`` if the asset doesn't exist, or ``AttributeError`` if any of the attributes are + built-in attributes which cannot be changed. + """ + raise NotImplementedError + + def get_attr(self, location: AssetKey, attr: str, default: Any = None) -> Any: + """ + Get the value of one attribute of the asset with the given ``AssetKey``, or ``default`` if it's not set. + + Raises ``NotFoundError`` if the asset doesn't exist. + """ + return self.get_attrs(location).get(attr, default) + + def set_attr(self, location: AssetKey, attr: str, value: Any = True) -> None: + """ + Set one attribute of the asset with the given ``AssetKey``. See ``set_attrs()``. + """ + self.set_attrs(location, {attr: value}) + + def export(self, location: AssetKey, output_directory: str) -> None: + """ + Write the asset with the given ``AssetKey`` to a file in ``output_directory``. + """ + raise NotImplementedError + + # Methods that act on all the assets of a course + + def get_all_content_for_course( + self, + course_key: CourseKey, + start: int = 0, + maxresults: int = -1, + sort: Sequence[tuple[str, int]] | None = None, + filter_params: dict[str, Any] | None = None, + ) -> tuple[list[dict[str, Any]], int]: + """ Returns a list of static assets for a course, followed by the total number of assets. By default all assets are returned, but start and maxresults can be provided to limit the query. @@ -369,27 +463,67 @@ def get_all_content_for_course(self, course_key, start=0, maxresults=-1, sort=No displayname: The human-readable name of the asset uploadDate (datetime.datetime): The date and time that the file was uploadDate contentType: The mimetype string of the asset - md5: An md5 hash of the asset content - ''' + length: The size of the asset, in bytes + locked: Whether the asset is restricted to enrolled users + thumbnail_location: The location of the asset's thumbnail, if any + import_path: The path that the asset was imported from, if any + """ + raise NotImplementedError + + def get_all_content_thumbnails_for_course(self, course_key: CourseKey) -> list[dict[str, Any]]: + """ + Returns a list of all the thumbnails for a course, in the same format as ``get_all_content_for_course()``. + """ + raise NotImplementedError + + def export_all_for_course(self, course_key: CourseKey, output_directory: str, assets_policy_file: str) -> None: + """ + Export all of the course's assets to ``output_directory``, and their attributes to ``assets_policy_file``. + """ raise NotImplementedError - def delete_all_course_assets(self, course_key): + def delete_all_course_assets(self, course_key: CourseKey) -> None: """ Delete all of the assets which use this course_key as an identifier :param course_key: """ raise NotImplementedError - def copy_all_course_assets(self, source_course_key, dest_course_key): + def copy_all_course_assets(self, source_course_key: CourseKey, dest_course_key: CourseKey) -> None: """ Copy all the course assets from source_course_key to dest_course_key """ raise NotImplementedError - def generate_thumbnail(self, content, tempfile_path=None, dimensions=None): + # Methods that act on all courses, or on the store itself + + def remove_redundant_content_for_courses(self) -> int: + """ + Delete redundant files (Mac OS metadata files like ".DS_Store") from all courses. + + Returns the number of assets deleted. + """ + raise NotImplementedError + + def close_connections(self) -> None: + """ + Close any database connections held by this store. + """ + + def _drop_database(self, database: bool = True, collections: bool = True, connections: bool = True) -> None: + """ + Delete all data in this store. Only for use in tests. + """ + + def generate_thumbnail( + self, + content: StaticContent, + tempfile_path: str | None = None, + dimensions: tuple[int, int] | None = None, + ) -> tuple[StaticContent | None, AssetKey]: """Create a thumbnail for a given image. - Returns a tuple of (StaticContent, AssetKey) + Returns a tuple of (StaticContent, AssetKey). The StaticContent is None if no thumbnail was created. `content` is the StaticContent representing the image you want to make a thumbnail out of. @@ -429,6 +563,7 @@ def generate_thumbnail(self, content, tempfile_path=None, dimensions=None): # My understanding is that PIL will maintain aspect ratios while restricting # the max-height/width to be whatever you pass in as 'size' # @todo: move the thumbnail size to a configuration setting?!? + source: BytesIO | str if tempfile_path is None: source = BytesIO(content.data) else: @@ -445,7 +580,7 @@ def generate_thumbnail(self, content, tempfile_path=None, dimensions=None): if not dimensions: dimensions = (128, 128) - thumbnail_image.thumbnail(dimensions, Image.LANCZOS) + thumbnail_image.thumbnail(dimensions, Image.Resampling.LANCZOS) thumbnail_image.save(thumbnail_file, 'JPEG') thumbnail_file.seek(0) @@ -463,7 +598,7 @@ def generate_thumbnail(self, content, tempfile_path=None, dimensions=None): return thumbnail_content, thumbnail_file_location - def ensure_indexes(self): + def ensure_indexes(self) -> None: """ Ensure that all appropriate indexes are created that are needed by this modulestore, or raise an exception if unable to. diff --git a/xmodule/contentstore/django.py b/xmodule/contentstore/django.py index 3d56edb58820..ac29e9f0b539 100644 --- a/xmodule/contentstore/django.py +++ b/xmodule/contentstore/django.py @@ -1,10 +1,16 @@ """Django integration utilities for loading and accessing the content store.""" +from __future__ import annotations + from importlib import import_module +from typing import TYPE_CHECKING, Any from django.conf import settings -_CONTENTSTORE = {} +if TYPE_CHECKING: + from .content import ContentStore + +_CONTENTSTORE: dict[str, ContentStore] = {} def load_function(path): @@ -18,15 +24,26 @@ def load_function(path): return getattr(import_module(module_path), name) -def contentstore(name="default"): - """Return a contentstore instance by name, creating and caching it if not already initialized.""" +def contentstore(name: str = "default") -> ContentStore: + """ + Return a contentstore instance by name, creating and caching it if not already initialized. + + The "default" contentstore routes each request to the appropriate backend for the course. Other named stores + (i.e. the deprecated "trashcan") are always legacy stores. + """ if name not in _CONTENTSTORE: - class_ = load_function(settings.CONTENTSTORE["ENGINE"]) - options = {} - options.update(settings.CONTENTSTORE["DOC_STORE_CONFIG"]) - if "ADDITIONAL_OPTIONS" in settings.CONTENTSTORE: - if name in settings.CONTENTSTORE["ADDITIONAL_OPTIONS"]: - options.update(settings.CONTENTSTORE["ADDITIONAL_OPTIONS"][name]) - _CONTENTSTORE[name] = class_(**options) + config: dict[str, Any] = settings.CONTENTSTORE + class_ = load_function(config["ENGINE"]) + options: dict[str, Any] = {} + options.update(config["DOC_STORE_CONFIG"]) + if "ADDITIONAL_OPTIONS" in config: + if name in config["ADDITIONAL_OPTIONS"]: + options.update(config["ADDITIONAL_OPTIONS"][name]) + store: ContentStore = class_(**options) + if name == "default": + # Imported here to avoid a circular import (content.py imports this module). + from .routing import RoutingContentStore # pylint: disable=import-outside-toplevel + store = RoutingContentStore(legacy_store=store) + _CONTENTSTORE[name] = store return _CONTENTSTORE[name] diff --git a/xmodule/contentstore/mongo.py b/xmodule/contentstore/mongo.py index 85f84225c27f..38f0b2e4d7f2 100644 --- a/xmodule/contentstore/mongo.py +++ b/xmodule/contentstore/mongo.py @@ -161,7 +161,7 @@ def delete(self, location_or_id): # Deletes of non-existent files are considered successful self.fs.delete(location_or_id) - def find(self, location, throw_on_not_found=True, as_stream=False): # pylint: disable=arguments-differ + def find(self, location, throw_on_not_found=True, as_stream=False): content_id, __ = self.asset_db_key(location) try: @@ -355,30 +355,6 @@ def _get_all_content_for_course(self, asset['asset_key'] = course_key.make_asset_key(asset_id['category'], asset_id['name']) return assets, count - def set_attr(self, asset_key, attr, value=True): - """ - Add/set the given attr on the asset at the given location. Does not allow overwriting gridFS built in - attrs such as _id, md5, uploadDate, length. Value can be any type which pymongo accepts. - - Returns nothing - - Raises NotFoundError if no such item exists - Raises AttributeError is attr is one of the build in attrs. - - :param asset_key: an AssetKey - :param attr: which attribute to set - :param value: the value to set it to (any type pymongo accepts such as datetime, number, string) - """ - self.set_attrs(asset_key, {attr: value}) - - def get_attr(self, location, attr, default=None): - """ - Get the value of attr set on location. If attr is unset, it returns default. Unlike set, this accessor - does allow getting the value of reserved keywords. - :param location: a c4x asset location - """ - return self.get_attrs(location).get(attr, default) - def set_attrs(self, location, attr_dict): """ Like set_attr but sets multiple key value pairs. diff --git a/xmodule/contentstore/routing.py b/xmodule/contentstore/routing.py new file mode 100644 index 000000000000..13c4a64f2cfc --- /dev/null +++ b/xmodule/contentstore/routing.py @@ -0,0 +1,164 @@ +""" +A ContentStore that routes each request to the appropriate backend for the course. + +Course "Files" are being moved out of the legacy MongoDB/GridFS contentstore and +into ``openedx_content`` (see openedx-core ADR 0016, "Replacing Contentstore"). +During the transition, each course run's files are stored in exactly one +backend, and :class:`RoutingContentStore` sends each request to the right one, +so that the rest of the platform can keep using the ``ContentStore`` API +without knowing where a given course's files are stored. + +Currently, the legacy store is the only backend. +""" + +from __future__ import annotations + +from collections.abc import Sequence +from typing import Any, Literal, overload + +from opaque_keys.edx.keys import AssetKey, CourseKey + +from .content import ContentStore, StaticContent, StaticContentStream + + +class RoutingContentStore(ContentStore): + """ + A ``ContentStore`` that delegates each request to the backend that stores the files for the relevant course. + """ + + def __init__(self, legacy_store: ContentStore): + """ + Initialize the router. + + ``legacy_store`` is the legacy (MongoDB/GridFS) ``ContentStore``. + """ + super().__init__() + self._legacy_store = legacy_store + + @property + def legacy_store(self) -> ContentStore: + """ + The legacy (MongoDB/GridFS) store. + + Most code should not use this directly, but it's needed for e.g. migrating assets out of the legacy store. + """ + return self._legacy_store + + @property + def _all_stores(self) -> list[ContentStore]: + """All of the backends, for operations that aren't specific to one course.""" + return [self._legacy_store] + + def _store_for_course(self, course_key: CourseKey) -> ContentStore: # pylint: disable=unused-argument + """Get the backend that stores the files for the given course.""" + return self._legacy_store + + def _store_for_asset(self, location: AssetKey) -> ContentStore: + return self._store_for_course(location.course_key) + + # Methods that act on a single asset + + def save(self, content: StaticContent) -> StaticContent: + return self._store_for_asset(content.location).save(content) + + @overload + def find( + self, location: AssetKey, throw_on_not_found: Literal[True] = True, as_stream: Literal[False] = False + ) -> StaticContent: ... + + @overload + def find( # pylint: disable=arguments-differ # (false positive with @overload) + self, location: AssetKey, throw_on_not_found: Literal[True] = True, *, as_stream: Literal[True] + ) -> StaticContentStream: ... + + @overload + def find( + self, location: AssetKey, throw_on_not_found: bool = True, as_stream: bool = False + ) -> StaticContent | None: ... + + def find( + self, location: AssetKey, throw_on_not_found: bool = True, as_stream: bool = False + ) -> StaticContent | None: + return self._store_for_asset(location).find( + location, throw_on_not_found=throw_on_not_found, as_stream=as_stream + ) + + def delete(self, location: AssetKey | Any) -> None: + if not isinstance(location, AssetKey): + # MongoContentStore also accepts its own internal IDs. + self._legacy_store.delete(location) + else: + self._store_for_asset(location).delete(location) + + def get_attrs(self, location: AssetKey) -> dict[str, Any]: + return self._store_for_asset(location).get_attrs(location) + + def set_attrs(self, location: AssetKey, attr_dict: dict[str, Any]) -> None: + self._store_for_asset(location).set_attrs(location, attr_dict) + + def get_attr(self, location: AssetKey, attr: str, default: Any = None) -> Any: + return self._store_for_asset(location).get_attr(location, attr, default) + + def set_attr(self, location: AssetKey, attr: str, value: Any = True) -> None: + self._store_for_asset(location).set_attr(location, attr, value) + + def export(self, location: AssetKey, output_directory: str) -> None: + self._store_for_asset(location).export(location, output_directory) + + def generate_thumbnail( + self, + content: StaticContent, + tempfile_path: str | None = None, + dimensions: tuple[int, int] | None = None, + ) -> tuple[StaticContent | None, AssetKey]: + # Delegate, so that each backend can decide how (or whether) to store thumbnails. + return self._store_for_asset(content.location).generate_thumbnail( + content, tempfile_path=tempfile_path, dimensions=dimensions + ) + + # Methods that act on all the assets of a course + + def get_all_content_for_course( + self, + course_key: CourseKey, + start: int = 0, + maxresults: int = -1, + sort: Sequence[tuple[str, int]] | None = None, + filter_params: dict[str, Any] | None = None, + ) -> tuple[list[dict[str, Any]], int]: + return self._store_for_course(course_key).get_all_content_for_course( + course_key, start=start, maxresults=maxresults, sort=sort, filter_params=filter_params + ) + + def get_all_content_thumbnails_for_course(self, course_key: CourseKey) -> list[dict[str, Any]]: + return self._store_for_course(course_key).get_all_content_thumbnails_for_course(course_key) + + def export_all_for_course(self, course_key: CourseKey, output_directory: str, assets_policy_file: str) -> None: + self._store_for_course(course_key).export_all_for_course(course_key, output_directory, assets_policy_file) + + def delete_all_course_assets(self, course_key: CourseKey) -> None: + self._store_for_course(course_key).delete_all_course_assets(course_key) + + def copy_all_course_assets(self, source_course_key: CourseKey, dest_course_key: CourseKey) -> None: + source_store = self._store_for_course(source_course_key) + dest_store = self._store_for_course(dest_course_key) + if source_store is not dest_store: + raise NotImplementedError("Copying assets between different ContentStore backends is not yet supported.") + source_store.copy_all_course_assets(source_course_key, dest_course_key) + + # Methods that act on all courses, or on the stores themselves + + def remove_redundant_content_for_courses(self) -> int: + return sum(store.remove_redundant_content_for_courses() for store in self._all_stores) + + def ensure_indexes(self) -> None: + for store in self._all_stores: + store.ensure_indexes() + + def close_connections(self) -> None: + for store in self._all_stores: + store.close_connections() + + def _drop_database(self, database: bool = True, collections: bool = True, connections: bool = True) -> None: + for store in self._all_stores: + store._drop_database(database, collections, connections) # pylint: disable=protected-access diff --git a/xmodule/tests/test_contentstore_routing.py b/xmodule/tests/test_contentstore_routing.py new file mode 100644 index 000000000000..8e673b886808 --- /dev/null +++ b/xmodule/tests/test_contentstore_routing.py @@ -0,0 +1,216 @@ +""" +Tests for the RoutingContentStore +""" + +from unittest import mock + +import ddt +import pytest +from django.test import SimpleTestCase, override_settings +from opaque_keys.edx.keys import CourseKey + +from xmodule.contentstore import django as contentstore_django +from xmodule.contentstore.content import ContentStore, StaticContent +from xmodule.contentstore.routing import RoutingContentStore + +COURSE_A = CourseKey.from_string("course-v1:OpenedX+Routing+A") +COURSE_B = CourseKey.from_string("course-v1:OpenedX+Routing+B") +ASSET_A = COURSE_A.make_asset_key("asset", "a.png") +ASSET_B = COURSE_B.make_asset_key("asset", "b.png") +CONTENT_A = StaticContent(ASSET_A, "a.png", "image/png", b"a") +CONTENT_B = StaticContent(ASSET_B, "b.png", "image/png", b"b") + + +class FakeContentStore(ContentStore): + """A ContentStore that does nothing, for testing the contentstore() factory.""" + + def __init__(self, **kwargs): + super().__init__() + self.options = kwargs + + +class TwoBackendRoutingContentStore(RoutingContentStore): + """ + A router with a second backend, which stores the files for ``COURSE_B``. + """ + + def __init__(self, legacy_store, other_store): + super().__init__(legacy_store) + self.other_store = other_store + + @property + def _all_stores(self): + return [self.legacy_store, self.other_store] + + def _store_for_course(self, course_key): + return self.other_store if course_key == COURSE_B else self.legacy_store + + +def mock_store(): + return mock.Mock(spec=ContentStore) + + +# (method name, args, kwargs, the args and kwargs we expect the backend method to receive) +DELEGATED_CALLS = [ + ("save", (CONTENT_A,), {}, (CONTENT_A,), {}), + ("find", (ASSET_A,), {}, (ASSET_A,), {"throw_on_not_found": True, "as_stream": False}), + ( + "find", + (ASSET_A,), + {"throw_on_not_found": False, "as_stream": True}, + (ASSET_A,), + {"throw_on_not_found": False, "as_stream": True}, + ), + ("delete", (ASSET_A,), {}, (ASSET_A,), {}), + ("get_attrs", (ASSET_A,), {}, (ASSET_A,), {}), + ("set_attrs", (ASSET_A, {"locked": True}), {}, (ASSET_A, {"locked": True}), {}), + ("get_attr", (ASSET_A, "locked", False), {}, (ASSET_A, "locked", False), {}), + ("set_attr", (ASSET_A, "locked", False), {}, (ASSET_A, "locked", False), {}), + ("export", (ASSET_A, "/tmp/out"), {}, (ASSET_A, "/tmp/out"), {}), + ( + "generate_thumbnail", + (CONTENT_A,), + {"dimensions": (10, 10)}, + (CONTENT_A,), + {"tempfile_path": None, "dimensions": (10, 10)}, + ), + ( + "get_all_content_for_course", + (COURSE_A,), + {"start": 10, "maxresults": 5, "sort": [("displayname", 1)]}, + (COURSE_A,), + {"start": 10, "maxresults": 5, "sort": [("displayname", 1)], "filter_params": None}, + ), + ("get_all_content_thumbnails_for_course", (COURSE_A,), {}, (COURSE_A,), {}), + ("export_all_for_course", (COURSE_A, "/tmp/out", "/tmp/a.json"), {}, (COURSE_A, "/tmp/out", "/tmp/a.json"), {}), + ("delete_all_course_assets", (COURSE_A,), {}, (COURSE_A,), {}), + ("copy_all_course_assets", (COURSE_A, COURSE_B), {}, (COURSE_A, COURSE_B), {}), +] +RETURNS_NONE = { + "delete", + "set_attrs", + "set_attr", + "export", + "export_all_for_course", + "delete_all_course_assets", + "copy_all_course_assets", +} + + +@ddt.ddt +class TestRoutingContentStore(SimpleTestCase): + """ + Tests for RoutingContentStore with only the legacy backend. + """ + + def setUp(self): + super().setUp() + self.legacy = mock_store() + self.router = RoutingContentStore(legacy_store=self.legacy) + + def test_legacy_store(self): + assert self.router.legacy_store is self.legacy + + @ddt.data(*DELEGATED_CALLS) + @ddt.unpack + def test_delegates_to_legacy(self, method_name, args, kwargs, expected_args, expected_kwargs): + """Each method is passed through to the legacy store, and returns its result (if any).""" + result = getattr(self.router, method_name)(*args, **kwargs) + backend_method = getattr(self.legacy, method_name) + backend_method.assert_called_once_with(*expected_args, **expected_kwargs) + if method_name in RETURNS_NONE: + assert result is None + else: + assert result is backend_method.return_value + + def test_delete_by_legacy_id(self): + """Deleting by a MongoContentStore internal ID (not an AssetKey) goes to the legacy store.""" + self.router.delete("some-mongo-id") + self.legacy.delete.assert_called_once_with("some-mongo-id") + + def test_global_methods(self): + self.legacy.remove_redundant_content_for_courses.return_value = 3 + assert self.router.remove_redundant_content_for_courses() == 3 + self.router.ensure_indexes() + self.legacy.ensure_indexes.assert_called_once_with() + self.router.close_connections() + self.legacy.close_connections.assert_called_once_with() + self.router._drop_database(database=False) # pylint: disable=protected-access + self.legacy._drop_database.assert_called_once_with(False, True, True) # pylint: disable=protected-access + + +@ddt.ddt +class TestRoutingBetweenBackends(SimpleTestCase): + """ + Tests that a router with more than one backend sends each request to the right one. + """ + + def setUp(self): + super().setUp() + self.legacy = mock_store() + self.other = mock_store() + self.router = TwoBackendRoutingContentStore(self.legacy, self.other) + + @ddt.data( + ("save", (CONTENT_B,)), + ("find", (ASSET_B,)), + ("delete", (ASSET_B,)), + ("set_attr", (ASSET_B, "locked", True)), + ("generate_thumbnail", (CONTENT_B,)), + ("get_all_content_for_course", (COURSE_B,)), + ("export_all_for_course", (COURSE_B, "/tmp/out", "/tmp/a.json")), + ("delete_all_course_assets", (COURSE_B,)), + ) + @ddt.unpack + def test_routes_by_course(self, method_name, args): + """Requests for COURSE_B go to the other store, and nothing goes to the legacy store.""" + getattr(self.router, method_name)(*args) + getattr(self.other, method_name).assert_called_once() + assert not self.legacy.method_calls + + def test_routes_to_legacy(self): + """Requests for COURSE_A still go to the legacy store.""" + self.router.find(ASSET_A) + self.legacy.find.assert_called_once() + assert not self.other.method_calls + + def test_copy_within_backend(self): + self.router.copy_all_course_assets(COURSE_B, COURSE_B) + self.other.copy_all_course_assets.assert_called_once_with(COURSE_B, COURSE_B) + + def test_copy_between_backends(self): + with pytest.raises(NotImplementedError): + self.router.copy_all_course_assets(COURSE_A, COURSE_B) + + def test_global_methods_use_all_backends(self): + self.legacy.remove_redundant_content_for_courses.return_value = 3 + self.other.remove_redundant_content_for_courses.return_value = 2 + assert self.router.remove_redundant_content_for_courses() == 5 + self.router.ensure_indexes() + self.legacy.ensure_indexes.assert_called_once_with() + self.other.ensure_indexes.assert_called_once_with() + + +@override_settings(CONTENTSTORE={ + "ENGINE": f"{__name__}.FakeContentStore", + "DOC_STORE_CONFIG": {"db": "test"}, + "ADDITIONAL_OPTIONS": {"trashcan": {"bucket": "trash_fs"}}, +}) +@mock.patch.dict(contentstore_django._CONTENTSTORE, clear=True) # pylint: disable=protected-access +class TestContentstoreFactory(SimpleTestCase): + """ + Tests for the contentstore() function. + """ + + def test_default_is_router(self): + store = contentstore_django.contentstore() + assert isinstance(store, RoutingContentStore) + assert isinstance(store.legacy_store, FakeContentStore) + assert store.legacy_store.options == {"db": "test"} + # It's cached: + assert contentstore_django.contentstore() is store + + def test_trashcan_is_legacy(self): + store = contentstore_django.contentstore("trashcan") + assert isinstance(store, FakeContentStore) + assert store.options == {"db": "test", "bucket": "trash_fs"} From 103168d64a3e4174b1ca25cd07f10efea0ede5d0 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 13 Nov 2026 15:42:27 -0700 Subject: [PATCH 2/3] fix: read large SVG uploads in binary mode when generating thumbnails When an uploaded file is large enough to be written to a temporary file, generate_thumbnail() reads it from tempfile_path. For SVGs, it opened the file in text mode and passed the resulting str to BytesIO(), which raised a TypeError. The error was logged and swallowed, so large SVGs never got a thumbnail. Co-Authored-By: Claude Opus 5.5 --- xmodule/contentstore/content.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xmodule/contentstore/content.py b/xmodule/contentstore/content.py index 21c8cc7c072a..a97a27c1689d 100644 --- a/xmodule/contentstore/content.py +++ b/xmodule/contentstore/content.py @@ -553,7 +553,7 @@ def generate_thumbnail( if tempfile_path is None: thumbnail_file = BytesIO(content.data) else: - with open(tempfile_path) as f: + with open(tempfile_path, "rb") as f: thumbnail_file = BytesIO(f.read()) thumbnail_content = StaticContent(thumbnail_file_location, thumbnail_name, 'image/svg+xml', thumbnail_file) From 8295eb1cc5873650b44cd5e3766f5085c0c3ecba Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 13 Nov 2026 16:37:25 -0700 Subject: [PATCH 3/3] refactor: define a ContentstoreAsset TypedDict for asset listings Formally specify the format of the dicts returned by ContentStore.get_all_content_for_course() and get_all_content_thumbnails_for_course(), listing only the keys that callers rely on. The unreliable "md5" key is deliberately omitted. Co-Authored-By: Claude Opus 5.5 --- xmodule/contentstore/content.py | 50 ++++++++++++++++++++++++--------- xmodule/contentstore/routing.py | 6 ++-- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/xmodule/contentstore/content.py b/xmodule/contentstore/content.py index a97a27c1689d..9c6300388bf9 100644 --- a/xmodule/contentstore/content.py +++ b/xmodule/contentstore/content.py @@ -5,8 +5,9 @@ import re import uuid from collections.abc import Sequence +from datetime import datetime from io import BytesIO -from typing import Any, Literal, overload +from typing import Any, Literal, NotRequired, TypedDict, overload from urllib.parse import parse_qsl, quote_plus, urlencode, urlparse, urlunparse from opaque_keys import InvalidKeyError @@ -350,6 +351,38 @@ def copy_to_in_mem(self): # pylint: disable=missing-function-docstring return content +class ContentstoreAsset(TypedDict): + """ + Information about an asset, as returned by ``ContentStore.get_all_content_for_course()``. + + Backends may include additional keys (``MongoContentStore`` returns the raw GridFS document, with keys like + ``_id``, ``filename`` and ``custom_md5``), but callers should only rely on the keys defined here. + + Note: there is deliberately no ``md5`` key. GridFS only stored ``md5`` for files uploaded before PyMongo 4, so it + was never reliably present. Use ``ContentStore.find(...).content_digest`` to get an asset's MD5 hash. + """ + + asset_key: AssetKey + """The key of the asset""" + displayname: str + """The human-readable name of the asset""" + contentType: str | None + """The MIME type of the asset, if known""" + uploadDate: datetime + """When the asset was last uploaded/saved""" + length: int + """The size of the asset, in bytes""" + locked: NotRequired[bool] + """Whether the asset is restricted to enrolled users (default False)""" + thumbnail_location: NotRequired[list[str | None] | None] + """ + The location of the asset's thumbnail, if any, in the deprecated "c4x" list format: + ``['c4x', org, course, 'thumbnail', name, None]`` + """ + import_path: NotRequired[str | None] + """The path that the asset was imported from (e.g. ``images/foo.png``), if any""" + + class ContentStore: """ Abstraction for all ContentStore providers. @@ -452,25 +485,16 @@ def get_all_content_for_course( maxresults: int = -1, sort: Sequence[tuple[str, int]] | None = None, filter_params: dict[str, Any] | None = None, - ) -> tuple[list[dict[str, Any]], int]: + ) -> tuple[list[ContentstoreAsset], int]: """ Returns a list of static assets for a course, followed by the total number of assets. By default all assets are returned, but start and maxresults can be provided to limit the query. - The return format is a list of asset data dictionaries. - The asset data dictionaries have the following keys: - asset_key (:class:`opaque_keys.edx.AssetKey`): The key of the asset - displayname: The human-readable name of the asset - uploadDate (datetime.datetime): The date and time that the file was uploadDate - contentType: The mimetype string of the asset - length: The size of the asset, in bytes - locked: Whether the asset is restricted to enrolled users - thumbnail_location: The location of the asset's thumbnail, if any - import_path: The path that the asset was imported from, if any + See :class:`ContentstoreAsset` for the format of each asset in the list. """ raise NotImplementedError - def get_all_content_thumbnails_for_course(self, course_key: CourseKey) -> list[dict[str, Any]]: + def get_all_content_thumbnails_for_course(self, course_key: CourseKey) -> list[ContentstoreAsset]: """ Returns a list of all the thumbnails for a course, in the same format as ``get_all_content_for_course()``. """ diff --git a/xmodule/contentstore/routing.py b/xmodule/contentstore/routing.py index 13c4a64f2cfc..d75d449c1944 100644 --- a/xmodule/contentstore/routing.py +++ b/xmodule/contentstore/routing.py @@ -18,7 +18,7 @@ from opaque_keys.edx.keys import AssetKey, CourseKey -from .content import ContentStore, StaticContent, StaticContentStream +from .content import ContentStore, ContentstoreAsset, StaticContent, StaticContentStream class RoutingContentStore(ContentStore): @@ -125,12 +125,12 @@ def get_all_content_for_course( maxresults: int = -1, sort: Sequence[tuple[str, int]] | None = None, filter_params: dict[str, Any] | None = None, - ) -> tuple[list[dict[str, Any]], int]: + ) -> tuple[list[ContentstoreAsset], int]: return self._store_for_course(course_key).get_all_content_for_course( course_key, start=start, maxresults=maxresults, sort=sort, filter_params=filter_params ) - def get_all_content_thumbnails_for_course(self, course_key: CourseKey) -> list[dict[str, Any]]: + def get_all_content_thumbnails_for_course(self, course_key: CourseKey) -> list[ContentstoreAsset]: return self._store_for_course(course_key).get_all_content_thumbnails_for_course(course_key) def export_all_for_course(self, course_key: CourseKey, output_directory: str, assets_policy_file: str) -> None: