Merge pull request #45 from nottrobin/caching-headers

v0.8.0: Update default cache-control headers

CHANGELOG.md

@@ -1,3 +1,8 @@
+# 0.8.0 (2021-03-19)
+
+Change default caching headers to `cache-control: max-age=60, stale-while-revalidate=86400, stale-if-error=300`.
+Make them individually overrideable.
+
 # 0.7.2 (2021-01-07)
 
 ### Added

canonicalwebteam/flask_base/app.py

@@ -36,18 +36,91 @@ def set_security_headers(response):
 
 
 def set_cache_control_headers(response):
+    """
+    Default caching rules that should work for most pages
+    """
+
     if flask.request.path.startswith("/_status"):
+        # Our status endpoints need to be uncached
+        # to report accurate information at all times
         response.cache_control.no_store = True
-        response.cache_control.max_age = 0
-    elif not response.cache_control and response.status_code == 200:
-        # response.cache_control does not support stale_while_revalidate so...
-        response.headers[
-            "Cache-Control"
-        ] = "public, max-age=300, stale-while-revalidate=360"
+
     elif (
-        flask.request.path.startswith("/static") and "v" in flask.request.args
+        response.status_code == 200
+        and not response.cache_control.no_store
+        and not response.cache_control.no_cache
+        and not response.cache_control.private
     ):
-        response.headers["Cache-Control"] = "public, max-age=31536000"
+        # Normal responses, where the cache-control object hasn't
+        # been independently modified, should:
+
+        if not response.cache_control.max_age:
+            # Hard-cache for a minimal amount of time so content can be easily
+            # refreshed.
+            #
+            # The considerations here are as follows:
+            # - To avoid caching headaches, it's best if when people need to
+            #   refresh a page (e.g. after they've just done a release) they
+            #   can do so by simply refreshing their browser.
+            # - However, it needs to be long enough to protect our
+            #   applications from excessive requests from all the pages being
+            #   generated (a factor)
+            #
+            # 1 minute seems like a good compromise.
+            response.cache_control.max_age = "60"
+
+        if not response.cache_control._get_cache_value(
+            "stale-while-revalidate", False, int
+        ):
+            # stale-while-revalidate defines a period after the cache has
+            # expired (max-age) during which users will get sent the stale
+            # cache, while the cache updates in the background. This mean
+            # that users will continue to get speedy (potentially stale)
+            # responses even if the refreshing of the cache takes a while,
+            # but the content should still be no more than a few seconds
+            # out of date.
+            #
+            # We want this to be pretty long, so users will still get a
+            # quick response (while triggering a background update) for
+            # as long as possible after the cache has expired.
+            #
+            # An additional day will hopefully be long enough for most cases.
+            response.cache_control._set_cache_value(
+                "stale-while-revalidate", "86400", int
+            )
+
+        if not response.cache_control._get_cache_value(
+            "stale-if-error", False, int
+        ):
+            # stale-if-error defines a period of time during which a stale
+            # cache will be served back to the client if the cache observes
+            # an error code response (>=500) from the backend. When the cache
+            # receives a request for an erroring page, after serving the stale
+            # page it will ping off a background request to attempt the
+            # revalidate the page, as long as it's not currently waiting for a
+            # response.
+            #
+            # We set this value to protect us from transitory errors. The
+            # trade-off here is that we may also be masking errors from
+            # ourselves. While we have Sentry and Greylog to potentially alert
+            # us, we might miss these alerts, which could lead to much more
+            # serious issues (e.g. bringing down the whole site). The most
+            # clear manifestation of these error would be simply an error on
+            # the site.
+            #
+            # So we set this to 5 minutes following expiry as a trade-off.
+            response.cache_control._set_cache_value(
+                "stale-if-error", "300", int
+            )
+
+    # TODO: Disabling this until we've got to the bottom of the
+    # cache-poisoning issue for static files
+    #
+    # elif (
+    #     flask.request.path.startswith("/static") and
+    #     "v" in flask.request.args
+    # ):
+    #     response.headers["Cache-Control"] = "public, max-age=31536000"
 
     return response
 

setup.py

@@ -1,8 +1,10 @@
+#! /usr/bin/env python3
+
 from setuptools import setup, find_packages
 
 setup(
     name="canonicalwebteam.flask-base",
-    version="0.7.3",
+    version="0.8.0",
     description=(
         "Flask extension that applies common configurations"
         "to all of webteam's flask apps."

tests/test_app/webapp/app.py

@@ -34,11 +34,42 @@ def soft_redirect():
         flask.session["test"] = True
         return "Found", 302
 
-    @app.route("/cache")
-    def cache():
+    @app.route("/cache/max-age")
+    def cache_max_age():
         response = flask.make_response()
+        response.cache_control.max_age = 4321
+
+        return response, 200
+
+    @app.route("/cache/none")
+    def cache_empty():
+        response = flask.make_response()
+        response.cache_control.max_age = None
+        response.cache_control._set_cache_value(
+            "stale-while-revalidate", None, int
+        )
+        response.cache_control._set_cache_value("stale-if-error", None, int)
+
+        return response, 200
+
+    @app.route("/cache/stale")
+    def cache_stale():
+        response = flask.make_response()
+        response.cache_control._set_cache_value(
+            "stale-while-revalidate", 4321, int
+        )
+
+        return response, 200
+
+    @app.route("/cache/all")
+    def cache_all():
+        response = flask.make_response()
+        response.cache_control.max_age = 4321
         response.cache_control.public = True
-        response.cache_control.max_age = 1000
+        response.cache_control._set_cache_value(
+            "stale-while-revalidate", 4321, int
+        )
+        response.cache_control._set_cache_value("stale-if-error", 4321, int)
 
         return response, 200
 

tests/test_flask_base.py

@@ -63,10 +63,11 @@ def test_security_headers(self):
     def test_default_cache_headers(self):
         with create_test_app().test_client() as client:
             cached_response = client.get("page")
-            self.assertEqual(
-                cached_response.headers.get("Cache-Control"),
-                "public, max-age=300, stale-while-revalidate=360",
-            )
+            cache = cached_response.headers.get("Cache-Control")
+            self.assertNotIn("public", cache)
+            self.assertIn("max-age=60", cache)
+            self.assertIn("stale-while-revalidate=86400", cache)
+            self.assertIn("stale-if-error=300", cache)
 
     def test_redirects_have_no_cache_headers(self):
         with create_test_app().test_client() as client:
@@ -82,23 +83,49 @@ def test_vary_cookie_when_session(self):
                 cached_response_with_session.headers.get("Vary"), "Cookie"
             )
 
-    def test_cache_does_not_overide(self):
+    def test_cache_override(self):
+        """
+        Check each part of the cache-control instruction can be overridden
+        individually, or nullified
+        """
+
         with create_test_app().test_client() as client:
-            cached_response = client.get("cache")
-            self.assertEqual(
-                cached_response.headers.get("Cache-Control"),
-                "public, max-age=1000",
-            )
+            # all 3 values are overriden, and "public" is added
+            all_response = client.get("cache/all")
+            all_cache = all_response.headers.get("Cache-Control")
+            self.assertIn("public", all_cache)
+            self.assertIn("max-age=4321", all_cache)
+            self.assertIn("stale-while-revalidate=4321", all_cache)
+            self.assertIn("stale-if-error=4321", all_cache)
+
+            # all values are nullified, leading no cache-control header
+            none_response = client.get("cache/none")
+            none_cache = none_response.headers.get("Cache-Control")
+            self.assertIsNone(none_cache)
+
+            # only max-age is overridden, so the "stale" instructions remain
+            max_age_response = client.get("cache/max-age")
+            max_age_cache = max_age_response.headers.get("Cache-Control")
+            self.assertNotIn("public", max_age_cache)
+            self.assertIn("max-age=4321", max_age_cache)
+            self.assertIn("stale-while-revalidate=86400", max_age_cache)
+            self.assertIn("stale-if-error=300", max_age_cache)
+
+            # only "stale-while-revalidate" is overridden
+            stale_response = client.get("cache/stale")
+            stale_cache = stale_response.headers.get("Cache-Control")
+            self.assertNotIn("public", stale_cache)
+            self.assertIn("max-age=60", stale_cache)
+            self.assertIn("stale-while-revalidate=4321", stale_cache)
+            self.assertIn("stale-if-error=300", stale_cache)
 
     def test_status_endpoints(self):
         with create_test_app().test_client() as client:
             os.environ["TALISKER_REVISION_ID"] = "a-build-id"
             response = client.get("_status/check")
             self.assertEqual(response.status_code, 200)
             self.assertEqual(response.data.decode(), "a-build-id")
-            self.assertEqual(
-                response.headers.get("Cache-Control"), "no-store, max-age=0"
-            )
+            self.assertEqual(response.headers.get("Cache-Control"), "no-store")
 
     def test_redirects_deleted(self):
         """