From 8d03e22ce00f0423a1d210f4f86111abdf59b32e Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 1 Feb 2024 15:26:40 -0500 Subject: [PATCH] refactor: normalize component types with ComponentType (#148) 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 | 48 ++++++---- .../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, 142 insertions(+), 85 deletions(-) diff --git a/olx_importer/management/commands/load_components.py b/olx_importer/management/commands/load_components.py index 6b8d2e42..575f01ed 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 c5fb9b16..1e84391b 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 7891ed37..2bec7b34 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -18,15 +18,28 @@ 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. + """ + 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 +47,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 +55,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 +175,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 +186,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 +211,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 +220,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 +230,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 +240,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 +257,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 +277,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 fdfb5e40..b72d1aee 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 b7eba42d..920b3efb 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 923dc119..0b27690a 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 69e50364..b1d46929 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 9342e232..d4bb20e3 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 c880a008..96390cb4 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