From fdc11eef115de39abed5f38fa10c2b0682fa7e66 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Thu, 8 Feb 2024 23:54:56 -0500 Subject: [PATCH] refactor: incorporate suggestions from Kyle's review --- .../management/commands/load_components.py | 8 +++--- openedx_learning/__init__.py | 2 +- .../contrib/media_server/views.py | 4 +-- openedx_learning/core/components/api.py | 12 +++++---- openedx_learning/core/contents/api.py | 15 +++++------ openedx_learning/core/contents/models.py | 25 +++++++++++++------ openedx_learning/core/publishing/models.py | 22 +++++++++++++--- .../core/components/test_api.py | 22 ++++++++-------- 8 files changed, 68 insertions(+), 42 deletions(-) diff --git a/olx_importer/management/commands/load_components.py b/olx_importer/management/commands/load_components.py index b08761ca..49c07b1e 100644 --- a/olx_importer/management/commands/load_components.py +++ b/olx_importer/management/commands/load_components.py @@ -122,9 +122,9 @@ def create_content(self, static_local_path, now, component_version): mime_type=mime_type, created=now, ) - components_api.add_content_to_component_version( + components_api.create_component_version_content( component_version, - content_id=content.id, + content.id, key=key, learner_downloadable=True, ) @@ -173,9 +173,9 @@ def import_block_type(self, block_type_name, now): # , publish_log_entry): created=now, ) # Add the OLX source text to the ComponentVersion - components_api.add_content_to_component_version( + components_api.create_component_version_content( component_version, - content_id=text_content.pk, + text_content.pk, key="block.xml", learner_downloadable=False ) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index fa8a6955..916c50b1 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.5.2" +__version__ = "0.6.0" diff --git a/openedx_learning/contrib/media_server/views.py b/openedx_learning/contrib/media_server/views.py index 752c915c..79a8abdd 100644 --- a/openedx_learning/contrib/media_server/views.py +++ b/openedx_learning/contrib/media_server/views.py @@ -8,7 +8,7 @@ from django.core.exceptions import ObjectDoesNotExist, PermissionDenied from django.http import FileResponse, Http404 -from openedx_learning.core.components.api import get_component_version_content +from openedx_learning.core.components.api import look_up_component_version_content def component_asset( @@ -28,7 +28,7 @@ def component_asset( * Serving from a different domain than the rest of the service """ try: - cvc = get_component_version_content( + cvc = look_up_component_version_content( learning_package_key, component_key, version_num, asset_path ) except ObjectDoesNotExist: diff --git a/openedx_learning/core/components/api.py b/openedx_learning/core/components/api.py index 373ef050..d0c6632f 100644 --- a/openedx_learning/core/components/api.py +++ b/openedx_learning/core/components/api.py @@ -285,17 +285,19 @@ def get_components( 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 + Q(publishable_entity__draft__version__title__icontains=draft_title) | + Q(local_key__icontains=draft_title) ) if published_title is not None: qset = qset.filter( - publishable_entity__published__version__title__icontains=published_title + Q(publishable_entity__published__version__title__icontains=published_title) | + Q(local_key__icontains=published_title) ) return qset -def get_component_version_content( +def look_up_component_version_content( learning_package_key: str, component_key: str, version_num: int, @@ -323,10 +325,10 @@ def get_component_version_content( ).get(queries) -def add_content_to_component_version( +def create_component_version_content( component_version_id: int, - /, content_id: int, + /, key: str, learner_downloadable=False, ) -> ComponentVersionContent: diff --git a/openedx_learning/core/contents/api.py b/openedx_learning/core/contents/api.py index ae88204e..06ec49ac 100644 --- a/openedx_learning/core/contents/api.py +++ b/openedx_learning/core/contents/api.py @@ -64,8 +64,8 @@ def get_content(content_id: int, /) -> Content: def get_or_create_text_content( learning_package_id: int, - /, media_type_id: int, + /, text: str, created: datetime, create_file: bool = False, @@ -86,10 +86,6 @@ def get_or_create_text_content( If you want to create a large text file, or want to create a text file that doesn't need to be stored in the database, call ``create_file_content`` instead of this function. - - The `hash_digest` is included as an optional argument because a very common - pattern is going to be "I have this data, let's see if a Content already - exists for it." """ text_as_bytes = text.encode('utf-8') hash_digest = create_hash_digest(text_as_bytes) @@ -122,8 +118,8 @@ def get_or_create_text_content( def get_or_create_file_content( learning_package_id: int, - /, media_type_id: int, + /, data: bytes, created: datetime, ) -> Content: @@ -131,9 +127,10 @@ def get_or_create_file_content( Get or create a Content with data stored in a file storage backend. Use this function to store non-text data, large data, or data where low - latency access is not necessary. Also use this function to store any Content - that you want to be downloadable by browsers in the LMS since the static - asset serving system will only work with file-backed Content. + latency access is not necessary. Also use this function (or + ``get_or_create_text_content`` with ``create_file=True``) to store any + Content that you want to be downloadable by browsers in the LMS, since the + static asset serving system will only work with file-backed Content. """ hash_digest = create_hash_digest(data) with atomic(): diff --git a/openedx_learning/core/contents/models.py b/openedx_learning/core/contents/models.py index 0b0e110c..58565c07 100644 --- a/openedx_learning/core/contents/models.py +++ b/openedx_learning/core/contents/models.py @@ -18,7 +18,7 @@ from ..publishing.models import LearningPackage -def get_storage_class() -> Storage: +def get_storage() -> Storage: """ Return the Storage instance for our Content file persistence. @@ -227,7 +227,7 @@ class Content(models.Model): # This hash value may be calculated using create_hash_digest from the # openedx.lib.fields module. When storing text, we hash the UTF-8 # encoding of that text value, regardless of whether we also write it to a - # file or not. + # file or not. When storing just a file, we hash the bytes in the file. hash_digest = hash_field() # Do we have file data stored for this Content in our file storage backend? @@ -263,17 +263,28 @@ class Content(models.Model): created = manual_date_time_field() @cached_property - def mime_type(self): + def mime_type(self) -> str: + """ + The IANA media type (a.k.a. MIME type) of the Content, in string form. + + MIME types reference: + https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types + """ return str(self.media_type) def file_path(self): + """ + Path at which this content is stored (or would be stored). + + This path is relative to configured storage root. + """ return f"{self.learning_package.uuid}/{self.hash_digest}" - def write_file(self, file: File): + def write_file(self, file: File) -> None: """ Write file contents to the file storage backend. """ - storage = get_storage_class() + storage = get_storage() file_path = self.file_path() # There are two reasons why a file might already exist even if the the @@ -290,11 +301,11 @@ def write_file(self, file: File): if not storage.exists(file_path): storage.save(file_path, file) - def file_url(self): + def file_url(self) -> str: """ This will sometimes be a time-limited signed URL. """ - return get_storage_class().url(self.file_path()) + return get_storage().url(self.file_path()) def clean(self): """ diff --git a/openedx_learning/core/publishing/models.py b/openedx_learning/core/publishing/models.py index 43928d7c..3f668a9f 100644 --- a/openedx_learning/core/publishing/models.py +++ b/openedx_learning/core/publishing/models.py @@ -30,10 +30,11 @@ class LearningPackage(models.Model): # type: ignore[django-manager-missing] Each PublishableEntity belongs to exactly one LearningPackage. """ + # Explictly declare a 4-byte ID instead of using the app-default 8-byte ID. # We do not expect to have more than 2 billion LearningPackages on a given - # site, but many, many things have foreign keys to this model and uniqueness - # indexes on those foreign keys + their own fields, so going from the - # app-default of 8-byte IDs to 4-byte IDs will add up over time. + # site. Furthermore, many, many things have foreign keys to this model and + # uniqueness indexes on those foreign keys + their own fields, so the 4 + # bytes saved will add up over time. id = models.AutoField(primary_key=True) uuid = immutable_uuid_field() @@ -366,6 +367,21 @@ class PublishLog(models.Model): Open question: Empty publishes are allowed at this time, and might be useful for "fake" publishes that are necessary to invoke other post-publish actions. It's not clear at this point how useful this will actually be. + + The absence of a ``version_num`` field in this model is intentional, because + having one would potentially cause write contention/locking issues when + there are many people working on different entities in a very large library. + We already see some contention issues occuring in ModuleStore for courses, + and we want to support Libraries that are far larger. + + If you need a LearningPackage-wide indicator for version and the only thing + you care about is "has *something* changed?", you can make a foreign key to + the most recent PublishLog, or use the most recent PublishLog's primary key. + This should be monotonically increasing, though there will be large gaps in + values, e.g. (5, 190, 1291, etc.). Be warned that this value will not port + across sites. If you need site-portability, the UUIDs for this model are a + safer bet, though there's a lot about import/export that we haven't fully + mapped out yet. """ uuid = immutable_uuid_field() diff --git a/tests/openedx_learning/core/components/test_api.py b/tests/openedx_learning/core/components/test_api.py index 36d83fb9..cd5543ed 100644 --- a/tests/openedx_learning/core/components/test_api.py +++ b/tests/openedx_learning/core/components/test_api.py @@ -100,7 +100,7 @@ def setUpTestData(cls) -> None: cls.published_problem, _version = components_api.create_component_and_version( cls.learning_package.id, component_type=v2_problem_type, - local_key="published_problem", + local_key="pp_lk", title="Published Problem", created=cls.now, created_by=None, @@ -108,7 +108,7 @@ def setUpTestData(cls) -> None: cls.published_html, _version = components_api.create_component_and_version( cls.learning_package.id, component_type=cls.html_type, - local_key="published_html", + local_key="ph_lk", title="Published HTML", created=cls.now, created_by=None, @@ -122,7 +122,7 @@ def setUpTestData(cls) -> None: cls.unpublished_problem, _version = components_api.create_component_and_version( cls.learning_package.id, component_type=v2_problem_type, - local_key="unpublished_problem", + local_key="upp_lk", title="Unpublished Problem", created=cls.now, created_by=None, @@ -130,7 +130,7 @@ def setUpTestData(cls) -> None: cls.unpublished_html, _version = components_api.create_component_and_version( cls.learning_package.id, component_type=cls.html_type, - local_key="unpublished_html", + local_key="uph_lk", title="Unpublished HTML", created=cls.now, created_by=None, @@ -141,7 +141,7 @@ def setUpTestData(cls) -> None: cls.deleted_video, _version = components_api.create_component_and_version( cls.learning_package.id, component_type=cls.video_type, - local_key="deleted_video", + local_key="dv_lk", title="Deleted Video", created=cls.now, created_by=None, @@ -369,13 +369,13 @@ def test_add(self): ) new_content = contents_api.get_or_create_text_content( self.learning_package.pk, - media_type_id=self.text_media_type.id, + self.text_media_type.id, text="This is some data", created=self.now, ) - components_api.add_content_to_component_version( + components_api.create_component_version_content( new_version.pk, - content_id=new_content.pk, + new_content.pk, key="hello.txt", learner_downloadable=False, ) @@ -391,19 +391,19 @@ def test_add(self): def test_multiple_versions(self): hello_content = contents_api.get_or_create_text_content( self.learning_package.id, - media_type_id=self.text_media_type.id, + self.text_media_type.id, text="Hello World!", created=self.now, ) goodbye_content = contents_api.get_or_create_text_content( self.learning_package.id, - media_type_id=self.text_media_type.id, + self.text_media_type.id, text="Goodbye World!", created=self.now, ) blank_content = contents_api.get_or_create_text_content( self.learning_package.id, - media_type_id=self.text_media_type.id, + self.text_media_type.id, text="", created=self.now, )