Fixed #31949 -- Made builtin decorator async compatible

[LomaxOnTheRun]

Fix for #31949.

Makes builtin decorators async compatible so that they can be applied directly to async views without having to be wrapped in additional @async_to_sync and @sync_to_async decorators.

The two main changes are the creation of sync_async_wrapper and the update to make_middleware_decorator which allow for most of the decorators to be simply converted to having a sync and an async implementation. The sync_async_wrapper works by offering "hooks" where calling code can pass in functions which affect the request and the response, in a cut down but very similar way to how make_middleware_decorator currently works.

Decorators:

• [x] @cache_page (uses decorator_from_middleware_with_args)
• [x] @cache_control
• [x] @never_cache
• [x] @xframe_options_deny
• [x] @xframe_options_sameorigin
• [x] @xframe_options_exempt
• [x] no_append_slash
• [x] @csrf_protect (uses decorator_from_middleware)
• [x] @requires_csrf_token (uses decorator_from_middleware)
• [x] @ensure_csrf_cookie (uses decorator_from_middleware)
• [x] @csrf_exempt
• [x] @sensitive_variables
• [x] @sensitive_post_parameters
• [x] @gzip_page (uses decorator_from_middleware)
• [x] conditional_page (uses decorator_from_middleware)
• [x] @require_http_methods
• [x] @require_GET
• [x] @require_POST
• [x] @require_safe
• [x] @condition
• [x] @etag
• [x] @last_modified
• [x] @vary_on_headers
• [x] @vary_on_cookie

[hendrikfrentrup]

Great to see work on this - I recently stumbled upon problems with the HTTP decorators and tried a similar fix (https://github.com/django/django/compare/master...hendrikfrentrup:master). As far as I can see you haven't gotten to them yet, so I can give these a go if you don't mind. I haven't looked into async testing yet, so cannot comment much on your questions.

[LomaxOnTheRun]

@hendrikfrentrup if you want to wire up the http decorator please be my guest :slightly_smiling_face: I've pushed up my latest changes, I'll avoid force pushing from here onwards to avoid overwiting any of your stuff :slightly_smiling_face:

[hendrikfrentrup]

OK, I slightly rewrote my fix to adjust to the syntax suggested by felixxm. Also, it wasn't entirely straight forward to write an async test because the test suite runs slightly different to how an application routes to a view via the URL paths (the view is not awaited in the URL path while in the test await my_safe_view(request) would await for the return of the HttpResponse)

Anyway, @LomaxOnTheRun if you add me as a contributor to your forked repo, I can push to this PR. Or do you want me to push to my own forked repo?

[hendrikfrentrup]

For visibility, I've pushed the changes to an equivalent branch in my forked repo here - I can push the same changes to this PR or merge it onto your branch. Not sure which one is more convenient.

[LomaxOnTheRun]

@hendrikfrentrup I've sent you an invite to be a collaborator on my repo's fork. It's probably best to have everything on the same branch so that it can all be checked at the same time, given it's all similar / connected functionality.

[LomaxOnTheRun]

@hendrikfrentrup as a heads up I've tweaked the base sync_async_wrapper in my last commit so if you need to change it (which it looks like you may need to to allow for changes to the request, not just the response), it might be worth pulling the branch again :slightly_smiling_face:

Oh, and I think my plan is to just add multiple commits not and look at squashing them towards the end. There's a way to record co-authors in Github too, so I think we should be able to get both of our names in recorded, even if we need to squash this branch to a single commit (as per Django guidelines). Let me know if that doesn't work for you for any reason.

[hendrikfrentrup]

Ok, I've pushed the changes - as far as I can tell your sync_async_wrapper isn't suitable as is for the require_http_methods decorator because it doesn't process the respone at all, it simply checks the request and returns a straightforward Not-Allowed Response if the method isn't allowed. It should come in handy for the condition based decorators though (started looking into that a bit already).

So, the test directly probes whether the decorator is sync and async capable as suggested here and also direcly probes an async view being a coroutine and a sync view staying synchronous. I think that is sufficient because the functionality is essentially tested by the existing test already.

It does in deed require the provided patch to make the test pass. If I had just changed the decorated view(my_safe_view) to async, it doesn't actually throw an exception, just a warning which has been debated here

[hendrikfrentrup]

Also, @LomaxOnTheRun I've also gave some thoughts to your point #2 above. I think it should suffice to write an async test for the async version (in a way it actually would test the code twice). Also, the way I see it, any sync or async runtimes can probe whether functions are sync and/or async capable and the respective view function are decorated according to whether the view itself is sync/async. That's also why I decorated the decorator with @sync_and_async_middleware (I guess I am interpreting this as middleware, in this case builtin middleware). Is there a reason why your decorators aren't decorated sync/async compatible?

[LomaxOnTheRun]

@hendrikfrentrup thanks for all of that info, I'm actually finishing up at my current job this week so I might not have time to look at this until the weekend, but I'll ping you a soon as I do.

The lack of @sync_and_async_middleware decorators was just because I wanted to get a draft up quickly so others could view and comment on it, but I'll need to circle back to them and add them. Thanks for the reminder 😁

[hendrikfrentrup]

Yup, sounds good - have a good week :) I've just force pushed some minor amends hoping you have not pulled the commit. Need to get in the habit of running isort and flake8 in addition to the test suite before pushing. Green check is back now though.

[LomaxOnTheRun]

@hendrikfrentrup just a heads up that I've changed the remaining decorators (and make_middleware_decorator), added the documentation updates, and changed the PR to Ready for checkin in the Django ticket system.

I've also squashed all the commits into one and added you as a co-author (which you can see in the Commits tab) and added you to the AUTHORS page, along with your Github email address (I did the same with mine). Let me know if you'd like me to change either your name or email address, or remove one or both of them and I'll happily do that :slightly_smiling_face:

Otherwise just waiting for the reviews to start rolling in :smile:

[hendrikfrentrup]

@LomaxOnTheRun Nice one! Thanks for adding the author notes 🙂. Also looking forward to the comments.

Hard to wrap my head around the make_middleware_decorator, but looks good to me. While it seems like the methods check_request_method and add_response_headers could also be re-written as class-based Middleware using the process_request and process_response approach, it's not like much would be gained from it - I find it more readable as is (prefer functional over object-oriented personally). Plus, it is closer to the current implementation.

[auvipy]

@andrewgodwin might be of your interest

[rednaks]

login_required ?

[auvipy]

its a major change so it have to be targetted for v4.0

[roysmith]

I didn't see anything that was broken, these were all just style and refactoring suggestions, so I'm good with whichever ones you decided to pick up or reject. Thanks for working on the async support; this will be a nice step forward in django's feature set.

On May 3, 2021, at 4:53 PM, Ben Lomax @.***> wrote:

@LomaxOnTheRun commented on this pull request.

@roysmith https://github.com/roysmith thank you for all of your reviews, I've either changed things accordingly, or pushed back with an explanation. If you feel like any of the points merit further discussion I'm very happy to discus them further 🙂

I've also not added release notes yet since I doubt that it'll be shipped with version 3.2.1 which is due out tomorrow, so I'll add it in for (I imagine) 3.2.2, once that document has been created in the repo.

[LomaxOnTheRun]

@carltongibson a huge thanks for the review, this had dropped off my radar for a while due to a lack of reviews but since there's renewed interest in it I'll spend some time to get it back into a good state. I'll also keep this PR to a minimum and avoid "clean-up" changes.

[carltongibson]

Hey @LomaxOnTheRun — great, thanks! Just FYI, I'm trying to pick up and process a bunch of these async tickets (targeting Django 4.2) so this is on my list for that. 👍

Thanks for the input! 🎁

[LomaxOnTheRun]

@auvipy I'm looking at getting this rebased now (and updated as per the latest code review). As for:

also did you considered sync_to_async from asgiref?

The Django ticket which prompted this work is about not having to use sync_to_async and async_to_sync to switch between async and sync decorators, and to allow the builtin view decorators to just be able to handle both. If you meant using sync_to_async in the code, it seems to me that a direct implementation is better than relying on a third party library when we're able to implement it directly :slightly_smiling_face:

[LomaxOnTheRun]

@carltongibson how would you feel about my splitting this PR into several commits, at least while I work on it to get it approved (I can always squash them into a single commit right at the end)? It feels like it might make the whole workflow significantly easier to work on and review for both of us (and obviously any other interested reviewers) :slightly_smiling_face:

[carltongibson]

@LomaxOnTheRun No problem! Whatever makes it easiest/clearest FTW 😄

[LomaxOnTheRun]

@carltongibson just to let you know that I'm working my way through all of the decorators again one by one, as it's been 14 months since I last worked on this branch and things on main have changed since then. I'm using this branch on my personal repo to go through and makes changes and notes as I go, so I can have a scratchpad while keeping all of your comments on this branch as they are.

From my initial foray, it looks like the decorators split into roughly 3 piles:

1. Easy to refactor decorators.
2. Decorators that use decorator_from_middleware(_with_args)
   • These don't currently work on my branch but it seems that once I figure out how to make one work the rest should be easy
3. Other misc awkward ones for various reasons
   • These are more of a pain as they will likely need attention given to each one individually

My plan is to get at least pile 1 done in this PR, and look at possibly splitting piles 2 and 3 into separate PRs. Let me know if that seems like a good / bad idea, and also feel free to move the conversation to the other branch (fair warning, it's quite fluid and prone to getting rebased) :slightly_smiling_face:

[carltongibson]

@LomaxOnTheRun — great. Plug away until you feel like your progress is slowing, and then I can have another look. Thanks for the input!

[LomaxOnTheRun]

@carltongibson I think I'm at a good point to push up a PR which covers most of the decorators. Would it be better to rebase this one (to keep the full history of comments) or to create a new PR (which would be cleaner)?

I've also got some commits which are an attempt to reduce the duplication in the decorator code, but I think they would be best shown in a separate PR, based on the main PR.

[auvipy]

you can push here for now

[LomaxOnTheRun]

@carltongibson @auvipy I've pushed my latest work to this branch, split out into smaller commits (which will hopefully make things a little easier to review). As per the PR description there are still 4 decorators which are not covered, but I'm open to either working on fixing those in this PR or creating a new PR specifically to address those.

I've also got a separate draft PR open where I investigated how we might DRY up the code both between the sync_async_wrapper and make_middleware_decorator functions, as well as the sync and async inner function for both. It gets complicated enough that I think it warrants a different PR and discussion to talk about how to best do it.

For the tests, I've not yet found any way to dry up the sync / async repetition, especially since we're specifically testing functions with decorators. I'm keen to hear from anyone if they have ideas on how to approach this.

[gnat]

Thank you for your excellent work so far @LomaxOnTheRun !

As per the PR description there are still 4 decorators which are not covered, but I'm open to either working on fixing those in this PR or creating a new PR specifically to address those.

May I suggest splitting out the unfinished decorators into a seperate ticket to clear the path forward?

Speaking as a newcomer to Django as of 4.1, I've bumped into a lack of async support for @csrf_exempt already while implementing an async endpoint.

I suspect there may be a lot of newcomers running into these as a blocker as people re-visit Django to try out the excellent new async ORM, and it would be ideal if this were shipped out soon. @carltongibson

[LomaxOnTheRun]

@carltongibson @gnat I've actually worked out how to make the last few decorators work for sync and async views. I've also updated the documentation and the 4.2.txt release notes, so I think this PR just needs a review now.

[carltongibson]

Thanks @LomaxOnTheRun — you're on my list 😉

[carltongibson]

Hi @LomaxOnTheRun — I've left some initial comments on the ticket, as I'm not sure about the approach here. 😬

I'm still thinking about this but, to begin, looking at @login_required and LoginRequiredMixin — could we just wrap @login_required to force a sync view (if needed) and use the approach from View.options (linked in comment on ticket) for LoginRequiredMixin? (Then can we generalise from there? 🤔) — That's where I'm thinking next.