diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index 43ec3909bcd6..dbb7c824676b 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/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) diff --git a/openedx/core/djangoapps/xblock/apps.py b/openedx/core/djangoapps/xblock/apps.py index 5ba2361322ec..848470a3a90c 100644 --- a/openedx/core/djangoapps/xblock/apps.py +++ b/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( diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index 26aa7af60f0b..41fd79f5a068 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/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): """ diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 5746af491d10..fe633f686f02 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/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