From e064bf9600d26aca9b03fa6ea0255aaac8883c4b Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Fri, 12 Jan 2024 16:38:56 -0500 Subject: [PATCH] fix: move BlockKey and derived_key to avoid cyclical import 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. --- mypy.ini | 3 +- .../djangoapps/content_libraries/tasks.py | 17 ++--- xmodule/modulestore/split_mongo/__init__.py | 18 +----- xmodule/modulestore/split_mongo/split.py | 7 ++- xmodule/modulestore/store_utilities.py | 30 --------- .../modulestore/tests/test_store_utilities.py | 47 +------------- xmodule/tests/test_util_keys.py | 45 ++++++++++++++ xmodule/util/keys.py | 62 +++++++++++++++++++ 8 files changed, 123 insertions(+), 106 deletions(-) create mode 100644 xmodule/tests/test_util_keys.py create mode 100644 xmodule/util/keys.py diff --git a/mypy.ini b/mypy.ini index 4d2f566fa311..5027c3bb5595 100644 --- a/mypy.ini +++ b/mypy.ini @@ -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. diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 13a6a32a1987..de21d1e96249 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -47,7 +47,7 @@ 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 generate_block_key(source_key, 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 generate_block_key(source_key, 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) diff --git a/xmodule/modulestore/split_mongo/__init__.py b/xmodule/modulestore/split_mongo/__init__.py index d7664b6faac9..8ebc86efacc7 100644 --- a/xmodule/modulestore/split_mongo/__init__.py +++ b/xmodule/modulestore/split_mongo/__init__.py @@ -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') diff --git a/xmodule/modulestore/split_mongo/split.py b/xmodule/modulestore/split_mongo/split.py index c6e4c7889adf..64e19420a152 100644 --- a/xmodule/modulestore/split_mongo/split.py +++ b/xmodule/modulestore/split_mongo/split.py @@ -99,11 +99,12 @@ 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 @@ def _copy_from_template( 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) diff --git a/xmodule/modulestore/store_utilities.py b/xmodule/modulestore/store_utilities.py index e9710fbc92ce..c177104be5ad 100644 --- a/xmodule/modulestore/store_utilities.py +++ b/xmodule/modulestore/store_utilities.py @@ -1,5 +1,4 @@ # lint-amnesty, pylint: disable=missing-module-docstring -import hashlib import logging import re import uuid @@ -7,7 +6,6 @@ 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) diff --git a/xmodule/modulestore/tests/test_store_utilities.py b/xmodule/modulestore/tests/test_store_utilities.py index a0b5cfcbdbb6..57908abc7f59 100644 --- a/xmodule/modulestore/tests/test_store_utilities.py +++ b/xmodule/modulestore/tests/test_store_utilities.py @@ -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 @@ def test_get_draft_subtree_roots(self, node_arguments_list, expected_roots_urls) 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) diff --git a/xmodule/tests/test_util_keys.py b/xmodule/tests/test_util_keys.py new file mode 100644 index 000000000000..dcd16d6b7873 --- /dev/null +++ b/xmodule/tests/test_util_keys.py @@ -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 diff --git a/xmodule/util/keys.py b/xmodule/util/keys.py new file mode 100644 index 000000000000..ceb35b269eb5 --- /dev/null +++ b/xmodule/util/keys.py @@ -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)