Opened 5 months ago

Closed 5 months ago

Last modified 5 months 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 Changed 5 months ago 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.)

comment:2 Changed 5 months ago by Carlton Gibson

Component: UncategorizedHTTP handling

comment:3 Changed 5 months ago by Carlton Gibson

The check has been in sensitive_post_parameters for a long time.

comment:4 Changed 5 months ago by Mariusz Felisiak

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.

comment:5 in reply to:  4 ; Changed 5 months ago by jason

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.

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.

Last edited 5 months ago by jason (previous) (diff)

comment:6 in reply to:  5 Changed 5 months ago by Mariusz Felisiak

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 Changed 5 months ago 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
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 Changed 5 months ago by Mariusz Felisiak

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

comment:9 Changed 5 months ago by Mariusz Felisiak

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 Changed 5 months ago by Mariusz Felisiak

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.

comment:11 in reply to:  8 Changed 5 months ago by jason

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 Changed 5 months ago by 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.

comment:13 in reply to:  12 Changed 5 months ago by jason

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 Changed 5 months ago by Mariusz Felisiak

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

comment:15 Changed 5 months ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:16 Changed 5 months ago by GitHub <noreply@…>

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 Changed 5 months ago by Mariusz Felisiak <felisiak.mariusz@…>

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