Allow builtin view decorators to be applied directly to async views.

[Michael Galler]

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

[Mariusz Felisiak]

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?

[Michael Galler]

Replying to 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 had not checked 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 process_response(resp):
        if resp.get('X-Frame-Options') is None:
            resp['X-Frame-Options'] = 'DENY'
        return resp

    def wrapped_view(*args, **kwargs):
        resp = view_func(*args, **kwargs)
        return process_response(resp)

    async def wrapped_view_async(*args, **kwargs):
        resp = await view_func(*args, **kwargs)
        return process_response(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

[Carlton Gibson]

OK, so let's accept this as a new feature: Yes, it would be nice if the decorators could handle async views directly.

[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)

[Ben Lomax]

Picking this up to apply the solution discussed above.

[Ben Lomax]

I've opened a draft PR ​here that I would love some feedback on if / when anyone has a chance to take a look at it. I'm updating it as I go, but having someone cast an eye on it to make sure I'm not going down the wrong rabbit hole would be useful.

[Ben Lomax]

Alright, I think I have a PR that's ready to check. I've left a couple of questions on the PR description, but they're not essential to the functionality (i.e. it all seems to be working).

I'm going to update the triage stage to Ready for checkin (as I suspect it'll get more eyes on it that way) but if that's bad form please let me know and I'll happily change it back as required.

PR in question: ​https://github.com/django/django/pull/13483

[Jacob Walls]

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

[Roy Smith]

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?

[Carlton Gibson]

Hi Roy.

Currently this is awaiting review. If you wanted to look at the PR (​the patch review checklist helps) and see what you think, that would be great.

See the ​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.

[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?

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

[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.

[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.

[Roy Smith]

Replying to Tim Graham:

OK. In any case, I stood up a clean debian 10 box and I'm picking this up there.

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

[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).

[Carlton Gibson]

I'm still worried about the proposed approach on the ​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 ​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.)

[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?

[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... 🤔

[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.

[Carlton Gibson]

Thanks Ben. I’ll uncheck Patch needs improvement, and give it another look

[Carlton Gibson]

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 is quite a few times 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 though, 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.

[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 (​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?

[Ben Lomax]

To get the ball rolling, I've thrown together the first of these PRs ​here, which looks at the cache decorators.

[GitHub <noreply@…>]

In 23cbed21:

Refs #31949 -- Enabled @sensitive_variables to work with async functions.

[GitHub <noreply@…>]

In 203a15c:

Refs #31949 -- Adjusted error reporting docs.

[GitHub <noreply@…>]

In 7330408:

Reverted "Refs #31949 -- Enabled @sensitive_variables to work with async functions."

This reverts commits 23cbed21876bf02f4600c0dac3a5277db5b2afbb and
203a15cadbf8d03b51df1b28d89b2e7ab4264973.

[Carlton Gibson]

​PR for the cache decorators is nearly ready to go. (Tweaks and docs.)

[Mariusz Felisiak]

Waiting for rebase.

[Carlton Gibson]

​Additional PR addressing the http decorators.

[Mariusz Felisiak]

Per Adam's ​comment.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4dfc6ff:

Refs #31949 -- Made @never_cache and @cache_control() decorators to work with async functions.

Thanks Carlton Gibson and Mariusz Felisiak for reviews.

[Ben Lomax]

As an update, I've got 3 PRs to contribute to this issue in various states:

1. ​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. ​Made @no_append_slash work with async functions
   • Passing all tests, needs a review
3. ​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 ​this PR by ​Th3nn3ss, so I'm leaving those alone for now.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 00f5d2d1:

Refs #31949 -- Made @xframe_options_(deny/sameorigin/exempt) decorators to work with async functions.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 23abec9:

Refs #31949 -- Made @no_append_slash decorator to work with async functions.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 38e391e:

Refs #31949 -- Made @sensitive_variables/sensitive_post_parameters decorators to work with async functions.

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@…>

[Mariusz Felisiak <felisiak.mariusz@…>]

In 6523860:

Refs #31949 -- Simplified @sensitive_variables a bit.

Follow up to 38e391e95fe5258bc6d2467332dc9cd44ce6ba52.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 3152f9de:

Refs #31949 -- Made http decorators to work with async functions.

[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 😄):

• ​Made @csrf_exempt decorator work with async functions
• ​Made @vary_on_(cookie/headers) decorators work with async functions

For the decorator_from_middleware decorators, I've put together a ​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 🙂

[Mariusz Felisiak <felisiak.mariusz@…>]

In 953f81e0:

Refs #31949 -- Made @csrf_exempt decorator to work with async functions.

[GitHub <noreply@…>]

In 99bd3733:

Refs #31949 -- Mentioned @sensitive_variables/sensitive_post_parameters decorators in async topic.

Follow up to 38e391e95fe5258bc6d2467332dc9cd44ce6ba52.

[Mariusz Felisiak <felisiak.mariusz@…>]

In b7a17b0:

Refs #31949 -- Made @vary_on_(cookie/headers) decorators work with async functions.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 74f7deec:

Refs #31949 -- Made make_middleware_decorator to work with async functions.