feat: Start conversion of StaticContentServer from middleware into view #34703
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timmc-edx
force-pushed
the
timmc/asset-view
branch
from
8399d85 to
7355726
Compare
2 tasks
See #34702 This necessarily involves switching from calling `StaticContent.is_versioned_asset_path` to determine whether to handle the request to having a hardcoded urlpattern. I've made the choice to hardcode the other two patterns similarly rather than using imported constants. The mapping of URL patterns to database records should be explicit (even though we don't expect those constants to change out from under us.) I've renamed the middleware rather than choosing a new name for the implementation because there are other references in tests and other code. This was the smaller change. A note on HTTP methods: The middleware currently completely ignores the request's HTTP method, so I wanted to confirm that only GETs were being used in practice. This query reveals that 99.8% of requests that this middleware handles are GET, with just a smattering of PROPFIND and OPTIONS and a tiny number of HEAD and POST: ``` from Transaction select count(*) facet request.method where name = 'WebTransaction/Function/openedx.core.djangoapps.contentserver.middleware:StaticContentServer' since 4 weeks ago ```
timmc-edx
force-pushed
the
timmc/asset-view
branch
from
7355726 to
f805c16
Compare
kdmccormick
added
the
code health
ormsbee
approved these changes
Jun 5, 2024
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #34702
This necessarily involves switching from calling
StaticContent.is_versioned_asset_pathto determine whether to handle the request to having a hardcoded urlpattern. I've made the choice to hardcode the other two patterns similarly rather than using imported constants. The mapping of URL patterns to database records should be explicit (even though we don't expect those constants to change out from under us.)I've renamed the middleware rather than choosing a new name for the implementation because there are other references in tests and other code. This was the option with the smaller diff.
A note on HTTP methods: The middleware currently completely ignores the request's HTTP method, so I wanted to confirm that only GETs were being used in practice. This query reveals that 99.8% of requests that this middleware handles are GET, with just a smattering of PROPFIND and OPTIONS and a tiny number of HEAD and POST:
Testing instructions
I've tested this in devstack using http://localhost:18000/asset-v1%253AedX+DemoX+Demo_Course+type@asset+block@images_images_course_image.jpg with the flag on and off. There's some minor difference in response headers around accept-language and vary, but they both have
Vary: cookieso I don't think that will affect much.I also tested 404 responses, and both 200 and 404 from the CMS as well.
When testing this in deployed environments we can watch for changes in response status, new or different errors, and unexpected telemetry data (from the "shouldn't happen" branches).
Deadline
None