From 0feec5c1a9af45676453a76c34037c5269a96152 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 17 Oct 2023 13:07:36 -0400 Subject: [PATCH 1/7] feat: first iteration of migration script for blockstore to openedx-learning models --- cms/envs/common.py | 15 ++ lms/envs/common.py | 17 +- .../djangoapps/content_libraries/admin.py | 6 +- .../core/djangoapps/content_libraries/api.py | 38 +++- .../commands/migrate_lib_to_learning_core.py | 202 ++++++++++++++++++ .../djangoapps/content_libraries/models.py | 14 ++ .../djangoapps/content_libraries/views.py | 3 + .../xblock/runtime/blockstore_runtime.py | 80 ++++++- openedx/core/lib/blockstore_api/methods.py | 1 + 9 files changed, 366 insertions(+), 10 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py diff --git a/cms/envs/common.py b/cms/envs/common.py index 917f6859a613..b4becaf86031 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1808,9 +1808,24 @@ # alternative swagger generator for CMS API 'drf_spectacular', + 'openedx_events', + + # Learning Core Apps + "openedx_learning.core.components.apps.ComponentsConfig", + "openedx_learning.core.contents.apps.ContentsConfig", + "openedx_learning.core.publishing.apps.PublishingConfig", + + # Learning Contrib Apps + # "openedx_learning.contrib.media_server.apps.MediaServerConfig", ] +OPENEDX_LEARNING = { + # Custom file storage, though this is better done through Django's + # STORAGES setting in Django >= 4.2 + "STORAGE": None, +} + ################# EDX MARKETING SITE ################################## diff --git a/lms/envs/common.py b/lms/envs/common.py index b4ae77d78409..fa706aa1e8ca 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -40,7 +40,6 @@ # and throws spurious errors. Therefore, we disable invalid-name checking. # pylint: disable=invalid-name - import importlib.util import sys import os @@ -3312,9 +3311,25 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring # Notifications 'openedx.core.djangoapps.notifications', + + 'openedx_events', + + # Learning Core Apps + "openedx_learning.core.components.apps.ComponentsConfig", + "openedx_learning.core.contents.apps.ContentsConfig", + "openedx_learning.core.publishing.apps.PublishingConfig", + + # Learning Contrib Apps + # "openedx_learning.contrib.media_server.apps.MediaServerConfig", ] +OPENEDX_LEARNING = { + # Custom file storage, though this is better done through Django's + # STORAGES setting in Django >= 4.2 + "STORAGE": None, +} + ######################### CSRF ######################################### # Forwards-compatibility with Django 1.7 diff --git a/openedx/core/djangoapps/content_libraries/admin.py b/openedx/core/djangoapps/content_libraries/admin.py index 559d2471cee3..e715841187e6 100644 --- a/openedx/core/djangoapps/content_libraries/admin.py +++ b/openedx/core/djangoapps/content_libraries/admin.py @@ -2,7 +2,7 @@ Admin site for content libraries """ from django.contrib import admin -from .models import ContentLibrary, ContentLibraryPermission +from .models import ContentLibrary, ContentLibraryLearningPackage, ContentLibraryPermission class ContentLibraryPermissionInline(admin.TabularInline): @@ -40,3 +40,7 @@ def get_readonly_fields(self, request, obj=None): return ["library_key", "org", "slug", "bundle_uuid"] else: return ["library_key", ] + +@admin.register(ContentLibraryLearningPackage) +class ContentLibraryLearningPackageAdmin(admin.ModelAdmin): + pass diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 737673b32e3f..fb2c2af3d074 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -733,7 +733,13 @@ def get_library_block(usage_key): ) -def get_library_block_olx(usage_key): +from openedx_learning.core.components.api import get_component_version_content +from openedx_learning.core.components.models import Component, ComponentVersion, ComponentVersionRawContent +from openedx_learning.core.contents.models import RawContent, TextContent +from openedx_learning.core.publishing.models import Draft, LearningPackage +from django.db.models import Q + +def get_library_block_olx(usage_key: LibraryUsageLocatorV2): """ Get the OLX source of the given XBlock. """ @@ -747,6 +753,36 @@ def get_library_block_olx(usage_key): return xml_str + #xml_str = get_component_version_content( + # str(usage_key.lib_key), + # # xblock.v1:problem@3e8c88ea8ec545b4b3c7066c89527d5e_cvo3nwt344t7ytqts + # f"xblock.v1:{usage_key.definition_key}", + # 1, + # "definition.xml", + #) +# cv_rc = ComponentVersionRawContent.objects.select_related( +# "raw_content", +# "raw_content__text_content" +# "component_version", +# "component_version__component", +# "component_version__component__learning_package", +# ) + # Inefficient but simple approach first + learning_package = LearningPackage.objects.get(key=str(usage_key.lib_key)) + component = Component.objects.get( + learning_package=learning_package, + namespace='xblock.v1', + type=usage_key.block_type, + local_key=usage_key.block_id, + ) + component_version = component.versioning.draft + text_content = component_version.raw_contents.get(key="definition.xml").text_content + + print("I really executed! Not a hallucination!") + + return text_content.text + + def set_library_block_olx(usage_key, new_olx_str): """ Replace the OLX source of the given XBlock. diff --git a/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py b/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py new file mode 100644 index 000000000000..6f9a3f8516b4 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py @@ -0,0 +1,202 @@ +""" +Command to import Blockstore-backed v2 Libraries to Learning Core data models. +""" +from datetime import datetime, timezone +from xml.etree import ElementTree as ET +import logging + +from django.conf import settings +from django.db import transaction +from django.core.management import BaseCommand, CommandError + +from opaque_keys.edx.locator import LibraryLocatorV2 +from openedx.core.djangoapps.content_libraries import api as lib_api +from openedx.core.djangoapps.content_libraries import models as lib_models +from openedx.core.djangoapps.content_libraries import constants as lib_constants +from openedx.core.lib.blockstore_api import ( + get_bundle, + get_bundle_file_metadata, + get_bundle_file_data, + get_bundle_files_dict, +) +from openedx_learning.core.publishing import api as publishing_api +from openedx_learning.core.components import api as components_api +from openedx_learning.core.contents import api as contents_api + +# These imports should all be removed at some point. + +from openedx_learning.core.publishing.models import Draft + + +log = logging.getLogger(__name__) + + +class Command(BaseCommand): + """ + Create a LearningPackage and initialize with contents from Library + """ + + def add_arguments(self, parser): + """ + Add arguments to the argument parser. + """ + parser.add_argument( + 'library-key', + type=LibraryLocatorV2.from_string, + help=('Content Library Key to import content from.'), + ) + + def handle(self, *args, **options): + # Search for the library. + lib_key = options['library-key'] + lib_data = lib_api.get_library(lib_key) + lib = lib_models.ContentLibrary.objects.get_by_key(lib_key) + + COMPONENT_NAMESPACE = 'xblock.v1' + + with transaction.atomic(): + # This is a migration script and we're assuming there's no important + # state attached to the LearningPackage yet. That makes it safe to + # just wipe out everything and recreate it. + if hasattr(lib, 'contents'): + lp = lib.contents.learning_package + log.info(f"Deleting existing LearningPackage {lp.key} ({lp.uuid})") + lib.contents.delete() + lp.delete() + + # Initialize a new LearningPackage + learning_package = publishing_api.create_learning_package( + key=lib_key, + title=lib_data.title, + ) + log.info(f"Created LearningPackage {learning_package.key} ({learning_package.uuid})") + lib_models.ContentLibraryLearningPackage.objects.create( + content_library=lib, + learning_package=learning_package, + ) + + # We only really need to get the most recent version in Studio draft + # for now. + bundle = get_bundle(lib.bundle_uuid) + published_files = get_bundle_files_dict(lib.bundle_uuid) + + now = datetime.now(timezone.utc) + + # First get the published version into openedx-learning models. This + # creates ComponentVersions and puts them as the current Draft. + published_metadata_dict = {} + published_component_pks = {} + published_definition_files = { + file_path: metadata + for file_path, metadata in published_files.items() + if file_path.endswith('/definition.xml') + } + for file_path, metadata in published_definition_files.items(): + block_type, block_id, _def_xml = file_path.split('/') + published_metadata_dict[file_path] = metadata + xml_bytes = get_bundle_file_data(bundle.uuid, file_path) + display_name = extract_display_name(xml_bytes, file_path) + + component, component_version = components_api.create_component_and_version( + learning_package_id=learning_package.id, + namespace=COMPONENT_NAMESPACE, + type=block_type, + local_key=block_id, + title=display_name, + created=now, + created_by=None, + ) + published_component_pks[file_path] = component.pk + text_content, _created = contents_api.get_or_create_text_content_from_bytes( + learning_package_id=learning_package.id, + data_bytes=xml_bytes, + mime_type=f"application/vnd.openedx.xblock.v1.{block_type}+xml", + created=now, + ) + components_api.add_content_to_component_version( + component_version, + raw_content_id=text_content.pk, + key="definition.xml", + learner_downloadable=False + ) + # Publish all the Draft versions we created. + publishing_api.publish_all_drafts( + learning_package_id=learning_package.id, + message="Initial import from Blockstore", + published_at=now, + ) + + # Now grab the draft versions from blockstore, and copy those... + draft_files = get_bundle_files_dict(lib.bundle_uuid, use_draft=lib_constants.DRAFT_NAME) + draft_definition_files = { + file_path: metadata + for file_path, metadata in draft_files.items() + if file_path.endswith("definition.xml") + } + for file_path, draft_metadata in draft_definition_files.items(): + published_metadata = published_metadata_dict.get(file_path) + if draft_metadata.modified: + block_type, block_id, def_xml = file_path.split('/') + xml_bytes = get_bundle_file_data(bundle.uuid, file_path, use_draft=lib_constants.DRAFT_NAME) + display_name = extract_display_name(xml_bytes, file_path) + + # If this is newly created in the draft... + if published_metadata is None: + component = components_api.create_component( + learning_package_id=learning_package.id, + namespace=COMPONENT_NAMESPACE, + type=block_type, + local_key=block_id, + created=now, + created_by=None, + ) + component_pk = component.pk + version_num = 1 + # Otherwise, it's been modified... + else: + component_pk = published_component_pks[file_path] + version_num = 2 + + component_version = components_api.create_component_version( + component_pk=component_pk, + version_num=version_num, + title=display_name, + created=now, + created_by=None, + ) + text_content, _created = contents_api.get_or_create_text_content_from_bytes( + learning_package_id=learning_package.id, + data_bytes=xml_bytes, + mime_type=f"application/vnd.openedx.xblock.v1.{block_type}+xml", + created=now, + ) + components_api.add_content_to_component_version( + component_version, + raw_content_id=text_content.pk, + key="definition.xml", + learner_downloadable=False + ) + + # Now remove stuff that was present in the published set but was + # deleted in the draft. + deleted_definition_files = set(published_definition_files) - set(draft_definition_files) + for deleted_definition_file in deleted_definition_files: + log.info(f"Deleting {deleted_definition_file} from draft") + component_pk = published_component_pks[deleted_definition_file] + draft = Draft.objects.get(entity_id=component_pk) + draft.version = None + draft.save() + + +def extract_display_name(xml_bytes, file_path): + # Do some basic parsing of the content to see if it's even well + # constructed enough to add (or whether we should skip/error on it). + try: + xml_str = xml_bytes.decode('utf-8') + block_root = ET.fromstring(xml_str) + display_name = block_root.attrib.get("display_name", "") + except ET.ParseError as err: + log.error(f"Parse error for {file_path}: {err}") + display_name = "" + + return display_name diff --git a/openedx/core/djangoapps/content_libraries/models.py b/openedx/core/djangoapps/content_libraries/models.py index 7bf8792699f9..7970c9097d7f 100644 --- a/openedx/core/djangoapps/content_libraries/models.py +++ b/openedx/core/djangoapps/content_libraries/models.py @@ -56,6 +56,7 @@ LIBRARY_TYPES, COMPLEX, LICENSE_OPTIONS, ALL_RIGHTS_RESERVED, ) +from openedx_learning.core.publishing.models import LearningPackage from organizations.models import Organization # lint-amnesty, pylint: disable=wrong-import-order from .apps import ContentLibrariesConfig @@ -527,3 +528,16 @@ def update_score(self, weighted_earned, weighted_possible, timestamp): def __str__(self): return str(self.usage_key) + + +class ContentLibraryLearningPackage(models.Model): + content_library = models.OneToOneField( + ContentLibrary, + primary_key=True, + on_delete=models.CASCADE, + related_name='contents', + ) + learning_package = models.ForeignKey( + LearningPackage, + on_delete=models.RESTRICT + ) diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index c83a9c4fcb56..76221a878de5 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -680,6 +680,9 @@ def get(self, request, usage_key_str): """ key = LibraryUsageLocatorV2.from_string(usage_key_str) api.require_permission_for_library_key(key.lib_key, request.user, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) + + print("You called me!") + xml_str = api.get_library_block_olx(key) return Response(LibraryXBlockOlxSerializer({"olx": xml_str}).data) diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py index 33ea8514b632..e96a45084212 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py @@ -25,6 +25,15 @@ log = logging.getLogger(__name__) + +from openedx_learning.core.components.api import get_component_version_content +from openedx_learning.core.components.models import Component, ComponentVersion, ComponentVersionRawContent +from openedx_learning.core.contents.models import RawContent, TextContent +from openedx_learning.core.publishing.models import Draft, LearningPackage +from django.db.models import Q + + + class BlockstoreXBlockRuntime(XBlockRuntime): """ A runtime designed to work with Blockstore, reading and writing @@ -33,13 +42,8 @@ class BlockstoreXBlockRuntime(XBlockRuntime): def parse_xml_file(self, fileobj, id_generator=None): raise NotImplementedError("Use parse_olx_file() instead") - def get_block(self, usage_id, for_parent=None): - """ - Create an XBlock instance in this runtime. - - Args: - usage_key(OpaqueKey): identifier used to find the XBlock class and data. - """ + def get_block_blockstore(self, usage_key, for_parent=None): + usage_id = usage_key def_id = self.id_reader.get_definition_id(usage_id) if def_id is None: raise ValueError(f"Definition not found for usage {usage_id}") @@ -81,6 +85,68 @@ def get_block(self, usage_id, for_parent=None): block._parent_block_id = for_parent.scope_ids.usage_id # pylint: disable=protected-access return block + def get_block_learning_core(self, usage_key, for_parent=None): + # We should use the lookup table here instead of taking the lib_key directly. + learning_package = LearningPackage.objects.get(key=str(usage_key.lib_key)) + + # We can do this more efficiently in a single query later, but for now + # just get it the easy way. + component = Component.objects.get( + learning_package=learning_package, + namespace='xblock.v1', + type=usage_key.block_type, + local_key=usage_key.block_id, + ) + component_version = component.versioning.draft + raw_content = component_version.raw_contents.get( + componentversionrawcontent__key="definition.xml" + ) + + xml_str = raw_content.text_content.text + xml_node = etree.fromstring(xml_str) + block_type = usage_id.block_type + + # At this point, how much value do we really get out of explicitly + # differentiating definitions from usages? + def_id = self.id_reader.get_definition_id(usage_id) + keys = ScopeIds(self.user_id, block_type, def_id, usage_id) + + if xml_node.get("url_name", None): + log.warning("XBlock at %s should not specify an old-style url_name attribute.", def_id.olx_path) + block_class = self.mixologist.mix(self.load_block_type(block_type)) + if hasattr(block_class, 'parse_xml_new_runtime'): + # This is a (former) XModule with messy XML parsing code; let its parse_xml() method continue to work + # as it currently does in the old runtime, but let this parse_xml_new_runtime() method parse the XML in + # a simpler way that's free of tech debt, if defined. + # In particular, XmlMixin doesn't play well with this new runtime, so this is mostly about + # bypassing that mixin's code. + # When a former XModule no longer needs to support the old runtime, its parse_xml_new_runtime method + # should be removed and its parse_xml() method should be simplified to just call the super().parse_xml() + # plus some minor additional lines of code as needed. + block = block_class.parse_xml_new_runtime(xml_node, runtime=self, keys=keys) + else: + block = block_class.parse_xml(xml_node, runtime=self, keys=keys, id_generator=None) + # Update field data with parsed values. We can't call .save() because it will call save_block(), below. + block.force_save_fields(block._get_fields_to_save()) # pylint: disable=protected-access + self.system.authored_data_store.cache_fields(block) + # There is no way to set the parent via parse_xml, so do what + # HierarchyMixin would do: + if for_parent is not None: + block._parent_block = for_parent # pylint: disable=protected-access + block._parent_block_id = for_parent.scope_ids.usage_id # pylint: disable=protected-access + return block + + + + def get_block(self, usage_id, for_parent=None): + """ + Create an XBlock instance in this runtime. + + Args: + usage_key(OpaqueKey): identifier used to find the XBlock class and data. + """ + return self.get_block_blockstore(usage_id, for_parent=for_parent) + def add_node_as_child(self, block, node, id_generator=None): """ This runtime API should normally be used via diff --git a/openedx/core/lib/blockstore_api/methods.py b/openedx/core/lib/blockstore_api/methods.py index 7d7c65decdc1..561d1dc5ecea 100644 --- a/openedx/core/lib/blockstore_api/methods.py +++ b/openedx/core/lib/blockstore_api/methods.py @@ -363,6 +363,7 @@ def get_bundle_files_dict(bundle_uuid, use_draft=None): BundleFileData or DraftFileData tuples. """ bundle = get_bundle(bundle_uuid) + if use_draft and use_draft in bundle.drafts: # pylint: disable=unsupported-membership-test draft_uuid = bundle.drafts[use_draft] # pylint: disable=unsubscriptable-object return get_draft(draft_uuid).files From 365df4793118868fb735c2806558810bf7e1d4c5 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 24 Oct 2023 11:54:36 -0400 Subject: [PATCH 2/7] fixup!: add migrations, clean up import code, add comments --- cms/envs/common.py | 5 +- lms/envs/common.py | 5 +- .../commands/migrate_lib_to_learning_core.py | 92 +++++++++++++------ .../0010_contentlibrarylearningpackage.py | 22 +++++ .../djangoapps/content_libraries/models.py | 16 +++- 5 files changed, 101 insertions(+), 39 deletions(-) create mode 100644 openedx/core/djangoapps/content_libraries/migrations/0010_contentlibrarylearningpackage.py diff --git a/cms/envs/common.py b/cms/envs/common.py index b4becaf86031..8f09290201fd 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1811,13 +1811,10 @@ 'openedx_events', - # Learning Core Apps + # Learning Core Apps, used by v2 content libraries (content_libraries app) "openedx_learning.core.components.apps.ComponentsConfig", "openedx_learning.core.contents.apps.ContentsConfig", "openedx_learning.core.publishing.apps.PublishingConfig", - - # Learning Contrib Apps - # "openedx_learning.contrib.media_server.apps.MediaServerConfig", ] OPENEDX_LEARNING = { diff --git a/lms/envs/common.py b/lms/envs/common.py index fa706aa1e8ca..d33800317bd4 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3315,13 +3315,10 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring 'openedx_events', - # Learning Core Apps + # Learning Core Apps, used by v2 content libraries (content_libraries app) "openedx_learning.core.components.apps.ComponentsConfig", "openedx_learning.core.contents.apps.ContentsConfig", "openedx_learning.core.publishing.apps.PublishingConfig", - - # Learning Contrib Apps - # "openedx_learning.contrib.media_server.apps.MediaServerConfig", ] OPENEDX_LEARNING = { diff --git a/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py b/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py index 6f9a3f8516b4..925d4f473bd0 100644 --- a/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py +++ b/openedx/core/djangoapps/content_libraries/management/commands/migrate_lib_to_learning_core.py @@ -1,13 +1,15 @@ """ Command to import Blockstore-backed v2 Libraries to Learning Core data models. + +This will hopefully be very short-lived code. """ from datetime import datetime, timezone from xml.etree import ElementTree as ET import logging -from django.conf import settings from django.db import transaction from django.core.management import BaseCommand, CommandError +from django.core.exceptions import ObjectDoesNotExist from opaque_keys.edx.locator import LibraryLocatorV2 from openedx.core.djangoapps.content_libraries import api as lib_api @@ -15,7 +17,6 @@ from openedx.core.djangoapps.content_libraries import constants as lib_constants from openedx.core.lib.blockstore_api import ( get_bundle, - get_bundle_file_metadata, get_bundle_file_data, get_bundle_files_dict, ) @@ -23,17 +24,21 @@ from openedx_learning.core.components import api as components_api from openedx_learning.core.contents import api as contents_api -# These imports should all be removed at some point. - -from openedx_learning.core.publishing.models import Draft - log = logging.getLogger(__name__) class Command(BaseCommand): """ - Create a LearningPackage and initialize with contents from Library + Create a new LearningPackage and initialize with contents from Library. + + If you run this and specify a Library that already has a LearningPackage + (using -f), this command will delete that LearningPackage and create a new + one to associate with the Libary. It does not modify the existing one. + + All the work is done in a transaction, so errors partway through the + process shouldn't cause state inconsistency in the database. A partly- + imported course *can* cause data to end up in Django Storages. """ def add_arguments(self, parser): @@ -45,20 +50,43 @@ def add_arguments(self, parser): type=LibraryLocatorV2.from_string, help=('Content Library Key to import content from.'), ) + parser.add_argument( + '-f', + '--force', + action='store_true', + default=False, + ) def handle(self, *args, **options): + """ + Does the work of parsing content from Blockstore and writing it into + openedx-learning core models (publishing, components, contents). + """ # Search for the library. - lib_key = options['library-key'] - lib_data = lib_api.get_library(lib_key) - lib = lib_models.ContentLibrary.objects.get_by_key(lib_key) + try: + lib_key = options['library-key'] + lib_data = lib_api.get_library(lib_key) + lib = lib_models.ContentLibrary.objects.get_by_key(lib_key) + except ObjectDoesNotExist: + raise CommandError(f"Library not found: {lib_key}") COMPONENT_NAMESPACE = 'xblock.v1' + learning_package_already_exists = ( + hasattr(lib, 'contents') and + lib.contents.learning_package is not None + ) + + if learning_package_already_exists and not options['force']: + raise CommandError( + f"Learning Package already exists for {lib_key} (use -f to overwrite)" + ) + with transaction.atomic(): # This is a migration script and we're assuming there's no important # state attached to the LearningPackage yet. That makes it safe to # just wipe out everything and recreate it. - if hasattr(lib, 'contents'): + if learning_package_already_exists: lp = lib.contents.learning_package log.info(f"Deleting existing LearningPackage {lp.key} ({lp.uuid})") lib.contents.delete() @@ -74,22 +102,23 @@ def handle(self, *args, **options): content_library=lib, learning_package=learning_package, ) - - # We only really need to get the most recent version in Studio draft - # for now. + + # We don't need the full history stored in Blockstore, just the most + # recently published version and the most recent draft. bundle = get_bundle(lib.bundle_uuid) published_files = get_bundle_files_dict(lib.bundle_uuid) now = datetime.now(timezone.utc) - - # First get the published version into openedx-learning models. This - # creates ComponentVersions and puts them as the current Draft. + + # First get the published version into openedx-learning models. On + # the openedx-learning side, we'll create them as Drafts and then + # publish at the end. published_metadata_dict = {} published_component_pks = {} published_definition_files = { file_path: metadata for file_path, metadata in published_files.items() - if file_path.endswith('/definition.xml') + if file_path.endswith('/definition.xml') # This is the OLX } for file_path, metadata in published_definition_files.items(): block_type, block_id, _def_xml = file_path.split('/') @@ -126,7 +155,7 @@ def handle(self, *args, **options): published_at=now, ) - # Now grab the draft versions from blockstore, and copy those... + # Now grab the draft version from blockstore, and copy those... draft_files = get_bundle_files_dict(lib.bundle_uuid, use_draft=lib_constants.DRAFT_NAME) draft_definition_files = { file_path: metadata @@ -136,11 +165,12 @@ def handle(self, *args, **options): for file_path, draft_metadata in draft_definition_files.items(): published_metadata = published_metadata_dict.get(file_path) if draft_metadata.modified: - block_type, block_id, def_xml = file_path.split('/') + block_type, block_id, _def_xml = file_path.split('/') xml_bytes = get_bundle_file_data(bundle.uuid, file_path, use_draft=lib_constants.DRAFT_NAME) display_name = extract_display_name(xml_bytes, file_path) - # If this is newly created in the draft... + # If this is newly created in the draft, we have to create a + # whole new Component... if published_metadata is None: component = components_api.create_component( learning_package_id=learning_package.id, @@ -151,8 +181,8 @@ def handle(self, *args, **options): created_by=None, ) component_pk = component.pk - version_num = 1 - # Otherwise, it's been modified... + version_num = 1 + # Otherwise, it's just been modified... else: component_pk = published_component_pks[file_path] version_num = 2 @@ -176,21 +206,23 @@ def handle(self, *args, **options): key="definition.xml", learner_downloadable=False ) - + # Now remove stuff that was present in the published set but was # deleted in the draft. deleted_definition_files = set(published_definition_files) - set(draft_definition_files) for deleted_definition_file in deleted_definition_files: log.info(f"Deleting {deleted_definition_file} from draft") component_pk = published_component_pks[deleted_definition_file] - draft = Draft.objects.get(entity_id=component_pk) - draft.version = None - draft.save() + publishing_api.set_draft_version(component_pk, None) def extract_display_name(xml_bytes, file_path): - # Do some basic parsing of the content to see if it's even well - # constructed enough to add (or whether we should skip/error on it). + """ + Parse the display_name out of the XML. + + This will return an empty string if no display_name is specified, or if + there is a parsing error. + """ try: xml_str = xml_bytes.decode('utf-8') block_root = ET.fromstring(xml_str) @@ -198,5 +230,5 @@ def extract_display_name(xml_bytes, file_path): except ET.ParseError as err: log.error(f"Parse error for {file_path}: {err}") display_name = "" - + return display_name diff --git a/openedx/core/djangoapps/content_libraries/migrations/0010_contentlibrarylearningpackage.py b/openedx/core/djangoapps/content_libraries/migrations/0010_contentlibrarylearningpackage.py new file mode 100644 index 000000000000..0549e09b2497 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/migrations/0010_contentlibrarylearningpackage.py @@ -0,0 +1,22 @@ +# Generated by Django 3.2.22 on 2023-10-24 15:44 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_publishing', '0002_alter_fk_on_delete'), + ('content_libraries', '0009_alter_contentlibrary_authorized_lti_configs'), + ] + + operations = [ + migrations.CreateModel( + name='ContentLibraryLearningPackage', + fields=[ + ('content_library', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, related_name='contents', serialize=False, to='content_libraries.contentlibrary')), + ('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.RESTRICT, to='oel_publishing.learningpackage')), + ], + ), + ] diff --git a/openedx/core/djangoapps/content_libraries/models.py b/openedx/core/djangoapps/content_libraries/models.py index 7970c9097d7f..e64aa16c19ae 100644 --- a/openedx/core/djangoapps/content_libraries/models.py +++ b/openedx/core/djangoapps/content_libraries/models.py @@ -531,6 +531,20 @@ def __str__(self): class ContentLibraryLearningPackage(models.Model): + """ + Associates ContentLibrary with a LearningPackage. + + This essentially maps the abstract, catalog concept of a Content Library + from the actual bucket of content that is stored in that library. + + If the referenced ContentLibrary is deleted, this association is also + deleted, but the LearningPackage being referenced is _not_ deleted. + + This association actively prevents the referenced LearningPackage from being + deleted, i.e. you cannot just remove the content that is backing a + ContentLibrary by mistake. You have to explicitly disassociate it from the + ContentLibrary first. + """ content_library = models.OneToOneField( ContentLibrary, primary_key=True, @@ -539,5 +553,5 @@ class ContentLibraryLearningPackage(models.Model): ) learning_package = models.ForeignKey( LearningPackage, - on_delete=models.RESTRICT + on_delete=models.RESTRICT, ) From db4e42e31ee256bfae5cf331791b275964107707 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 26 Oct 2023 22:20:59 -0400 Subject: [PATCH 3/7] feat: read/write v2 lib data using openedx-learning models This is very, very, hacky. Writes have not been tested at all. --- .../core/djangoapps/content_libraries/api.py | 71 +++- openedx/core/djangoapps/xblock/api.py | 61 ++++ .../xblock/runtime/blockstore_field_data.py | 13 - .../xblock/runtime/blockstore_runtime.py | 79 +---- .../xblock/runtime/learning_core_runtime.py | 333 ++++++++++++++++++ .../core/djangoapps/xblock/runtime/runtime.py | 11 +- 6 files changed, 477 insertions(+), 91 deletions(-) create mode 100644 openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index fb2c2af3d074..248fe649258f 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -638,8 +638,51 @@ def delete_library(library_key): log.exception("Failed to delete blockstore bundle %s when deleting library. Delete it manually.", bundle_uuid) raise - def get_library_blocks(library_key, text_search=None, block_types=None): + return get_library_blocks_learning_core( + #return get_library_blocks_blockstore( + library_key, + text_search=text_search, + block_types=block_types, + ) + + +from openedx_learning.core.publishing.models import Draft, LearningPackage +from .models import ContentLibraryLearningPackage + +def get_library_blocks_learning_core(library_key, text_search=None, block_types=None): + """ + We are absolutely ignoring the optional args in the first pass. + """ + lib = ContentLibrary.objects.get_by_key(library_key) + learning_package = ContentLibraryLearningPackage.objects.get(pk=lib.pk).learning_package + drafts = Draft.objects \ + .select_related( + 'version', + 'version__componentversion', + 'entity', + 'entity__component', + ).filter( + entity__learning_package=learning_package, + version__isnull=False, + ).all() + return [ + LibraryXBlockMetadata( + usage_key=LibraryUsageLocatorV2( + library_key, + draft.entity.component.type, + draft.entity.component.local_key, + ), + def_key=None, + display_name=draft.version.title, + has_unpublished_changes=False, + ) + for draft in drafts + ] + + + +def get_library_blocks_blockstore(library_key, text_search=None, block_types=None): """ Get the list of top-level XBlocks in the specified library. @@ -717,6 +760,30 @@ def _lookup_usage_key(usage_key): def get_library_block(usage_key): + return get_library_block_learning_core(usage_key) + + +def get_library_block_learning_core(usage_key): + library_key = usage_key.lib_key + lib = ContentLibrary.objects.get_by_key(library_key) + learning_package = ContentLibraryLearningPackage.objects.get(pk=lib.pk).learning_package + entity = PublishableEntity.objects.select_related('published', 'draft').get( + learning_package=learning_package, + component__type=usage_key.block_type, + component__local_key=usage_key.block_id, + ) + draft_version = entity.draft.version + published_version = entity.published.version + + return LibraryXBlockMetadata( + usage_key=usage_key, + def_key=None, + display_name=draft_version.title, + has_unpublished_changes=(draft_version != published_version), + ) + + +def get_library_block_blockstore(usage_key): """ Get metadata (LibraryXBlockMetadata) about one specific XBlock in a library @@ -736,7 +803,7 @@ def get_library_block(usage_key): from openedx_learning.core.components.api import get_component_version_content from openedx_learning.core.components.models import Component, ComponentVersion, ComponentVersionRawContent from openedx_learning.core.contents.models import RawContent, TextContent -from openedx_learning.core.publishing.models import Draft, LearningPackage +from openedx_learning.core.publishing.models import Draft, LearningPackage, PublishableEntity from django.db.models import Q def get_library_block_olx(usage_key: LibraryUsageLocatorV2): diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index 7c24308f6bfe..21aac12cfeb7 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -23,6 +23,14 @@ from openedx.core.djangoapps.xblock.apps import get_xblock_app_config from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl from openedx.core.djangoapps.xblock.runtime.blockstore_runtime import BlockstoreXBlockRuntime, xml_for_definition + +from openedx.core.djangoapps.xblock.runtime.learning_core_runtime import ( + LearningCoreFieldData, + LearningCoreOpaqueKeyReader, + LearningCoreXBlockRuntime, +) + + from openedx.core.djangoapps.xblock.runtime.runtime import XBlockRuntimeSystem as _XBlockRuntimeSystem from openedx.core.djangolib.blockstore_cache import BundleCache from .utils import get_secure_token_for_xblock_handler, get_xblock_id_for_anonymous_user @@ -69,6 +77,21 @@ def get_runtime_system(): setattr(get_runtime_system, cache_name, _XBlockRuntimeSystem(**params)) return getattr(get_runtime_system, cache_name) +from datetime import datetime + +def get_runtime_system(): + params = get_xblock_app_config().get_runtime_system_params() + params.update( + runtime_class=LearningCoreXBlockRuntime, + handler_url=get_handler_url, + authored_data_store=LearningCoreFieldData(), + ) + start = datetime.now() + runtime = _XBlockRuntimeSystem(**params) + end = datetime.now() + log.info(f"Runtime initiated in {end - start}") + + return runtime def load_block(usage_key, user): """ @@ -172,7 +195,42 @@ def xblock_type_display_name(block_type): return block_type # Just use the block type as the name +def use_learning_core(block_or_key): + pass + +from openedx_learning.core.publishing.models import Draft, LearningPackage +from openedx_learning.core.components.models import Component, ComponentVersion, ComponentVersionRawContent + + +def _get_component_from_usage_key(self, usage_key): + learning_package = LearningPackage.objects.get(key=str(usage_key.lib_key)) + component = Component.objects.get( + learning_package=learning_package, + namespace='xblock.v1', + type=usage_key.block_type, + local_key=usage_key.block_id, + ) + return component + + def get_block_display_name(block_or_key): + return get_block_display_name_learning_core(block_or_key) + +def get_block_display_name_learning_core(block_or_key): + if isinstance(block_or_key, XBlock): + return block_or_key.display_name + elif isinstance(block_or_key, UsageKeyV2): + component = _get_component_from_usage_key(block_or_key) + return component.draft.title if component.draft else "" + + raise TypeError( + "display_name lookup expects a UsageKeyV2 or XBlock, " + + f"got {type(block_or_key)}: {block_or_key} instead" + ) + + + +def get_block_display_name_blockstore(block_or_key): """ Efficiently get the display name of the specified block. This is done in a way that avoids having to load and parse the block's entire XML field data @@ -186,8 +244,11 @@ def get_block_display_name(block_or_key): Returns the display name as a string """ + log.info("CALLING get_block_display_name") + def_key = resolve_definition(block_or_key) use_draft = get_xblock_app_config().get_learning_context_params().get('use_draft') + cache = BundleCache(def_key.bundle_uuid, draft_name=use_draft) cache_key = ('block_display_name', str(def_key)) display_name = cache.get(cache_key) diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py b/openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py index ac17a9550b0d..217b8c5ea389 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py +++ b/openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py @@ -140,19 +140,6 @@ def _check_field(self, block, name): # it's mostly relevant as Scope.preferences(UserScope.ONE, BlockScope.TYPE) # Which would be handled by a user-aware FieldData implementation - def _get_active_block(self, block): - """ - Get the ActiveBlock entry for the specified block, creating it if - necessary. - """ - key = get_weak_key_for_block(block) - if key not in self.active_blocks: - self.active_blocks[key] = ActiveBlock( - olx_hash=get_olx_hash_for_definition_key(block.scope_ids.def_id), - changed_fields={}, - ) - return self.active_blocks[key] - def get(self, block, name): """ Get the given field value from Blockstore diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py index e96a45084212..1a887aac26bf 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py @@ -25,15 +25,6 @@ log = logging.getLogger(__name__) - -from openedx_learning.core.components.api import get_component_version_content -from openedx_learning.core.components.models import Component, ComponentVersion, ComponentVersionRawContent -from openedx_learning.core.contents.models import RawContent, TextContent -from openedx_learning.core.publishing.models import Draft, LearningPackage -from django.db.models import Q - - - class BlockstoreXBlockRuntime(XBlockRuntime): """ A runtime designed to work with Blockstore, reading and writing @@ -42,7 +33,13 @@ class BlockstoreXBlockRuntime(XBlockRuntime): def parse_xml_file(self, fileobj, id_generator=None): raise NotImplementedError("Use parse_olx_file() instead") - def get_block_blockstore(self, usage_key, for_parent=None): + def get_block(self, usage_key, for_parent=None): + """ + Create an XBlock instance in this runtime. + + Args: + usage_key(OpaqueKey): identifier used to find the XBlock class and data. + """ usage_id = usage_key def_id = self.id_reader.get_definition_id(usage_id) if def_id is None: @@ -85,68 +82,6 @@ def get_block_blockstore(self, usage_key, for_parent=None): block._parent_block_id = for_parent.scope_ids.usage_id # pylint: disable=protected-access return block - def get_block_learning_core(self, usage_key, for_parent=None): - # We should use the lookup table here instead of taking the lib_key directly. - learning_package = LearningPackage.objects.get(key=str(usage_key.lib_key)) - - # We can do this more efficiently in a single query later, but for now - # just get it the easy way. - component = Component.objects.get( - learning_package=learning_package, - namespace='xblock.v1', - type=usage_key.block_type, - local_key=usage_key.block_id, - ) - component_version = component.versioning.draft - raw_content = component_version.raw_contents.get( - componentversionrawcontent__key="definition.xml" - ) - - xml_str = raw_content.text_content.text - xml_node = etree.fromstring(xml_str) - block_type = usage_id.block_type - - # At this point, how much value do we really get out of explicitly - # differentiating definitions from usages? - def_id = self.id_reader.get_definition_id(usage_id) - keys = ScopeIds(self.user_id, block_type, def_id, usage_id) - - if xml_node.get("url_name", None): - log.warning("XBlock at %s should not specify an old-style url_name attribute.", def_id.olx_path) - block_class = self.mixologist.mix(self.load_block_type(block_type)) - if hasattr(block_class, 'parse_xml_new_runtime'): - # This is a (former) XModule with messy XML parsing code; let its parse_xml() method continue to work - # as it currently does in the old runtime, but let this parse_xml_new_runtime() method parse the XML in - # a simpler way that's free of tech debt, if defined. - # In particular, XmlMixin doesn't play well with this new runtime, so this is mostly about - # bypassing that mixin's code. - # When a former XModule no longer needs to support the old runtime, its parse_xml_new_runtime method - # should be removed and its parse_xml() method should be simplified to just call the super().parse_xml() - # plus some minor additional lines of code as needed. - block = block_class.parse_xml_new_runtime(xml_node, runtime=self, keys=keys) - else: - block = block_class.parse_xml(xml_node, runtime=self, keys=keys, id_generator=None) - # Update field data with parsed values. We can't call .save() because it will call save_block(), below. - block.force_save_fields(block._get_fields_to_save()) # pylint: disable=protected-access - self.system.authored_data_store.cache_fields(block) - # There is no way to set the parent via parse_xml, so do what - # HierarchyMixin would do: - if for_parent is not None: - block._parent_block = for_parent # pylint: disable=protected-access - block._parent_block_id = for_parent.scope_ids.usage_id # pylint: disable=protected-access - return block - - - - def get_block(self, usage_id, for_parent=None): - """ - Create an XBlock instance in this runtime. - - Args: - usage_key(OpaqueKey): identifier used to find the XBlock class and data. - """ - return self.get_block_blockstore(usage_id, for_parent=for_parent) - def add_node_as_child(self, block, node, id_generator=None): """ This runtime API should normally be used via diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py new file mode 100644 index 000000000000..f3a4f6e28092 --- /dev/null +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -0,0 +1,333 @@ +""" + +""" +from __future__ import annotations + +import logging +from collections import namedtuple +from uuid import UUID +from weakref import WeakKeyDictionary + +from django.db.models import Q +from openedx_learning.core.components.api import get_component_version_content +from openedx_learning.core.components.models import Component, ComponentVersion, ComponentVersionRawContent +from openedx_learning.core.contents.models import RawContent, TextContent +from openedx_learning.core.publishing.models import Draft, LearningPackage +from lxml import etree +from opaque_keys.edx.keys import UsageKeyV2 + +from xblock.exceptions import InvalidScopeError, NoSuchDefinition +from xblock.fields import Field, BlockScope, Scope, ScopeIds, UserScope, Sentinel +from xblock.field_data import FieldData + +from opaque_keys.edx.keys import AssetKey, CourseKey, DefinitionKey, LearningContextKey, UsageKey, UsageKeyV2 +from opaque_keys.edx.locator import CheckFieldMixin + +from .blockstore_runtime import BlockstoreXBlockRuntime +from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl +from openedx.core.djangoapps.xblock.runtime.id_managers import OpaqueKeyReader +from openedx.core.lib.xblock_serializer.api import serialize_modulestore_block_for_blockstore + +log = logging.getLogger(__name__) + + +DELETED = Sentinel('DELETED') # Special value indicating a field was reset to its default value +CHILDREN_INCLUDES = Sentinel('CHILDREN_INCLUDES') # Key for a pseudo-field that stores the XBlock's children info + +MAX_DEFINITIONS_LOADED = 100 # How many of the most recently used XBlocks' field data to keep in memory at max. + +ActiveBlock = namedtuple('ActiveBlock', ['olx_hash', 'changed_fields']) + + + + +class LearningCoreFieldData(FieldData): + """ + Chunks of this are copied from BlockstoreFieldData + """ + + def __init__(self): + """ + Initialize this BlockstoreFieldData instance. + """ + # Both of these have UsageKeys for keys and have dicts for values. + self.usage_keys_to_loaded_fields = {} + self.usage_keys_to_changed_fields = {} + + super().__init__() + + def _getfield(self, block, name): + """ + Return the field with the given `name` from `block`. + If the XBlock doesn't have such a field, raises a KeyError. + """ + # First, get the field from the class, if defined + block_field = getattr(block.__class__, name, None) + if block_field is not None and isinstance(block_field, Field): + return block_field + # Not in the class, so name really doesn't name a field + raise KeyError(name) + + def _check_field(self, block, name): + """ + Given a block and the name of one of its fields, check that we will be + able to read/write it. + """ + if name == CHILDREN_INCLUDES: + return # This is a pseudo-field used in conjunction with BlockstoreChildrenData + field = self._getfield(block, name) + if field.scope in (Scope.children, Scope.parent): # lint-amnesty, pylint: disable=no-else-raise + # This field data store is focused on definition-level field data, and children/parent is mostly + # relevant at the usage level. Scope.parent doesn't even seem to be used? + raise NotImplementedError("Setting Scope.children/parent is not supported by LearningCoreFieldData.") + + if field.scope.user != UserScope.NONE: + raise InvalidScopeError("LearningCoreFieldData only supports UserScope.NONE fields") + + if field.scope.block not in (BlockScope.DEFINITION, BlockScope.USAGE): + raise InvalidScopeError( + f"LearningCoreFieldData does not support BlockScope.{field.scope.block} fields" + ) + # There is also BlockScope.TYPE but we don't need to support that; + # it's mostly relevant as Scope.preferences(UserScope.ONE, BlockScope.TYPE) + # Which would be handled by a user-aware FieldData implementation + + def get(self, block, name): + """ + Get the given field value from Blockstore + + If the XBlock has been making changes to its fields, the value will be + in self._get_active_block(block).changed_fields[name] + + Otherwise, the value comes from self.loaded_definitions which is a dict + of OLX file field data, keyed by the hash of the OLX file. + """ + self._check_field(block, name) + usage_key = block.scope_ids.usage_id + + # First check if it's on our dict of changed fields that haven't been + # persisted yet. + changed_fields = self.usage_keys_to_changed_fields.get(usage_key, {}) + if name in changed_fields: + value = changed_fields[name] + if value == DELETED: + raise KeyError # KeyError means use the default value, since this field was deliberately set to default + + loaded_fields = self.usage_keys_to_loaded_fields.get(usage_key, {}) + try: + return loaded_fields[name] + except KeyError: + log.exception( + "XBlock %s tried to read from field data (%s) that wasn't loaded from Learning Core!", + block.scope_ids.usage_id, + name, + ) + raise + + # Otherwise, check to see if it + entry = self._get_active_block(block) + if name in entry.changed_fields: + value = entry.changed_fields[name] + if value == DELETED: + raise KeyError # KeyError means use the default value, since this field was deliberately set to default + return value + try: + saved_fields = self.loaded_definitions[entry.olx_hash] + except KeyError: + if name == CHILDREN_INCLUDES: + # Special case: parse_xml calls add_node_as_child which calls 'block.children.append()' + # BEFORE parse_xml is done, and .append() needs to read the value of children. So + return [] # start with an empty list, it will get filled in. + # Otherwise, this is an anomalous get() before the XML was fully loaded: + # This could happen if an XBlock's parse_xml() method tried to read a field before setting it, + # if an XBlock read field data in its constructor (forbidden), or if an XBlock was loaded via + # some means other than runtime.get_block(). One way this can happen is if you log/print an XBlock during + # XML parsing, because ScopedStorageMixin.__repr__ will try to print all field values, and any fields which + # aren't mentioned in the XML (which are left at their default) will be "not loaded yet." + log.exception( + "XBlock %s tried to read from field data (%s) that wasn't loaded from Blockstore!", + block.scope_ids.usage_id, name, + ) + raise # Just use the default value for now; any exception raised here is caught anyways + return saved_fields[name] + # If 'name' is not found, this will raise KeyError, which means to use the default value + + def has_changes(self, block): + usage_key = block.scope_ids.usage_id + changed_fields = self.usage_keys_to_changed_fields.get(usage_key, {}) + return bool(changed_fields) + + def cache_fields(self, block): + """ + Cache field data: + This is called by the runtime after a block has parsed its OLX via its + parse_xml() methods and written all of its field values into this field + data store. The values will be stored in + self._get_active_block(block).changed_fields + so we know at this point that that isn't really "changed" field data, + it's the result of parsing the OLX. Save a copy into loaded_definitions. + """ + usage_key = block.scope_ids.usage_id + supported_scopes = {Scope.content, Scope.settings} + loaded_fields = { + key: getattr(block, key) + for key, field in block.fields.items() + if field.scope in supported_scopes + } + self.usage_keys_to_loaded_fields[usage_key] = loaded_fields + # Reset changed_fields to indicate this block hasn't actually made any field data changes, just loaded from XML: + if usage_key in self.usage_keys_to_changed_fields: + self.usage_keys_to_changed_fields[usage_key].clear() + + #self.usage_keys_to_loaded_fields[usage_key] = self.usage_keys_to_changed_fields[usage_key].copy() + #self.usage_keys_to_changed_fields[usage_key].clear() + + + def delete(self, block, name): + self.set(block, name, DELETED) + + def set(self, block, name, value): + usage_key = block.scope_ids.usage_id + changed_fields = self.usage_keys_to_changed_fields.get(usage_key, {}) + changed_fields[name] = value + self.usage_keys_to_changed_fields[usage_key] = changed_fields + + def default(self, block, name): + raise KeyError(name) + + +class LearningCoreDefinitionLocator(CheckFieldMixin, DefinitionKey): + CANONICAL_NAMESPACE = 'oel-cv' # openedx-learning component version + KEY_FIELDS = ('uuid',) + #uuid = UUID + #__slots__ = KEY_FIELDS + + def __init__(self, uuid: UUID | str): + pass + + +class LearningCoreOpaqueKeyReader(OpaqueKeyReader): + def get_definition_id(self, usage_id): + 1/0 + if not isinstance(usage_id, UsageKeyV2): + raise TypeError( + f"This version of get_definition_id doesn't support the key type for {usage_id}." + ) + + return get_learning_context_impl(usage_id).definition_for_usage(usage_id) + +class NullDefinitionKey(DefinitionKey): + """This isn't really used.""" + + + +class LearningCoreXBlockRuntime(BlockstoreXBlockRuntime): + """ + XBlock runtime that uses openedx-learning apps for content storage. + + This subclasses BlockstoreXBlockRuntime as a temporary measure so that we + keep having a mostly functional libraries experience while this class is + being developed. It will eventually stand alone. + """ + # UNUSED_DEF_KEY = DefinitionKey() + + def _get_component_from_usage_key(self, usage_key): + learning_package = LearningPackage.objects.get(key=str(usage_key.lib_key)) + component = Component.objects.get( + learning_package=learning_package, + namespace='xblock.v1', + type=usage_key.block_type, + local_key=usage_key.block_id, + ) + return component + + def get_block(self, usage_key, for_parent=None): + log.info("Executing get_block from Learning Core") + # We can do this more efficiently in a single query later, but for now + # just get it the easy way. + component = self._get_component_from_usage_key(usage_key) + component_version = component.versioning.draft + raw_content = component_version.raw_contents.get( + componentversionrawcontent__key="definition.xml" + ) + xml_str = raw_content.text_content.text + + + xml_node = etree.fromstring(xml_str) + block_type = usage_key.block_type + + # At this point, how much value do we really get out of explicitly + # differentiating definitions from usages? + def_id = self.id_reader.get_definition_id(usage_key) + + print(f"DEF_ID: {def_id}") + print(self.id_reader) + + keys = ScopeIds(self.user_id, block_type, None, usage_key) + + if xml_node.get("url_name", None): + log.warning("XBlock at %s should not specify an old-style url_name attribute.", usage_key) + block_class = self.mixologist.mix(self.load_block_type(block_type)) + if hasattr(block_class, 'parse_xml_new_runtime'): + # This is a (former) XModule with messy XML parsing code; let its parse_xml() method continue to work + # as it currently does in the old runtime, but let this parse_xml_new_runtime() method parse the XML in + # a simpler way that's free of tech debt, if defined. + # In particular, XmlMixin doesn't play well with this new runtime, so this is mostly about + # bypassing that mixin's code. + # When a former XModule no longer needs to support the old runtime, its parse_xml_new_runtime method + # should be removed and its parse_xml() method should be simplified to just call the super().parse_xml() + # plus some minor additional lines of code as needed. + block = block_class.parse_xml_new_runtime(xml_node, runtime=self, keys=keys) + else: + block = block_class.parse_xml(xml_node, runtime=self, keys=keys, id_generator=None) + + # Update field data with parsed values. We can't call .save() because it will call save_block(), below. + block.force_save_fields(block._get_fields_to_save()) # pylint: disable=protected-access + print("IS THIS YOUR SYSTEM!>!") + print(type(self.system.authored_data_store)) + self.system.authored_data_store.cache_fields(block) + # There is no way to set the parent via parse_xml, so do what + # HierarchyMixin would do: + if for_parent is not None: + block._parent_block = for_parent # pylint: disable=protected-access + block._parent_block_id = for_parent.scope_ids.usage_id # pylint: disable=protected-access + return block + + + def temp_save_block(self, block): + """ + Save any pending field data values to openedx-learning. + + This gets called by block.save() - do not call this directly. + """ + if not self.system.authored_data_store.has_changes(block): + return # No changes, so no action needed. + + # Verify that the user has permission to write to authored data in this + # learning context: + if self.user is not None: + learning_context = get_learning_context_impl(block.scope_ids.usage_id) + if not learning_context.can_edit_block(self.user, block.scope_ids.usage_id): + log.warning("User %s does not have permission to edit %s", self.user.username, block.scope_ids.usage_id) + raise RuntimeError("You do not have permission to edit this XBlock") + + serialized = serialize_modulestore_block_for_blockstore(block) + olx_str = serialized.olx_str + static_files = serialized.static_files + + + + + # Write the OLX file to the bundle: + draft_uuid = blockstore_api.get_or_create_bundle_draft( + definition_key.bundle_uuid, definition_key.draft_name + ).uuid + olx_path = definition_key.olx_path + blockstore_api.write_draft_file(draft_uuid, olx_path, olx_str) + # And the other files, if any: + olx_static_path = os.path.dirname(olx_path) + '/static/' + for fh in static_files: + new_path = olx_static_path + fh.name + blockstore_api.write_draft_file(draft_uuid, new_path, fh.data) + # Now invalidate the blockstore data cache for the bundle: + BundleCache(definition_key.bundle_uuid, draft_name=definition_key.draft_name).clear() diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 74771833fedf..f9e96aac8a7c 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -3,7 +3,7 @@ """ from __future__ import annotations import logging -from typing import Callable +from typing import Callable, Optional from urllib.parse import urljoin # pylint: disable=import-error import crum @@ -21,7 +21,8 @@ from xblock.exceptions import NoSuchServiceError from xblock.field_data import FieldData, SplitFieldData from xblock.fields import Scope, ScopeIds -from xblock.runtime import KvsFieldData, MemoryIdManager, Runtime +from xblock.field_data import FieldData +from xblock.runtime import IdReader, KvsFieldData, MemoryIdManager, Runtime from xmodule.errortracker import make_error_tracker from xmodule.contentstore.django import contentstore @@ -420,6 +421,8 @@ def __init__( handler_url: Callable[[UsageKey, str, UserType | None], str], student_data_mode: StudentDataMode, runtime_class: type[XBlockRuntime], + id_reader: Optional[IdReader] = None, + authored_data_store: Optional[FieldData] = None, ): """ args: @@ -436,10 +439,10 @@ def __init__( runtime_class: What runtime to use, e.g. BlockstoreXBlockRuntime """ self.handler_url = handler_url - self.id_reader = OpaqueKeyReader() + self.id_reader = id_reader or OpaqueKeyReader() self.id_generator = MemoryIdManager() # We don't really use id_generator until we need to support asides self.runtime_class = runtime_class - self.authored_data_store = BlockstoreFieldData() + self.authored_data_store = authored_data_store or BlockstoreFieldData() self.children_data_store = BlockstoreChildrenData(self.authored_data_store) assert student_data_mode in (StudentDataMode.Ephemeral, StudentDataMode.Persisted) self.student_data_mode = student_data_mode From 6bdacf9c2f27ff2f3339c1da588d41631363889d Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 26 Oct 2023 22:26:58 -0400 Subject: [PATCH 4/7] fix: handle case where there is no published verison --- openedx/core/djangoapps/content_libraries/api.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 248fe649258f..f20b89927447 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -773,7 +773,10 @@ def get_library_block_learning_core(usage_key): component__local_key=usage_key.block_id, ) draft_version = entity.draft.version - published_version = entity.published.version + if hasattr(entity, 'published'): + published_version = entity.published.version + else: + published_version = None return LibraryXBlockMetadata( usage_key=usage_key, From bb50ffbaebb2f6572cb4cf70c54718f9108af3e2 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 27 Oct 2023 09:48:06 -0400 Subject: [PATCH 5/7] fixup!: get rid of some confusing experimentation cruft --- .../xblock/runtime/blockstore_field_data.py | 13 +++++ .../xblock/runtime/blockstore_runtime.py | 3 +- .../xblock/runtime/learning_core_runtime.py | 48 +++++-------------- 3 files changed, 25 insertions(+), 39 deletions(-) diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py b/openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py index 217b8c5ea389..ac17a9550b0d 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py +++ b/openedx/core/djangoapps/xblock/runtime/blockstore_field_data.py @@ -140,6 +140,19 @@ def _check_field(self, block, name): # it's mostly relevant as Scope.preferences(UserScope.ONE, BlockScope.TYPE) # Which would be handled by a user-aware FieldData implementation + def _get_active_block(self, block): + """ + Get the ActiveBlock entry for the specified block, creating it if + necessary. + """ + key = get_weak_key_for_block(block) + if key not in self.active_blocks: + self.active_blocks[key] = ActiveBlock( + olx_hash=get_olx_hash_for_definition_key(block.scope_ids.def_id), + changed_fields={}, + ) + return self.active_blocks[key] + def get(self, block, name): """ Get the given field value from Blockstore diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py index 1a887aac26bf..33ea8514b632 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py @@ -33,14 +33,13 @@ class BlockstoreXBlockRuntime(XBlockRuntime): def parse_xml_file(self, fileobj, id_generator=None): raise NotImplementedError("Use parse_olx_file() instead") - def get_block(self, usage_key, for_parent=None): + def get_block(self, usage_id, for_parent=None): """ Create an XBlock instance in this runtime. Args: usage_key(OpaqueKey): identifier used to find the XBlock class and data. """ - usage_id = usage_key def_id = self.id_reader.get_definition_id(usage_id) if def_id is None: raise ValueError(f"Definition not found for usage {usage_id}") diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index f3a4f6e28092..81c0b9a9fa1e 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -122,35 +122,9 @@ def get(self, block, name): block.scope_ids.usage_id, name, ) + # If 'name' is not found, this will raise KeyError, which means to use the default value raise - # Otherwise, check to see if it - entry = self._get_active_block(block) - if name in entry.changed_fields: - value = entry.changed_fields[name] - if value == DELETED: - raise KeyError # KeyError means use the default value, since this field was deliberately set to default - return value - try: - saved_fields = self.loaded_definitions[entry.olx_hash] - except KeyError: - if name == CHILDREN_INCLUDES: - # Special case: parse_xml calls add_node_as_child which calls 'block.children.append()' - # BEFORE parse_xml is done, and .append() needs to read the value of children. So - return [] # start with an empty list, it will get filled in. - # Otherwise, this is an anomalous get() before the XML was fully loaded: - # This could happen if an XBlock's parse_xml() method tried to read a field before setting it, - # if an XBlock read field data in its constructor (forbidden), or if an XBlock was loaded via - # some means other than runtime.get_block(). One way this can happen is if you log/print an XBlock during - # XML parsing, because ScopedStorageMixin.__repr__ will try to print all field values, and any fields which - # aren't mentioned in the XML (which are left at their default) will be "not loaded yet." - log.exception( - "XBlock %s tried to read from field data (%s) that wasn't loaded from Blockstore!", - block.scope_ids.usage_id, name, - ) - raise # Just use the default value for now; any exception raised here is caught anyways - return saved_fields[name] - # If 'name' is not found, this will raise KeyError, which means to use the default value def has_changes(self, block): usage_key = block.scope_ids.usage_id @@ -197,6 +171,12 @@ def default(self, block, name): class LearningCoreDefinitionLocator(CheckFieldMixin, DefinitionKey): + """ + Skeleton of a new definition locator, in case we need it. + + Hopefully this isn't necessary at all. So far just passing Nones for def + keys and ignoring the def key elsewhere seems to be working out. + """ CANONICAL_NAMESPACE = 'oel-cv' # openedx-learning component version KEY_FIELDS = ('uuid',) #uuid = UUID @@ -208,17 +188,11 @@ def __init__(self, uuid: UUID | str): class LearningCoreOpaqueKeyReader(OpaqueKeyReader): def get_definition_id(self, usage_id): + """ + This is mostly here to make sure LearningCore-based things *don't* call + it. By making it explode if it's called. + """ 1/0 - if not isinstance(usage_id, UsageKeyV2): - raise TypeError( - f"This version of get_definition_id doesn't support the key type for {usage_id}." - ) - - return get_learning_context_impl(usage_id).definition_for_usage(usage_id) - -class NullDefinitionKey(DefinitionKey): - """This isn't really used.""" - class LearningCoreXBlockRuntime(BlockstoreXBlockRuntime): From 7b822816b8d03a091c3de882c85ea6066a87a724 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 27 Oct 2023 16:19:50 -0400 Subject: [PATCH 6/7] feat: stub in some placeholder asset handling --- .../xblock/runtime/learning_core_runtime.py | 41 +++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py index 81c0b9a9fa1e..f8f64348fcff 100644 --- a/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/learning_core_runtime.py @@ -16,6 +16,7 @@ from lxml import etree from opaque_keys.edx.keys import UsageKeyV2 +from xblock.core import XBlock from xblock.exceptions import InvalidScopeError, NoSuchDefinition from xblock.fields import Field, BlockScope, Scope, ScopeIds, UserScope, Sentinel from xblock.field_data import FieldData @@ -113,18 +114,25 @@ def get(self, block, name): if value == DELETED: raise KeyError # KeyError means use the default value, since this field was deliberately set to default - loaded_fields = self.usage_keys_to_loaded_fields.get(usage_key, {}) try: - return loaded_fields[name] + loaded_fields = self.usage_keys_to_loaded_fields[usage_key] except KeyError: + # If there's no entry for that usage key, then we're trying to read + # field data from a block that was never loaded, which we don't + # expect to happen. Log an exception for this. log.exception( "XBlock %s tried to read from field data (%s) that wasn't loaded from Learning Core!", block.scope_ids.usage_id, name, ) - # If 'name' is not found, this will raise KeyError, which means to use the default value raise + # If 'name' is not found, this will raise KeyError, which means to use + # the default value. This is expected–it means that we did load a block + # for it, but the block data didn't specify a value for this particular + # field. + return loaded_fields[name] + def has_changes(self, block): usage_key = block.scope_ids.usage_id @@ -215,6 +223,33 @@ def _get_component_from_usage_key(self, usage_key): ) return component + def _lookup_asset_url(self, block: XBlock, asset_path: str): # pylint: disable=unused-argument + """ + Return an absolute URL for the specified static asset file that may + belong to this XBlock. + + e.g. if the XBlock settings have a field value like "/static/foo.png" + then this method will be called with asset_path="foo.png" and should + return a URL like https://cdn.none/xblock/f843u89789/static/foo.png + + If the asset file is not recognized, return None + + This is called by the XBlockRuntime superclass in the .runtime module. + + CURRENT STATUS + + Right now we're not recognizing anything. We'd need to hook up something + to serve the static assets, and the biggest issue around that is + figuring out the permissions that need to be applied. + + Idea: Have openedx-learning provide a simple view that will stream the + content, but have apps explicitly subclass or wrap it with permissions + checks and such. That way the actual logic of figuring out the + permissions stays out of openedx-learning, since it requires access to + tables that don't exist there. + """ + return None + def get_block(self, usage_key, for_parent=None): log.info("Executing get_block from Learning Core") # We can do this more efficiently in a single query later, but for now From 93d70e4a8a9fb803d0f7a39c7d0ad891bb7828ca Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 16 Nov 2023 23:04:06 -0500 Subject: [PATCH 7/7] fixup: remove unintentional edit --- openedx/core/lib/blockstore_api/methods.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx/core/lib/blockstore_api/methods.py b/openedx/core/lib/blockstore_api/methods.py index 561d1dc5ecea..7d7c65decdc1 100644 --- a/openedx/core/lib/blockstore_api/methods.py +++ b/openedx/core/lib/blockstore_api/methods.py @@ -363,7 +363,6 @@ def get_bundle_files_dict(bundle_uuid, use_draft=None): BundleFileData or DraftFileData tuples. """ bundle = get_bundle(bundle_uuid) - if use_draft and use_draft in bundle.drafts: # pylint: disable=unsupported-membership-test draft_uuid = bundle.drafts[use_draft] # pylint: disable=unsubscriptable-object return get_draft(draft_uuid).files