refactor: get rid of XBlockRuntimeSystem

openedx/core/djangoapps/xblock/api.py

@@ -25,13 +25,13 @@
 from xblock.exceptions import NoSuchUsage, NoSuchViewError
 from xblock.plugin import PluginMissingError
 
+from openedx.core.types import User as UserType
 from openedx.core.djangoapps.xblock.apps import get_xblock_app_config
 from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl
 from openedx.core.djangoapps.xblock.runtime.learning_core_runtime import (
     LearningCoreFieldData,
     LearningCoreXBlockRuntime,
 )
-from openedx.core.djangoapps.xblock.runtime.runtime import XBlockRuntimeSystem as _XBlockRuntimeSystem
 from .utils import get_secure_token_for_xblock_handler, get_xblock_id_for_anonymous_user
 
 from .runtime.learning_core_runtime import LearningCoreXBlockRuntime
@@ -54,33 +54,21 @@ class CheckPerm(Enum):
     CAN_EDIT = 3
 
 
-def get_runtime_system():
+def get_runtime(user: UserType):
     """
-    Return a new XBlockRuntimeSystem.
-
-    TODO: Refactor to get rid of the XBlockRuntimeSystem entirely and just
-    create the LearningCoreXBlockRuntime and return it. We used to want to keep
-    around a long lived runtime system (a factory that returns runtimes) for
-    caching purposes, and have it dynamically construct a runtime on request.
-    Now we're just re-constructing both the system and the runtime in this call
-    and returning it every time, because:
-
-    1. We no longer have slow, Blockstore-style definitions to cache, so the
-       performance of this is perfectly acceptable.
-    2. Having a singleton increases complexity and the chance of bugs.
-    3. Creating the XBlockRuntimeSystem every time only takes about 10-30 µs.
-
-    Given that, the extra XBlockRuntimeSystem class just adds confusion. But
-    despite that, it's tested, working code, and so I'm putting off refactoring
-    for now.
+    Return a new XBlockRuntime.
+
+    Each XBlockRuntime is bound to one user (and usually one request or one
+    celery task). It is typically used just to load and render a single block,
+    but the API _does_ allow a single runtime instance to load multiple blocks
+    (as long as they're for the same user).
     """
-    params = get_xblock_app_config().get_runtime_system_params()
+    params = get_xblock_app_config().get_runtime_params()
     params.update(
-        runtime_class=LearningCoreXBlockRuntime,
         handler_url=get_handler_url,
         authored_data_store=LearningCoreFieldData(),
     )
-    runtime = _XBlockRuntimeSystem(**params)
+    runtime = LearningCoreXBlockRuntime(user, **params)
 
     return runtime
 
@@ -121,7 +109,7 @@ def load_block(usage_key, user, *, check_permission: CheckPerm | None = CheckPer
     # e.g. a course might specify that all 'problem' XBlocks have 'max_attempts'
     # set to 3.
     # field_overrides = context_impl.get_field_overrides(usage_key)
-    runtime = get_runtime_system().get_runtime(user=user)
+    runtime = get_runtime(user=user)
 
     try:
         return runtime.get_block(usage_key)

openedx/core/djangoapps/xblock/apps.py

@@ -16,9 +16,9 @@ class XBlockAppConfig(AppConfig):
     verbose_name = 'New XBlock Runtime'
     label = 'xblock_new'  # The name 'xblock' is already taken by ORA2's 'openassessment.xblock' app :/
 
-    def get_runtime_system_params(self):
+    def get_runtime_params(self):
         """
-        Get the XBlockRuntimeSystem parameters appropriate for viewing and/or
+        Get the LearningCoreXBlockRuntime parameters appropriate for viewing and/or
         editing XBlock content.
         """
         raise NotImplementedError
@@ -43,9 +43,9 @@ class LmsXBlockAppConfig(XBlockAppConfig):
     LMS-specific configuration of the XBlock Runtime django app.
     """
 
-    def get_runtime_system_params(self):
+    def get_runtime_params(self):
         """
-        Get the XBlockRuntimeSystem parameters appropriate for viewing and/or
+        Get the LearningCoreXBlockRuntime parameters appropriate for viewing and/or
         editing XBlock content in the LMS
         """
         return dict(
@@ -65,9 +65,9 @@ class StudioXBlockAppConfig(XBlockAppConfig):
     Studio-specific configuration of the XBlock Runtime django app.
     """
 
-    def get_runtime_system_params(self):
+    def get_runtime_params(self):
         """
-        Get the XBlockRuntimeSystem parameters appropriate for viewing and/or
+        Get the LearningCoreXBlockRuntime parameters appropriate for viewing and/or
         editing XBlock content in Studio
         """
         return dict(

openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py

@@ -211,7 +211,7 @@ def get_block(self, usage_key, for_parent=None):
         # We've pre-loaded the fields for this block, so the FieldData shouldn't
         # consider these values "changed" in its sense of "you have to persist
         # these because we've altered the field values from what was stored".
-        self.system.authored_data_store.mark_unchanged(block)
+        self.authored_data_store.mark_unchanged(block)
 
         return block
 
@@ -221,7 +221,7 @@ def save_block(self, block):
 
         This gets called by block.save() - do not call this directly.
         """
-        if not self.system.authored_data_store.has_changes(block):
+        if not self.authored_data_store.has_changes(block):
             return  # No changes, so no action needed.
 
         # Verify that the user has permission to write to authored data in this
@@ -254,7 +254,7 @@ def save_block(self, block):
                 },
                 created=now,
             )
-        self.system.authored_data_store.mark_unchanged(block)
+        self.authored_data_store.mark_unchanged(block)
 
     def _get_component_from_usage_key(self, usage_key):
         """

openedx/core/djangoapps/xblock/runtime/runtime.py

@@ -95,18 +95,30 @@ class XBlockRuntime(RuntimeShim, Runtime):
     # currently only used to track if we're in the studio_view (see below under service())
     view_name: str | None
 
-    def __init__(self, system: XBlockRuntimeSystem, user: UserType | None):
+    def __init__(
+        self,
+        user: UserType | None,
+        *,
+        handler_url: Callable[[UsageKey, str, UserType | None], str],
+        student_data_mode: StudentDataMode,
+        id_reader: Optional[IdReader] = None,
+        authored_data_store: Optional[FieldData] = None,
+    ):
         super().__init__(
-            id_reader=system.id_reader,
+            id_reader=id_reader or OpaqueKeyReader(),
             mixins=(
                 LmsBlockMixin,  # Adds Non-deprecated LMS/Studio functionality
                 XBlockShim,  # Adds deprecated LMS/Studio functionality / backwards compatibility
             ),
             default_class=None,
             select=None,
-            id_generator=system.id_generator,
+            id_generator=MemoryIdManager(),  # We don't really use id_generator until we need to support asides
         )
-        self.system = system
+        assert student_data_mode in (StudentDataMode.Ephemeral, StudentDataMode.Persisted)
+        self.authored_data_store = authored_data_store
+        self.children_data_store = None
+        self.student_data_mode = student_data_mode
+        self.handler_url_fn = handler_url
         self.user = user
         # self.user_id must be set as a separate attribute since base class sets it:
         if self.user is None:
@@ -126,7 +138,7 @@ def handler_url(self, block, handler_name: str, suffix='', query='', thirdparty=
         if thirdparty:
             log.warning("thirdparty handlers are not supported by this runtime for XBlock %s.", type(block))
 
-        url = self.system.handler_url(block.scope_ids.usage_id, handler_name, self.user)
+        url = self.handler_url_fn(block.scope_ids.usage_id, handler_name, self.user)
         if suffix:
             if not url.endswith('/'):
                 url += '/'
@@ -275,7 +287,7 @@ def service(self, block: XBlock, service_name: str):
             # the preview engine, and 'main' otherwise.
             # For backwards compatibility, we check the student_data_mode (Ephemeral indicates CMS) and the
             # view_name for 'studio_view.' self.view_name is set by render() below.
-            if self.system.student_data_mode == StudentDataMode.Ephemeral and self.view_name != 'studio_view':
+            if self.student_data_mode == StudentDataMode.Ephemeral and self.view_name != 'studio_view':
                 return MakoService(namespace_prefix='lms.')
             return MakoService()
         elif service_name == "i18n":
@@ -301,14 +313,12 @@ def service(self, block: XBlock, service_name: str):
             return EventPublishingService(self.user, context_key, make_track_function())
         elif service_name == 'enrollments':
             return EnrollmentsService()
+        elif service_name == 'error_tracker':
+            return make_error_tracker()
 
-        # Check if the XBlockRuntimeSystem wants to handle this:
-        service = self.system.get_service(block, service_name)
         # Otherwise, fall back to the base implementation which loads services
         # defined in the constructor:
-        if service is None:
-            service = super().service(block, service_name)
-        return service
+        return super().service(block, service_name)
 
     def _init_field_data_for_block(self, block: XBlock) -> FieldData:
         """
@@ -322,7 +332,7 @@ def _init_field_data_for_block(self, block: XBlock) -> FieldData:
             assert isinstance(self.user_id, str) and self.user_id.startswith("anon")
             kvs = EphemeralKeyValueStore()
             student_data_store = KvsFieldData(kvs)
-        elif self.system.student_data_mode == StudentDataMode.Ephemeral:
+        elif self.student_data_mode == StudentDataMode.Ephemeral:
             # We're in an environment like Studio where we want to let the
             # author test blocks out but not permanently save their state.
             kvs = EphemeralKeyValueStore()
@@ -341,10 +351,10 @@ def _init_field_data_for_block(self, block: XBlock) -> FieldData:
             student_data_store = KvsFieldData(kvs=DjangoKeyValueStore(field_data_cache))
 
         return SplitFieldData({
-            Scope.content: self.system.authored_data_store,
-            Scope.settings: self.system.authored_data_store,
-            Scope.parent: self.system.authored_data_store,
-            Scope.children: self.system.children_data_store,
+            Scope.content: self.authored_data_store,
+            Scope.settings: self.authored_data_store,
+            Scope.parent: self.authored_data_store,
+            Scope.children: self.children_data_store,
             Scope.user_state_summary: student_data_store,
             Scope.user_state: student_data_store,
             Scope.user_info: student_data_store,
@@ -407,62 +417,3 @@ def _lookup_asset_url(self, block: XBlock, asset_path: str):  # pylint: disable=
         """
         # Subclasses should override this
         return None
-
-
-class XBlockRuntimeSystem:
-    """
-    This class is essentially a factory for XBlockRuntimes. This is a
-    long-lived object which provides the behavior specific to the application
-    that wants to use XBlocks. Unlike XBlockRuntime, a single instance of this
-    class can be used with many different XBlocks, whereas each XBlock gets its
-    own instance of XBlockRuntime.
-    """
-    def __init__(
-        self,
-        handler_url: Callable[[UsageKey, str, UserType | None], str],
-        student_data_mode: StudentDataMode,
-        runtime_class: type[XBlockRuntime],
-        id_reader: Optional[IdReader] = None,
-        authored_data_store: Optional[FieldData] = None,
-    ):
-        """
-        args:
-            handler_url: A method to get URLs to call XBlock handlers. It must
-                implement this signature:
-                handler_url(
-                    usage_key: UsageKey,
-                    handler_name: str,
-                    user: User | AnonymousUser | None
-                ) -> str
-            student_data_mode: Specifies whether student data should be kept
-                in a temporary in-memory store (e.g. Studio) or persisted
-                forever in the database.
-            runtime_class: What runtime to use, e.g. LearningCoreXBlockRuntime
-        """
-        self.handler_url = handler_url
-        self.id_reader = id_reader or OpaqueKeyReader()
-        self.id_generator = MemoryIdManager()  # We don't really use id_generator until we need to support asides
-        self.runtime_class = runtime_class
-        self.authored_data_store = authored_data_store
-        self.children_data_store = None
-        assert student_data_mode in (StudentDataMode.Ephemeral, StudentDataMode.Persisted)
-        self.student_data_mode = student_data_mode
-
-    def get_runtime(self, user: UserType | None) -> XBlockRuntime:
-        """
-        Get the XBlock runtime for the specified Django user. The user can be
-        a regular user, an AnonymousUser, or None.
-        """
-        return self.runtime_class(self, user)
-
-    def get_service(self, block, service_name: str):
-        """
-        Get a runtime service
-
-        Runtime services may come from this XBlockRuntimeSystem,
-        or if this method returns None, they may come from the
-        XBlockRuntime.
-        """
-        if service_name == 'error_tracker':
-            return make_error_tracker()
-        return None  # None means see if XBlockRuntime offers this service