Merge pull request #78 from JWCook/bugfixes

Bugfixes

HISTORY.md

@@ -1,5 +1,10 @@
 # History
 
+## 0.4.3 (2021-07-27)
+* Fix bug in which reponse header `Expires` was used for cache expiration even with `cache_control=False`
+* Fix bug in which HTTP dates parsed from response headers weren't converted to UTC
+* Add handling for invalid timestamps in `CachedResponse.is_expired`
+
 ## 0.4.2 (2021-07-26)
 * Fix handling of `CachedResponse.encoding` when the response body is `None`
 

aiohttp_client_cache/__init__.py

@@ -1,4 +1,4 @@
-__version__ = '0.4.2'
+__version__ = '0.4.3'
 
 # flake8: noqa: F401, F403
 try:

aiohttp_client_cache/cache_control.py

@@ -6,9 +6,9 @@
 from logging import getLogger
 from typing import Any, Dict, Mapping, Optional, Tuple, Union
 
-import attr
 from aiohttp import ClientResponse
 from aiohttp.typedefs import StrOrURL
+from attr import define, field
 from multidict import CIMultiDict
 
 # Value that may be set by either Cache-Control headers or CacheBackend params to disable caching
@@ -33,7 +33,7 @@
 logger = getLogger(__name__)
 
 
-@attr.s(slots=True)
+@define()
 class CacheActions:
     """A dataclass that contains info on specific actions to take for a given cache item.
     This is determined by a combination of CacheBackend settings and request + response headers.
@@ -46,11 +46,12 @@ class CacheActions:
     5. Per-session expiration
     """
 
-    key: str = attr.ib(default=None)
-    expire_after: ExpirationTime = attr.ib(default=None)
-    skip_read: bool = attr.ib(default=False)
-    skip_write: bool = attr.ib(default=False)
-    revalidate: bool = attr.ib(default=False)  # Revalidation is not currently implemented
+    cache_control: bool = field(default=False)
+    expire_after: ExpirationTime = field(default=None)
+    key: str = field(default=None)
+    revalidate: bool = field(default=False)  # Note: Revalidation is not currently implemented
+    skip_read: bool = field(default=False)
+    skip_write: bool = field(default=False)
 
     @classmethod
     def from_request(
@@ -65,14 +66,15 @@ def from_request(
         if cache_control and has_cache_headers(headers):
             return cls.from_headers(key, headers)
         else:
-            return cls.from_settings(key, **kwargs)
+            return cls.from_settings(key, cache_control=cache_control, **kwargs)
 
     @classmethod
     def from_headers(cls, key: str, headers: Mapping):
         """Initialize from request headers"""
         directives = get_cache_directives(headers)
         do_not_cache = directives.get('max-age') == DO_NOT_CACHE
         return cls(
+            cache_control=True,
             key=key,
             expire_after=directives.get('max-age'),
             skip_read=do_not_cache or 'no-store' in directives or 'no-cache' in directives,
@@ -85,6 +87,7 @@ def from_settings(
         cls,
         key: str,
         url: StrOrURL,
+        cache_control: bool = False,
         request_expire_after: ExpirationTime = None,
         session_expire_after: ExpirationTime = None,
         urls_expire_after: ExpirationPatterns = None,
@@ -100,6 +103,7 @@ def from_settings(
 
         do_not_cache = expire_after == DO_NOT_CACHE
         return cls(
+            cache_control=cache_control,
             key=key,
             expire_after=expire_after,
             skip_read=do_not_cache,
@@ -112,9 +116,11 @@ def expires(self) -> Optional[datetime]:
         """Convert the user/header-provided expiration value to a datetime"""
         return get_expiration_datetime(self.expire_after)
 
-    # TODO: If we add any more headers, maybe extract this + from_headers() into a separate function
     def update_from_response(self, response: ClientResponse):
         """Update expiration + actions based on response headers, if not previously set by request"""
+        if not self.cache_control:
+            return
+
         directives = get_cache_directives(response.headers)
         do_not_cache = directives.get('max-age') == DO_NOT_CACHE
         self.expire_after = coalesce(
@@ -132,14 +138,15 @@ def coalesce(*values: Any, default=None) -> Any:
 def get_expiration_datetime(expire_after: ExpirationTime) -> Optional[datetime]:
     """Convert an expiration value in any supported format to an absolute datetime"""
     logger.debug(f'Determining expiration time based on: {expire_after}')
+    if isinstance(expire_after, str):
+        expire_after = parse_http_date(expire_after)
     if expire_after is None or expire_after == -1:
         return None
-    elif isinstance(expire_after, datetime):
+    if isinstance(expire_after, datetime):
         return to_utc(expire_after)
-    elif isinstance(expire_after, str):
-        return parse_http_date(expire_after)
 
     if not isinstance(expire_after, timedelta):
+        assert isinstance(expire_after, (int, float))
         expire_after = timedelta(seconds=expire_after)
     return datetime.utcnow() + expire_after
 

aiohttp_client_cache/response.py

@@ -131,7 +131,11 @@ def host(self) -> str:
     @property
     def is_expired(self) -> bool:
         """Determine if this cached response is expired"""
-        return self.expires is not None and datetime.utcnow() > self.expires
+        # If for any reason the expiration check fails, consider it expired and fetch a new response
+        try:
+            return self.expires is not None and datetime.utcnow() > self.expires
+        except (AttributeError, TypeError, ValueError):
+            return False
 
     @property
     def links(self) -> LinkMultiDict:

pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "aiohttp-client-cache"
-version = "0.4.2"
+version = "0.4.3"
 description = "Persistent cache for aiohttp requests"
 authors = ["Jordan Cook"]
 license = "MIT License"

test/conftest.py

@@ -28,7 +28,7 @@
 ]
 
 HTTPDATE_STR = 'Fri, 16 APR 2021 21:13:00 GMT'
-HTTPDATE_DATETIME = datetime(2021, 4, 16, 21, 13, tzinfo=timezone.utc)
+HTTPDATE_DATETIME = datetime(2021, 4, 16, 21, 13)
 
 
 # Configure logging for pytest session

test/unit/test_cache_control.py

@@ -138,7 +138,7 @@ def test_update_from_response(headers, expected_expiration):
     """Test with Cache-Control response headers"""
     url = 'https://img.site.com/base/img.jpg'
     response = MagicMock(url=url, headers=CIMultiDictProxy(CIMultiDict(headers)))
-    actions = CacheActions(key='key')
+    actions = CacheActions(key='key', cache_control=True)
     actions.update_from_response(response)
 
     if expected_expiration == DO_NOT_CACHE:
@@ -148,6 +148,17 @@ def test_update_from_response(headers, expected_expiration):
         assert actions.skip_write is False
 
 
+def test_update_from_response__disabled():
+    """Response headers should not be used if cache_control=False"""
+    url = 'https://img.site.com/base/img.jpg'
+    headers = [('Cache-Control', 'max-age=60')]
+    response = MagicMock(url=url, headers=CIMultiDictProxy(CIMultiDict(headers)))
+
+    actions = CacheActions(key='key', cache_control=False, expire_after=30)
+    actions.update_from_response(response)
+    assert actions.expire_after == 30
+
+
 @pytest.mark.parametrize('directive', IGNORED_DIRECTIVES)
 def test_ignored_headers(directive):
     """Ensure that currently unimplemented Cache-Control headers do not affect behavior"""

test/unit/test_response.py

@@ -68,6 +68,11 @@ async def test_is_expired(aiohttp_client):
     assert response.is_expired is True
 
 
+async def test_is_expired__invalid(aiohttp_client):
+    response = await get_test_response(aiohttp_client, expires='asdf')
+    assert response.is_expired is False
+
+
 async def test_content_disposition(aiohttp_client):
     response = await get_test_response(aiohttp_client, '/valid_url')
     assert response.content_disposition.type == 'attachment'