Repository navigation
ADR for shared Assets and how course files are referenced/served from openedx_content [FC-0138] - #822
bradenmacdonald wants to merge 23 commits into
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @bradenmacdonald! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
6fe58fa to
ee4a005
Compare
bradenmacdonald
changed the title
08aa7b9 to
0bbe03b
Compare
|
For those following this so far, I have made major revisions to everything, primarily renaming it from "Upload" to "File component" and adding a "Consequences" section. |
0ebd33f to
d5a8ae0
Compare
| Naturally, the ``FileComponentMetadata`` table will enforce that ``legacy_path`` is unique per learning package. | ||
|
|
||
| 2. File components can be referenced using new ``oex-asset:`` URL | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
I just pushed this new ADR 14.
Decision 1 is the only part that is relevant to our short-term project of "move static assets into openedx_content and serve them using the contentstore API". The rest is trying to paint a picture of the long-term functionality we'll get when we also migrate courseware itself to openedx_content.
The reason I'm sketching this all out now is so we can decide if we want all this, or if we'll instead go with the dumb, simple approach that does the bare minimum for contentstore compatibility but drops versioning, tracking, linking, etc.
|
OK, I still have some TODOs and things to clean up, but I'd like to get more input before focusing on the fine details. This is ready for review, @ChrisChV @ormsbee @kdmccormick. CC @xitij2000 and @Kelketek on ADR 14 in case you have any thoughts on this after working on the XBlock Asset Picker UI. |
| When copying a Component with references to shared File components to a new Learning Package, each referenced File component is handled as follows: | ||
|
|
||
| - First, if the destination Learning Package already has a File component that is a downstream copy of the same upstream File component (see decision 6), that existing copy is reused, even if its ``component_code`` or content differs. The copy is not updated as a side effect of pasting, since that would change every other component that uses it; if the source uses a newer upstream version, the author can sync the File component as usual. Matching on upstream prevents repeated pastes or imports of the same library content from creating a new copy of the File component each time. | ||
| - Otherwise, if the destination Learning Package already has a File component with identical ``component_code`` and media asset file hash(es), it is reused and no File components need to be copied. Identical bytes don't prove that it's the same asset, but reusing a File component with the same code and content is harmless. |
There was a problem hiding this comment.
I'm a bit worried about this one. If this occurs, the File Component in the destination Learning Package might already be in use by another component. If an author updates the asset, it will be updated in both the existing component and the copied component. However, the author is unaware of this; as far as they are concerned, they are only changing the asset in the existing component.
There was a problem hiding this comment.
True, but we will presumably have something in the UI that shows authors where each File component is being used, so they would see a list of which changes will happen when they update it.
There is actually something like that in the "Files" page already, but it's currently not 100% accurate.
| 11. Image metadata will be in separate models | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| As mentioned in the :class:`Media` docstring, we can use a separate model called ``ImageMedia`` to capture metadata like image dimensions (assuming it can be purely derived from the image byte data itself). We can also use a separate model like ``ImageComponentVersion`` to store *editable* metadata about an image, such as its default alt text and whether or not the image is purely decorative; saving edits of such metadata would create a new version of the File component. |
There was a problem hiding this comment.
Does this mean that a File component that bundles an image with its thumbnail will have separate ImageMedia entries for each thumbnail size and the original asset?
There was a problem hiding this comment.
No, I don't think thumbnails should be considered part of the asset, nor bundled with the image. But if you did upload a pair of images with a regular and "retina" @2x resolution versions, each of those would have its own ImageMedia entry. The ImageMedia is 1:1 with each actual image file that we store (de-duplicated by hash), so it doesn't matter how they are logically organized. (Two different-resolution versions in one component or two separate components each with a different resolution.)
As for how thumbnails are stored, see the draft proposal from @ChrisChV at #837 which is more or less a separate system for storing thumbnails, not storing them as versioned file components.
|
|
||
| This is an appealing alternative, especially when you consider that the number of use cases that require multiple files per File component is likely very small. Videos require multiple files but are rarely if ever stored in Course Files (instead hosted on video-specific services like YouTube or edX.org's video platform). This is rejected primarily to support the use case of HTML "interactive" files that in turn reference image, JavaScript, and/or CSS files. Examples of this can be seen in the `Studio Advanced course <https://github.com/HarvardX/studio-advanced/tree/5cf820c29e3439c9f45cb88e5cf22fd4ab269739/course/static>`_. | ||
|
|
||
| TODO: can we quantify how much this feature would be used? |
There was a problem hiding this comment.
I think images are probably a big user of this and I expect would be very common as assets. I think embracing this would allow uploading the original asset and using a web-friendly resized version in a course and potential a mobile version on mobile.
There was a problem hiding this comment.
Also, for the PDF XBlock I'd worked on a feature to convert a .docx or other file type to pdf. It would be great that could be represented via this mechanism.
There was a problem hiding this comment.
@bradenmacdonald I think we need to identify which assets are authored and which are derived/generated. This is because adding an asset to a File Component means a new version of that File Component will be created. That is correct for authored assets, but creating a new version for derived assets is not ideal. I’ve explored this further in the thumbnails ADR (#837). Thumbnails are derived/generated and disposable; it doesn't make much sense to version-control them.
There was a problem hiding this comment.
Good points guys. If we are auto-converting .docx to .pdf or something, I think that it should be treated differently than if an author uploads both a .docx and a .pdf.
Derived files, like thumbnails and auto-converted docs:
- Are safe to delete
- Can be (re-)created on demand
- Are versioned differently: based on when the original file or the code that produces them changes (e.g. if we upgrade "pdflib" to a new version, we may want to re-create all the .pdf auto-conversions)
I think it makes sense to store such things in completely different models, not re-using the PublishableEntity machinery that we're using for the original file components themselves.
There was a problem hiding this comment.
There is another use-case for images, which is to allow editing while retaining the original. I think the versioning ability would probably be a better fit for that though. i.e. if a the authoring MFE or a plugin allows editing image it could save that as v2 of the asset or as another file in the same component.
Just pointing out a potential use case, not sure what would be the correct approach.
kdmccormick
left a comment
kdmccormick
left a comment
There was a problem hiding this comment.
thanks for the writeups! this is making the whole problem space much more concrete.
i'm still chewing on all three ADRs. here are just a couple specific concerns on ADR 14 I wanted to capture, but don't feel like you need to address these in depth right now as I'm still not sure about ADR 13
| - First, if the destination Learning Package already has a File component that is a downstream copy of the same upstream File component (see decision 6), that existing copy is reused, even if its ``component_code`` or content differs. The copy is not updated as a side effect of pasting, since that would change every other component that uses it; if the source uses a newer upstream version, the author can sync the File component as usual. Matching on upstream prevents repeated pastes or imports of the same library content from creating a new copy of the File component each time. | ||
| - Otherwise, if the destination Learning Package already has a File component with identical ``component_code`` and media asset file hash(es), it is reused and no File components need to be copied. Identical bytes don't prove that it's the same asset, but reusing a File component with the same code and content is harmless. | ||
| - In both of the above cases, only the main Component needs to be copied, but we still have to create ``FileComponentLink`` and :class:`PublishableEntityVersionDependency` objects to track the relationship, using the original ``component_code`` as the ``alias`` if the reused File component's code differs. | ||
| - Otherwise, the File component is copied into the destination Learning Package, and becomes a downstream of the original if the original is in a library. If the destination Learning Package already has a conflicting File component, with identical ``component_code`` but different media asset file hash(es), the copy is given a different ``component_code``, and the ``FileComponentLink`` created for it will use the ``alias`` column to alias this new code to the old code, so that no changes to the OLX are required. |
There was a problem hiding this comment.
how does the FileComponentLink's alias get factored in when the OLX is moved out of the target LP? notably:
- copy
- export/import
would each of those systems now need to read FileComponentLink in order to make sense of the OLX? is that tradeoff worth the benefit of not rewriting media paths upon OLX paste?
There was a problem hiding this comment.
Export/import would presumably be at the LP level, and would need to include some list of all the FileComponentLinks in the package in any case.
Copy and paste into another LP would have to follow the same algorithm described above for handling references in the new learning package, creating an alias if needed.
|
|
||
| This scheme: | ||
|
|
||
| - **Is easy to detect.** ``oex-asset:…`` does not occur in content for any other reason, so a single pattern finds every reference in any field type (HTML, problem XML, JSON, CSS ``url()``, etc.) with no false positives from platform or theme ``/static/`` URLs. (The only exception is that course content *about* OLX authoring itself would need to escape ``oex-asset:`` if it occurs in text.) |
There was a problem hiding this comment.
I imagine we'll need to support rewriting of /static/, basically forever. So, adding rewrite-detection for oex-asset: would not simplify the system, right?
Beyond that, it seems like all the other listed benefits of oex-asset: also apply to /static.
The net-new thing I see oex-asset: doing is that it provides for both the component-local format (oex-asset:/{path}) and the cross-component format (oex-asset:{component_alias}/{path}).
There was a problem hiding this comment.
I mentioned this in the meeting, but to recap here:
The problem with /static/ is that it can occur in many unrelated contexts, such as a course referencing https://www.python.org/static/img/python-logo.png or https://en.wikipedia.org/static/images/icons/wikipedia.png (although they shouldn't be hotlinking).
So we use this regular expression to try to only match quoted references that begin with /static/, like <img src="/static/..." .... But this doesn't match every use case; e.g. "/static/..." would not be matched, but could be a valid reference to a file.
Because oex-asset: doesn't occur in any other contexts (I'm pretty sure), we can detect it reliably and robustly.
d16b37a to
96febfb
Compare
1232161 to
9f56c54
Compare
bradenmacdonald
changed the title
7 participants
This is an ADR for representing Course Files in openedx_content.
Builds on #812 and #818 (merged).
View the rendered ADRs here:
Private ref: FAL-4383
AI disclosure: Claude helped me with research, writing, and formatting the RST, but I wrote most of the content myself.