Contentstore wrapper that can route requests to different implementations [FC-0138]

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)

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
 

xmodule/contentstore/content.py

@@ -4,7 +4,10 @@
 import os
 import re
 import uuid
+from collections.abc import Sequence
+from datetime import datetime
 from io import BytesIO
+from typing import Any, Literal, NotRequired, TypedDict, overload
 from urllib.parse import parse_qsl, quote_plus, urlencode, urlparse, urlunparse
 
 from opaque_keys import InvalidKeyError
@@ -348,48 +351,203 @@ 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 (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
+
+    @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 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 find(self, filename):
+    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_all_content_for_course(self, course_key, start=0, maxresults=-1, sort=None, filter_params=None):
-        '''
+    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[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
-            md5: An md5 hash of the asset content
-        '''
+        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[ContentstoreAsset]:
+        """
+        Returns a list of all the thumbnails for a course, in the same format as ``get_all_content_for_course()``.
+        """
         raise NotImplementedError
 
-    def delete_all_course_assets(self, course_key):
+    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: 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.
@@ -419,7 +577,7 @@ def generate_thumbnail(self, content, tempfile_path=None, dimensions=None):
                 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)
@@ -429,6 +587,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 +604,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 +622,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.

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]

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.