-
Notifications
You must be signed in to change notification settings - Fork 27
feat: handle file content logic when creating next component version #248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
ff8f794
7226354
cfe4984
da5ecfc
d41cc64
c6bb9e8
07b4d07
74ac2d9
d92e8e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ | |
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import mimetypes | ||
| from datetime import datetime, timezone | ||
| from enum import StrEnum, auto | ||
| from logging import getLogger | ||
|
|
@@ -129,7 +130,7 @@ def create_component_version( | |
| def create_next_component_version( | ||
| component_pk: int, | ||
| /, | ||
| content_to_replace: dict[str, int | None], | ||
| content_to_replace: dict[str, int | None | bytes], | ||
| created: datetime, | ||
| title: str | None = None, | ||
| created_by: int | None = None, | ||
|
|
@@ -143,8 +144,8 @@ def create_next_component_version( | |
| API, since ``content_to_replace`` needs Content IDs for the values. | ||
|
|
||
| The ``content_to_replace`` dict is a mapping of strings representing the | ||
| local path/key for a file, to ``Content.id`` values. Using a `None` for | ||
| a value in this dict means to delete that key in the next version. | ||
| local path/key for a file, to ``Content.id`` or content bytes values. Using | ||
| `None` for a value in this dict means to delete that key in the next version. | ||
|
|
||
| It is okay to mark entries for deletion that don't exist. For instance, if a | ||
| version has ``a.txt`` and ``b.txt``, sending a ``content_to_replace`` value | ||
|
|
@@ -186,11 +187,27 @@ def create_next_component_version( | |
| component_id=component_pk, | ||
| ) | ||
| # First copy the new stuff over... | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [non-blocking question] Since we're potentially doing multiple insert operations here, should this method wrap its contents in Would also be ok with updating the function docs to recommend wrapping the method call with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, this was an oversight on my part in the review. Whenever possible, my intent was for these API functions to be atomic, so that we don't get inconsistent half-writes to the database. (Inconsistent half-writes to the file store are harder to guard against, but I feel better about that since they're idempotent inserts storing file data by hash–so "do it again" results in the same thing and hopefully the worst thing we do is write some unreferenced data.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ormsbee should we do a follow-up PR to wrap this function in a transaction?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The full API operation is wrapped in an atomic block:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😄 Ah, that makes sense. Thank you! |
||
| for key, content_pk in content_to_replace.items(): | ||
| for key, content_pk_or_bytes in content_to_replace.items(): | ||
| # If the content_pk is None, it means we want to remove the | ||
| # content represented by our key from the next version. Otherwise, | ||
|
Ian2012 marked this conversation as resolved.
|
||
| # we add our key->content_pk mapping to the next version. | ||
| if content_pk is not None: | ||
| if content_pk_or_bytes is not None: | ||
| if isinstance(content_pk_or_bytes, bytes): | ||
| file_path, file_content = key, content_pk_or_bytes | ||
| media_type_str, _encoding = mimetypes.guess_type(file_path) | ||
| # We use "application/octet-stream" as a generic fallback media type, per | ||
| # RFC 2046: https://datatracker.ietf.org/doc/html/rfc2046 | ||
| media_type_str = media_type_str or "application/octet-stream" | ||
| media_type = contents_api.get_or_create_media_type(media_type_str) | ||
| content = contents_api.get_or_create_file_content( | ||
| component.learning_package.id, | ||
| media_type.id, | ||
| data=file_content, | ||
| created=created, | ||
| ) | ||
| content_pk = content.pk | ||
| else: | ||
| content_pk = content_pk_or_bytes | ||
| ComponentVersionContent.objects.create( | ||
| content_id=content_pk, | ||
| component_version=component_version, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.