Merge pull request #654 from requests-cache/cleanup

Misc cleanup

10 files changed, 87 insertions, 69 deletions

diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml
index 9091d72..680fba5 100644
--- a/.github/workflows/deploy.yml
+++ b/.github/workflows/deploy.yml
@@ -23,13 +23,13 @@ env:
   STRESS_TEST_MULTIPLIER: 5
 
 jobs:
-  # Run tests for all supported requests versions
+  # Run tests for all supported requests versions and minimum supported python version
   test:
     runs-on: ubuntu-latest
     strategy:
       matrix:
         python-version: [3.7]
-        requests-version: [2.22, 2.23, 2.24, 2.25, 2.26, latest]
+        requests-version: [2.22, 2.23, 2.24, 2.25, 2.26, 2.27, latest]
       fail-fast: false
     services:
       nginx:
diff --git a/HISTORY.md b/HISTORY.md
index 8926679..62249f2 100644
--- a/HISTORY.md
+++ b/HISTORY.md
@@ -30,7 +30,7 @@
 
 **Backends:**
 * SQLite:
-  * Improve performance for removing expired items
+  * Improve performance for removing expired responses with `delete()`
   * Add `size()` method to get estimated size of the database (including in-memory databases)
   * Add `sorted()` method with sorting and other query options
   * Add `wal` parameter to enable write-ahead logging
diff --git a/requests_cache/backends/base.py b/requests_cache/backends/base.py
index e307cb4..5e00db0 100644
--- a/requests_cache/backends/base.py
+++ b/requests_cache/backends/base.py
@@ -329,7 +329,7 @@ class BaseStorage(MutableMapping[KT, VT], ABC):
         # Wrap in a SerializerPipeline, if needed
         if not isinstance(serializer, SerializerPipeline):
             serializer = SerializerPipeline([serializer], name=str(serializer))
-        serializer.decode_content = decode_content
+        serializer.set_decode_content(decode_content)
 
         self.serializer = serializer
         logger.debug(f'Initialized {type(self).__name__} with serializer: {self.serializer}')
diff --git a/requests_cache/backends/sqlite.py b/requests_cache/backends/sqlite.py
index 0fa5d99..17a34c2 100644
--- a/requests_cache/backends/sqlite.py
+++ b/requests_cache/backends/sqlite.py
@@ -17,6 +17,8 @@ from typing import Collection, Iterator, List, Tuple, Type, Union
 
 from platformdirs import user_cache_dir
 
+from requests_cache.models.response import CachedResponse
+
 from .._utils import chunkify, get_valid_kwargs
 from . import BaseCache, BaseStorage
 
@@ -71,21 +73,31 @@ class SQLiteCache(BaseCache):
         expired: bool = False,
         **kwargs,
     ):
-        """More efficient implementation of :py:meth:`BaseCache.delete`"""
+        """A more efficient SQLite implementation of :py:meth:`BaseCache.delete`"""
         if keys:
             self.responses.bulk_delete(keys)
         if expired:
-            self.responses.delete_expired()
+            self._delete_expired()
+
+        # For any remaining conditions, use base implementation
         if kwargs:
             with self.responses._lock, self.redirects._lock:
                 return super().delete(**kwargs)
         else:
             self._prune_redirects()
+
         self.responses.vacuum()
         self.redirects.vacuum()
 
+    def _delete_expired(self):
+        """A more efficient implementation deleting expired responses in SQL"""
+        with self.responses._lock, self.responses.connection(commit=True) as con:
+            con.execute(
+                f'DELETE FROM {self.responses.table_name} WHERE expires <= ?', (round(time()),)
+            )
+
     def _prune_redirects(self):
-        """More efficient implementation of :py:meth:`BaseCache.remove_invalid_redirects`"""
+        """A more efficient implementation of removing invalid redirects in SQL"""
         with self.redirects.connection(commit=True) as conn:
             t1 = self.redirects.table_name
             t2 = self.responses.table_name
@@ -97,12 +109,23 @@ class SQLiteCache(BaseCache):
                 ')'
             )
 
+    def filter(  # type: ignore
+        self, valid: bool = True, expired: bool = True, **kwargs
+    ) -> Iterator[CachedResponse]:
+        """A more efficient implementation of :py:meth:`BaseCache.filter`, in the case where we want
+        to get **only** expired responses
+        """
+        if expired and not valid and not kwargs:
+            return self.responses.sorted(expired=True)
+        else:
+            return super().filter(valid, expired, **kwargs)
+
     def sorted(
         self,
         key: str = 'expires',
         reversed: bool = False,
         limit: int = None,
-        exclude_expired=False,
+        expired: bool = True,
     ):
         """Get cached responses, with sorting and other query options.
 
@@ -110,9 +133,9 @@ class SQLiteCache(BaseCache):
             key: Key to sort by; either 'expires', 'size', or 'key'
             reversed: Sort in descending order
             limit: Maximum number of responses to return
-            exclude_expired: Only return unexpired responses
+            expired: Set to ``False`` to exclude expired responses
         """
-        return self.responses.sorted(key, reversed, limit, exclude_expired)
+        return self.responses.sorted(key, reversed, limit, expired)
 
 
 class SQLiteDict(BaseStorage):
@@ -260,12 +283,6 @@ class SQLiteDict(BaseStorage):
             self.init_db()
             self.vacuum()
 
-    def delete_expired(self):
-        """Delete expired items from the cache"""
-        with self._lock, self.connection(commit=True) as con:
-            con.execute(f"DELETE FROM {self.table_name} WHERE expires <= ?", (round(time()),))
-        self.vacuum()
-
     def size(self) -> int:
         """Return the size of the database, in bytes. For an in-memory database, this will be an
         estimate based on page size.
@@ -283,7 +300,7 @@ class SQLiteDict(BaseStorage):
             return page_count * page_size
 
     def sorted(
-        self, key: str = 'expires', reversed: bool = False, limit: int = None, exclude_expired=False
+        self, key: str = 'expires', reversed: bool = False, limit: int = None, expired: bool = True
     ):
         """Get cache values in sorted order; see :py:meth:`.SQLiteCache.sorted` for usage details"""
         # Get sort key, direction, and limit
@@ -297,7 +314,7 @@ class SQLiteDict(BaseStorage):
         # Filter out expired items, if specified
         filter_expr = ''
         params: Tuple = ()
-        if exclude_expired:
+        if not expired:
             filter_expr = 'WHERE expires is null or expires > ?'
             params = (time(),)
 
diff --git a/requests_cache/models/raw_response.py b/requests_cache/models/raw_response.py
index f94a543..5850b2a 100644
--- a/requests_cache/models/raw_response.py
+++ b/requests_cache/models/raw_response.py
@@ -1,6 +1,6 @@
 from io import BytesIO
 from logging import getLogger
-from typing import Mapping
+from typing import TYPE_CHECKING
 
 from attr import define, field, fields_dict
 from requests import Response
@@ -15,50 +15,49 @@ from . import RichMixin
 logger = getLogger(__name__)
 
 
+if TYPE_CHECKING:
+    from . import CachedResponse
+
+
 @define(auto_attribs=False, repr=False, slots=False)
 class CachedHTTPResponse(RichMixin, HTTPResponse):
-    """A serializable dataclass that emulates :py:class:`~urllib3.response.HTTPResponse`.
-    Supports streaming requests and generator usage.
+    """A wrapper class that emulates :py:class:`~urllib3.response.HTTPResponse`.
 
-    The only action this doesn't support is explicitly calling :py:meth:`.read` with
-    ``decode_content=False``.
+    This enables consistent behavior for streaming requests and generator usage in the following
+    cases:
+    * On an original response, after reading its content to write to the cache
+    * On a cached response
     """
 
     decode_content: bool = field(default=None)
-    # These headers are redundant and not serialized; copied in init and CachedResponse post-init
-    headers: HTTPHeaderDict = None  # type: ignore
+    headers: HTTPHeaderDict = field(factory=HTTPHeaderDict)
     reason: str = field(default=None)
     request_url: str = field(default=None)
     status: int = field(default=0)
     strict: int = field(default=0)
     version: int = field(default=0)
 
-    def __init__(self, *args, body: bytes = None, headers: Mapping = None, **kwargs):
+    def __init__(self, body: bytes = None, **kwargs):
         """First initialize via HTTPResponse, then via attrs"""
         kwargs = {k: v for k, v in kwargs.items() if v is not None}
         super().__init__(body=BytesIO(body or b''), preload_content=False, **kwargs)
-
         self._body = body
-        self.headers = HTTPHeaderDict(headers)
-        self.__attrs_init__(*args, **kwargs)  # type: ignore # False positive in mypy 0.920+?
+        self.__attrs_init__(**kwargs)  # type: ignore # False positive in mypy 0.920+?
 
     @classmethod
-    def from_response(cls, original_response: Response):
+    def from_response(cls, response: Response):
         """Create a CachedHTTPResponse based on an original response"""
         # Copy basic attributes
-        raw = original_response.raw
-        copy_attrs = list(fields_dict(cls).keys()) + ['headers']
-        kwargs = {k: getattr(raw, k, None) for k in copy_attrs}
-
-        # Note: _request_url is not available in urllib <=1.21
-        kwargs['request_url'] = getattr(raw, '_request_url', None)
+        raw = response.raw
+        kwargs = {k: getattr(raw, k, None) for k in fields_dict(cls).keys()}
+        kwargs['request_url'] = raw._request_url
 
         # Copy response data and restore response object to its original state
         if hasattr(raw, '_fp') and not is_fp_closed(raw._fp):
             body = raw.read(decode_content=False)
             kwargs['body'] = body
             raw._fp = BytesIO(body)
-            original_response.content  # This property reads, decodes, and stores response content
+            response.content  # This property reads, decodes, and stores response content
 
             # After reading, reset file pointer on original raw response
             raw._fp = BytesIO(body)
@@ -67,6 +66,18 @@ class CachedHTTPResponse(RichMixin, HTTPResponse):
 
         return cls(**kwargs)  # type: ignore  # False positive in mypy 0.920+?
 
+    @classmethod
+    def from_cached_response(cls, response: 'CachedResponse'):
+        """Create a CachedHTTPResponse based on a cached response"""
+        obj = cls(
+            headers=HTTPHeaderDict(response.headers),
+            reason=response.reason,
+            status=response.status_code,
+            request_url=response.request.url,
+        )
+        obj.reset(response._content)
+        return obj
+
     def release_conn(self):
         """No-op for compatibility"""
 
@@ -74,7 +85,7 @@ class CachedHTTPResponse(RichMixin, HTTPResponse):
         """Simplified reader for cached content that emulates
         :py:meth:`urllib3.response.HTTPResponse.read()`
         """
-        if 'content-encoding' in self.headers and decode_content is False:
+        if 'Content-Encoding' in self.headers and decode_content is False:
             logger.warning('read(decode_content=False) is not supported for cached responses')
 
         data = self._fp.read(amt)
diff --git a/requests_cache/models/response.py b/requests_cache/models/response.py
index 68df763..c1f85e1 100755
--- a/requests_cache/models/response.py
+++ b/requests_cache/models/response.py
@@ -10,7 +10,6 @@ from attr import define, field
 from requests import PreparedRequest, Response
 from requests.cookies import RequestsCookieJar
 from requests.structures import CaseInsensitiveDict
-from urllib3._collections import HTTPHeaderDict
 
 from ..policy.expiration import ExpirationTime, get_expiration_datetime
 from . import CachedHTTPResponse, CachedRequest, RichMixin
@@ -73,18 +72,15 @@ class CachedResponse(RichMixin, BaseResponse):
     expires: Optional[datetime] = field(default=None)
     headers: CaseInsensitiveDict = field(factory=CaseInsensitiveDict)
     history: List['CachedResponse'] = field(factory=list)  # type: ignore
-    raw: CachedHTTPResponse = field(factory=CachedHTTPResponse, repr=False)
+    raw: CachedHTTPResponse = None  # type: ignore  # Not serialized; populated from CachedResponse attrs
     reason: str = field(default=None)
     request: CachedRequest = field(factory=CachedRequest)  # type: ignore
     status_code: int = field(default=0)
     url: str = field(default=None)
 
     def __attrs_post_init__(self):
-        """Re-initialize raw response body after deserialization"""
-        if self.raw._body is None and self._content is not None:
-            self.raw.reset(self._content)
-        if not self.raw.headers:
-            self.raw.headers = HTTPHeaderDict(self.headers)
+        """Re-initialize raw (urllib3) response after deserialization"""
+        self.raw = self.raw or CachedHTTPResponse.from_cached_response(self)
 
     @classmethod
     def from_response(cls, response: Response, **kwargs):
@@ -101,8 +97,8 @@ class CachedResponse(RichMixin, BaseResponse):
             setattr(obj, k, getattr(response, k, None))
 
         # Store request, raw response, and next response (if it's a redirect response)
-        obj.request = CachedRequest.from_request(response.request)
         obj.raw = CachedHTTPResponse.from_response(response)
+        obj.request = CachedRequest.from_request(response.request)
         obj._next = CachedRequest.from_request(response.next) if response.next else None
 
         # Store response body, which will have been read & decoded by requests.Response by now
diff --git a/requests_cache/serializers/cattrs.py b/requests_cache/serializers/cattrs.py
index befc4b5..d5c3dda 100644
--- a/requests_cache/serializers/cattrs.py
+++ b/requests_cache/serializers/cattrs.py
@@ -13,13 +13,13 @@ serialization formats.
 """
 from datetime import datetime, timedelta
 from decimal import Decimal
+from json import JSONDecodeError
 from typing import Callable, Dict, ForwardRef, MutableMapping
 
 from cattr import GenConverter
 from requests.cookies import RequestsCookieJar, cookiejar_from_dict
-from requests.exceptions import JSONDecodeError
+from requests.exceptions import RequestException
 from requests.structures import CaseInsensitiveDict
-from urllib3._collections import HTTPHeaderDict
 
 from ..models import CachedResponse, DecodedContent
 from .pipeline import Stage
@@ -102,9 +102,6 @@ def init_converter(
     converter.register_structure_hook(
         CaseInsensitiveDict, lambda obj, cls: CaseInsensitiveDict(obj)
     )
-    converter.register_unstructure_hook(HTTPHeaderDict, dict)
-    converter.register_structure_hook(HTTPHeaderDict, lambda obj, cls: HTTPHeaderDict(obj))
-
     # Convert decoded JSON body back to string
     converter.register_structure_hook(
         DecodedContent, lambda obj, cls: json.dumps(obj) if isinstance(obj, dict) else obj
@@ -140,7 +137,7 @@ def _decode_content(response: CachedResponse, response_dict: Dict) -> Dict:
         try:
             response_dict['_decoded_content'] = response.json()
             response_dict.pop('_content', None)
-        except JSONDecodeError:
+        except (JSONDecodeError, RequestException):
             pass
 
     # Decode body as text
diff --git a/requests_cache/serializers/pipeline.py b/requests_cache/serializers/pipeline.py
index 08e1cac..652c8f5 100644
--- a/requests_cache/serializers/pipeline.py
+++ b/requests_cache/serializers/pipeline.py
@@ -60,20 +60,11 @@ class SerializerPipeline:
             value = step(value)
         return value
 
-    # TODO: I don't love this. Could BaseStorage init be refactored to not need this getter/setter?
-    @property
-    def decode_content(self) -> bool:
-        for stage in self.stages:
-            if hasattr(stage, 'decode_content'):
-                return stage.decode_content
-        return False
-
-    @decode_content.setter
-    def decode_content(self, value: bool):
+    def set_decode_content(self, decode_content: bool):
         """Set decode_content, if the pipeline contains a CattrStage or compatible object"""
         for stage in self.stages:
             if hasattr(stage, 'decode_content'):
-                stage.decode_content = value
+                stage.decode_content = decode_content
 
     def __str__(self) -> str:
         return f'SerializerPipeline(name={self.name}, n_stages={len(self.dump_stages)})'
diff --git a/tests/integration/test_filesystem.py b/tests/integration/test_filesystem.py
index 54c782d..dfe9414 100644
--- a/tests/integration/test_filesystem.py
+++ b/tests/integration/test_filesystem.py
@@ -82,15 +82,16 @@ class TestFileCache(BaseCacheTest):
         """Test all relevant combinations of response formats X serializers"""
         if not _valid_serializer(serializer):
             pytest.skip(f'Dependencies not installed for {serializer}')
+        serializer.set_decode_content(False)
         super().test_all_response_formats(response_format, serializer)
 
     @pytest.mark.parametrize('serializer', [json_serializer, yaml_serializer])
     @pytest.mark.parametrize('response_format', HTTPBIN_FORMATS)
     def test_all_response_formats__no_decode_content(self, response_format, serializer):
-        """Test with decode_content=False for text-based serialization formats"""
+        """Test with decode_content=True for text-based serialization formats"""
         if not _valid_serializer(serializer):
             pytest.skip(f'Dependencies not installed for {serializer}')
-        serializer.decode_content = False
+        serializer.set_decode_content(True)
         self.test_all_response_formats(response_format, serializer)
 
     @pytest.mark.parametrize('serializer_name', SERIALIZERS.keys())
diff --git a/tests/integration/test_sqlite.py b/tests/integration/test_sqlite.py
index 54b0ceb..580948b 100644
--- a/tests/integration/test_sqlite.py
+++ b/tests/integration/test_sqlite.py
@@ -211,7 +211,7 @@ class TestSQLiteDict(BaseStorageTest):
             cache[f'key_{i}'] = response
 
         # Items should only include unexpired (even numbered) items, and still be in sorted order
-        items = list(cache.sorted(key='expires', exclude_expired=True))
+        items = list(cache.sorted(key='expires', expired=False))
         assert len(items) == 50
         prev_item = None
 
@@ -272,6 +272,13 @@ class TestSQLiteCache(BaseCacheTest):
         session = self.init_session()
         assert session.cache.db_path == session.cache.responses.db_path
 
+    @patch.object(SQLiteDict, 'sorted')
+    def test_filter__expired_only(self, mock_sorted):
+        """Filtering by expired only should use a more efficient SQL query"""
+        session = self.init_session()
+        session.cache.filter(valid=False, expired=True)
+        mock_sorted.assert_called_once_with(expired=True)
+
     def test_sorted(self):
         """Test wrapper method for SQLiteDict.sorted(), with all arguments combined"""
         session = self.init_session(clear=False)
@@ -287,9 +294,7 @@ class TestSQLiteCache(BaseCacheTest):
             session.cache.responses[f'key_{i}'] = response
 
         # Sorted items should be in ascending order by expiration time
-        items = list(
-            session.cache.sorted(key='expires', exclude_expired=True, reversed=True, limit=100)
-        )
+        items = list(session.cache.sorted(key='expires', expired=False, reversed=True, limit=100))
         assert len(items) == 100
 
         prev_item = None