-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: move BlockKey and derived_key to avoid cyclical import
After we merged this PR: #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.
- Loading branch information
1 parent
d772ed1
commit e064bf9
Showing
8 changed files
with
123 additions
and
106 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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') |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) |