refactor!: use String field instead of Dict field to store top_level_downstream_parent_key (openedx#37448)

* refactor!: use String field instead of Dict field to store top_level_downstream_parent_key

Since this is a new field no production instance should have this field
yet. Developers need to delete their old courses as this change will
raise error in all course pages.

* chore: add `top_level_parent` field in ComponentLink and ContainerLink admin

* refactor: use ":" as separator

* refactor: block key parsing and tests

Author: navinkarkera

cms/djangoapps/contentstore/admin.py

@@ -100,6 +100,7 @@ class ComponentLinkAdmin(admin.ModelAdmin):
         "upstream_context_key",
         "downstream_usage_key",
         "downstream_context_key",
+        "top_level_parent",
         "version_synced",
         "version_declined",
         "created",
@@ -139,6 +140,7 @@ class ContainerLinkAdmin(admin.ModelAdmin):
         "upstream_context_key",
         "downstream_usage_key",
         "downstream_context_key",
+        "top_level_parent",
         "version_synced",
         "version_declined",
         "created",

cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstream_sync_integration.py

@@ -11,11 +11,10 @@
 from freezegun import freeze_time
 
 from openedx.core.djangoapps.content_libraries.tests import ContentLibrariesRestApiTest
-from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_block_key_dict
+from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_block_key_string
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory
-from xmodule.xml_block import serialize_field
 
 
 @ddt.ddt
@@ -296,9 +295,9 @@ def test_unit_sync(self):
             parent_usage_key=str(self.course_subsection.usage_key),
             upstream_key=self.upstream_unit["id"],
         )
-        downstream_unit_block_key = serialize_field(get_block_key_dict(
+        downstream_unit_block_key = get_block_key_string(
             UsageKey.from_string(downstream_unit["locator"]),
-        )).replace('"', '"')
+        )
         status = self._get_sync_status(downstream_unit["locator"])
         self.assertDictContainsEntries(status, {
             'upstream_ref': self.upstream_unit["id"],  # e.g. 'lct:CL-TEST:testlib:unit:u1'
@@ -898,9 +897,9 @@ def test_unit_sync_with_modified_downstream(self):
             parent_usage_key=str(self.course_subsection.usage_key),
             upstream_key=self.upstream_unit["id"],
         )
-        downstream_unit_block_key = serialize_field(get_block_key_dict(
+        downstream_unit_block_key = get_block_key_string(
             UsageKey.from_string(downstream_unit["locator"]),
-        )).replace('"', '"')
+        )
         status = self._get_sync_status(downstream_unit["locator"])
         self.assertDictContainsEntries(status, {
             'upstream_ref': self.upstream_unit["id"],  # e.g. 'lct:CL-TEST:testlib:unit:u1'
@@ -1259,9 +1258,9 @@ def test_unit_decline_sync(self):
             parent_usage_key=str(self.course_subsection.usage_key),
             upstream_key=self.upstream_unit["id"],
         )
-        downstream_unit_block_key = serialize_field(get_block_key_dict(
+        downstream_unit_block_key = get_block_key_string(
             UsageKey.from_string(downstream_unit["locator"]),
-        )).replace('"', '"')
+        )
         children_downstream_keys = self._get_course_block_children(downstream_unit["locator"])
         downstream_problem1 = children_downstream_keys[1]
         assert "type@problem" in downstream_problem1

cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py

@@ -16,7 +16,7 @@
 from cms.djangoapps.contentstore.helpers import StaticFileNotices
 from cms.djangoapps.contentstore.tests.utils import CourseTestCase
 from cms.djangoapps.contentstore.xblock_storage_handlers import view_handlers as xblock_view_handlers
-from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_block_key_dict
+from cms.djangoapps.contentstore.xblock_storage_handlers.xblock_helpers import get_block_key_string
 from cms.lib.xblock.upstream_sync import BadUpstream, UpstreamLink
 from common.djangoapps.student.auth import add_users
 from common.djangoapps.student.roles import CourseStaffRole
@@ -157,7 +157,7 @@ def setUp(self):
             parent=self.top_level_downstream_unit,
             upstream=self.html_lib_id_2,
             upstream_version=1,
-            top_level_downstream_parent_key=get_block_key_dict(
+            top_level_downstream_parent_key=get_block_key_string(
                 self.top_level_downstream_unit.usage_key,
             )
         ).usage_key
@@ -171,7 +171,7 @@ def setUp(self):
             parent=self.top_level_downstream_chapter,
             upstream=self.top_level_subsection_id,
             upstream_version=1,
-            top_level_downstream_parent_key=get_block_key_dict(
+            top_level_downstream_parent_key=get_block_key_string(
                 self.top_level_downstream_chapter.usage_key,
             ),
         )
@@ -180,7 +180,7 @@ def setUp(self):
             parent=self.top_level_downstream_sequential,
             upstream=self.top_level_unit_id_2,
             upstream_version=1,
-            top_level_downstream_parent_key=get_block_key_dict(
+            top_level_downstream_parent_key=get_block_key_string(
                 self.top_level_downstream_chapter.usage_key,
             ),
         )
@@ -189,7 +189,7 @@ def setUp(self):
             parent=self.top_level_downstream_unit_2,
             upstream=self.video_lib_id_2,
             upstream_version=1,
-            top_level_downstream_parent_key=get_block_key_dict(
+            top_level_downstream_parent_key=get_block_key_string(
                 self.top_level_downstream_chapter.usage_key,
             )
         ).usage_key
@@ -455,17 +455,14 @@ def test_unlink_parent_should_update_children_top_level_parent(self):
 
         unit = modulestore().get_item(self.top_level_downstream_unit_2.usage_key)
         # The sequential is the top-level parent for the unit
-        assert unit.top_level_downstream_parent_key == {
-            "id": str(self.top_level_downstream_sequential.usage_key.block_id),
-            "type": str(self.top_level_downstream_sequential.usage_key.block_type),
-        }
+        sequential_block_key = get_block_key_string(
+            self.top_level_downstream_sequential.usage_key
+        )
+        assert unit.top_level_downstream_parent_key == sequential_block_key
 
         video = modulestore().get_item(self.top_level_downstream_video_key)
         # The sequential is the top-level parent for the video
-        assert video.top_level_downstream_parent_key == {
-            "id": str(self.top_level_downstream_sequential.usage_key.block_id),
-            "type": str(self.top_level_downstream_sequential.usage_key.block_type),
-        }
+        assert video.top_level_downstream_parent_key == sequential_block_key
 
         all_downstreams = self.client.get(
             "/api/contentstore/v2/downstreams/",
@@ -1249,8 +1246,6 @@ def test_200_get_ready_to_sync_top_level_parents_with_components(self):
                 'downstream_is_modified': False,
             },
         ]
-        print(data["results"])
-        print(expected)
         self.assertListEqual(data["results"], expected)
 
     def test_200_get_ready_to_sync_top_level_parents_with_containers(self):

cms/djangoapps/contentstore/tasks.py

@@ -92,6 +92,7 @@
 from xmodule.modulestore.xml_exporter import export_course_to_xml, export_library_to_xml
 from xmodule.modulestore.xml_importer import CourseImportException, import_course_from_xml, import_library_from_xml
 from xmodule.tabs import StaticTab
+from xmodule.util.keys import BlockKey
 
 from .models import ComponentLink, ContainerLink, LearningContextLinksStatus, LearningContextLinksStatusChoices
 from .outlines import update_outline_from_modulestore
@@ -1649,10 +1650,11 @@ def handle_create_xblock_upstream_link(usage_key):
     if not xblock.upstream or not xblock.upstream_version:
         return
     if xblock.top_level_downstream_parent_key is not None:
+        block_key = BlockKey.from_string(xblock.top_level_downstream_parent_key)
         top_level_parent_usage_key = BlockUsageLocator(
             xblock.course_id,
-            xblock.top_level_downstream_parent_key.get('type'),
-            xblock.top_level_downstream_parent_key.get('id'),
+            block_key.type,
+            block_key.id,
         )
         try:
             ContainerLink.get_by_downstream_usage_key(top_level_parent_usage_key)

cms/djangoapps/contentstore/utils.py

@@ -117,6 +117,7 @@
     get_all_partitions_for_course,  # lint-amnesty, pylint: disable=wrong-import-order
 )
 from xmodule.services import ConfigurationService, SettingsService, TeamsConfigurationService
+from xmodule.util.keys import BlockKey
 
 from .models import ComponentLink, ContainerLink
 
@@ -2411,10 +2412,11 @@ def _create_or_update_component_link(created: datetime | None, xblock):
 
     top_level_parent_usage_key = None
     if xblock.top_level_downstream_parent_key is not None:
+        block_key = BlockKey.from_string(xblock.top_level_downstream_parent_key)
         top_level_parent_usage_key = BlockUsageLocator(
             xblock.usage_key.course_key,
-            xblock.top_level_downstream_parent_key.get('type'),
-            xblock.top_level_downstream_parent_key.get('id'),
+            block_key.type,
+            block_key.id,
         )
 
     ComponentLink.update_or_create(
@@ -2444,10 +2446,11 @@ def _create_or_update_container_link(created: datetime | None, xblock):
 
     top_level_parent_usage_key = None
     if xblock.top_level_downstream_parent_key is not None:
+        block_key = BlockKey.from_string(xblock.top_level_downstream_parent_key)
         top_level_parent_usage_key = BlockUsageLocator(
             xblock.usage_key.course_key,
-            xblock.top_level_downstream_parent_key.get('type'),
-            xblock.top_level_downstream_parent_key.get('id'),
+            block_key.type,
+            block_key.id,
         )
 
     ContainerLink.update_or_create(

cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py

@@ -33,7 +33,7 @@
 from pytz import UTC
 from xblock.core import XBlock
 from xblock.fields import Scope
-from .xblock_helpers import get_block_key_dict
+from .xblock_helpers import get_block_key_string
 
 from cms.djangoapps.contentstore.config.waffle import SHOW_REVIEW_RULES_FLAG
 from cms.djangoapps.contentstore.helpers import StaticFileNotices
@@ -602,7 +602,7 @@ def sync_library_content(
                         block_id=f"{block_type}{uuid4().hex[:8]}",
                         fields={
                             "upstream": upstream_key,
-                            "top_level_downstream_parent_key": get_block_key_dict(
+                            "top_level_downstream_parent_key": get_block_key_string(
                                 top_level_downstream_parent.usage_key,
                             ),
                         },

cms/djangoapps/contentstore/xblock_storage_handlers/xblock_helpers.py

@@ -18,11 +18,11 @@ def usage_key_with_run(usage_key_string: str) -> UsageKey:
     return usage_key
 
 
-def get_block_key_dict(usage_key: UsageKey) -> dict:
+def get_block_key_string(usage_key: UsageKey) -> str:
     """
-    Converts the usage_key in a dict with the form: `{"type": block_type, "id": block_id}`
+    Extract block key from UsageKey in string format: `html:my-id`.
     """
-    return BlockKey.from_usage_key(usage_key)._asdict()
+    return str(BlockKey.from_usage_key(usage_key))
 
 
 def get_tags_count(xblock: XBlock, include_children=False) -> dict[str, int]:

cms/lib/xblock/upstream_sync.py

@@ -26,7 +26,7 @@
 from opaque_keys.edx.locator import LibraryContainerLocator, LibraryUsageLocatorV2
 from opaque_keys.edx.keys import UsageKey
 from xblock.exceptions import XBlockNotFoundError
-from xblock.fields import Scope, String, Integer, Dict, List
+from xblock.fields import Scope, String, Integer, List
 from xblock.core import XBlockMixin, XBlock
 from xmodule.util.keys import BlockKey
 
@@ -337,7 +337,7 @@ def decline_sync(downstream: XBlock, user_id=None) -> None:
 
 def _update_children_top_level_parent(
     downstream: XBlock,
-    new_top_level_parent_key: dict[str, str] | None
+    new_top_level_parent_key: str | None,
 ) -> list[XBlock]:
     """
     Given a new top-level parent block, update the `top_level_downstream_parent_key` field on the downstream block
@@ -357,7 +357,7 @@ def _update_children_top_level_parent(
         # If the `new_top_level_parent_key` is None, the current level assume the top-level
         # parent key for its children.
         child_top_level_parent_key = new_top_level_parent_key if new_top_level_parent_key is not None else (
-            BlockKey.from_usage_key(child.usage_key)._asdict()
+            str(BlockKey.from_usage_key(child.usage_key))
         )
 
         affected_blocks.extend(_update_children_top_level_parent(child, child_top_level_parent_key))
@@ -466,7 +466,7 @@ class UpstreamSyncMixin(XBlockMixin):
         default=None, scope=Scope.settings, hidden=True, enforce_type=True,
     )
 
-    top_level_downstream_parent_key = Dict(
+    top_level_downstream_parent_key = String(
         help=(
             "The block key ('block_type@block_id') of the downstream block that is the top-level parent of "
             "this block. This is present if the creation of this block is a consequence of "

xmodule/tests/test_util_keys.py

@@ -2,6 +2,7 @@
 Tests for xmodule/util/keys.py
 """
 import ddt
+import pytest
 from unittest import TestCase
 from unittest.mock import Mock
 
@@ -43,3 +44,29 @@ def test_derive_key(self, source, parent, expected):
         Test that derive_key returns the expected value.
         """
         assert derive_key(source, parent) == expected
+
+
+@ddt.ddt
+class TestBlockKeyParsing(TestCase):
+    """
+    Tests for parsing BlockKeys.
+    """
+
+    @ddt.data(['chapter:some-id', 'chapter', 'some-id'], ['section:one-more-id', 'section', 'one-more-id'])
+    @ddt.unpack
+    def test_block_key_from_string(self, block_key_str, blockType, blockId):
+        block_key = BlockKey.from_string(block_key_str)
+        assert block_key.type == blockType
+        assert block_key.id == blockId
+
+    @ddt.data('chapter:invalid:some-id', 'sectionone-more-id')
+    def test_block_key_from_string_error(self, block_key_str):
+        with pytest.raises(ValueError):
+            BlockKey.from_string(block_key_str)
+
+    @ddt.data(
+        [BlockKey('chapter', 'some-id'), 'chapter:some-id'], [BlockKey('section', 'one-more-id'), 'section:one-more-id']
+    )
+    @ddt.unpack
+    def test_block_key_to_string(self, block_key, block_key_str):
+        assert str(block_key) == block_key_str

xmodule/util/keys.py

@@ -4,8 +4,7 @@
 Consider moving these into opaque-keys if they generalize well.
 """
 import hashlib
-from typing import NamedTuple
-
+from typing import NamedTuple, Self
 
 from opaque_keys.edx.keys import UsageKey
 
@@ -28,6 +27,19 @@ class BlockKey(NamedTuple):
     def from_usage_key(cls, usage_key):
         return cls(usage_key.block_type, usage_key.block_id)
 
+    def __str__(self) -> str:
+        return f"{self.type}:{self.id}"
+
+    @classmethod
+    def from_string(cls, s: str) -> Self:
+        """
+        Convert a BlockKey string into a BlockKey object.
+        """
+        parts = s.split(':')
+        if len(parts) != 2 or not parts[0] or not parts[1]:
+            raise ValueError(f"Invalid string format for BlockKey: {s}")
+        return cls(parts[0], parts[1])
+
 
 def derive_key(source: UsageKey, dest_parent: BlockKey) -> BlockKey:
     """