[Django] #33350: some view decorators do not work with Django REST framework in Django 4.0
Django
------------------------------------------+--------------------------------
Reporter: Terence Honles | Owner: Terence Honles
Type: Bug | Status: assigned
Component: Uncategorized | Version: 4.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+--------------------------------
The cache decorators `cache_control`, `never_cache` and
`sensitive_post_parameters` no longer work with Django REST framework
because they strictly check for an `HttpRequest` instance.
--
Ticket URL: <https://code.djangoproject.com/ticket/33350>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Terence Honles | Owner: Terence Honles
Type: Bug | Status: assigned
Component: Uncategorized | Version: 4.0
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hi Terence — Thanks for the report. Can you post a traceback quickly, or
better a reproduce? (But a traceback would be good.)
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:1>
Django
Reporter: Terence Honles | Owner: Terence Honles
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* component: Uncategorized => HTTP handling
Comment:
3fd82a62415e748002435e7bad06b5017507777c for #32468 looks likely.
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:2>
Django
Reporter: Terence Honles | Owner: Terence Honles
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
The check has been in sensitive_post_parameters
[https://github.com/django/django/blame/64839512a6ed04a29e49e246acf8337b1be2cb8e/django/views/decorators/debug.py#L80
for a long time].
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:3>
Django
Reporter: Terence Honles | Owner: Terence Honles
Component: HTTP handling | Version: 4.0
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: assigned => closed
* resolution: => needsinfo
Comment:
I don't see how this can be a regression, these decorators use functions
from [https://docs.djangoproject.com/en/4.0/ref/utils/#module-
django.utils.cache django.utils.cache] which are documented as accepting
`HttpResponse` 🤔 Also `Response` defined in `django-rest-framework` is a
subclass of `HttpResponse`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:4>
Django
Reporter: Terence Honles | Owner: Terence Honles
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jason):
* status: closed => new
* resolution: needsinfo =>
Comment:
Replying to [comment:4 Mariusz Felisiak]:
> I don't see how this can be a regression, these decorators use functions
from [https://docs.djangoproject.com/en/4.0/ref/utils/#module-
django.utils.cache django.utils.cache] which are documented as accepting
`HttpResponse` 🤔 Also `Response` defined in `django-rest-framework` is a
subclass of `HttpResponse`.
[https://github.com/encode/django-rest-
framework/blob/master/rest_framework/request.py#L140 Response] has never
inherited from `HttpResponse` which has been the source of many issues
like this even before 4.0. I am experiencing a lot of issues right now
converting Django method views into DRF class-based views right now
because all of the decorators keep breaking.
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:5>
Django
Reporter: Terence Honles | Owner: Terence Honles
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Tom Christie, Carlton Gibson (added)
Comment:
Replying to [comment:5 jason]:
> [https://github.com/encode/django-rest-
framework/blob/master/rest_framework/request.py#L140 Request] has never
inherited from `HttpRequest` which has been the source of many issues like
this even before 4.0. I am experiencing a lot of issues right now
converting Django method views into DRF class-based views right now
because all of the decorators keep breaking.
Sorry, I confused `HttpRequest` with `HttpResponse`. Nevertheless, IMO, if
DRF is duck typing `HttpRequest` it should be fixed in DRF, e.g. by adding
`__instancecheck__()`. What do you think Tom? I'm happy to provide a patch
to DRF.
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:6>
Django
Reporter: Terence Honles | Owner: Terence Honles
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
I think this is going to cause trouble for a lot of folks.
The `sensitive_post_parameters` has been as it is for many years, but the
[https://www.django-rest-framework.org/api-guide/caching/#using-cache-
with-apiview-and-viewsets caching usage is standard DRF]. (Again that's
been there a long time.)
The check in 3fd82a62415e748002435e7bad06b5017507777c seems overly tight.
What methods are being called?
Surely any Request-a-like exposing those is OK?
I don't think we can just change `Request` to claim to be a `HttpRequest`.
[https://github.com/encode/django-rest-
framework/blob/master/rest_framework/request.py#L154-L158 DRF has an
`isinstance()` check for this very thing].
Introduced in https://github.com/encode/django-rest-framework/pull/5618
See also:
- https://github.com/encode/django-rest-framework/issues/5446
- https://github.com/encode/django-rest-framework/issues/3848
(and others)
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:7>
Django
Reporter: Terence Honles | Owner: Terence Honles
Component: HTTP handling | Version: 4.0
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
This error was added to catch misuses of `never_cache()` and
`cache_control()` decorators without `method_decorator()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:8>
Django
Reporter: Terence Honles | Owner: Terence
| Honles
Component: HTTP handling | Version: 4.0
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* has_patch: 1 => 0
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
We've discussed this with Carlton, and see two possible solutions:
- allow for `HttpRequest` duck-typing, or
- changing errors to be based on missing `method_decorator()` call, maybe
checking that `request` is not a callable 🤔 or sth similar.
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:9>
Django
requests.
-------------------------------------+-------------------------------------
Reporter: Terence Honles | Owner: Terence
| Honles
Component: HTTP handling | Version: 4.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:10>
Django
requests.
-------------------------------------+-------------------------------------
| Honles
Component: HTTP handling | Version: 4.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jason):
Replying to [comment:8 Mariusz Felisiak]:
> This error was added to catch misuses of `never_cache()` and
`cache_control()` decorators without `method_decorator()`.
Breaking functionality in order to provide a nice error message seems
backwards.
Then again, I can't think of a good excuse for DRF not to simply inherit
from HttpRequest.
I recently fell down a rabbit-hole of github issues that were closed
without resolution regarding this conflict, where DRF refuses to inherit
because "composition pattern", and django refuses not to check because
"nice error message" over and over.
Something has to give right?
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:11>
Django
requests.
-------------------------------------+-------------------------------------
| Honles
Component: HTTP handling | Version: 4.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
> Something has to give right?
This ticket is accepted. Have you seen my last
[https://code.djangoproject.com/ticket/33350#comment:9 comment]? We agreed
that this should be fixed in Django itself.
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:12>
Django
requests.
-------------------------------------+-------------------------------------
| Honles
Component: HTTP handling | Version: 4.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jason):
Replying to [comment:12 Mariusz Felisiak]:
> > Something has to give right?
>
> This ticket is accepted. Have you seen my last
[https://code.djangoproject.com/ticket/33350#comment:9 comment]? We agreed
that this should be fixed in Django itself.
Just saw it, don't mean to beat a dead horse, thank you for your quick
response ^_^
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:13>
Django
requests.
-------------------------------------+-------------------------------------
| Felisiak
Type: Bug | Status: assigned
Component: HTTP handling | Version: 4.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Terence Honles => Mariusz Felisiak
* status: new => assigned
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/15206 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:14>
Django
requests.
-------------------------------------+-------------------------------------
Reporter: Terence Honles | Owner: Mariusz
| Felisiak
Component: HTTP handling | Version: 4.0
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:15>
Django
requests.
-------------------------------------+-------------------------------------
Reporter: Terence Honles | Owner: Mariusz
| Felisiak
Component: HTTP handling | Version: 4.0
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"40165eecc40f9e223702a41a0cb0958515bb1f82" 40165eec]:
{{{
#!CommitTicketReference repository=""
revision="40165eecc40f9e223702a41a0cb0958515bb1f82"
Fixed #33350 -- Reallowed using cache decorators with duck-typed
HttpRequest.
Regression in 3fd82a62415e748002435e7bad06b5017507777c.
Thanks Terence Honles for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:16>
Django
requests.
-------------------------------------+-------------------------------------
Reporter: Terence Honles | Owner: Mariusz
| Felisiak
Type: Bug | Status: closed
Component: HTTP handling | Version: 4.0
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"c1d2e8b9b8f41d3effef03badc78c8b8995a99b6" c1d2e8b]:
{{{
#!CommitTicketReference repository=""
revision="c1d2e8b9b8f41d3effef03badc78c8b8995a99b6"
[4.0.x] Fixed #33350 -- Reallowed using cache decorators with duck-typed
HttpRequest.
Regression in 3fd82a62415e748002435e7bad06b5017507777c.
Thanks Terence Honles for the report.
Backport of 40165eecc40f9e223702a41a0cb0958515bb1f82 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33350#comment:17>