-
Notifications
You must be signed in to change notification settings - Fork 11
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
Basic asset response support #217
Conversation
@bradenmacdonald, @kdmccormick: This is an implementation of static asset serving on the Learning Core side of things. I'm still writing tests and I need to do cleanup on the management command side, but the API and model code should be more or less there. Please feel free to take an early pass at it to make sure it looks sane. Otherwise, I'll ping you folks when it's baked enough for real review. |
7daf487
to
ccff59f
Compare
ff93940
to
83eaf20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this out but the code makes sense to me! Nice work.
"X-Learning-Core-Component-Version-Num": component_version.version_num, | ||
# Learning Package | ||
"X-Learning-Core-Learning-Package-Key": learning_package.key, | ||
"X-Learning-Core-Learning-Package-Uuid": learning_package.uuid, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny tiny nit: I know these are mostly for debugging, but it seems weird to me to call these X-Learning-Core-...
instead of something like X-OEX-...
. Are these Open edX components that just happen to be implemented in the learning core package, or is Learning Core an end user brand?
Even from a developer perspective, the name "Learning Core" is not helpful for letting people know where the corresponding implementation is located, because the python package and github repo are both called openedx-learning
which is not obviously the same thing as Learning-Core
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something in middleware will force this to come out mixed case, i.e. X-OEX-
will get turned into X-Oex-
. Regardless, I'm fine with changing this. Maybe X-Open-edX-Learning-Package-Key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no concern about length of the header name, then I'd say spell it out-- no room for ambiguity with X-Open-edX-
:)
*This function does not do auth checking of any sort*–it will never return | ||
a ``401`` or ``403`` response code. That is by design. Figuring out who is | ||
making the request and whether they have permission to do so is the | ||
responsiblity of whatever is calling this function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, the learner_downloadable_only
does seem like a type of auth check to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I feel a bit weird about that flag in general, but it's mostly there to try to keep people from shooting themselves in the foot when they're adding random things like LaTeX files and such–and to avoid issues we've had in the past around accidental downloads we've had in the past (e.g. python-lib.zip
). So I thought it'd be better if the entire endpoint would make those assets unfindable/servable, and not just leave it up to the authz code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a solid design choice. I don't feel strongly, but maybe just reword the comment to be clear about it:
Other than checking the coarse-grained ``learner_downloadable_only`` flag,
*this function does not do auth checking*–it will never return a ``401`` or ``403``
response code. That is by design. Figuring out who is making the request and
whether they have permission to do so is the responsiblity of whatever is calling
this function.
Management command to add files to a Component. | ||
|
||
This is mostly meant to be a debugging tool to let us to easily load some test | ||
asset data into the system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This will be very helpful for dev/debugging.
83eaf20
to
4c74010
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't do an in-depth review, but based on what you've solidly specced out in https://github.com/openedx/openedx-learning/blob/main/docs/decisions/0015-serving-static-assets.rst, plus Braden's review, I say merge away 👍🏻
55ee873
to
0279baa
Compare
This adds a number of API calls to the Authoring public API in order to support associating static assets with Components and allowing them to be downloaded. Added: * get_component_by_uuid * get_component_version_by_uuid * get_content_info_headers * get_redirect_response_for_component_asset Modified: * create_next_component_version - made title optional * create_component_version_content - annotation for learner_downloadable Most of the justification for this approach can be found in the docstring for get_redirect_response_for_component_asset. Note that there is currently no backend redis/memcached caching of the ComponentVersion and Content information. This means that every time a request is made, we will do a couple of database queries in order to do this lookup. I had actually done a version with such caching in an earlier iteration of this PR, but it added a lot of complexity for what I thought would be minimal gain, since going through the middleware will cause about 20 database queries anyway. This implements the first part of the static assets ADR for Learning Core: openedx#110 Important notes: * No view or auth is implemented. That is the responsibility of edx-platform. Learning Core only provides the storage and response generation. * The responses generated will require the use of a reverse proxy that can handle the X-Accel-Redirect header. See ADR for details.
0279baa
to
3e25927
Compare
Low-level Learning Core API calls for returning references to content associated with a Component. Uses
X-Accel-Redirect
.