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

fix: move BlockKey and derived_key to avoid cyclical import

Browse Source 
After we merged this PR: https://github.com/openedx/edx-platform/pull/33920
this error began popping up in logs:

    Unable to load XBlock 'staffgradedxblock'
    ....
    ImportError: cannot import name 'get_course_blocks' from
    partially initialized module 'lms.djangoapps.course_blocks.api'
    (most likely due to a circular import) ...

The root cause was the new imports of `derived_key` and `BlockKey` into
xmodule/library_content_block.py. Those new imports come from
xmodule/modulestore/store_utilities.py, which runs
`XBlock.load_classes()` at the module level, which fails because we are
still in the process of loading xmodule/library_content_block.

As a solution, we move both `derived_key` and `BlockKey` to
xmodule/util/keys.py. We could potentially move that file to opaque-keys
eventually, depending on how well we think that those concepts generalize.

Also:

* We rename the function from derived_key to derive_key, as
  functions should be verbs.
* We combine the first to parameters of derive_key (a source ContextKey
  and a source BlockKey) into a single parameter (a source UsageKey). In
  my opinion, this makes the function call easier to understand.
This commit is contained in:
Kyle D. McCormick
2024-01-12 16:38:56 -05:00
committed by Kyle McCormick
parent d772ed1519
commit e8b60aef60
8 changed files with 123 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
mypy.ini
View File

@@ -11,7 +11,8 @@ files =
openedx/core/djangoapps/content_libraries,
openedx/core/djangoapps/xblock,
openedx/core/types,
openedx/core/djangoapps/content_tagging
openedx/core/djangoapps/content_tagging,
xmodule/util/keys.py
[mypy.plugins.django-stubs]
# content_staging only works with CMS; others work with either, so we run mypy with CMS settings.

17
openedx/core/djangoapps/content_libraries/tasks.py
View File

@@ -47,7 +47,7 @@ from xmodule.modulestore.django import modulestore
from xmodule.modulestore.exceptions import ItemNotFoundError
from xmodule.modulestore.mixed import MixedModuleStore
from xmodule.modulestore.split_mongo import BlockKey
from xmodule.modulestore.store_utilities import derived_key
from xmodule.util.keys import derive_key
from . import api
from .models import ContentLibraryBlockImportTask
@@ -92,7 +92,7 @@ def _import_block(store, user_id, source_block, dest_parent_key):
"""
Deterministically generate an ID for the new block and return the key.
Keys are generated such that they appear identical to a v1 library with
the same input block_id, library name, library organization, and parent block using derived_key
the same input block_id, library name, library organization, and parent block using derive_key
"""
if not isinstance(source_key.lib_key, LibraryLocatorV2):
raise TypeError(f"Expected source library key of type LibraryLocatorV2, got {source_key.lib_key} instead.")
@@ -101,16 +101,11 @@ def _import_block(store, user_id, source_block, dest_parent_key):
library=source_key.lib_key.slug,
branch='library'
)
source_key_usage_id_as_block_key = BlockKey(
type=source_key.block_type,
id=source_key.usage_id,
derived_block_key = derive_key(
source=source_key_as_v1_course_key.make_usage_key(source_key.block_type, source_key.usage_id),
dest_parent=BlockKey(dest_parent_key.block_type, dest_parent_key.block_id),
)
block_id = derived_key(
source_key_as_v1_course_key,
source_key_usage_id_as_block_key,
BlockKey(dest_parent_key.block_type, dest_parent_key.block_id)
)
return dest_parent_key.context_key.make_usage_key(source_key.block_type, block_id.id)
return dest_parent_key.context_key.make_usage_key(*derived_block_key)
source_key = source_block.scope_ids.usage_id
new_block_key = generate_block_key(source_key, dest_parent_key)

18
xmodule/modulestore/split_mongo/__init__.py
View File

@@ -1,22 +1,10 @@
"""
General utilities
"""
from collections import namedtuple
from opaque_keys.edx.locator import BlockUsageLocator
class BlockKey(namedtuple('BlockKey', 'type id')): # lint-amnesty, pylint: disable=missing-class-docstring
__slots__ = ()
def __new__(cls, type, id): # lint-amnesty, pylint: disable=redefined-builtin
return super().__new__(cls, type, id)
@classmethod
def from_usage_key(cls, usage_key):
return cls(usage_key.block_type, usage_key.block_id)
# We import BlockKey here for backwards compatibility with modulestore code.
# Feel free to remove this and fix the imports if you have time.
from xmodule.util.keys import BlockKey
CourseEnvelope = namedtuple('CourseEnvelope', 'course_key structure')

7
xmodule/modulestore/split_mongo/split.py
View File

@@ -99,11 +99,12 @@ from xmodule.modulestore.exceptions import (
MultipleLibraryBlocksFound,
VersionConflictError
)
from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope
from xmodule.modulestore.split_mongo import CourseEnvelope
from xmodule.modulestore.split_mongo.mongo_connection import DuplicateKeyError, DjangoFlexPersistenceBackend
from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES, derived_key
from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES
from xmodule.partitions.partitions_service import PartitionService
from xmodule.util.misc import get_library_or_course_attribute
from xmodule.util.keys import BlockKey, derive_key
from ..exceptions import ItemNotFoundError
from .caching_descriptor_system import CachingDescriptorSystem
@@ -2452,7 +2453,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
raise ItemNotFoundError(usage_key)
source_block_info = source_structure['blocks'][block_key]
new_block_key = derived_key(src_course_key, block_key, new_parent_block_key)
new_block_key = derive_key(usage_key, new_parent_block_key)
# Now clone block_key to new_block_key:
new_block_info = copy.deepcopy(source_block_info)

30
xmodule/modulestore/store_utilities.py
View File

@@ -1,5 +1,4 @@
# lint-amnesty, pylint: disable=missing-module-docstring
import hashlib
import logging
import re
import uuid
@@ -7,7 +6,6 @@ from collections import namedtuple
from xblock.core import XBlock
from xmodule.modulestore.split_mongo import BlockKey
DETACHED_XBLOCK_TYPES = {name for name, __ in XBlock.load_tagged_classes("detached")}
@@ -106,31 +104,3 @@ def get_draft_subtree_roots(draft_nodes):
for draft_node in draft_nodes:
if draft_node.parent_url not in urls:
yield draft_node
def derived_key(courselike_source_key, block_key, dest_parent: BlockKey):
"""
Return a new reproducible block ID for a given root, source block, and destination parent.
When recursively copying a block structure, we need to generate new block IDs for the
blocks. We don't want to use the exact same IDs as we might copy blocks multiple times.
However, we do want to be able to reproduce the same IDs when copying the same block
so that if we ever need to re-copy the block from its source (that is, to update it with
upstream changes) we don't affect any data tied to the ID, such as grades.
This is used by the copy_from_template function of the modulestore, and can be used by
pluggable django apps that need to copy blocks from one course to another in an
idempotent way. In the future, this should be created into a proper API function
in the spirit of OEP-49.
"""
hashable_source_id = courselike_source_key.for_version(None)
# Compute a new block ID. This new block ID must be consistent when this
# method is called with the same (source_key, dest_structure) pair
unique_data = "{}:{}:{}".format(
str(hashable_source_id).encode("utf-8"),
block_key.id,
dest_parent.id,
)
new_block_id = hashlib.sha1(unique_data.encode('utf-8')).hexdigest()[:20]
return BlockKey(block_key.type, new_block_id)

47
xmodule/modulestore/tests/test_store_utilities.py
View File

@@ -2,17 +2,12 @@
Tests for store_utilities.py
"""
import unittest
from unittest import TestCase
from unittest.mock import Mock
import ddt
from opaque_keys.edx.keys import CourseKey
from xmodule.modulestore.split_mongo import BlockKey
from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots, derived_key
from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots
@ddt.ddt
@@ -86,43 +81,3 @@ class TestUtils(unittest.TestCase):
subtree_roots_urls = [root.url for root in get_draft_subtree_roots(block_nodes)]
# check that we return the expected urls
assert set(subtree_roots_urls) == set(expected_roots_urls)
mock_block = Mock()
mock_block.id = CourseKey.from_string('course-v1:Beeper+B33P+BOOP')
derived_key_scenarios = [
{
'courselike_source_key': CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'),
'block_key': BlockKey('chapter', 'interactive_demonstrations'),
'parent': mock_block,
'expected': BlockKey(
'chapter', '5793ec64e25ed870a7dd',
),
},
{
'courselike_source_key': CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'),
'block_key': BlockKey('chapter', 'interactive_demonstrations'),
'parent': BlockKey(
'chapter', 'thingy',
),
'expected': BlockKey(
'chapter', '599792a5622d85aa41e6',
),
}
]
@ddt.ddt
class TestDerivedKey(TestCase):
"""
Test reproducible block ID generation.
"""
@ddt.data(*derived_key_scenarios)
@ddt.unpack
def test_derived_key(self, courselike_source_key, block_key, parent, expected):
"""
Test that derived_key returns the expected value.
"""
self.assertEqual(derived_key(courselike_source_key, block_key, parent), expected)

45
xmodule/tests/test_util_keys.py Normal file
View File

@@ -0,0 +1,45 @@
"""
Tests for xmodule/util/keys.py
"""
import ddt
from unittest import TestCase
from unittest.mock import Mock
from opaque_keys.edx.locator import BlockUsageLocator
from opaque_keys.edx.keys import CourseKey
from xmodule.util.keys import BlockKey, derive_key
mock_block = Mock()
mock_block.id = CourseKey.from_string('course-v1:Beeper+B33P+BOOP')
derived_key_scenarios = [
{
'source': BlockUsageLocator.from_string(
'block-v1:edX+DemoX+Demo_Course+type@chapter+block@interactive_demonstrations'
),
'parent': mock_block,
'expected': BlockKey('chapter', '5793ec64e25ed870a7dd'),
},
{
'source': BlockUsageLocator.from_string(
'block-v1:edX+DemoX+Demo_Course+type@chapter+block@interactive_demonstrations'
),
'parent': BlockKey('chapter', 'thingy'),
'expected': BlockKey('chapter', '599792a5622d85aa41e6'),
}
]
@ddt.ddt
class TestDeriveKey(TestCase):
"""
Test reproducible block ID generation.
"""
@ddt.data(*derived_key_scenarios)
@ddt.unpack
def test_derive_key(self, source, parent, expected):
"""
Test that derive_key returns the expected value.
"""
assert derive_key(source, parent) == expected

62
xmodule/util/keys.py Normal file
View File

@@ -0,0 +1,62 @@
"""
Utilities for working with opaque-keys.
Consider moving these into opaque-keys if they generalize well.
"""
import hashlib
from typing import NamedTuple
from opaque_keys.edx.keys import UsageKey
class BlockKey(NamedTuple):
"""
A pair of strings (type, id) that uniquely identify an XBlock Usage within a LearningContext.
Put another way: LearningContextKey * BlockKey = UsageKey.
Example:
"course-v1:myOrg+myCourse+myRun" <- LearningContextKey string
("html", "myBlock") <- BlockKey
"course-v1:myOrg+myCourse+myRun+type@html+block@myBlock" <- UsageKey string
"""
type: str
id: str
@classmethod
def from_usage_key(cls, usage_key):
return cls(usage_key.block_type, usage_key.block_id)
def derive_key(source: UsageKey, dest_parent: BlockKey) -> BlockKey:
"""
Return a new reproducible BlockKey for a given source usage and destination parent block.
When recursively copying a block structure, we need to generate new block IDs for the
blocks. We don't want to use the exact same IDs as we might copy blocks multiple times.
However, we do want to be able to reproduce the same IDs when copying the same block
so that if we ever need to re-copy the block from its source (that is, to update it with
upstream changes) we don't affect any data tied to the ID, such as grades.
This is used by the copy_from_template function of the modulestore, and can be used by
pluggable django apps that need to copy blocks from one course to another in an
idempotent way. In the future, this should be created into a proper API function
in the spirit of OEP-49.
"""
source_context = source.context_key
if hasattr(source_context, 'for_version'):
source_context = source_context.for_version(None)
# Compute a new block ID. This new block ID must be consistent when this
# method is called with the same (source, dest_parent) pair.
# Note: years after this was written, mypy pointed out that the way we are
# encoding & formatting the source context means it looks like b'....', ie
# it literally contains the character 'b' and single quotes within the unique_data
# string. So that's a little silly, but it's fine, and we can't change it now.
unique_data = "{}:{}:{}".format(
str(source_context).encode("utf-8"), # type: ignore[str-bytes-safe]
source.block_id,
dest_parent.id,
)
new_block_id = hashlib.sha1(unique_data.encode('utf-8')).hexdigest()[:20]
return BlockKey(source.block_type, new_block_id)