Fix caching issues for remote content API calls

kolibri/core/content/api.py

@@ -15,6 +15,7 @@
 from django.db.models import Sum
 from django.db.models.aggregates import Count
 from django.http import Http404
+from django.utils.cache import add_never_cache_headers
 from django.utils.decorators import method_decorator
 from django.utils.encoding import force_bytes
 from django.utils.encoding import iri_to_uri
@@ -91,59 +92,97 @@
 logger = logging.getLogger(__name__)
 
 
+REMOTE_ETAG_CACHE_KEY = "remote_content_etag_{}"
+
+REMOTE_URL_PARAM = "baseurl"
+
+
 def get_cache_key(*args, **kwargs):
     return str(ContentCacheKey.get_cache_key())
 
 
-def metadata_cache(view_func):
+def metadata_cache(view_func, cache_key_func=get_cache_key):
     """
-    Decorator for patch_response_headers function
+    Decorator to apply an Etag sensitive page cache
     """
 
-    @etag(get_cache_key)
+    @etag(cache_key_func)
     def wrapper_func(*args, **kwargs):
         try:
             request = args[0]
             request = kwargs.get("request", request)
         except IndexError:
             request = kwargs.get("request", None)
-        key_prefix = get_cache_key()
+        # Prevent the Django caching middleware from caching
+        # this response, as we want to cache it ourselves
+        request._cache_update_cache = False
+        key_prefix = get_cache_key(request)
         url_key = hashlib.md5(
             force_bytes(iri_to_uri(request.build_absolute_uri()))
         ).hexdigest()
-        cache_key = "{}:{}".format(key_prefix, url_key)
-        response = cache.get(cache_key)
+        response = None
+        if key_prefix is not None:
+            cache_key = "{}:{}".format(key_prefix, url_key)
+            response = cache.get(cache_key)
         if response is None:
             response = view_func(*args, **kwargs)
-            response.add_post_render_callback(
-                lambda r: cache.set(cache_key, r, timeout=3600)
-            )
+            if response.status_code == 200:
+                if key_prefix is None:
+                    key_prefix = get_cache_key(request)
+                if (
+                    key_prefix is not None
+                    and hasattr(response, "render")
+                    and callable(response.render)
+                ):
+                    cache_key = "{}:{}".format(key_prefix, url_key)
+                    response.add_post_render_callback(
+                        lambda r: cache.set(cache_key, r, timeout=3600)
+                    )
+            else:
+                # Don't cache responses that returned an error code
+                add_never_cache_headers(response)
         return response
 
-    return session_exempt(wrapper_func)
+    return wrapper_func
 
 
-class RemoteMixin(object):
-    remote_url_param = "baseurl"
+def get_remote_cache_key(request, *args, **kwargs):
+    if REMOTE_URL_PARAM in request.GET:
+        return cache.get(REMOTE_ETAG_CACHE_KEY.format(request.GET[REMOTE_URL_PARAM]))
+    return get_cache_key(*args, **kwargs)
+
 
+def remote_metadata_cache(view_func):
+    return session_exempt(
+        metadata_cache(view_func, cache_key_func=get_remote_cache_key)
+    )
+
+
+class RemoteMixin(object):
     def _should_proxy_request(self, request):
-        return self.remote_url_param in request.GET
+        return REMOTE_URL_PARAM in request.GET
 
     def _get_request_headers(self, request):
         return {
             "Accept": request.META.get("HTTP_ACCEPT"),
-            # Don't proxy client's accept headers as it may include br for brotli
+            # Don't proxy client's accept encoding headers as it may include br for brotli
             # that we cannot rely on having decompression for available on the server.
             "Accept-Language": request.META.get("HTTP_ACCEPT_LANGUAGE"),
             "Content-Type": request.META.get("CONTENT_TYPE"),
             "If-None-Match": request.META.get("HTTP_IF_NONE_MATCH", ""),
         }
 
     def _get_response_headers(self, response):
-        return {
-            "Cache-Control": response.headers.get("cache-control"),
-            "Etag": response.headers.get("etag"),
-        }
+        headers = {}
+        header_names = ["Cache-Control", "Etag", "Expires", "Date", "Last-Modified"]
+        for header_name in header_names:
+            if header_name in response.headers:
+                headers[header_name] = response.headers[header_name.lower()]
+        return headers
+
+    def _cache_etag(self, baseurl, headers):
+        cache_key = REMOTE_ETAG_CACHE_KEY.format(baseurl)
+        cache.set(cache_key, headers["Etag"], 3600)
 
     def update_data(self, response_data, baseurl):
         return response_data
@@ -156,9 +195,9 @@ def _hande_proxied_request(self, request):
             ),
             "/api/public/v2/",
         )
-        baseurl = request.GET[self.remote_url_param]
+        baseurl = request.GET[REMOTE_URL_PARAM]
         qs = request.GET.copy()
-        del qs[self.remote_url_param]
+        del qs[REMOTE_URL_PARAM]
         try:
             validator(baseurl)
         except ValidationError:
@@ -177,10 +216,12 @@ def _hande_proxied_request(self, request):
                 content = self.update_data(response.json(), baseurl)
             except ValueError:
                 content = response.content
+            headers = self._get_response_headers(response)
+            self._cache_etag(baseurl, headers)
             return Response(
                 content,
                 status=response.status_code,
-                headers=self._get_response_headers(response),
+                headers=headers,
             )
         except RequestException:
             # If any sort of error due to connection or timeout, raise a resource gone error
@@ -308,7 +349,7 @@ def filter_options(self, request, **kwargs):
         return Response(data)
 
 
-@method_decorator(metadata_cache, name="dispatch")
+@method_decorator(remote_metadata_cache, name="dispatch")
 class ChannelMetadataViewSet(BaseChannelMetadataMixin, RemoteViewSet):
     pass
 
@@ -690,9 +731,9 @@ def update_data(self, response_data, baseurl):
                 # This is a paginated object
                 if response_data["more"] is not None:
                     if type(response_data["more"].get("params", None)) is dict:
-                        response_data["more"]["params"][self.remote_url_param] = baseurl
+                        response_data["more"]["params"][REMOTE_URL_PARAM] = baseurl
                     else:
-                        response_data["more"][self.remote_url_param] = baseurl
+                        response_data["more"][REMOTE_URL_PARAM] = baseurl
                 response_data["results"] = self.update_data(
                     response_data["results"], baseurl
                 )
@@ -713,7 +754,7 @@ def update_data(self, response_data, baseurl):
         return response_data
 
     def add_base_url_to_node(self, node, baseurl):
-        baseurl_querystring = "?{}={}".format(self.remote_url_param, baseurl)
+        baseurl_querystring = "?{}={}".format(REMOTE_URL_PARAM, baseurl)
         if node["thumbnail"]:
             node["thumbnail"] += baseurl_querystring
         for file in node["files"]:
@@ -766,7 +807,7 @@ def get_paginated_response_schema(self, schema):
         }
 
 
-@method_decorator(metadata_cache, name="dispatch")
+@method_decorator(remote_metadata_cache, name="dispatch")
 class ContentNodeViewset(InternalContentNodeMixin, RemoteMixin, ReadOnlyValuesViewset):
     pagination_class = OptionalContentNodePagination
 
@@ -1110,7 +1151,7 @@ def retrieve(self, request, pk=None):
         return Response(parent)
 
 
-@method_decorator(metadata_cache, name="dispatch")
+@method_decorator(remote_metadata_cache, name="dispatch")
 class ContentNodeTreeViewset(BaseContentNodeTreeViewset, RemoteMixin):
     def retrieve(self, request, pk=None):
         if pk is None:

kolibri/core/public/api.py

@@ -11,6 +11,7 @@
 from django.utils import timezone
 from django.utils.cache import patch_cache_control
 from django.utils.decorators import method_decorator
+from django.utils.http import http_date
 from django.views.decorators.csrf import csrf_exempt
 from django.views.decorators.gzip import gzip_page
 from django_filters.rest_framework import DjangoFilterBackend
@@ -27,6 +28,7 @@
 from .constants.user_sync_statuses import SYNC
 from kolibri.core.api import BaseValuesViewset
 from kolibri.core.api import ReadOnlyValuesViewset
+from kolibri.core.auth.middleware import session_exempt
 from kolibri.core.auth.models import Facility
 from kolibri.core.auth.models import FacilityUser
 from kolibri.core.content.api import BaseChannelMetadataMixin
@@ -115,10 +117,13 @@ def public_metadata_cache(view_func):
 
     def wrapped_view(*args, **kwargs):
         response = view_func(*args, **kwargs)
-        patch_cache_control(response, max_age=60)
+        patch_cache_control(
+            response, max_age=300, stale_while_revalidate=100, public=True
+        )
+        response["Expires"] = http_date(time.time() + 300)
         return response
 
-    return wrapped_view
+    return session_exempt(wrapped_view)
 
 
 @method_decorator(public_metadata_cache, name="dispatch")