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.

Sign up for GitHub

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

Jump to bottom

Course assets should be served by a view rather than a middleware #34702

Open
3 tasks
timmc-edx opened this issue May 6, 2024 · 3 comments
Open
3 tasks

Course assets should be served by a view rather than a middleware #34702

timmc-edx opened this issue May 6, 2024 · 3 comments
Labels
code health Proactive technical investment via refactorings, removals, etc. maintenance Routine upkeep necessary for the health of the platform

Comments

@timmc-edx
Copy link
Contributor

timmc-edx commented May 6, 2024 •
edited

Acceptance criteria:

  • StaticContentServer is converted from a middleware to a view
  • The cutover is managed with a Waffle flag or similar to allow careful rollout
  • Documentation added to next Open edX named-release notes

Course asset URLs like /asset-v1:... are served from a middleware at openedx.core.djangoapps.contentserver.middleware.StaticContentServer rather than from a view. This means that Django starts handling the request as a Not Found (because there is no matching urlpattern), but then the middleware intercepts the request and returns a response of its own. There wasn't any good reason for doing this instead of using a view, as far as I can tell. It causes some problems for telemetry: When the code-owner middleware asks Django what view handled the request, it does so by looking at the result of the resolve utility, but these URLs get a Resolver404 (because there's no registered urlpattern).

Implementation suggestion:

  1. Register equivalent urlpatterns and a view that calls into the same code the middleware currently contains.
  2. Add a Waffle flag that causes the middleware to fall through to the view.
  3. Try turning the flag on.
  4. Remove the middleware so that the view is always used.
  5. [Beyond this ticket:] Bring the new view's code up to standards (better use of urlpatterns, etc.)

PRs:
@timmc-edx timmc-edx mentioned this issue May 6, 2024
Merged
timmc-edx added a commit that referenced this issue May 6, 2024
@timmc-edx
feat: Start conversion of StaticContentServer from middleware into view
7355726 
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 timmc-edx mentioned this issue May 7, 2024
Open
2 tasks
timmc-edx added a commit that referenced this issue May 15, 2024
@timmc-edx
feat: Start conversion of StaticContentServer from middleware into view
f805c16 
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 timmc-edx mentioned this issue May 20, 2024
Open
@kdmccormick
Copy link
Member

+1. This seriously confused Connor and I today!

@feanil feanil added maintenance Routine upkeep necessary for the health of the platform code health Proactive technical investment via refactorings, removals, etc. labels Jun 7, 2024
timmc-edx added a commit that referenced this issue Jun 10, 2024
@timmc-edx
feat: Start conversion of StaticContentServer from middleware into vi…
0eb61e2 
…ew (#34703)

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
Copy link
Contributor Author

Private 2U dashboard for monitoring cutover: https://app.datadoghq.com/dashboard/vjs-j3j-czr

@timmc-edx timmc-edx changed the title Course assets are served by a middleware rather than a view Course assets should be served by a view rather than a middleware Jun 10, 2024
@robrap
Copy link
Contributor

robrap commented Jun 11, 2024

We don't actually own this, and it is no longer a high priority, but we might be able to help with rollout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
code health Proactive technical investment via refactorings, removals, etc. maintenance Routine upkeep necessary for the health of the platform
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@feanil @kdmccormick @robrap @timmc-edx