Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #22320 from open-craft/fix-blockstore-field-bug

Browse Source 
Improve handling of 'children' field in Blockstore XBlock runtime
This commit is contained in:
David Ormsbee
2020-01-10 14:32:04 -05:00
committed by GitHub
parent d89a1b872d ac39692a8a
commit d0c764b4d8
5 changed files with 145 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

38
openedx/core/djangoapps/content_libraries/tests/test_runtime.py
View File

@@ -20,7 +20,7 @@ from openedx.core.djangoapps.content_libraries.tests.base import (
)
from openedx.core.djangoapps.content_libraries.tests.user_state_block import UserStateTestBlock
from openedx.core.djangoapps.xblock import api as xblock_api
from openedx.core.djangolib.testing.utils import skip_unless_lms
from openedx.core.djangolib.testing.utils import skip_unless_lms, skip_unless_cms
from openedx.core.lib import blockstore_api
from student.tests.factories import UserFactory
from xmodule.unit_block import UnitBlock
@@ -62,13 +62,45 @@ class ContentLibraryRuntimeTest(ContentLibraryContentTestMixin, TestCase):
content library.
"""
@skip_unless_cms # creating child blocks only works properly in Studio
def test_identical_olx(self):
"""
Test library blocks with children that also have identical OLX. Since
the blockstore runtime caches authored field data based on the hash of
the OLX, this can catch some potential bugs, especially given that the
"children" field stores usage IDs, not definition IDs.
"""
# Create a unit containing a <problem>
unit_block_key = library_api.create_library_block(self.library.key, "unit", "u1").usage_key
library_api.create_library_block_child(unit_block_key, "problem", "p1")
library_api.publish_changes(self.library.key)
# Now do the same in a different library:
library2 = library_api.create_library(
collection_uuid=self.collection.uuid,
org=self.organization,
slug="idolx",
title=("Identical OLX Test Lib 2"),
description="",
)
unit_block2_key = library_api.create_library_block(library2.key, "unit", "u1").usage_key
library_api.create_library_block_child(unit_block2_key, "problem", "p1")
library_api.publish_changes(library2.key)
# Load both blocks:
unit_block = xblock_api.load_block(unit_block_key, self.student_a)
unit_block2 = xblock_api.load_block(unit_block2_key, self.student_a)
self.assertEqual(
library_api.get_library_block_olx(unit_block_key),
library_api.get_library_block_olx(unit_block2_key),
)
self.assertNotEqual(unit_block.children, unit_block2.children)
def test_has_score(self):
"""
Test that the LMS-specific 'has_score' attribute is getting added to
blocks.
"""
unit_block_key = library_api.create_library_block(self.library.key, "unit", "u1").usage_key
problem_block_key = library_api.create_library_block(self.library.key, "problem", "p1").usage_key
unit_block_key = library_api.create_library_block(self.library.key, "unit", "score-unit1").usage_key
problem_block_key = library_api.create_library_block(self.library.key, "problem", "score-prob1").usage_key
library_api.publish_changes(self.library.key)
unit_block = xblock_api.load_block(unit_block_key, self.student_a)
problem_block = xblock_api.load_block(problem_block_key, self.student_a)

2
openedx/core/djangoapps/xblock/apps.py
View File

@@ -5,10 +5,8 @@ Django app configuration for the XBlock Runtime django app
from django.apps import AppConfig, apps
from django.conf import settings
from xblock.runtime import DictKeyValueStore, KvsFieldData
from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
from openedx.core.djangoapps.xblock.runtime.blockstore_field_data import BlockstoreFieldData
class XBlockAppConfig(AppConfig):

109
openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py
View File

@@ -10,6 +10,7 @@ from xblock.exceptions import InvalidScopeError, NoSuchDefinition
from xblock.fields import Field, BlockScope, Scope, UserScope, Sentinel
from xblock.field_data import FieldData
from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl
from openedx.core.djangolib.blockstore_cache import (
get_bundle_version_files_cached,
get_bundle_draft_files_cached,
@@ -20,6 +21,7 @@ log = logging.getLogger(__name__)
ActiveBlock = namedtuple('ActiveBlock', ['olx_hash', 'changed_fields'])
DELETED = Sentinel('DELETED') # Special value indicating a field was reset to its default value
CHILDREN_INCLUDES = Sentinel('CHILDREN_INCLUDES') # Key for a pseudo-field that stores the XBlock's children info
MAX_DEFINITIONS_LOADED = 100 # How many of the most recently used XBlocks' field data to keep in memory at max.
@@ -120,14 +122,13 @@ class BlockstoreFieldData(FieldData):
Given a block and the name of one of its fields, check that we will be
able to read/write it.
"""
if name == CHILDREN_INCLUDES:
return # This is a pseudo-field used in conjunction with BlockstoreChildrenData
field = self._getfield(block, name)
if field.scope == Scope.children:
if name != 'children':
raise InvalidScopeError("Expect Scope.children only for field named 'children', not '{}'".format(name))
elif field.scope == Scope.parent:
# This field data store is focused on definition-level field data, and parent is mostly
# relevant at the usage level. Luckily this doesn't even seem to be used?
raise NotImplementedError("Setting Scope.parent is not supported by BlockstoreFieldData.")
if field.scope in (Scope.children, Scope.parent):
# This field data store is focused on definition-level field data, and children/parent is mostly
# relevant at the usage level. Scope.parent doesn't even seem to be used?
raise NotImplementedError("Setting Scope.children/parent is not supported by BlockstoreFieldData.")
else:
if field.scope.user != UserScope.NONE:
raise InvalidScopeError("BlockstoreFieldData only supports UserScope.NONE fields")
@@ -172,7 +173,7 @@ class BlockstoreFieldData(FieldData):
try:
saved_fields = self.loaded_definitions[entry.olx_hash]
except KeyError:
if name == 'children':
if name == CHILDREN_INCLUDES:
# Special case: parse_xml calls add_node_as_child which calls 'block.children.append()'
# BEFORE parse_xml is done, and .append() needs to read the value of children. So
return [] # start with an empty list, it will get filled in.
@@ -255,3 +256,95 @@ class BlockstoreFieldData(FieldData):
# we have only half as many as MAX_DEFINITIONS_LOADED in memory, if possible.
while olx_hashes_safe_to_delete and (len(self.loaded_definitions) > MAX_DEFINITIONS_LOADED / 2):
del self.loaded_definitions[olx_hashes_safe_to_delete.pop()]
class BlockstoreChildrenData(FieldData):
"""
An XBlock FieldData implementation that reads 'children' data out of
the definition fields in BlockstoreFieldData.
The children field contains usage keys and so is usage-specific; the
BlockstoreFieldData can only store field data that is not usage-specific. So
we store data about the <xblock-include /> elements that define the children
in BlockstoreFieldData (since that is not usage-specific), and this field
data implementation loads that <xblock-include /> data and transforms it
into the usage keys that comprise the standard .children field.
"""
def __init__(self, blockstore_field_data):
"""
Initialize this BlockstoreChildrenData instance.
"""
# The data store that holds Scope.usage and Scope.definition data:
self.authored_data_store = blockstore_field_data
super(BlockstoreChildrenData, self).__init__()
def _check_field(self, block, name): # pylint: disable=unused-argument
"""
Given a block and the name of one of its fields, check that we will be
able to read/write it.
"""
if name != 'children':
raise InvalidScopeError("BlockstoreChildrenData can only read/write from a field named 'children'")
def get(self, block, name):
"""
Get the "children' field value.
We do this by reading the parsed <xblock-include /> values from
the regular authored data store and then transforming them to usage IDs.
"""
self._check_field(block, name)
children_includes = self.get_includes(block)
if not children_includes:
return []
# Now the .children field is required to be a list of usage IDs:
learning_context = get_learning_context_impl(block.scope_ids.usage_id)
child_usages = []
for parsed_include in children_includes:
child_usages.append(
learning_context.usage_for_child_include(
block.scope_ids.usage_id, block.scope_ids.def_id, parsed_include,
)
)
return child_usages
def set(self, block, name, value):
"""
Set the value of the field; requires name='children'
"""
self._check_field(block, name)
children_includes = self.authored_data_store.get(block, CHILDREN_INCLUDES)
if len(value) != len(children_includes):
raise RuntimeError(
"This runtime does not allow changing .children directly - use runtime.add_child_include instead."
)
# This is a no-op; the value of 'children' is derived from CHILDREN_INCLUDES
# so we never write to the children field directly. All we do is make sure it
# looks like it's still in sync with CHILDREN_INCLUDES
def get_includes(self, block):
"""
Get the list of <xblock-include /> elements representing this XBlock's
children.
"""
try:
return self.authored_data_store.get(block, CHILDREN_INCLUDES)
except KeyError:
# KeyError raised by an XBlock field data store means "use the
# default value", and the default value for the children field is an
# empty list.
return []
def append_include(self, block, parsed_include):
"""
Append an <xblock-include /> element to this XBlock's list of children
"""
self.authored_data_store.set(block, CHILDREN_INCLUDES, self.get_includes(block) + [parsed_include])
def delete(self, block, name):
"""
Reset the value of the field named `name` to the default
"""
self._check_field(block, name)
self.authored_data_store.set(block, CHILDREN_INCLUDES, [])
self.set(block, name, [])

37
openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py
View File

@@ -85,15 +85,8 @@ class BlockstoreXBlockRuntime(XBlockRuntime):
This runtime API should normally be used via
runtime.get_block() -> block.parse_xml() -> runtime.add_node_as_child
"""
parent_usage = block.scope_ids.usage_id
parent_definition = block.scope_ids.def_id
learning_context = get_learning_context_impl(parent_usage)
parsed_include = parse_xblock_include(node)
usage_key = learning_context.usage_for_child_include(parent_usage, parent_definition, parsed_include)
block.children.append(usage_key)
if parent_definition.draft_name:
# Store the <xblock-include /> data which we'll need later if saving changes to this block
self.child_includes_of(block).append(parsed_include)
self.add_child_include(block, parsed_include)
def add_child_include(self, block, parsed_include):
"""
@@ -103,33 +96,15 @@ class BlockstoreXBlockRuntime(XBlockRuntime):
modify block.children to append a usage ID, since that doesn't provide
enough information to serialize the block's <xblock-include /> elements.
"""
learning_context = get_learning_context_impl(block.scope_ids.usage_id)
child_usage_key = learning_context.usage_for_child_include(
block.scope_ids.usage_id, block.scope_ids.def_id, parsed_include,
)
block.children.append(child_usage_key)
self.child_includes_of(block).append(parsed_include)
self.system.children_data_store.append_include(block, parsed_include)
block.children = self.system.children_data_store.get(block, 'children')
def child_includes_of(self, block):
"""
Get the (mutable) list of <xblock-include /> directives that define the
children of this block's definition.
Get the list of <xblock-include /> directives that define the children
of this block's definition.
"""
# A hack: when serializing an XBlock, we need to re-create the <xblock-include definition="..." usage="..." />
# elements that were in its original XML. But doing so requires the usage="..." hint attribute, which is
# technically part of the parent definition but which is not otherwise stored anywhere; we only have the derived
# usage_key, but asking the learning context to transform the usage_key back to the usage="..." hint attribute
# is non-trivial and could lead to bugs, because it could happen differently if the same parent definition is
# used in a library compared to a course (each would have different usage keys for the same usage hint).
# So, if this is a draft XBlock (we are editing it), we store the actual parsed <xblock-includes> so we can
# re-use them exactly when serializing this block back to XML.
# This implies that changes to an XBlock's children cannot be made by manipulating the .children field and
# then calling save().
assert block.scope_ids.def_id.draft_name, "Manipulating includes is only relevant for draft XBlocks."
attr_name = "children_includes_{}".format(id(block)) # Force use of this accessor method
if not hasattr(block, attr_name):
setattr(block, attr_name, [])
return getattr(block, attr_name)
return self.system.children_data_store.get_includes(block)
def save_block(self, block):
"""

5
openedx/core/djangoapps/xblock/runtime/runtime.py
View File

@@ -24,7 +24,7 @@ from web_fragments.fragment import Fragment
from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataCache
from lms.djangoapps.grades.api import signals as grades_signals
from openedx.core.djangoapps.xblock.apps import get_xblock_app_config
from openedx.core.djangoapps.xblock.runtime.blockstore_field_data import BlockstoreFieldData
from openedx.core.djangoapps.xblock.runtime.blockstore_field_data import BlockstoreFieldData, BlockstoreChildrenData
from openedx.core.djangoapps.xblock.runtime.ephemeral_field_data import EphemeralKeyValueStore
from openedx.core.djangoapps.xblock.runtime.mixin import LmsBlockMixin
from openedx.core.djangoapps.xblock.utils import get_xblock_id_for_anonymous_user
@@ -269,7 +269,7 @@ class XBlockRuntime(RuntimeShim, Runtime):
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.authored_data_store,
Scope.children: self.system.children_data_store,
Scope.user_state_summary: student_data_store,
Scope.user_state: student_data_store,
Scope.user_info: student_data_store,
@@ -395,6 +395,7 @@ class XBlockRuntimeSystem(object):
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 = BlockstoreFieldData()
self.children_data_store = BlockstoreChildrenData(self.authored_data_store)
assert student_data_mode in (self.STUDENT_DATA_EPHEMERAL, self.STUDENT_DATA_PERSISTED)
self.student_data_mode = student_data_mode
self._error_trackers = {}