From 327b36d4cab20215884499aec119fac6f7efb493 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Thu, 30 May 2024 11:07:11 -0700 Subject: [PATCH 1/4] feat: v2 XBlocks --- xblock/core.py | 81 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 80 insertions(+), 1 deletion(-) diff --git a/xblock/core.py b/xblock/core.py index fcea0f597..82b0fda10 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -7,6 +7,8 @@ import json import logging import os +from types import MappingProxyType +from typing import Final, final import warnings from collections import OrderedDict, defaultdict @@ -23,7 +25,7 @@ ) from xblock.fields import Field, List, Reference, ReferenceList, Scope, String from xblock.internal import class_lazy -from xblock.plugin import Plugin +from xblock.plugin import Plugin, PluginMissingError from xblock.validation import Validation # OrderedDict is used so that namespace attributes are put in predictable order @@ -930,6 +932,83 @@ def has_support(self, view, functionality): return hasattr(view, "_supports") and functionality in view._supports # pylint: disable=protected-access +class XBlock2(Plugin): + """ + Base class for all v2 XBlocks, whether they are wrappers around a v1 XBlock, + or pure v2-only XBlocks. This ensures that `issubclass(block, XBlock2)` is + always useful. + """ + has_children: Final = False + + entry_point = 'xblock.v2' + + @classmethod + def load_class(cls, identifier, default=None, select=None, fallback_to_v1=False): + """ + Load a v2 XBlock, with optional fallback to v1 + """ + try: + return super().load_class(identifier, default, select) + except PluginMissingError: + if fallback_to_v1: + return XBlock.load_class(identifier, default, select) + if default: + return default + raise + + def __init__(self, runtime, scope_ids): + """ + Arguments: + + runtime (:class:`.Runtime`): Use it to access the environment. + It is available in XBlock code as ``self.runtime``. + + scope_ids (:class:`.ScopeIds`): Identifiers needed to resolve + scopes. + """ + if self.has_children is not False: + raise ValueError('v2 XBlocks cannot declare has_children = True') + if hasattr(self, "student_view"): + raise AttributeError("v2 XBlocks must not declare a student_view() method.") + + super().__init__(runtime=runtime, scope_ids=scope_ids) + + # Prevent changes to _dirty_fields by forcing it to be a read-only dict. V2 XBlocks are expected to use the + # self.set_field[s]() API instead of writing to fields directly. + self._dirty_fields = MappingProxyType({}) + + @final + def save(self): + raise AttributeError("Calling .save() on a v2 XBlock is forbidden") + + @property + def parent(self): + warnings.warn("Accessing .parent of v2 XBlocks is forbidden", stacklevel=2) + return None + + @property + def _parent_block_id(self): + warnings.warn("Accessing ._parent_block_id of v2 XBlocks is forbidden", stacklevel=2) + return None + + +class XBlock2Mixin(XBlock2): + """ + Mixin that allows subclassing a v1 XBlock to become a v2 XBlock, allowing + both versions of the XBlock to be used, each with a different entry point. + """ + + @final + def student_view(self, _context): + raise AttributeError("v2 XBlocks do not support student_view()") + + +class PureXBlock2(XBlock2, XBlock): + """ + Base class for pure "v2" XBlocks, that don't need backwards compatibility with v1 + """ + + class XBlockAside(Plugin, Blocklike): """ Base class for XBlock-like objects that are rendered alongside :class:`.XBlock` views. From 316bb505f69abf86f8d6328d6373591935616439 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 31 May 2024 18:30:45 -0700 Subject: [PATCH 2/4] WIP - combine v1 and v2 together --- xblock/core.py | 114 +++++++++++++++++++++++++++------------------- xblock/runtime.py | 7 +-- 2 files changed, 72 insertions(+), 49 deletions(-) diff --git a/xblock/core.py b/xblock/core.py index 82b0fda10..821161622 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -1,6 +1,7 @@ """ Base classes for all XBlock-like objects. Used by all XBlock Runtimes. """ +from contextlib import contextmanager import copy import functools import inspect @@ -189,14 +190,28 @@ def wrapper(self, request, suffix=''): request_json = json.loads(request.body.decode('utf-8')) except ValueError: return JsonHandlerError(400, "Invalid JSON").get_response() - try: - response = func(self, request_json, suffix) - except JsonHandlerError as err: - return err.get_response() - if isinstance(response, Response): - return response + if isinstance(self, XBlock2Mixin): + # For XBlock v2 blocks, a json_handler is one of the only times where field edits are allowed. + with self._track_field_writes() as field_updates: + try: + response = func(self, request_json, suffix) + except JsonHandlerError as err: + return err.get_response(updated_fields=field_updates["updated_fields"]) + else: + return Response( + json.dumps({"data": response, "updated_fields": field_updates["updated_fields"]}), + content_type='application/json', + charset='utf8', + ) else: - return Response(json.dumps(response), content_type='application/json', charset='utf8') + try: + response = func(self, request_json, suffix) + except JsonHandlerError as err: + return err.get_response() + if isinstance(response, Response): + return response + else: + return Response(json.dumps(response), content_type='application/json', charset='utf8') return wrapper @classmethod @@ -932,31 +947,23 @@ def has_support(self, view, functionality): return hasattr(view, "_supports") and functionality in view._supports # pylint: disable=protected-access -class XBlock2(Plugin): +class XBlock2Mixin: """ - Base class for all v2 XBlocks, whether they are wrappers around a v1 XBlock, - or pure v2-only XBlocks. This ensures that `issubclass(block, XBlock2)` is - always useful. + Mixin with shared implementation for all v2 XBlocks, whether they are + wrappers around a v1 XBlock, or pure v2-only XBlocks. + + Note: check if an XBlock is "v2" using `issubclass(block, XBlock2Mixin)`, + not `issubclass(block, XBlock2)` """ has_children: Final = False - entry_point = 'xblock.v2' - - @classmethod - def load_class(cls, identifier, default=None, select=None, fallback_to_v1=False): - """ - Load a v2 XBlock, with optional fallback to v1 - """ - try: - return super().load_class(identifier, default, select) - except PluginMissingError: - if fallback_to_v1: - return XBlock.load_class(identifier, default, select) - if default: - return default - raise - - def __init__(self, runtime, scope_ids): + def __init__( + self, + runtime, + field_data=None, + scope_ids=UNSET, + for_parent=None, + **kwargs): """ Arguments: @@ -968,14 +975,14 @@ def __init__(self, runtime, scope_ids): """ if self.has_children is not False: raise ValueError('v2 XBlocks cannot declare has_children = True') - if hasattr(self, "student_view"): - raise AttributeError("v2 XBlocks must not declare a student_view() method.") - - super().__init__(runtime=runtime, scope_ids=scope_ids) + + if field_data is not None: + raise ValueError('v2 XBlocks do not allow the deprecated field_data init parameter.') + + if for_parent is not None: + warnings.warn("Ignoring for_parent kwarg passed to a v2 XBlock init method", stacklevel=2) - # Prevent changes to _dirty_fields by forcing it to be a read-only dict. V2 XBlocks are expected to use the - # self.set_field[s]() API instead of writing to fields directly. - self._dirty_fields = MappingProxyType({}) + super().__init__(runtime, scope_ids=scope_ids, **kwargs) @final def save(self): @@ -986,24 +993,39 @@ def parent(self): warnings.warn("Accessing .parent of v2 XBlocks is forbidden", stacklevel=2) return None + @parent.setter + def parent(self, value): + if value is not None: + raise ValueError("v2 XBlocks cannot have a parent.") + warnings.warn("Accessing .parent of v2 XBlocks is forbidden", stacklevel=2) + @property def _parent_block_id(self): warnings.warn("Accessing ._parent_block_id of v2 XBlocks is forbidden", stacklevel=2) return None - -class XBlock2Mixin(XBlock2): - """ - Mixin that allows subclassing a v1 XBlock to become a v2 XBlock, allowing - both versions of the XBlock to be used, each with a different entry point. - """ - - @final - def student_view(self, _context): - raise AttributeError("v2 XBlocks do not support student_view()") + @_parent_block_id.setter + def _parent_block_id(self, value): + if value is not None: + raise ValueError("v2 XBlocks cannot have a parent.") + + @contextmanager + def _track_field_writes(self): + if not isinstance(self, XBlock2Mixin): + raise TypeError("track_field_writes() is only compatible with XBlock2 instances") + if hasattr(self, "_collect_field_writes"): + raise RuntimeError("Nested _track_field_writes calls detected") + print("Starting handler...") + field_updates = {"updated_fields": {"user": {}, "content": {}}} + self._collect_field_writes = field_updates["updated_fields"] + try: + yield field_updates + finally: + delattr(self, "_collect_field_writes") + print("Ending handler...") -class PureXBlock2(XBlock2, XBlock): +class XBlock2(XBlock2Mixin, XBlock): """ Base class for pure "v2" XBlocks, that don't need backwards compatibility with v1 """ diff --git a/xblock/runtime.py b/xblock/runtime.py index 8aa822dda..dc7d7f0ab 100644 --- a/xblock/runtime.py +++ b/xblock/runtime.py @@ -19,7 +19,7 @@ from web_fragments.fragment import Fragment -from xblock.core import XBlock, XBlockAside, XML_NAMESPACES +from xblock.core import XBlock, XBlockAside, XBlock2Mixin, XML_NAMESPACES from xblock.fields import Field, BlockScope, Scope, ScopeIds, UserScope from xblock.field_data import FieldData from xblock.exceptions import ( @@ -1063,8 +1063,9 @@ def handle(self, block, handler_name, request, suffix=''): else: raise NoSuchHandlerError(f"Couldn't find handler {handler_name!r} for {block!r}") - # Write out dirty fields - block.save() + # Write out dirty fields (v1 XBlocks); for v2 this is handled by @json_handler + if not isinstance(block, XBlock2Mixin): + block.save() return results # Services From aa1cd10f41a8c3875127675f4c6bbc8284135413 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 31 May 2024 18:49:09 -0700 Subject: [PATCH 3/4] WIP - close to implementing handlers --- xblock/core.py | 46 ++++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/xblock/core.py b/xblock/core.py index 821161622..4fea1f10a 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -24,7 +24,7 @@ KeyValueMultiSaveError, XBlockSaveError, ) -from xblock.fields import Field, List, Reference, ReferenceList, Scope, String +from xblock.fields import Field, List, Reference, ReferenceList, Scope, String, UserScope from xblock.internal import class_lazy from xblock.plugin import Plugin, PluginMissingError from xblock.validation import Validation @@ -192,17 +192,22 @@ def wrapper(self, request, suffix=''): return JsonHandlerError(400, "Invalid JSON").get_response() if isinstance(self, XBlock2Mixin): # For XBlock v2 blocks, a json_handler is one of the only times where field edits are allowed. - with self._track_field_writes() as field_updates: - try: + field_updates = {"updated_fields": {"user": {}, "content": {}}} + try: + with self._track_field_writes(field_updates): response = func(self, request_json, suffix) - except JsonHandlerError as err: - return err.get_response(updated_fields=field_updates["updated_fields"]) - else: - return Response( - json.dumps({"data": response, "updated_fields": field_updates["updated_fields"]}), - content_type='application/json', - charset='utf8', - ) + except JsonHandlerError as err: + return err.get_response(updated_fields=field_updates["updated_fields"]) + else: + if response is None: + response = {} + elif not isinstance(response, dict): + raise TypeError("json_handler functions must return a dict") + return Response( + json.dumps({"data": response, "updated_fields": field_updates["updated_fields"]}), + content_type='application/json', + charset='utf8', + ) else: try: response = func(self, request_json, suffix) @@ -1010,18 +1015,23 @@ def _parent_block_id(self, value): raise ValueError("v2 XBlocks cannot have a parent.") @contextmanager - def _track_field_writes(self): + def _track_field_writes(self, field_updates): if not isinstance(self, XBlock2Mixin): raise TypeError("track_field_writes() is only compatible with XBlock2 instances") - if hasattr(self, "_collect_field_writes"): - raise RuntimeError("Nested _track_field_writes calls detected") + if self._dirty_fields: + raise ValueError("Found dirty fields before handler even started - shouldn't happen") print("Starting handler...") - field_updates = {"updated_fields": {"user": {}, "content": {}}} - self._collect_field_writes = field_updates["updated_fields"] try: - yield field_updates + yield + for field in self._dirty_fields.keys(): + scope_type = "user" if field.scope.user != UserScope.NONE else "content" + field_updates["updated_fields"][scope_type][field.name] = field.to_json(getattr(self, field.name)) + print("success, dirty fields: ", self._dirty_fields) + print("success, dirty fields: ", field_updates["updated_fields"]) + self.force_save_fields([field.name for field in self._dirty_fields.keys()]) + self.runtime.save_block(self) finally: - delattr(self, "_collect_field_writes") + self._dirty_fields.clear() print("Ending handler...") From a56d8db1ac2892256d64a434537317a37c78f307 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Fri, 31 May 2024 19:34:54 -0700 Subject: [PATCH 4/4] WIP better separation of mixin vs pure v2 --- xblock/core.py | 76 ++++++++++++++++++++++++++------------------------ 1 file changed, 40 insertions(+), 36 deletions(-) diff --git a/xblock/core.py b/xblock/core.py index 4fea1f10a..0dc41e887 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -955,28 +955,58 @@ def has_support(self, view, functionality): class XBlock2Mixin: """ Mixin with shared implementation for all v2 XBlocks, whether they are - wrappers around a v1 XBlock, or pure v2-only XBlocks. - + keeping backwards compatibility with v1 or not. + Note: check if an XBlock is "v2" using `issubclass(block, XBlock2Mixin)`, not `issubclass(block, XBlock2)` """ has_children: Final = False + def __init__(self, *args, **kwargs): + """ + Validation during init + """ + super().__init__(*args, **kwargs) + if self.has_children is not False: + raise ValueError('v2 XBlocks cannot declare has_children = True') + + @contextmanager + def _track_field_writes(self, field_updates): + if not isinstance(self, XBlock2Mixin): + raise TypeError("track_field_writes() is only compatible with XBlock2 instances") + if self._dirty_fields: + raise ValueError("Found dirty fields before handler even started - shouldn't happen") + print("Starting handler...") + try: + yield + for field in self._dirty_fields.keys(): + scope_type = "user" if field.scope.user != UserScope.NONE else "content" + field_updates["updated_fields"][scope_type][field.name] = field.to_json(getattr(self, field.name)) + print("success, dirty fields: ", self._dirty_fields) + print("success, dirty fields: ", field_updates["updated_fields"]) + print(f"{self}") + self.force_save_fields([field.name for field in self._dirty_fields.keys()]) + self.runtime.save_block(self) + finally: + self._dirty_fields.clear() + print("Ending handler...") + + +class XBlock2(XBlock2Mixin, XBlock): + """ + Base class for pure "v2" XBlocks, that don't need backwards compatibility with v1 + """ + def __init__( self, runtime, field_data=None, scope_ids=UNSET, for_parent=None, - **kwargs): + **kwargs, + ): """ - Arguments: - - runtime (:class:`.Runtime`): Use it to access the environment. - It is available in XBlock code as ``self.runtime``. - - scope_ids (:class:`.ScopeIds`): Identifiers needed to resolve - scopes. + Initialize this v2 XBlock, checking for deprecated usage first """ if self.has_children is not False: raise ValueError('v2 XBlocks cannot declare has_children = True') @@ -1014,32 +1044,6 @@ def _parent_block_id(self, value): if value is not None: raise ValueError("v2 XBlocks cannot have a parent.") - @contextmanager - def _track_field_writes(self, field_updates): - if not isinstance(self, XBlock2Mixin): - raise TypeError("track_field_writes() is only compatible with XBlock2 instances") - if self._dirty_fields: - raise ValueError("Found dirty fields before handler even started - shouldn't happen") - print("Starting handler...") - try: - yield - for field in self._dirty_fields.keys(): - scope_type = "user" if field.scope.user != UserScope.NONE else "content" - field_updates["updated_fields"][scope_type][field.name] = field.to_json(getattr(self, field.name)) - print("success, dirty fields: ", self._dirty_fields) - print("success, dirty fields: ", field_updates["updated_fields"]) - self.force_save_fields([field.name for field in self._dirty_fields.keys()]) - self.runtime.save_block(self) - finally: - self._dirty_fields.clear() - print("Ending handler...") - - -class XBlock2(XBlock2Mixin, XBlock): - """ - Base class for pure "v2" XBlocks, that don't need backwards compatibility with v1 - """ - class XBlockAside(Plugin, Blocklike): """