Opened 3 years ago

Last modified 22 months ago

#33588 assigned Bug

@never_cache and @cache_page decorators are applied out of order for TemplateResponse.

Reported by: Jacek Wojna Owned by: noneNote
Component: Core (Cache system) Version: 4.0
Severity: Normal Keywords: cache, never_cache, cache_page, TemplateResponse, make_middleware_decorator
Cc: Carlton Gibson Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The never_cache decorator is a simple decorator, which applies add_never_cache_headers on a response. On the other hand cache_page decorator is based on CacheMiddleware by using decorator_from_middleware_with_args adapter. The impact of this issue is not applying cache_page functionality to some of the Django views.

If both are used simultaneously, everything behaves fine for a regular django view, which returns HttpResponse.
In the case of working with TemplateResponse, the decorators are invoked in the correct order but applied out of order.

The difference between the responses is that the TemplateResponse has a callable render method and the decorator (based on CacheMiddleware) has a process_response method.

Because of that, we register a new post_render_callback here:
https://github.com/django/django/blob/fdf209eab8949ddc345aa0212b349c79fc6fdebb/django/utils/decorators.py#L145

# Defer running of process_response until after the template
# has been rendered:
if hasattr(middleware, "process_response"):

    def callback(response):
        return middleware.process_response(request, response)

    response.add_post_render_callback(callback)

In comparison with never_cache, which is not based on middleware, we don't postpone applying the decorator, thus it's always applied before cache_page regardless of their order.

It means, the following definitions are returning the same output, spite of the different decorators' order:

# I used here Wagtail's Page model as an example, because their views return `TemplateResponse`
@method_decorator(never_cache, name="serve")
@method_decorator(cache_page(settings.CACHE_TIME), name="serve")
class SomePage(Page):
    ...
# or

@method_decorator(cache_page(settings.CACHE_TIME), name="serve")
@method_decorator(never_cache, name="serve")
class SomePage(Page):
    ...

This issue was originally posted on Wagtail project, but after figuring out what the actual issue is, I decided to post it here. For reference and described debugging process, please go to:
https://github.com/wagtail/wagtail/issues/7666


Workaround

To temporarily solve the issue, I decided to create a workaround by redefining what never_cache is:

class NeverCacheMiddleware:
    def process_response(self, request, response):
        add_never_cache_headers(response)

        return response

never_cache = decorator_from_middleware(NeverCacheMiddleware)

As you can see, it tries to mimic how cache_page was implemented. Because of this it will also add_post_render_callback and postpone applying add_never_cache_headers.


Tests

views.py

from django.http import HttpResponse
from django.template.response import TemplateResponse
from django.views.decorators.cache import cache_page, never_cache


@never_cache
@cache_page(3600)
def never_cache_first_http_response(request):
    return HttpResponse("never_cache_first_http_response")


@cache_page(3600)
@never_cache
def cache_page_first_http_response(request):
    return HttpResponse("cache_page_first_http_response")


@never_cache
@cache_page(3600)
def never_cache_first_template_response(request):
    return TemplateResponse(request, "app/template.html")


@cache_page(3600)
@never_cache
def cache_page_first_template_response(request):
    return TemplateResponse(request, "app/template.html")

test_views.py

from django.test import Client, SimpleTestCase
from freezegun import freeze_time
from email.utils import parsedate_to_datetime

@freeze_time("2022-01-14T10:00:00Z")
class TestHttpResponse(SimpleTestCase):
    def test_different_expires_header_value(self):
        client = Client()
        first_never_cache = client.get("/never-cache-first-http-response")
        first_cache_page = client.get("/cache-page-first-http-response")

        assert first_never_cache.headers['Expires'] != first_cache_page.headers['Expires']

        first_never_cache_datetime = parsedate_to_datetime(first_never_cache.headers['Expires'])
        first_cache_page_datetime = parsedate_to_datetime(first_cache_page.headers['Expires'])

        # Check that `cache_page` correcty sets the Expires header (to be +1 hour)
        assert (first_never_cache_datetime - first_cache_page_datetime).seconds == 3600

@freeze_time("2022-01-14T10:00:00Z")
class TestTemplateResponse(SimpleTestCase):
    def test_different_expires_header_value(self):
        client = Client()
        first_never_cache = client.get("/never-cache-first-template-response")
        first_cache_page = client.get("/cache-page-first-template-response")

        # Fails, because headers have the same value
        assert first_never_cache.headers['Expires'] != first_cache_page.headers['Expires']

fails with:

E       AssertionError: assert 'Fri, 14 Jan 2022 10:00:00 GMT' != 'Fri, 14 Jan 2022 10:00:00 GMT'

app/test_views.py:28: AssertionError

Change History (5)

comment:1 by Jacek Wojna, 3 years ago

Version: 3.24.0

comment:2 by Mariusz Felisiak, 3 years ago

Cc: Carlton Gibson added
Summary: never_cache and cache_page applied out of order for TemplateResponse@never_cache and @cache_page decorators are applied out of order for TemplateResponse.
Triage Stage: UnreviewedAccepted

Thanks, sounds valid. Unfortunately other decorators are probably also affected e.g. @cache_control (see also #11479 about @last_modified). As far as I'm aware, we could fix both tickets by adding a few hooks to FetchFromCacheMiddleware and UpdateCacheMiddleware and use them in the @cache_page decorator instead of relying directly on the CacheMiddleware.

comment:3 by Domen Blenkuš, 2 years ago

Owner: changed from nobody to Domen Blenkuš
Status: newassigned

comment:4 by noneNote, 22 months ago

Owner: changed from Domen Blenkuš to noneNote

comment:5 by noneNote, 22 months ago

@never_cache
@cache_page(3600)
def tem_view(request):
    r = random.randint(1000, 9999)
    return TemplateResponse(request, 'index.html', context=dict(value=r))

Here is what going on at 1st request,
never_cache -> CacheMiddleware.process_request[no cache found] -> response.add_post_render_callback(CacheMiddleware.process_response) -> add_never_cache_headers(response) -> after render calling CacheMiddleware.process_response but returning response without setting cache because of this,

        if "private" in response.get("Cache-Control", ()):
            return response

Suppose we somehow set cache, Here, what going to happen on at 2nd request,
never_cache -> CacheMiddleware.process_request[cache found] -> add_never_cache_headers(response)

What is the problem:
1) never_cache, cache_control, last_modified setting the headers immediately while the cache is going to be set after render(), Thus cache-Control, and other headers are present at the time of setting the cache
2) there is no good way of detecting @decorator's orders and numbers, which means we can not detect which and how many decorator users applied before CacheMiddleware
3) deferring CacheMiddleware.process_response execution making whole @decorator's orders meaningless.

Here is what I suggest as, a solution:

We already have add_post_render_callback so how about we add an add_apply_last_callback to HttpResponse / HttpResponseBase,
apply_last_callback is going to execute just before we send the final response.
It is a multipurpose solution, a user also can use it from view to modify the response object at end of the chain

@last_modified(latest_entry, apply_last=True)
@never_cache(apply_last=True)
@cache_page(3600)
def tem_view(request):
    r = random.randint(1000, 9999)
    return TemplateResponse(request, 'index.html', context=dict(value=r))

Please Let me Know, your views on this.

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