Skip to content

Commit

Permalink
refactor: get rid of XBlockRuntimeSystem
Browse files Browse the repository at this point in the history
  • Loading branch information
bradenmacdonald committed Oct 10, 2024
1 parent f2ad108 commit d42adee
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 107 deletions.
34 changes: 11 additions & 23 deletions openedx/core/djangoapps/xblock/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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)
Expand Down
12 changes: 6 additions & 6 deletions openedx/core/djangoapps/xblock/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(
Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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):
"""
Expand Down
101 changes: 26 additions & 75 deletions openedx/core/djangoapps/xblock/runtime/runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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 += '/'
Expand Down Expand Up @@ -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":
Expand All @@ -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:
"""
Expand All @@ -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()
Expand All @@ -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,
Expand Down Expand Up @@ -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

0 comments on commit d42adee

Please sign in to comment.