Improve exception handling for the cached decorator. Fixes #444

Author: sh4nks

CHANGES.rst

@@ -48,6 +48,9 @@ Unreleased
   cache hit with the cached value. Unlike ``forced_update`` it uses the value itself
   to check whether the cache is stale. Can be used in combination with ``forced_update``
   :issue:`392`
+- ``@cached`` now caches an ``HTTPException`` raised by the view (i.e. through ``abort()``).
+  This exception will now be re-raises on a cache hit. Use ``response_filter``,
+  to keep the exception out of the cache. :issue:`444`
 - Fix a ``@memoize`` cache-key collision when a parameter has a falsy
   default (e.g. ``0``, ``""``, ``False``): calling with the default was
   keyed the same as passing ``None``, returning the wrong cached result.

docs/usage.rst

@@ -460,6 +460,37 @@ of the cache::
     def index():
         return render_template('index.html')
 
+.. versionchanged:: 2.5.0
+
+    An ``HTTPException`` raised by a view for example through
+    :func:`flask.abort` is cached like a returned response and re-raised on a
+    cache hit, so :meth:`~flask.Flask.errorhandler` functions still run. The
+    filter is called with the exception's response.
+
+.. warning::
+
+    A view that aborts because of a server problem (i.e. status code ``503``)
+    keeps returning that error until the entry expires. Use ``response_filter`` to
+    keep such a status code out of the cache.
+
+In case a view returns just a plain string (has no ``status_code``), use a default
+value for the response. For example::
+
+    def not_server_error(response):
+        return getattr(response, "status_code", 200) < 500
+
+    @app.route("/article/<slug>")
+    @cache.cached(timeout=50, response_filter=not_server_error)
+    def article(slug):
+        if not backend.healthy():
+            abort(503)  # raised again on every request
+
+        article = load(slug)
+        if article is None:
+            abort(404)  # cached and re-raised for 50 seconds
+
+        return render_template("article.html", article=article)
+
 
 cache_none
 ``````````

src/flask_caching/__init__.py

@@ -34,6 +34,7 @@
 from flask import request
 from flask import Response
 from flask import url_for
+from werkzeug.exceptions import HTTPException
 from werkzeug.utils import import_string
 
 from .backends.base import BaseCache
@@ -369,6 +370,15 @@ def _call_fn(self, fn: Callable[..., Any], *args: Any, **kwargs: Any) -> Any:
             return ensure_sync(fn)(*args, **kwargs)
         return fn(*args, **kwargs)
 
+    def _call_fn_or_exception(
+        self, fn: Callable[..., Any], *args: Any, **kwargs: Any
+    ) -> Any:
+        """Returns an ``HTTPException`` instead of propagating it."""
+        try:
+            return self._call_fn(fn, *args, **kwargs)
+        except HTTPException as e:
+            return e
+
     @property
     def cache(self) -> SimpleCache:
         """The backend instance the proxy methods delegate to. Use this to
@@ -572,6 +582,13 @@ def get_list():
         :param response_hit_indication: Default False.
                              If True, it will add to response header field 'hit_cache'
                              if used cache.
+
+        .. versionchanged:: 2.5.0
+            A ``werkzeug.exceptions.HTTPException`` raised by the decorated
+            function, for example through Flask's ``abort()``, is now cached
+            like a returned response and re-raised on a cache hit. Use
+            ``response_filter``, which is given the exception's response, to
+            keep it out of the cache.
         """
 
         def decorator(f: Callable[P, R]) -> _CachedFunction[P, R]:
@@ -642,12 +659,17 @@ def apply_caching(response: Response) -> Response:
                         cache=self, cache_key=cache_key, args=args, kwargs=kwargs
                     )
 
+                if found and isinstance(rv, HTTPException):
+                    raise rv
+
                 if not found:
-                    rv = self._call_fn(f, *args, **kwargs)
+                    rv = self._call_fn_or_exception(f, *args, **kwargs)
                     if inspect.isgenerator(rv):
                         rv = join_generator(rv)
 
-                    if response_filter is None or response_filter(rv):
+                    if response_filter is None or response_filter(
+                        rv.get_response() if isinstance(rv, HTTPException) else rv
+                    ):
                         cache_timeout = normalize_timeout(cached_fn.cache_timeout)
                         if isinstance(rv, CachedResponse):
                             cache_timeout = rv.timeout or cache_timeout
@@ -662,6 +684,9 @@ def apply_caching(response: Response) -> Response:
                             if self.app.debug:
                                 raise
                             logger.exception("Exception possibly due to cache backend.")
+
+                    if isinstance(rv, HTTPException):
+                        raise rv
                 return rv
 
             def default_make_cache_key(*args: Any, **kwargs: Any) -> str:

tests/test_view.py

@@ -2,6 +2,7 @@
 import itertools
 import time
 
+from flask import abort
 from flask import make_response
 from flask import request
 from flask.views import View
@@ -767,3 +768,88 @@ def view_user(name):
             == in_request
         )
         assert view_user.make_cache_key(name="bob") == in_request_user
+
+
+def test_cached_view_http_exception(app, cache):
+    calls = []
+
+    @app.route("/missing")
+    @cache.cached(2)
+    def cached_view():
+        calls.append(1)
+        abort(404, "no such thing")
+
+    tc = app.test_client()
+
+    first = tc.get("/missing")
+    second = tc.get("/missing")
+
+    assert first.status_code == second.status_code == 404
+    assert len(calls) == 1
+
+
+def test_cached_view_http_exception_runs_error_handler(app, cache):
+    calls = []
+
+    @app.errorhandler(404)
+    def handle_404(error):
+        return f"handled {error.description}", 404
+
+    @app.route("/missing")
+    @cache.cached(2)
+    def cached_view():
+        calls.append(1)
+        abort(404, "no such thing")
+
+    tc = app.test_client()
+
+    tc.get("/missing")
+    cached = tc.get("/missing")
+
+    assert cached.get_data(as_text=True) == "handled no such thing"
+    assert len(calls) == 1
+
+
+def test_cached_view_http_exception_expires(app, cache, clock):
+    calls = []
+
+    @app.route("/missing")
+    @cache.cached(2)
+    def cached_view():
+        calls.append(1)
+        abort(404)
+
+    tc = app.test_client()
+
+    tc.get("/missing")
+    clock.advance(1)
+    tc.get("/missing")
+
+    assert len(calls) == 1
+
+    clock.advance(2)
+    assert tc.get("/missing").status_code == 404
+    assert len(calls) == 2
+
+
+def test_cached_view_http_exception_response_filter_gets_response(app, cache):
+    calls = []
+    seen = []
+
+    def only_success(response):
+        seen.append(response)
+        return response.status_code == 200
+
+    @app.route("/down")
+    @cache.cached(2, response_filter=only_success)
+    def cached_view():
+        calls.append(1)
+        abort(503)
+
+    tc = app.test_client()
+
+    assert tc.get("/down").status_code == 503
+    assert tc.get("/down").status_code == 503
+
+    assert [response.status_code for response in seen] == [503, 503]
+    assert len(calls) == 2