From d4e5e837a4e8ec4f2ee019b8ca3476e6ab3136e7 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Fri, 3 Jan 2025 09:39:33 -0500 Subject: [PATCH] feat!: Assume that scope IDs are opaque key objects BREAKING CHANGE: Raise an exception when scope IDs are missing or are not the expected types. In particular, definition IDs must be DefinitionKey instances and usage IDs must be UsageKey instances. This has been effectively true within edx-platform (the lone production client of the XBlock library) for a long time, but explictly enforcing it will now allow us to add strong type annotations to XBlock in an upcoming release. Bump version to 6.0.0. Closes: https://github.com/openedx/XBlock/issues/708 --- CHANGELOG.rst | 10 +++ xblock/__init__.py | 2 +- xblock/core.py | 15 ++--- xblock/fields.py | 26 +++++++- xblock/test/test_field_data.py | 4 +- xblock/test/test_fields.py | 28 ++++---- xblock/test/tools.py | 115 +++++++++++++++++++++++++++++++++ 7 files changed, 175 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2b6a3b0f4..4822212c7 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,16 @@ Change history for XBlock Unreleased ---------- +6.0.0 - 2026-01-20 +------------------ + +* Raise an exception when scope IDs are missing or are not the expected types. In + particular, definition IDs must be DefinitionKey instances and usage IDs must be + UsageKey instances. This has been effectively true within edx-platform (the lone + production client of the XBlock library) for a long time, but explictly + enforcing it will now allow us to add strong type annotations to XBlock in an + upcoming release. + 5.3.0 - 2025-12-19 ------------------ diff --git a/xblock/__init__.py b/xblock/__init__.py index 779a96e05..8f666bd02 100644 --- a/xblock/__init__.py +++ b/xblock/__init__.py @@ -2,4 +2,4 @@ XBlock Courseware Components """ -__version__ = '5.3.0' +__version__ = '6.0.0' diff --git a/xblock/core.py b/xblock/core.py index 7ff362475..171aabcfe 100644 --- a/xblock/core.py +++ b/xblock/core.py @@ -20,7 +20,7 @@ KeyValueMultiSaveError, XBlockSaveError, ) -from xblock.fields import Field, List, Reference, ReferenceList, Scope, String +from xblock.fields import Field, List, Reference, ReferenceList, Scope, String, ScopeIds from xblock.internal import class_lazy from xblock.plugin import Plugin from xblock.validation import Validation @@ -393,6 +393,9 @@ def __init__(self, scope_ids, field_data=None, *, runtime, **kwargs): self._field_data_cache = {} self._dirty_fields = {} + if not isinstance(scope_ids, ScopeIds): + raise TypeError(f"got {scope_ids=}; should be a ScopeIds instance") + scope_ids.validate_types() self.scope_ids = scope_ids super().__init__(**kwargs) @@ -780,9 +783,8 @@ def __init__( self, runtime, field_data=None, - scope_ids=UNSET, - *args, # pylint: disable=keyword-arg-before-vararg - **kwargs + scope_ids=None, + **kwargs, ): """ Arguments: @@ -797,9 +799,6 @@ def __init__( scope_ids (:class:`.ScopeIds`): Identifiers needed to resolve scopes. """ - if scope_ids is UNSET: - raise TypeError('scope_ids are required') - # A cache of the parent block, retrieved from .parent self._parent_block = None self._parent_block_id = None @@ -811,7 +810,7 @@ def __init__( self._parent_block_id = for_parent.scope_ids.usage_id # Provide backwards compatibility for external access through _field_data - super().__init__(runtime=runtime, scope_ids=scope_ids, field_data=field_data, *args, **kwargs) + super().__init__(runtime=runtime, scope_ids=scope_ids, field_data=field_data, **kwargs) def render(self, view, context=None): """Render `view` with this block's runtime and the supplied `context`""" diff --git a/xblock/fields.py b/xblock/fields.py index 6ad3ca1bb..951794d3d 100644 --- a/xblock/fields.py +++ b/xblock/fields.py @@ -3,8 +3,9 @@ **scopes** to associate each field with particular sets of blocks and users. The hosting runtime application decides what actual storage mechanism to use for each scope. - """ +from __future__ import annotations + from collections import namedtuple import copy import datetime @@ -17,6 +18,8 @@ import traceback import warnings +from opaque_keys.edx.keys import UsageKey, DefinitionKey + import dateutil.parser from lxml import etree import pytz @@ -250,6 +253,27 @@ class ScopeIds(namedtuple('ScopeIds', 'user_id block_type def_id usage_id')): """ __slots__ = () + def validate_types(self): + """ + Raise an AssertionError if any of the ids are an unexpected type. + + Originally, these fields were all freely-typed; but in practice, + edx-platform's XBlock runtime would fail if the ids did not match the + types below. In order to make the XBlock library reflect the + edx-platform reality and improve type-safety, we've decided to actually + enforce the types here, per: + https://github.com/openedx/XBlock/issues/708 + """ + if self.user_id is not None: + if not isinstance(self.user_id, (int, str)): + raise TypeError(f"got {self.user_id=}; should be an int, str, or None") + if not isinstance(self.block_type, str): + raise TypeError(f"got {self.block_type=}; should be a str") + if not isinstance(self.def_id, DefinitionKey): + raise TypeError(f"got {self.def_id=}; should be a DefinitionKey") + if not isinstance(self.usage_id, UsageKey): + raise TypeError(f"got {self.usage_id=}; should be a UsageKey") + # Define special reference that can be used as a field's default in field # definition to signal that the field should default to a unique string value diff --git a/xblock/test/test_field_data.py b/xblock/test/test_field_data.py index eaf12281e..a4952f7e1 100644 --- a/xblock/test/test_field_data.py +++ b/xblock/test/test_field_data.py @@ -8,7 +8,7 @@ from xblock.exceptions import InvalidScopeError from xblock.fields import Scope, String from xblock.field_data import SplitFieldData, ReadOnlyFieldData -from xblock.test.tools import TestRuntime +from xblock.test.tools import TestRuntime, make_scope_ids_for_testing class TestingBlock(XBlock): @@ -48,7 +48,7 @@ def setup_method(self): self.runtime = TestRuntime(services={'field-data': self.split}) self.block = TestingBlock( runtime=self.runtime, - scope_ids=Mock(), + scope_ids=make_scope_ids_for_testing(), ) # pylint: enable=attribute-defined-outside-init diff --git a/xblock/test/test_fields.py b/xblock/test/test_fields.py index 1941e7410..630cb9f71 100644 --- a/xblock/test/test_fields.py +++ b/xblock/test/test_fields.py @@ -22,7 +22,7 @@ ScopeIds, Sentinel, UNIQUE_ID, scope_key, Date, Timedelta, RelativeTime, ScoreField, ListScoreField ) from xblock.scorable import Score -from xblock.test.tools import TestRuntime +from xblock.test.tools import TestRuntime, make_scope_ids_for_testing class FieldTest(unittest.TestCase): @@ -41,7 +41,7 @@ class TestBlock(XBlock): field_x = self.FIELD_TO_TEST(enforce_type=enforce_type) runtime = TestRuntime(services={'field-data': DictFieldData({})}) - return TestBlock(runtime, scope_ids=Mock(spec=ScopeIds)) + return TestBlock(runtime, scope_ids=make_scope_ids_for_testing()) def set_and_get_field(self, arg, enforce_type): """ @@ -717,10 +717,11 @@ class TestBlock(XBlock): pref_lst = List(scope=Scope.preferences, name='') user_info_lst = List(scope=Scope.user_info, name='') - sids = ScopeIds(user_id="_bob", - block_type="b.12#ob", - def_id="..", - usage_id="..") + sids = make_scope_ids_for_testing( + user_id="_bob", + block_type="b.12#ob", + block_id="..", + ) field_data = DictFieldData({}) @@ -763,10 +764,11 @@ class TestBlock(XBlock): field_a = String(default=UNIQUE_ID, scope=Scope.settings) field_b = String(default=UNIQUE_ID, scope=Scope.user_state) - sids = ScopeIds(user_id="bob", - block_type="bobs-type", - def_id="definition-id", - usage_id="usage-id") + sids = make_scope_ids_for_testing( + user_id="bob", + block_type="bobs-type", + block_id="usage-id", + ) runtime = TestRuntime(services={'field-data': DictFieldData({})}) block = TestBlock(runtime, DictFieldData({}), sids) @@ -828,7 +830,7 @@ class FieldTester(XBlock): not_timezone_aware = dt.datetime(2015, 1, 1) timezone_aware = dt.datetime(2015, 1, 1, tzinfo=pytz.UTC) runtime = TestRuntime(services={'field-data': DictFieldData({})}) - field_tester = FieldTester(runtime, scope_ids=Mock(spec=ScopeIds)) + field_tester = FieldTester(runtime, scope_ids=make_scope_ids_for_testing()) field_tester.incomparable = not_timezone_aware field_tester.incomparable = timezone_aware assert field_tester.incomparable == timezone_aware @@ -853,7 +855,7 @@ class FieldTester(XBlock): original_json = "YYY" runtime = TestRuntime(services={'field-data': DictFieldData({'how_many': original_json})}) - field_tester = FieldTester(runtime, scope_ids=Mock(spec=ScopeIds)) + field_tester = FieldTester(runtime, scope_ids=make_scope_ids_for_testing()) # Test that the native value isn't equal to the original json we specified. assert field_tester.how_many != original_json @@ -879,7 +881,7 @@ class FieldTester(XBlock): dict_field = Dict(scope=Scope.settings) runtime = TestRuntime(services={'field-data': DictFieldData({})}) - field_tester = FieldTester(runtime, scope_ids=Mock(spec=ScopeIds)) + field_tester = FieldTester(runtime, scope_ids=make_scope_ids_for_testing()) # precondition checks assert len(field_tester._dirty_fields) == 0 diff --git a/xblock/test/tools.py b/xblock/test/tools.py index 98a6e2a6e..4fbf02b5c 100644 --- a/xblock/test/tools.py +++ b/xblock/test/tools.py @@ -2,12 +2,127 @@ Tools for testing XBlocks """ from contextlib import contextmanager +from opaque_keys.edx.keys import UsageKeyV2, LearningContextKey, DefinitionKey from functools import partial +from xblock.fields import ScopeIds import warnings from xblock.runtime import Runtime, MemoryIdManager +def make_scope_ids_for_testing( + user_id=99, + context_slug="myContext", + block_type="myType", + block_id="myId", +): + """ + Make an instance of ScopeIds suitable for testing XBlock. + Any or all parameters can be omitted. + """ + return ScopeIds( + user_id=user_id, + block_type=block_type, + def_id=TestDefinitionKey(block_type, block_id), + usage_id=TestUsageKey(TestContextKey(context_slug), block_type, block_id), + ) + + +class TestDefinitionKey(DefinitionKey): + """ + A simple definition key type for testing XBlock + + When serialized, these keys look like: + td:myType.myId + """ + CANONICAL_NAMESPACE = 'td' # "Test Definition" + KEY_FIELDS = ('block_type', 'block_id') + block_type: str + block_id: str + __slots__ = KEY_FIELDS + CHECKED_INIT = False + + def __init__(self, block_type: str, block_id: str): + super().__init__(block_type=block_type, block_id=block_id) + + def _to_string(self) -> str: + """ + Serialize this key as a string + """ + return f"{self.block_type}.{self.block_id}" + + @classmethod + def _from_string(cls, serialized: str): + """ + Instantiate this key from a serialized string + """ + (block_type, block_id) = serialized.split('.') + return cls(block_type, block_id) + + +class TestContextKey(LearningContextKey): + """ + A simple context key type for testing XBlock + + When serialized, these keys look like: + tc:myContext + """ + CANONICAL_NAMESPACE = 'tc' # "Test Context" + KEY_FIELDS = ('slug',) + slug: str + __slots__ = KEY_FIELDS + CHECKED_INIT = False + + def __init__(self, slug: str): + super().__init__(slug=slug) + + def _to_string(self) -> str: + """ + Serialize this key as a string + """ + return self.slug + + @classmethod + def _from_string(cls, serialized: str): + """ + Instantiate this key from a serialized string + """ + return cls(serialized) + + +class TestUsageKey(UsageKeyV2): + """ + A simple usage key type for testing XBlock + + When serialized, these keys look like: + tu:myContext.myType.myId + """ + CANONICAL_NAMESPACE = 'tu' # "Test Usage" + KEY_FIELDS = ('context_key', 'block_type', 'block_id') + context_key: TestContextKey + block_type: str + block_id: str + __slots__ = KEY_FIELDS + CHECKED_INIT = False + + def __init__(self, context_key: TestContextKey, block_type: str, block_id: str): + super().__init__(context_key=context_key, block_type=block_type, block_id=block_id) + + def _to_string(self) -> str: + """ + Serialize this key as a string + """ + return ".".join((self.context_key.slug, self.block_type, self.block_id)) + + @classmethod + def _from_string(cls, serialized: str): + """ + Instantiate this key from a serialized string + """ + (context_slug, block_type, block_id) = serialized.split('.') + return cls(TestContextKey(context_slug), block_type, block_id) + + def blocks_are_equivalent(block1, block2): """Compare two blocks for equivalence. """