Merge pull request #223 from Flameeyes/permanent-redirect

Properly support modern 308 Permanent Redirect responses.

Author: ionrock

cachecontrol/adapter.py

@@ -8,7 +8,7 @@
 
 from requests.adapters import HTTPAdapter
 
-from .controller import CacheController
+from .controller import CacheController, PERMANENT_REDIRECT_STATUSES
 from .cache import DictCache
 from .filewrapper import CallbackFileWrapper
 
@@ -97,7 +97,7 @@ def build_response(
                 response = cached_response
 
             # We always cache the 301 responses
-            elif response.status == 301:
+            elif int(response.status) in PERMANENT_REDIRECT_STATUSES:
                 self.controller.cache_response(request, response)
             else:
                 # Wrap the response file with a wrapper that will cache the

cachecontrol/controller.py

@@ -21,6 +21,8 @@
 
 URI = re.compile(r"^(([^:/?#]+):)?(//([^/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))?")
 
+PERMANENT_REDIRECT_STATUSES = (301, 308)
+
 
 def parse_uri(uri):
     """Parses a URI using the regex given in Appendix B of RFC 3986.
@@ -41,7 +43,7 @@ def __init__(
         self.cache = DictCache() if cache is None else cache
         self.cache_etags = cache_etags
         self.serializer = serializer or Serializer()
-        self.cacheable_status_codes = status_codes or (200, 203, 300, 301)
+        self.cacheable_status_codes = status_codes or (200, 203, 300, 301, 308)
 
     @classmethod
     def _urlnorm(cls, uri):
@@ -151,17 +153,18 @@ def cached_request(self, request):
             logger.warning("Cache entry deserialization failed, entry ignored")
             return False
 
-        # If we have a cached 301, return it immediately. We don't
-        # need to test our response for other headers b/c it is
+        # If we have a cached permanent redirect, return it immediately. We
+        # don't need to test our response for other headers b/c it is
         # intrinsically "cacheable" as it is Permanent.
+        #
         # See:
         #   https://tools.ietf.org/html/rfc7231#section-6.4.2
         #
         # Client can try to refresh the value by repeating the request
         # with cache busting headers as usual (ie no-cache).
-        if resp.status == 301:
+        if int(resp.status) in PERMANENT_REDIRECT_STATUSES:
             msg = (
-                'Returning cached "301 Moved Permanently" response '
+                'Returning cached permanent redirect response '
                 "(ignoring date and etag information)"
             )
             logger.debug(msg)
@@ -310,14 +313,14 @@ def cache_response(self, request, response, body=None, status_codes=None):
         if self.cache_etags and "etag" in response_headers:
             logger.debug("Caching due to etag")
             self.cache.set(
-                cache_url, self.serializer.dumps(request, response, body=body)
+                cache_url, self.serializer.dumps(request, response, body)
             )
 
-        # Add to the cache any 301s. We do this before looking that
-        # the Date headers.
-        elif response.status == 301:
-            logger.debug("Caching permanant redirect")
-            self.cache.set(cache_url, self.serializer.dumps(request, response))
+        # Add to the cache any permanent redirects. We do this before looking
+        # that the Date headers.
+        elif int(response.status) in PERMANENT_REDIRECT_STATUSES:
+            logger.debug("Caching permanent redirect")
+            self.cache.set(cache_url, self.serializer.dumps(request, response, b''))
 
         # Add to the cache if the response headers demand it. If there
         # is no date header then we can't do anything about expiring
@@ -327,7 +330,7 @@ def cache_response(self, request, response, body=None, status_codes=None):
             if "max-age" in cc and cc["max-age"] > 0:
                 logger.debug("Caching b/c date exists and max-age > 0")
                 self.cache.set(
-                    cache_url, self.serializer.dumps(request, response, body=body)
+                    cache_url, self.serializer.dumps(request, response, body)
                 )
 
             # If the request can expire, it means we should cache it
@@ -336,7 +339,7 @@ def cache_response(self, request, response, body=None, status_codes=None):
                 if response_headers["expires"]:
                     logger.debug("Caching b/c of expires header")
                     self.cache.set(
-                        cache_url, self.serializer.dumps(request, response, body=body)
+                        cache_url, self.serializer.dumps(request, response, body)
                     )
 
     def update_cached_response(self, request, response):
@@ -375,6 +378,7 @@ def update_cached_response(self, request, response):
         cached_response.status = 200
 
         # update our cache
-        self.cache.set(cache_url, self.serializer.dumps(request, cached_response))
+        body = cached_response.read(decode_content=False)
+        self.cache.set(cache_url, self.serializer.dumps(request, cached_response, body))
 
         return cached_response

cachecontrol/serialize.py

@@ -21,26 +21,14 @@ def _b64_decode_str(s):
     return _b64_decode_bytes(s).decode("utf8")
 
 
+_default_body_read = object()
+
+
 class Serializer(object):
 
-    def dumps(self, request, response, body=None):
+    def dumps(self, request, response, body):
         response_headers = CaseInsensitiveDict(response.headers)
 
-        if body is None:
-            body = response.read(decode_content=False)
-
-            # NOTE: 99% sure this is dead code. I'm only leaving it
-            #       here b/c I don't have a test yet to prove
-            #       it. Basically, before using
-            #       `cachecontrol.filewrapper.CallbackFileWrapper`,
-            #       this made an effort to reset the file handle. The
-            #       `CallbackFileWrapper` short circuits this code by
-            #       setting the body as the content is consumed, the
-            #       result being a `body` argument is *always* passed
-            #       into cache_response, and in turn,
-            #       `Serializer.dump`.
-            response._fp = io.BytesIO(body)
-
         # NOTE: This is all a bit weird, but it's really important that on
         #       Python 2.x these objects are unicode and not str, even when
         #       they contain only ascii. The problem here is that msgpack

tests/test_cache_control.py

@@ -71,7 +71,7 @@ def test_no_cache_with_wrong_sized_body(self, cc):
         # When the body is the wrong size, then we don't want to cache it
         # because it is obviously broken.
         resp = self.resp({"cache-control": "max-age=3600", "Content-Length": "5"})
-        cc.cache_response(self.req(), resp, body=b"0" * 10)
+        cc.cache_response(self.req(), resp, b"0" * 10)
 
         assert not cc.cache.set.called
 
@@ -86,7 +86,7 @@ def test_cache_response_cache_max_age(self, cc):
         resp = self.resp({"cache-control": "max-age=3600", "date": now})
         req = self.req()
         cc.cache_response(req, resp)
-        cc.serializer.dumps.assert_called_with(req, resp, body=None)
+        cc.serializer.dumps.assert_called_with(req, resp, None)
         cc.cache.set.assert_called_with(self.url, ANY)
 
     def test_cache_response_cache_max_age_with_invalid_value_not_cached(self, cc):
@@ -156,7 +156,7 @@ def req(self, headers):
         return self.c.cached_request(mock_request)
 
     def test_cache_request_no_headers(self):
-        cached_resp = Mock(headers={"ETag": "jfd9094r808", "Content-Length": 100})
+        cached_resp = Mock(headers={"ETag": "jfd9094r808", "Content-Length": 100}, status=200)
         self.c.cache = DictCache({self.url: cached_resp})
         resp = self.req({})
         assert not resp
@@ -183,7 +183,7 @@ def test_cache_request_not_in_cache(self):
 
     def test_cache_request_fresh_max_age(self):
         now = time.strftime(TIME_FMT, time.gmtime())
-        resp = Mock(headers={"cache-control": "max-age=3600", "date": now})
+        resp = Mock(headers={"cache-control": "max-age=3600", "date": now}, status=200)
 
         cache = DictCache({self.url: resp})
         self.c.cache = cache
@@ -193,7 +193,7 @@ def test_cache_request_fresh_max_age(self):
     def test_cache_request_unfresh_max_age(self):
         earlier = time.time() - 3700  # epoch - 1h01m40s
         now = time.strftime(TIME_FMT, time.gmtime(earlier))
-        resp = Mock(headers={"cache-control": "max-age=3600", "date": now})
+        resp = Mock(headers={"cache-control": "max-age=3600", "date": now}, status=200)
         self.c.cache = DictCache({self.url: resp})
         r = self.req({})
         assert not r
@@ -202,7 +202,7 @@ def test_cache_request_fresh_expires(self):
         later = time.time() + 86400  # GMT + 1 day
         expires = time.strftime(TIME_FMT, time.gmtime(later))
         now = time.strftime(TIME_FMT, time.gmtime())
-        resp = Mock(headers={"expires": expires, "date": now})
+        resp = Mock(headers={"expires": expires, "date": now}, status=200)
         cache = DictCache({self.url: resp})
         self.c.cache = cache
         r = self.req({})
@@ -212,7 +212,7 @@ def test_cache_request_unfresh_expires(self):
         sooner = time.time() - 86400  # GMT - 1 day
         expires = time.strftime(TIME_FMT, time.gmtime(sooner))
         now = time.strftime(TIME_FMT, time.gmtime())
-        resp = Mock(headers={"expires": expires, "date": now})
+        resp = Mock(headers={"expires": expires, "date": now}, status=200)
         cache = DictCache({self.url: resp})
         self.c.cache = cache
         r = self.req({})
@@ -221,7 +221,7 @@ def test_cache_request_unfresh_expires(self):
     def test_cached_request_with_bad_max_age_headers_not_returned(self):
         now = time.strftime(TIME_FMT, time.gmtime())
         # Not a valid header; this would be from a misconfigured server
-        resp = Mock(headers={"cache-control": "max-age=xxx", "date": now})
+        resp = Mock(headers={"cache-control": "max-age=xxx", "date": now}, status=200)
 
         self.c.cache = DictCache({self.url: resp})
 

tests/test_serialization.py

@@ -93,7 +93,7 @@ def test_read_latest_version_streamable(self, url):
         original_resp = requests.get(url, stream=True)
         req = original_resp.request
 
-        resp = self.serializer.loads(req, self.serializer.dumps(req, original_resp.raw))
+        resp = self.serializer.loads(req, self.serializer.dumps(req, original_resp.raw, original_resp.content))
 
         assert resp.read()
 
@@ -103,7 +103,7 @@ def test_read_latest_version(self, url):
         req = original_resp.request
 
         resp = self.serializer.loads(
-            req, self.serializer.dumps(req, original_resp.raw, body=data)
+            req, self.serializer.dumps(req, original_resp.raw, data)
         )
 
         assert resp.read() == data
@@ -118,5 +118,5 @@ def test_no_vary_header(self, url):
         original_resp.raw.headers["vary"] = "Foo"
 
         assert self.serializer.loads(
-            req, self.serializer.dumps(req, original_resp.raw, body=data)
+            req, self.serializer.dumps(req, original_resp.raw, data)
         )