From 7c571fb5440310a7f16fe5a166a29cf48eab468a Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 31 Jan 2024 00:36:55 -0500 Subject: [PATCH 1/2] refactor: normalize component types with ComponentType MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, Components encoded their namespace + type info as two columns on the Component model itself. This wasted a lot of space on a very common index lookup–many rows of ('xblock.v1', 'video'), ('xblock.v1', 'problem'), and ('xblock.v1', 'text'). But worse, the lack of a separate ComponentType model meant that there was no first class entity to store per-type policy data against. For example, we might want to say "the following types are supported by libraries, while these other types are experimental", or "these types are enabled for these orgs", etc. Components are required to have a ComponentType. We're rewriting the first migration for the components app here, since this app hasn't been added to edx-platform yet. --- .../management/commands/load_components.py | 2 +- openedx_learning/core/components/admin.py | 7 +- openedx_learning/core/components/api.py | 50 ++++++---- .../components/migrations/0001_initial.py | 29 ++++-- openedx_learning/core/components/models.py | 92 +++++++++++++------ openedx_learning/core/contents/api.py | 9 +- .../core/components/test_api.py | 28 +++--- .../core/components/test_models.py | 2 +- .../core/contents/test_media_types.py | 10 +- 9 files changed, 144 insertions(+), 85 deletions(-) diff --git a/olx_importer/management/commands/load_components.py b/olx_importer/management/commands/load_components.py index 6b8d2e423..575f01edd 100644 --- a/olx_importer/management/commands/load_components.py +++ b/olx_importer/management/commands/load_components.py @@ -158,7 +158,7 @@ def import_block_type(self, block_type, now): # , publish_log_entry): _component, component_version = components_api.create_component_and_version( self.learning_package.id, namespace=namespace, - type=block_type, + type_name=block_type, local_key=local_key, title=display_name, created=now, diff --git a/openedx_learning/core/components/admin.py b/openedx_learning/core/components/admin.py index c5fb9b16f..1e84391bc 100644 --- a/openedx_learning/core/components/admin.py +++ b/openedx_learning/core/components/admin.py @@ -35,16 +35,15 @@ class ComponentAdmin(ReadOnlyModelAdmin): """ Django admin configuration for Component """ - list_display = ("key", "uuid", "namespace", "type", "created") + list_display = ("key", "uuid", "component_type", "created") readonly_fields = [ "learning_package", "uuid", - "namespace", - "type", + "component_type", "key", "created", ] - list_filter = ("type", "learning_package") + list_filter = ("component_type", "learning_package") search_fields = ["publishable_entity__uuid", "publishable_entity__key"] inlines = [ComponentVersionInline] diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 7891ed372..c08acde19 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -18,15 +18,30 @@ from django.db.models import Q, QuerySet from django.db.transaction import atomic +from ...lib.cache import lru_cache from ..publishing import api as publishing_api -from .models import Component, ComponentVersion, ComponentVersionRawContent +from .models import Component, ComponentType, ComponentVersion, ComponentVersionRawContent + + +@lru_cache(maxsize=128) +def get_or_create_component_type_id(namespace: str, name: str) -> int: + """ + Get the ID of a ComponentType, and create if missing. + + An example of + """ + component_type, _created = ComponentType.objects.get_or_create( + namespace=namespace, + name=name, + ) + return component_type.id def create_component( learning_package_id: int, /, namespace: str, - type: str, # pylint: disable=redefined-builtin + type_name: str, local_key: str, created: datetime, created_by: int | None, @@ -34,7 +49,7 @@ def create_component( """ Create a new Component (an entity like a Problem or Video) """ - key = f"{namespace}:{type}@{local_key}" + key = f"{namespace}:{type_name}@{local_key}" with atomic(): publishable_entity = publishing_api.create_publishable_entity( learning_package_id, key, created, created_by @@ -42,8 +57,7 @@ def create_component( component = Component.objects.create( publishable_entity=publishable_entity, learning_package_id=learning_package_id, - namespace=namespace, - type=type, + component_type_id=get_or_create_component_type_id(namespace, type_name), local_key=local_key, ) return component @@ -163,7 +177,7 @@ def create_component_and_version( learning_package_id: int, /, namespace: str, - type: str, # pylint: disable=redefined-builtin + type_name: str, local_key: str, title: str, created: datetime, @@ -174,7 +188,7 @@ def create_component_and_version( """ with atomic(): component = create_component( - learning_package_id, namespace, type, local_key, created, created_by + learning_package_id, namespace, type_name, local_key, created, created_by ) component_version = create_component_version( component.pk, @@ -199,7 +213,7 @@ def get_component_by_key( learning_package_id: int, /, namespace: str, - type: str, # pylint: disable=redefined-builtin + type_name: str, local_key: str, ) -> Component: """ @@ -208,8 +222,8 @@ def get_component_by_key( return Component.with_publishing_relations \ .get( learning_package_id=learning_package_id, - namespace=namespace, - type=type, + component_type__namespace=namespace, + component_type__name=type_name, local_key=local_key, ) @@ -218,7 +232,7 @@ def component_exists_by_key( learning_package_id: int, /, namespace: str, - type: str, # pylint: disable=redefined-builtin + type_name: str, local_key: str ) -> bool: """ @@ -228,10 +242,10 @@ def component_exists_by_key( no current Draft version for it), or if it's been unpublished. """ try: - _component = Component.objects.only('pk').get( + _component = Component.objects.only('pk', 'component_type').get( learning_package_id=learning_package_id, - namespace=namespace, - type=type, + component_type__namespace=namespace, + component_type__name=type_name, local_key=local_key, ) return True @@ -245,7 +259,7 @@ def get_components( draft: bool | None = None, published: bool | None = None, namespace: str | None = None, - types: list[str] | None = None, + type_names: list[str] | None = None, draft_title: str | None = None, published_title: str | None = None, ) -> QuerySet[Component]: @@ -265,9 +279,9 @@ def get_components( if published is not None: qset = qset.filter(publishable_entity__published__version__isnull=not published) if namespace is not None: - qset = qset.filter(namespace=namespace) - if types is not None: - qset = qset.filter(type__in=types) + qset = qset.filter(component_type__namespace=namespace) + if type_names is not None: + qset = qset.filter(component_type__name__in=type_names) if draft_title is not None: qset = qset.filter( publishable_entity__draft__version__title__icontains=draft_title diff --git a/openedx_learning/core/components/migrations/0001_initial.py b/openedx_learning/core/components/migrations/0001_initial.py index fdfb5e40b..b72d1aee8 100644 --- a/openedx_learning/core/components/migrations/0001_initial.py +++ b/openedx_learning/core/components/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.23 on 2024-01-22 00:38 +# Generated by Django 3.2.23 on 2024-01-31 05:34 import uuid @@ -13,8 +13,8 @@ class Migration(migrations.Migration): initial = True dependencies = [ - ('oel_contents', '0001_initial'), ('oel_publishing', '0001_initial'), + ('oel_contents', '0001_initial'), ] operations = [ @@ -22,16 +22,21 @@ class Migration(migrations.Migration): name='Component', fields=[ ('publishable_entity', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity')), - ('namespace', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)), - ('type', openedx_learning.lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)), ('local_key', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=500)), - ('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage')), ], options={ 'verbose_name': 'Component', 'verbose_name_plural': 'Components', }, ), + migrations.CreateModel( + name='ComponentType', + fields=[ + ('id', models.AutoField(primary_key=True, serialize=False)), + ('namespace', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)), + ('name', openedx_learning.lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)), + ], + ), migrations.CreateModel( name='ComponentVersion', fields=[ @@ -59,6 +64,16 @@ class Migration(migrations.Migration): name='raw_contents', field=models.ManyToManyField(related_name='component_versions', through='oel_components.ComponentVersionRawContent', to='oel_contents.RawContent'), ), + migrations.AddField( + model_name='component', + name='component_type', + field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='oel_components.componenttype'), + ), + migrations.AddField( + model_name='component', + name='learning_package', + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage'), + ), migrations.AddIndex( model_name='componentversionrawcontent', index=models.Index(fields=['raw_content', 'component_version'], name='oel_cvrawcontent_c_cv'), @@ -73,10 +88,10 @@ class Migration(migrations.Migration): ), migrations.AddIndex( model_name='component', - index=models.Index(fields=['learning_package', 'namespace', 'type', 'local_key'], name='oel_component_idx_lc_ns_t_lk'), + index=models.Index(fields=['component_type', 'local_key'], name='oel_component_idx_ct_lk'), ), migrations.AddConstraint( model_name='component', - constraint=models.UniqueConstraint(fields=('learning_package', 'namespace', 'type', 'local_key'), name='oel_component_uniq_lc_ns_t_lk'), + constraint=models.UniqueConstraint(fields=('learning_package', 'component_type', 'local_key'), name='oel_component_uniq_lc_ct_lk'), ), ] diff --git a/openedx_learning/core/components/models.py b/openedx_learning/core/components/models.py index b7eba42d6..920b3efb9 100644 --- a/openedx_learning/core/components/models.py +++ b/openedx_learning/core/components/models.py @@ -28,6 +28,45 @@ from ..publishing.models import LearningPackage +class ComponentType(models.Model): + """ + Normalized representation of a type of Component. + + The only namespace being used initially will be 'xblock.v1', but we will + probably add a few others over time, such as a component type to represent + packages of files for things like Files and Uploads or python_lib.zip files. + + Make a ForeignKey against this table if you have to set policy based on the + type of Components–e.g. marking certain types of XBlocks as approved vs. + experimental for use in libraries. + """ + id = models.AutoField(primary_key=True) + + # namespace and name work together to help figure out what Component needs + # to handle this data. A namespace is *required*. The namespace for XBlocks + # is "xblock.v1" (to match the setup.py entrypoint naming scheme). + namespace = case_sensitive_char_field(max_length=100, blank=False) + + # name is a way to help sub-divide namespace if that's convenient. This + # field cannot be null, but it can be blank if it's not necessary. For an + # XBlock, this corresponds to tag, e.g. "video". It's also the block_type in + # the UsageKey. + name = case_sensitive_char_field(max_length=100, blank=True) + + constraints = [ + models.UniqueConstraint( + fields=[ + "namespace", + "name", + ], + name="oel_component_type_uniq_ns_n", + ), + ] + + def __str__(self): + return f"{self.namespace}:{self.name}" + + class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] """ This represents any Component that has ever existed in a LearningPackage. @@ -63,7 +102,7 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] ----------------- The ``key`` field on Component's ``publishable_entity`` is dervied from the - ``(namespace, type, local_key)`` fields in this model. We don't support + ``component_type`` and ``local_key`` fields in this model. We don't support changing the keys yet, but if we do, those values need to be kept in sync. How build on this model @@ -75,9 +114,12 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] # Tell mypy what type our objects manager has. # It's actually PublishableEntityMixinManager, but that has the exact same # interface as the base manager class. - objects: models.Manager[Component] + objects: models.Manager[Component] = WithRelationsManager( + 'component_type' + ) - with_publishing_relations = WithRelationsManager( + with_publishing_relations: models.Manager[Component] = WithRelationsManager( + 'component_type', 'publishable_entity', 'publishable_entity__draft__version', 'publishable_entity__draft__version__componentversion', @@ -93,53 +135,43 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing] # columns from different tables). learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) - # namespace and type work together to help figure out what Component needs - # to handle this data. A namespace is *required*. The namespace for XBlocks - # is "xblock.v1" (to match the setup.py entrypoint naming scheme). - namespace = case_sensitive_char_field(max_length=100, blank=False) + # What kind of Component are we? This will usually represent a specific + # XBlock block_type, but we want it to be more flexible in the long term. + component_type = models.ForeignKey(ComponentType, on_delete=models.PROTECT) - # type is a way to help sub-divide namespace if that's convenient. This - # field cannot be null, but it can be blank if it's not necessary. For an - # XBlock, type corresponds to tag, e.g. "video". It's also the block_type in - # the UsageKey. - type = case_sensitive_char_field(max_length=100, blank=True) - - # local_key is an identifier that is local to the (namespace, type). The - # publishable.key should be calculated as a combination of (namespace, type, - # local_key). + # local_key is an identifier that is local to the learning_package and + # component_type. The publishable.key should be calculated as a + # combination of component_type and local_key. local_key = key_field() class Meta: constraints = [ - # The combination of (namespace, type, local_key) is unique within + # The combination of (component_type, local_key) is unique within # a given LearningPackage. Note that this means it is possible to - # have two Components that have the exact same local_key. An XBlock - # would be modeled as namespace="xblock.v1" with the type as the - # block_type, so the local_key would only be the block_id (the - # very last part of the UsageKey). + # have two Components in the same LearningPackage to have the same + # local_key if the component_types are different. So for example, + # you could have a ProblemBlock and VideoBlock that both have the + # local_key "week_1". models.UniqueConstraint( fields=[ "learning_package", - "namespace", - "type", + "component_type", "local_key", ], - name="oel_component_uniq_lc_ns_t_lk", + name="oel_component_uniq_lc_ct_lk", ), ] indexes = [ - # Global Namespace/Type/Local-Key Index: + # Global Component-Type/Local-Key Index: # * Search by the different Components fields across all Learning # Packages on the site. This would be a support-oriented tool # from Django Admin. models.Index( fields=[ - "learning_package", - "namespace", - "type", + "component_type", "local_key", ], - name="oel_component_idx_lc_ns_t_lk", + name="oel_component_idx_ct_lk", ), ] @@ -148,7 +180,7 @@ class Meta: verbose_name_plural = "Components" def __str__(self): - return f"{self.namespace}:{self.type}:{self.local_key}" + return f"{self.component_type.namespace}:{self.component_type.name}:{self.local_key}" class ComponentVersion(PublishableEntityVersionMixin): diff --git a/openedx_learning/core/contents/api.py b/openedx_learning/core/contents/api.py index 923dc1192..0b27690a1 100644 --- a/openedx_learning/core/contents/api.py +++ b/openedx_learning/core/contents/api.py @@ -12,9 +12,8 @@ from django.core.files.base import ContentFile from django.db.transaction import atomic -from openedx_learning.lib.cache import lru_cache -from openedx_learning.lib.fields import create_hash_digest - +from ...lib.cache import lru_cache +from ...lib.fields import create_hash_digest from .models import MediaType, RawContent, TextContent @@ -33,7 +32,7 @@ def create_raw_content( raw_content = RawContent.objects.create( learning_package_id=learning_package_id, - media_type_id=get_media_type_id(mime_type), + media_type_id=get_or_create_media_type_id(mime_type), hash_digest=hash_digest, size=len(data_bytes), created=created, @@ -58,7 +57,7 @@ def create_text_from_raw_content(raw_content: RawContent, encoding="utf-8-sig") @lru_cache(maxsize=128) -def get_media_type_id(mime_type: str) -> int: +def get_or_create_media_type_id(mime_type: str) -> int: """ Return the MediaType.id for the desired mime_type string. diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 69e503643..b1d46929c 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -46,7 +46,7 @@ def test_component_num_queries(self) -> None: component, _version = components_api.create_component_and_version( self.learning_package.id, namespace="xblock.v1", - type="problem", + type_name="problem", local_key="Query Counting", title="Querying Counting Problem", created=self.now, @@ -97,7 +97,7 @@ def setUpTestData(cls) -> None: cls.published_problem, _version = components_api.create_component_and_version( cls.learning_package.id, namespace="xblock.v2", - type="problem", + type_name="problem", local_key="published_problem", title="Published Problem", created=cls.now, @@ -106,7 +106,7 @@ def setUpTestData(cls) -> None: cls.published_html, _version = components_api.create_component_and_version( cls.learning_package.id, namespace="xblock.v1", - type="html", + type_name="html", local_key="published_html", title="Published HTML", created=cls.now, @@ -121,7 +121,7 @@ def setUpTestData(cls) -> None: cls.unpublished_problem, _version = components_api.create_component_and_version( cls.learning_package.id, namespace="xblock.v2", - type="problem", + type_name="problem", local_key="unpublished_problem", title="Unpublished Problem", created=cls.now, @@ -130,7 +130,7 @@ def setUpTestData(cls) -> None: cls.unpublished_html, _version = components_api.create_component_and_version( cls.learning_package.id, namespace="xblock.v1", - type="html", + type_name="html", local_key="unpublished_html", title="Unpublished HTML", created=cls.now, @@ -142,7 +142,7 @@ def setUpTestData(cls) -> None: cls.deleted_video, _version = components_api.create_component_and_version( cls.learning_package.id, namespace="xblock.v1", - type="html", + type_name="html", local_key="deleted_video", title="Deleted Video", created=cls.now, @@ -230,7 +230,7 @@ def test_types_filter(self): """ html_and_video_components = components_api.get_components( self.learning_package.id, - types=['html', 'video'] + type_names=['html', 'video'] ) assert list(html_and_video_components) == [ self.published_html, @@ -300,7 +300,7 @@ def setUpTestData(cls) -> None: cls.problem = components_api.create_component( cls.learning_package.id, namespace='xblock.v1', - type='problem', + type_name='problem', local_key='my_component', created=cls.now, created_by=None, @@ -308,7 +308,7 @@ def setUpTestData(cls) -> None: cls.html = components_api.create_component( cls.learning_package.id, namespace='xblock.v1', - type='html', + type_name='html', local_key='my_component', created=cls.now, created_by=None, @@ -323,14 +323,14 @@ def test_get_by_key(self): assert self.html == components_api.get_component_by_key( self.learning_package.id, namespace='xblock.v1', - type='html', + type_name='html', local_key='my_component', ) with self.assertRaises(ObjectDoesNotExist): components_api.get_component_by_key( self.learning_package.id, namespace='xblock.v1', - type='video', # 'video' doesn't match anything we have + type_name='video', # 'video' doesn't match anything we have local_key='my_component', ) @@ -338,13 +338,13 @@ def test_exists_by_key(self): assert components_api.component_exists_by_key( self.learning_package.id, namespace='xblock.v1', - type='problem', + type_name='problem', local_key='my_component', ) assert not components_api.component_exists_by_key( self.learning_package.id, namespace='xblock.v1', - type='problem', + type_name='problem', local_key='not_my_component', ) @@ -367,7 +367,7 @@ def setUpTestData(cls) -> None: cls.problem = components_api.create_component( cls.learning_package.id, namespace='xblock.v1', - type='problem', + type_name='problem', local_key='my_component', created=cls.now, created_by=None, diff --git a/tests/openedx_learning/core/components/test_models.py b/tests/openedx_learning/core/components/test_models.py index 9342e2328..d4bb20e3d 100644 --- a/tests/openedx_learning/core/components/test_models.py +++ b/tests/openedx_learning/core/components/test_models.py @@ -27,7 +27,7 @@ def test_latest_version(self) -> None: component, component_version = create_component_and_version( self.learning_package.id, namespace="xblock.v1", - type="problem", + type_name="problem", local_key="monty_hall", title="Monty Hall Problem", created=self.now, diff --git a/tests/openedx_learning/core/contents/test_media_types.py b/tests/openedx_learning/core/contents/test_media_types.py index c880a008b..96390cb45 100644 --- a/tests/openedx_learning/core/contents/test_media_types.py +++ b/tests/openedx_learning/core/contents/test_media_types.py @@ -11,18 +11,18 @@ class MediaTypeCachingTestCase(TestCase): """ def test_media_query_caching(self): """Test MediaType queries auto-create and caching.""" - assert contents_api.get_media_type_id.cache_info().currsize == 0 + assert contents_api.get_or_create_media_type_id.cache_info().currsize == 0 mime_type_str = "application/vnd.openedx.xblock.v1.problem+xml" - media_type_id = contents_api.get_media_type_id(mime_type_str) + media_type_id = contents_api.get_or_create_media_type_id(mime_type_str) # Now it should be loaded in the cache - assert contents_api.get_media_type_id.cache_info().currsize == 1 + assert contents_api.get_or_create_media_type_id.cache_info().currsize == 1 # Second call pulls from cache instead of the database with self.assertNumQueries(0): # Should also return the same thing it did last time. - assert media_type_id == contents_api.get_media_type_id(mime_type_str) + assert media_type_id == contents_api.get_or_create_media_type_id(mime_type_str) def test_media_query_caching_reset(self): """ @@ -31,4 +31,4 @@ def test_media_query_caching_reset(self): This test method's *must* execute after test_media_query_caching to be meaningful (they execute in string sort order). """ - assert contents_api.get_media_type_id.cache_info().currsize == 0 + assert contents_api.get_or_create_media_type_id.cache_info().currsize == 0 From fc33d66950bc5c167f53604cd3f9ba9568507e4e Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 31 Jan 2024 15:19:06 -0500 Subject: [PATCH 2/2] fix: remove incomplete comment thought --- openedx_learning/core/components/api.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index c08acde19..2bec7b340 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -27,8 +27,6 @@ def get_or_create_component_type_id(namespace: str, name: str) -> int: """ Get the ID of a ComponentType, and create if missing. - - An example of """ component_type, _created = ComponentType.objects.get_or_create( namespace=namespace,