coderedcorp / wagtail-cache

A simple page cache for Wagtail based on the Django cache middleware.
Other
87 stars 29 forks source link

direct attention to redis compatibility #45

Closed jonatron closed 2 years ago

jonatron commented 2 years ago

Just a little addition to the docs

vsalvino commented 2 years ago

By chance, if you are using redis, have you tried Django's new built-in redis backend? I would love to confirm that it works. https://docs.djangoproject.com/en/4.0/topics/cache/#redis

jonatron commented 2 years ago

I quickly set up a new test project with Django 4, but I haven't quite replicated the same problem that I had in the project I'm working on. I'll figure it out soon so we know what to do here

jonatron commented 2 years ago

I wanted to understand what was happening, so I spent a while on this. You can replicate the problem with django_redis.cache.RedisCache by creating a new project, and using cache_page+WagtailCacheMixin by having mysite/home/models.py like:

from django.utils.decorators import method_decorator
from wagtail.models import Page
from wagtailcache.cache import cache_page, WagtailCacheMixin

@method_decorator(cache_page, name="serve")
class HomePage(WagtailCacheMixin, Page):
    pass

settings like

 CACHES = {
    "default": {
        "BACKEND": "django_redis.cache.RedisCache",
        "LOCATION": "redis://127.0.0.1:6379/1",
    }
}

Switching to "BACKEND": "wagtailcache.compat_backends.django_redis.RedisCache", fixes the problem. As does "BACKEND": "django.core.cache.backends.redis.RedisCache", (to answer your question!)

There's another fix, by hacking wagtailcache/cache.py UpdateCacheMiddleware process_response , to use a function returning None rather than a lambda:

            if isinstance(response, SimpleTemplateResponse):
                def callback(r):
                    self._wagcache.set(cache_key, r, timeout)
                response.add_post_render_callback(
                    # lambda r: self._wagcache.set(cache_key, r, timeout)
                    callback
                )
vsalvino commented 2 years ago

Glad the new Django redis backend works! Please update the supported_backends doc to list it and confirm it is tested and working.

I'm not sure I understand the problem caused by the lambda vs callback function. Although I am generally in favor or switching to the callback function (lambdas are more difficult to understand). Are you saying that using django_redis's backend (django_redis.cache.RedisCache) works correctly when switching to using a callback function?

jonatron commented 2 years ago

Updated the docs. The reason why returning None from the callback makes django_redis's backend work is here: [by using a function rather than lambda] https://github.com/django/django/blob/main/django/template/response.py#L117

            for post_callback in self._post_render_callbacks:
                newretval = post_callback(retval)
                if newretval is not None:
                    retval = newretval
vsalvino commented 2 years ago

Go ahead and open a separate pull request with that callback change. As long as the tests pass, I think it would be good to merge it in. If that change will remove the need for our compat_backend, all the better, we can remove that as well.