Opened 4 years ago
Last modified 3 years 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 , 4 years ago
| Version: | 3.2 → 4.0 | 
|---|
comment:2 by , 4 years ago
| Cc: | 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: | Unreviewed → Accepted | 
comment:3 by , 3 years ago
| Owner: | changed from to | 
|---|---|
| Status: | new → assigned | 
comment:4 by , 3 years ago
| Owner: | changed from to | 
|---|
comment:5 by , 3 years 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.
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 toFetchFromCacheMiddlewareandUpdateCacheMiddlewareand use them in the@cache_pagedecorator instead of relying directly on theCacheMiddleware.