#37124 closed New feature (needsnewfeatureprocess)
AccessMixin: object HttpResponseRedirect can't be used in 'await' expression
| Reported by: | Johannes Maron | Owned by: | zky |
|---|---|---|---|
| Component: | Generic views | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Johannes Maron | Triage Stage: | Unreviewed |
| Has patch: | yes | Needs documentation: | yes |
| Needs tests: | no | Patch needs improvement: | yes |
| Easy pickings: | no | UI/UX: | no |
Description
The redirect response in AccessMixin.handle_no_permission isn't async compatible.
If you have access, everything works fine, since all HTTP method handlers are async and return coroutines; only if you lack permissions or are unauthenticated is the redirect response synchronous.
With access to view_is_async it should be a fairly easy fix to have handle_no_permission return a coroutine.
Thanks to @amirreza-sf80 for reporting:
https://github.com/django/new-features/issues/162#issuecomment-4562082172
I could find the bug on Stack Overflow as well, but no ticket on Trac:
https://stackoverflow.com/questions/66512353/using-loginrequiredmixin-with-async-views
Change History (12)
comment:1 by , 3 weeks ago
comment:2 by , 3 weeks ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
follow-up: 4 comment:3 by , 3 weeks ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
As much as I share your enthusiasm, @ZKY, please be so kind and follow the ticket workflow and either wait for triage or do it yourself. Tickets must be accepted before they are assigned.
comment:4 by , 3 weeks ago
Oh, apologies! I didn't realize I could triage and accept the status myself. I have successfully reproduced this issue locally with a minimal script now.
I'll update the Triage Stage to 'Accepted', re-assign it to myself, and open a PR with the sync_to_async and auser() fix this weekend. Thanks for guiding me through the workflow!
comment:5 by , 3 weeks ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
| Triage Stage: | Unreviewed → Accepted |
comment:6 by , 3 weeks ago
| Has patch: | set |
|---|
comment:7 by , 2 weeks ago
| Needs documentation: | set |
|---|---|
| Patch needs improvement: | set |
comment:8 by , 2 weeks ago
| Triage Stage: | Accepted → Unreviewed |
|---|---|
| Type: | Bug → New feature |
I don't think this is really a bug, it's more that it was never implemented, so I'd lean toward treating it as a new feature. 🙂
- LoginRequiredMixin
- PermissionRequiredMixin
- UserPassesTestMixin
These all rely heavily on sync code like request.user, request.user.has_perms, etc.
Also, request.user and request.auser() don't share a cache, so you'd end up doing two separate queries. To avoid that, I think we should adapt the views for async properly instead of applying partial hot-fixes.
I feel like https://github.com/django/new-features is the right place to discuss both the solution and the future design.
What do you all think?
comment:9 by , 2 weeks ago
| Resolution: | → needsnewfeatureprocess |
|---|---|
| Status: | assigned → closed |
Yes, these mixins were never in-scope for allowing View to define asynchronous handlers. (So New Feature is correct.)
Whilst I disagree with Mykhailo about duplicating View in a separate async version, I agree that some discussion of how to proceed is called for.
... so you'd end up doing two separate queries ...
Yes, any async version here would need to ensure it was using the same pathways as the view it's going to be applied to.
comment:10 by , 2 weeks ago
| Component: | Uncategorized → Generic views |
|---|
comment:11 by , 2 weeks ago
| Easy pickings: | unset |
|---|
comment:12 by , 2 weeks ago
I was suspicious when I transported the issue here already. It looked like a pretty big oversight.
That being said, should we maybe update the docs to reflect the limitation? Not everyone in the community can draw on the level of experience you two have with this code base.
A small note is probably sufficient, right?
should mention
LoginRequiredMixinalso usesrequest.user.is_authenticatedinsidedispatch, but it should useauserinstead,which requires awaiting.