fix: move BlockKey and derived_key to avoid cyclical import

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.

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.block_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)

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')

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)

xmodule/modulestore/store_utilities.py

@@ -1,13 +1,11 @@
 # lint-amnesty, pylint: disable=missing-module-docstring
-import hashlib
 import logging
 import re
 import uuid
 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)

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)

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

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)