Skip to content
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

feat: Start conversion of StaticContentServer from middleware into view #34703

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

timmc-edx
Copy link
Contributor

@timmc-edx timmc-edx commented May 6, 2024

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 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:

from Transaction select count(*) facet request.method
where name = 'WebTransaction/Function/openedx.core.djangoapps.contentserver.middleware:StaticContentServer'
since 4 weeks ago

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: cookie so 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

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
```
@kdmccormick kdmccormick added the code health Proactive technical investment via refactorings, removals, etc. label May 29, 2024
@ormsbee ormsbee self-requested a review May 29, 2024 17:10
@timmc-edx timmc-edx merged commit 0eb61e2 into master Jun 10, 2024
47 checks passed
@timmc-edx timmc-edx deleted the timmc/asset-view branch June 10, 2024 15:44
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Proactive technical investment via refactorings, removals, etc.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants