[Django] #31949: ASGI - Cache Middleware and some view decorators broken
Django
------------------------------------------+------------------------
Reporter: Michael Galler | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+------------------------
The cache middleware is broken because it inherits from MiddlewareMixin
and override the init method.
{{{
TypeError: object HttpResponse can't be used in 'await' expression
}}}
On both middlewares the self._async_check() is missing in init.
Also the most view decorators are broken on async views, because the dont
use async/await.
For example:
{{{
cache_control
xframe_options_deny
vary_on_cookie
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Michael Galler | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 3.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Michael Galler:
Old description:
> The cache middleware is broken because it inherits from MiddlewareMixin
> and override the init method.
>
> {{{
> TypeError: object HttpResponse can't be used in 'await' expression
> }}}
>
> On both middlewares the self._async_check() is missing in init.
>
> Also the most view decorators are broken on async views, because the dont
> use async/await.
>
> For example:
>
> {{{
> cache_control
> xframe_options_deny
> vary_on_cookie
> }}}
New description:
The cache middleware is broken because it inherits from MiddlewareMixin
and override the init method.
{{{
TypeError: object HttpResponse can't be used in 'await' expression
}}}
On both middlewares the self._async_check() is missing in init.
Also the most view decorators are broken on async views, because the dont
provide async/await.
For example:
{{{
cache_control
xframe_options_deny
vary_on_cookie
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:1>
Django
Reporter: Michael Galler | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* cc: Andrew Godwin (added)
* component: Uncategorized => Core (Other)
Comment:
> The cache middleware is broken because it inherits from MiddlewareMixin
and override the init method.
An issue with the MiddlewareMixin subclasses is already handled in #31928.
> Also the most view decorators are broken on async views, because the
dont provide async/await.
>
> For example:
>
> {{{
> cache_control
> xframe_options_deny
> vary_on_cookie
> }}}
You should be able to chain them with `@sync_to_async()`, if necessary. Am
I missing sth?
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:2>
Django
Reporter: Michael Galler | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michael Galler):
Replying to [comment:2 felixxm]:
> > The cache middleware is broken because it inherits from
MiddlewareMixin and override the init method.
>
> An issue with the MiddlewareMixin subclasses is already handled in
#31928.
I hadn't opened the ticket, so that's already addressed.
>
> You should be able to chain them with `@sync_to_async()`, if necessary.
Am I missing sth?
How it works for me
{{{
@sync_to_async
@xframe_options_deny
@async_to_sync
async def get(self, request, *args, **kw):
slug = kw['slug']
}}}
But I find this complicated, wouldn't it be easier if we changed the
decorators as follows
{{{
def xframe_options_deny(view_func):
def wrapped_view(*args, **kwargs):
resp = view_func(*args, **kwargs)
if resp.get('X-Frame-Options') is None:
resp['X-Frame-Options'] = 'DENY'
return resp
return wraps(view_func)(wrapped_view)
}}}
to
{{{
def xframe_options_deny(view_func):
def wrapped_view(*args, **kwargs):
resp = view_func(*args, **kwargs)
if resp.get('X-Frame-Options') is None:
resp['X-Frame-Options'] = 'DENY'
return resp
async def wrapped_view_async(*args, **kwargs):
resp = await view_func(*args, **kwargs)
if resp.get('X-Frame-Options') is None:
resp['X-Frame-Options'] = 'DENY'
return resp
return wraps(view_func)(wrapped_view_async if
asyncio.iscoroutinefunction(view_func) else wrapped_view)
}}}
This also prevents the code from being executed in a different thread,
which again leads to context changes and slightly slows down the speed
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:3>
Django
--------------------------------+------------------------------------
Reporter: Michael Galler | Owner: nobody
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* keywords: => async
* type: Bug => New feature
* stage: Unreviewed => Accepted
Comment:
OK, so let's accept this as a new feature: Yes, it would be nice if the
decorators could handle async views directly.
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:4>
Django
--------------------------------+------------------------------------
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andrew Godwin):
Yeah, while you can technically apply enough async/sync things to
decorators to make them work directly, only the style that adds attributes
to the wrapped function is actually going to work natively; we should at
least have the core Django ones detect what they're wrapping and do the
right thing, though due to Python this is impossible to do perfectly (but
we "can" make them perfectly raise nice errors if they're not wrapped
right)
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:5>
Django
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ben Lomax):
* cc: Ben Lomax (added)
* owner: nobody => Ben Lomax
* status: new => assigned
Comment:
Picking this up to apply the solution discussed above.
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:6>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* stage: Ready for checkin => Accepted
Comment:
Indeed, the single person can't set Ready for checkin is the author :-)
Having patch = Yes, and all "needs ..." flags = No is sufficient to get in
the review queue. A few weeks ain't bad. It'll come :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:9>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Roy Smith):
* cc: Roy Smith (added)
Comment:
Is this something that's being worked on? I've run into what I'm pretty
sure is the same problem with LoginRequiredMixin (see
https://stackoverflow.com/questions/66512353/using-loginrequiredmixin-
with-async-views). It sounds like there's a fix that's basically ready to
go, but got stalled with a merge conflict?
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:10>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hi Roy.
Currently this is awaiting review. If you wanted to look at the PR
([https://docs.djangoproject.com/en/3.1/internals/contributing/writing-
code/submitting-patches/#patch-review-checklist the patch review checklist
helps]) and see what you think, that would be great.
See the [https://docs.djangoproject.com/en/3.1/internals/contributing
/triaging-tickets/#triaging-tickets Triaging Tickets docs for the general
workflow] — you can comment, and mark as needs tests, or needs
improvement, or needs docs and so on, or set the triage state to Ready for
commit, that will let Mariusz and I know that you think it's ready.
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:11>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Roy Smith):
I took a quick look just now. I'll take a deeper dive in a few days when
I get some time.
I do note that this doesn't cover
django.contrib.auth.decorators.login_required. If I understand things
correctly, that (and LoginRequiredMixin) depend on the DB backend, which
is a whole other level of async complexity?
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:12>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ben Lomax):
Hi Roy,
As far as I know this PR mostly just needs reviewing. I'm pretty busy for
the next few days, but then I'll happily look at the merge conflicts and
see if I can get it to a state where it's mergeable again. Oh, and
apparently there was a comment left in December about annotations and
release notes required, so I'll look to fix those up too.
It would be great to finally get a bit of momentum going on this :)
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:13>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Roy Smith):
I'm afraid I'm not going to be as useful here as I had hoped. I got as
far as verifying that the docs all build, and that everything passes
flake8. Unfortunately, I don't have a proper django development
environment set up, and was unable to run the tests. When I get up to:
{{{
pip install -r requirements/py3.txt
}}}
it eventually fails with:
{{{
In file included from src/_pylibmcmodule.c:34:
src/_pylibmcmodule.h:42:10: fatal error: 'libmemcached/memcached.h'
file not found
}}}
Unfortunately, it's going to be impractical for me to give you guys a
proper review.
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:14>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Roy, the continuous integration server takes care of running the tests for
each pull request, so you don't need to do that locally. However, to
remedy your error you can install `libmemcached-dev` on most Unix/Linux, I
think (or do a web search for that error and your OS and you should find
the remedy).
Reviewing a patch is mostly about examining the content.
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:15>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Roy Smith):
Replying to [comment:15 Tim Graham]:
OK. In any case, I stood up a clean debian 10 box and I'm picking this up
there.
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:16>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Roy Smith):
Review is done. This is the first code review I've done for django, so I
hope what I did is what you were looking for :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:17>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ben Lomax):
Thank you for your review Roy, I've updated the PR now to both deal with
the merge conflict and enact proposed changes / offer explanations as to
why I didn't enact them. Let me know if there's anything you disagree with
and I'm happy to have another round of changes as required.
The only thing left is to add a release note, but that'll need to wait for
a day or two until the current minor update is pushed out (I think it's
due out tomorrow and I don't imagine the PR will get approved and merged
in that time).
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:18>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:19>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:20>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
I'm still worried about the proposed approach on the
[https://github.com/django/django/pull/13483 current PR].
* It's quite complex. There's a utility function `sync_async_wrapper`
which is called **inside** each decorator implementation, making it less
readable in each case I think.
* This also forces each implementation into the `process_request()`,
`process_response()` pattern, rather than allowing that to be inline
**and** ties the implementation of those as sync.
Then:
* The `contrib.auth` decorators and mixins aren't handled.
* Each of the decorators has the `sync_and_async_middleware()` decorator
applied (marking it as both async and sync capable) but the attributes set
are never checked in the code anywhere.
Marking as patch needs improvement again on that basis.
I'll continue to look at this, and try and discuss with folks over the
DjangoCon period this autumn but, I think:
* In some cases (certainly for the mixins) implementing the async-switch
inline in the same way as
[https://github.com/django/django/blob/0c3981eb5094419fe200eb46c71b5376a2266166/django/views/generic/base.py#L159-L166
the `View.options` handler does now] would be feasible. (This would solve
Roy's issue from the StackOverflow thread.)
* For others, it would be worth looking at a wrapper around the decorator
implementation that adjusts the wrapped callback, rather than having to
call a helper. (For example, `@xframe_options_deny` &co need a sync view,
currently, but can be adapted by `BaseHandler` after that, if needed.)
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:21>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Ben Lomax):
Thank you for your review, it's always very appreciated. I'll try to
address each of your points as best as I understand things:
1. **Complexity / forced implementation:** The utility function was my
attempt to make the decorator function work more similarly to the mixin
classes, which look for `process_request` and `process_response`
attributes amongst others. However, it might be that we don't want
decorator ''functions'' to mirror the decorator ''class'' pattern, and so
I'm happy to move away from it.
2. **Sync-only `process_X` functions:** As it stands, I would suggest that
allowing for async passed-in functions (e.g. the `etag_func` argument of
the `@condition` decorator) should be pushed back into a separate piece of
work after this one. It seems to me that converting the builtin decorators
to be async-able at the cost of only allowing sync functions to be passed
to them is a good iterative step forward.
3. **The `contrib.auth` decorators:** Fair point, I can cover those too.
4. **`sync_and_async_middleware` never checked in functions:** Correct me
if I've misunderstood this, but I thought that the decorator wasn't for
internal use, but was used by external code as a way of knowing if the
middleware could handle sync and/or async view functions. For example,
when layering middleware it would help Django figure out if it needed to
"adapt the request to fit the middleware's requirements" (as per [the
docs](https://docs.djangoproject.com/en/4.1/topics/http/middleware
/#asynchronous-support)).
5. **async-switch inline:** I think this should be possible for any
decorators which only act of the request. If a decorator needs to act on
the response, we need to await the view to get the response, _and then_
act on that response. If this isn't acceptable, we might be able to do
something with partial functions perhaps, but that seems like a non-
trivial jump in complexity.
6. **implementation that adjusts the wrapped callback:** I'll be honest, I
don't think I understand your suggestion here. Are you suggesting we
somehow flag a `process_request` function to be run when the `BaseHandler`
handles the async view? I think this would mean defining the response
processing in the decorator, but not actually running it "within the
decorator".
A good follow up question feels like is: is there a way to break this work
up logically to give partial functionality quicker? For example, only
doing this for decorators which don't act on the view response?
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:22>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hey Ben.
I think the shorter version would be that the `sync_async_wrapper` strikes
me as overly complex. I think it would be much easier on the eye inlining
the requisite code each time. I suspect for each decorator the needed code
is minimal. That wouldn't be as DRY™, but it would exhibit better locality
of behaviour, and be easier to maintain.
An incremental way forward would be looking at a subset of the decorators.
I suspect, the `contrib.auth` decorators are the ones I'm most likely to
want to use first... 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:23>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Jon Janzen):
* cc: Jon Janzen (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:24>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Ben Lomax):
Hi Carlton,
Thanks for that clarification, I've now updated the PR to:
1. Remove the `sync_async_wrapper` function completely; all the decorators
now handle their logic "inline". I've still pulled out some of the shared
code between the sync and async versions of the decorators, so you'll
still see a few `_process_request` and `_process_response` about the
place, but it's now much clearer what they actually do.
2. Make all of the `contrib.auth` decorators be able to handle sync and
asycn views.
The documentation update still points to version 4.2, let me know if
that's no longer correct.
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:25>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 1 => 0
Comment:
Thanks Ben. I’ll uncheck Patch needs improvement, and give it another look
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:26>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* has_patch: 1 => 0
Comment:
Hi Ben — Happy New Year!
I've looked again at the PR, both before and after Christmas. In summary,
I
think we should address this ticket in a series of smaller PRs, rather
than
trying to do them all in one big go.
As I see it, there are two groups of issue with the current approach:
* The bulk edit means the code in quite a few time not sensitive enough to
the individual decorator. Looking at them, couldn't we write this one that
way, or that one this way?, is the thought. (I left some more specific
comments on the PR itself.) If we address them one-by-one (or in small
related groups, likely) we can write the best code for each case. There
are repeated patterns and opportunities for code-reuse, but I think
extracting them as we go is going to work better than trying to determine
them in advance.
* Related, the tests for each decorator need to live with the existing
related tests, not be centralised in one place. Five years hence, when we
come to work on the login_required logic, say, we need to be able to open
the relevants tests and see them all together. Having to track down a
separate set of tests in another part of the test suite entirely would be
a maintenance headache. If we address the decorators individually this
tendency won't be there.
I hope both of those points make sense?
I think the general idea is correct thought, and I don't see anything to
stop
us being able to process smaller `Refs #31949 -- ...` PRs relatively
quickly.
I think it would be good to address the whole list of built-in decorators
within a single release cycle though. As such I'm going to put this on my
target list for 5.0. I'll pick it up mid-cycle to help make sure we can
get it over the line.
Thanks again for your patience and input. I know it's frustrating when it
takes a while to get it lined up right.
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:27>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Ben Lomax):
Hi Carlton, and a Happy New Year to you too 😁
Splitting up the work and moving the tests to separate files seems like a
good idea to me, I'll try to get started on those in the next few weeks at
the latest. The one caveat I would put forward is to keep the
`test_decorators_sync_and_async_capable` test
([https://github.com/LomaxOnTheRun/django/blob/65bf55b643300f5f314be691f3cbd96134c7f1d7/tests/decorators/tests.py#L496
link to code]) in the shared test file, as it's less of a test of the
behaviour of each decorator, and in theory shouldn't change for a while
anyway (i.e. none of them should go back to just being only sync or async
capable). How does that sound?
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:28>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Ben Lomax):
To get the ball rolling, I've thrown together the first of these PRs
[https://github.com/django/django/pull/16436 here], which looks at the
`cache` decorators.
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:29>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:30>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"23cbed21876bf02f4600c0dac3a5277db5b2afbb" 23cbed21]:
{{{
#!CommitTicketReference repository=""
revision="23cbed21876bf02f4600c0dac3a5277db5b2afbb"
Refs #31949 -- Enabled @sensitive_variables to work with async functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:31>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:32>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"203a15cadbf8d03b51df1b28d89b2e7ab4264973" 203a15c]:
{{{
#!CommitTicketReference repository=""
revision="203a15cadbf8d03b51df1b28d89b2e7ab4264973"
Refs #31949 -- Adjusted error reporting docs.
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"7330408ac36f893a4a11c668ba230b440ec09f41" 7330408]:
{{{
#!CommitTicketReference repository=""
revision="7330408ac36f893a4a11c668ba230b440ec09f41"
Reverted "Refs #31949 -- Enabled @sensitive_variables to work with async
functions."
This reverts commits 23cbed21876bf02f4600c0dac3a5277db5b2afbb and
203a15cadbf8d03b51df1b28d89b2e7ab4264973.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:33>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
* needs_docs: 0 => 1
Comment:
[https://github.com/django/django/pull/16436 PR for the cache decorators]
is nearly ready to go. (Tweaks and docs.)
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:34>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:35>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Waiting for rebase.
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:36>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/16752 Additional PR addressing the
http decorators].
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:37>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Per Adam's
[https://github.com/django/django/pull/16752#discussion_r1167769482
comment].
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:38>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"4dfc6ff8a81ed36dfc7c5b942ecab7217866b935" 4dfc6ff]:
{{{
#!CommitTicketReference repository=""
revision="4dfc6ff8a81ed36dfc7c5b942ecab7217866b935"
Refs #31949 -- Made @never_cache and @cache_control() decorators to work
with async functions.
Thanks Carlton Gibson and Mariusz Felisiak for reviews.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:39>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Ben Lomax):
As an update, I've got 3 PRs to contribute to this issue in various
states:
1. [https://github.com/django/django/pull/16800 Made
@xframe_options_(deny/sameorigin/exempt) decorators to work with async
functions]
* Reviewed once, tests are failing, but I think it might be a test
runner failure rather than an actual test failure (`ERROR: Step ‘Publish
JUnit test result report’ failed: No test report files were found.
Configuration error?`) and I'm not sure how to re-run the tests.
2. [https://github.com/django/django/pull/16874 Made @no_append_slash work
with async functions]
* Passing all tests, needs a review
3. [https://github.com/django/django/pull/16876 Made @sensitive_variables
and @sensitive_post_parameters work with async functions]
* Legitimately failing tests, I need to come back to it, but wanted to
flag that I've started somewhere
There's going to be a little bit of conflicts with all of them due to the
docs being updated in the same place, but that should be easy enough to
fix once they start to get merged.
I also believe that the `http` decorators are also being worked on in
[https://github.com/django/django/pull/16752 this PR] by
[https://github.com/Th3nn3ss Th3nn3ss], so I'm leaving those alone for
now.
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:40>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"00f5d2d110712af84fae2c5f9183a2ea48ce0a4a" 00f5d2d1]:
{{{
#!CommitTicketReference repository=""
revision="00f5d2d110712af84fae2c5f9183a2ea48ce0a4a"
Refs #31949 -- Made @xframe_options_(deny/sameorigin/exempt) decorators to
work with async functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:41>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"23abec9192c88f9372e5784ae69c9aa5be2b21be" 23abec9]:
{{{
#!CommitTicketReference repository=""
revision="23abec9192c88f9372e5784ae69c9aa5be2b21be"
Refs #31949 -- Made @no_append_slash decorator to work with async
functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:42>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"38e391e95fe5258bc6d2467332dc9cd44ce6ba52" 38e391e]:
{{{
#!CommitTicketReference repository=""
revision="38e391e95fe5258bc6d2467332dc9cd44ce6ba52"
Refs #31949 -- Made @sensitive_variables/sensitive_post_parameters
decorators to work with async functions.
Co-authored-by: Mariusz Felisiak <felisiak...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:43>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"6523860ca8c7127f850f2fbc24afb6ea63b76949" 6523860]:
{{{
#!CommitTicketReference repository=""
revision="6523860ca8c7127f850f2fbc24afb6ea63b76949"
Refs #31949 -- Simplified @sensitive_variables a bit.
Follow up to 38e391e95fe5258bc6d2467332dc9cd44ce6ba52.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:44>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"3152f9de47a317d658ebee6d5b517b7e1258aa1f" 3152f9de]:
{{{
#!CommitTicketReference repository=""
revision="3152f9de47a317d658ebee6d5b517b7e1258aa1f"
Refs #31949 -- Made http decorators to work with async functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:45>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:46>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Ben Lomax):
I've created a couple more PRs which I //think// cover all of the
remaining non-`decorator_from_middleware` decorators (and excellent work
on the sensitive parameter decorators by the way 😄):
* [https://github.com/django/django/pull/17055 Made @csrf_exempt decorator
work with async functions]
* [https://github.com/django/django/pull/17056 Made
@vary_on_(cookie/headers) decorators work with async functions]
For the `decorator_from_middleware` decorators, I've put together a
[https://github.com/django/django/pull/17057 proof of concept (POC) draft
PR], with a few notes:
1. It only implements tests for `gzip_page`, to make sure that it actually
works for at least one of the affected decorators.
2. The second commit attempts to DRY up the code (as there is a //lot// of
code duplication otherwise), but I'm not convinced it's actually helpful.
3. I've not added any documentation.
This POC PR is meant to act as a place to have a conversation which can
point to specific code changes, rather than a solution that I feel
strongly about 🙂
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:47>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"953f81e078fb7299b6d40d4e599e5f559e03952b" 953f81e0]:
{{{
#!CommitTicketReference repository=""
revision="953f81e078fb7299b6d40d4e599e5f559e03952b"
Refs #31949 -- Made @csrf_exempt decorator to work with async functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:48>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"99bd373367e54790da25b64c1d21610ac0f9feda" 99bd3733]:
{{{
#!CommitTicketReference repository=""
revision="99bd373367e54790da25b64c1d21610ac0f9feda"
Refs #31949 -- Mentioned @sensitive_variables/sensitive_post_parameters
decorators in async topic.
Follow up to 38e391e95fe5258bc6d2467332dc9cd44ce6ba52.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:49>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"b7a17b0ea0a2061bae752a3a2292007d41825814" b7a17b0]:
{{{
#!CommitTicketReference repository=""
revision="b7a17b0ea0a2061bae752a3a2292007d41825814"
Refs #31949 -- Made @vary_on_(cookie/headers) decorators work with async
functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:50>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Type: New feature | Status: assigned
Component: Core (Other) | Version: 3.1
Severity: Normal | Resolution:
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"74f7deec9e334a69bfbfdd068285618534198bd5" 74f7deec]:
{{{
#!CommitTicketReference repository=""
revision="74f7deec9e334a69bfbfdd068285618534198bd5"
Refs #31949 -- Made make_middleware_decorator to work with async
functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:51>
Django
--------------------------------+-------------------------------------
Reporter: Michael Galler | Owner: Ben Lomax
Component: Core (Other) | Version: 3.1
Keywords: async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/31949#comment:52>