Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33350 closed Bug (fixed)

never_cache()/cache_control() decorators raise error on duck-typed requests.

Reported by: Terence Honles Owned by: Mariusz Felisiak
Component: HTTP handling Version: 4.0
Severity: Release blocker Keywords:
Cc: Tom Christie, Carlton Gibson Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (17)

comment:1 by Carlton Gibson, 3 years ago

Hi Terence — Thanks for the report. Can you post a traceback quickly, or better a reproduce? (But a traceback would be good.)

comment:2 by Carlton Gibson, 3 years ago

Component: UncategorizedHTTP handling

comment:3 by Carlton Gibson, 3 years ago

The check has been in sensitive_post_parameters for a long time.

comment:4 by Mariusz Felisiak, 3 years ago

Resolution: needsinfo
Status: assignedclosed

I don't see how this can be a regression, these decorators use functions from django.utils.cache which are documented as accepting HttpResponse 🤔 Also Response defined in django-rest-framework is a subclass of HttpResponse.

in reply to:  4 ; comment:5 by jason, 3 years ago

Resolution: needsinfo
Status: closednew

Replying to Mariusz Felisiak:

I don't see how this can be a regression, these decorators use functions from django.utils.cache which are documented as accepting HttpResponse 🤔 Also Response defined in django-rest-framework is a subclass of HttpResponse.

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.

Version 0, edited 3 years ago by jason (next)

in reply to:  5 comment:6 by Mariusz Felisiak, 3 years ago

Cc: Tom Christie Carlton Gibson added

Replying to jason:

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.

comment:7 by Carlton Gibson, 3 years ago

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

DRF has an `isinstance()` check for this very thing.

Introduced in https://github.com/encode/django-rest-framework/pull/5618

See also:

(and others)

comment:8 by Mariusz Felisiak, 3 years ago

This error was added to catch misuses of never_cache() and cache_control() decorators without method_decorator().

comment:9 by Mariusz Felisiak, 3 years ago

Has patch: unset
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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.

comment:10 by Mariusz Felisiak, 3 years ago

Summary: some view decorators do not work with Django REST framework in Django 4.0never_cache()/cache_control() decorators raise error on duck-typed requests.

in reply to:  8 comment:11 by jason, 3 years ago

Replying to 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?

comment:12 by Mariusz Felisiak, 3 years ago

Something has to give right?

This ticket is accepted. Have you seen my last comment? We agreed that this should be fixed in Django itself.

in reply to:  12 comment:13 by jason, 3 years ago

Replying to Mariusz Felisiak:

Something has to give right?

This ticket is accepted. Have you seen my last 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 _

comment:14 by Mariusz Felisiak, 3 years ago

Has patch: set
Owner: changed from Terence Honles to Mariusz Felisiak
Status: newassigned

comment:15 by Carlton Gibson, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:16 by GitHub <noreply@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 40165eec:

Fixed #33350 -- Reallowed using cache decorators with duck-typed HttpRequest.

Regression in 3fd82a62415e748002435e7bad06b5017507777c.

Thanks Terence Honles for the report.

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In c1d2e8b:

[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

Note: See TracTickets for help on using tickets.
Back to Top