Repository: dwavesystems/dwave-cloud-client

Issue:
Make `CachingSession` respect SAPI's `Cache-Control`/`max-age` (when released)
In #645 we use a hard-coded default value for `max-age` (15 min), and validate cache via conditional requests after that.

That's because SAPI currently doesn't implement cache control, i.e. it returns none of the `Cache-Control`, `Expires` or `Last-Modified` header fields.

Implement support for `Cache-Control` response field, as that's the most modern and most robust cache control mechanism.

Discussion:

Patch:
diff --git a/dwave/cloud/api/client.py b/dwave/cloud/api/client.py
index 3b19f3b..366bd31 100644
--- a/dwave/cloud/api/client.py
+++ b/dwave/cloud/api/client.py
@@ -19,12 +19,12 @@ import hashlib
 import logging
 import os
 from collections import deque, namedtuple
-from typing import Callable, Mapping, Optional, TypedDict, Union, TYPE_CHECKING
+from typing import Callable, Mapping, Optional, Tuple, TypedDict, Union, TYPE_CHECKING
 
 import requests
 import urllib3
+import werkzeug
 from packaging.specifiers import SpecifierSet
-from werkzeug.http import parse_options_header, dump_options_header
 
 import dwave.cloud.config   # don't use `from` to break circular import (config <> api.constants)
 from dwave.cloud.api import constants, exceptions
@@ -189,7 +189,7 @@ class VersionedAPISession(LoggingSession):
             else:
                 return
 
-        media_type, params = parse_options_header(content_type)
+        media_type, params = werkzeug.http.parse_options_header(content_type)
 
         if media_type != self._media_type:
             raise exceptions.ResourceBadResponseError(
@@ -220,7 +220,7 @@ class VersionedAPISession(LoggingSession):
                 params['version'] = self._ask_version
 
             headers = {} if headers is None else headers.copy()
-            headers['Accept'] = dump_options_header(self._media_type, params)
+            headers['Accept'] = werkzeug.http.dump_options_header(self._media_type, params)
 
         response = super().request(*args, headers=headers, **kwargs)
 
@@ -285,7 +285,7 @@ class CachingSession(VersionedAPISession):
 
     class CacheConfig(TypedDict, total=False):
         enabled: bool
-        maxage: float
+        maxage: int
         store: Union[Mapping, Callable[[], Mapping]]
 
     # default cache config
@@ -315,7 +315,10 @@ class CachingSession(VersionedAPISession):
             logger.debug("[%s] cache disabled.", type(self).__name__)
             return
 
-        self._maxage = config['maxage']
+        if not isinstance(_maxage := config.get('maxage'), int) or _maxage < 0:
+            raise ValueError("A non-negative integer required for 'maxage'.")
+        self._maxage = _maxage
+
         self._store = config['store']
         if callable(self._store):
             self._store = self._store()
@@ -326,6 +329,67 @@ class CachingSession(VersionedAPISession):
         if hasattr(self._store, 'directory'):
             logger.debug("cache.store.directory=%r", self._store.directory)
 
+    def _parse_cache_control(
+            self,
+            cache_control: Optional[str] = None,
+            ) -> Tuple[bool, Optional[int]]:
+        # parse Cache-Control header field (if present) and derive a suitable max-age value
+        # returns: (cache?, maxage)
+
+        # note:
+        # - we always revalidate after expiry, so we can ignore `must-revalidate`
+        # - and we never transform the response, so safe to ignore `no-transform`
+        # - our cache is private (for a single user), so we can ignore `private`,
+        #   `public` and `s-maxage` directives
+
+        cc = werkzeug.http.parse_cache_control_header(
+                cache_control, cls=werkzeug.datastructures.ResponseCacheControl)
+
+        if cc.no_cache:
+            # cache, but always validate
+            return (True, 0)
+
+        if cc.no_store:
+            # skip cache
+            return (False, 0)
+
+        # cache for up to max_age
+        return (True, cc.max_age)
+
+    def _update_cache(
+            self,
+            response: requests.Response,
+            key_meta: str,
+            key_data: Optional[str] = None,
+            only_meta: bool = False
+            ) -> bool:
+        # update cache from response according to cache-control
+        # returns: updated?
+
+        # etag and cache-control are present in 304 if they're in 200 response
+        # see: https://datatracker.ietf.org/doc/html/rfc9110#name-304-not-modified
+        etag = response.headers.get('ETag')
+        cache_control = response.headers.get('Cache-Control')
+
+        use_cache, maxage = self._parse_cache_control(cache_control)
+        if use_cache:
+            meta = dict(created=epochnow(), maxage=maxage, etag=etag)
+            self._store[key_meta] = meta
+            if not only_meta:
+                self._store[key_data] = response.content
+
+            logger.debug("response cached for maxage=%r", maxage)
+            return True
+
+        else:
+            if key_meta in self._store:
+                del self._store[key_meta]
+            if key_data in self._store:
+                del self._store[key_data]
+
+            logger.debug("response caching skipped at server's request")
+            return False
+
     def request(self, method, url, *,
                 params: Optional[dict] = None,
                 headers: Optional[dict] = None,
@@ -335,10 +399,10 @@ class CachingSession(VersionedAPISession):
         # read CachingSession-specific params
         refresh = kwargs.pop('refresh_', False)
         no_cache = kwargs.pop('no_cache_', False)
-        suggested_maxage = kwargs.pop('maxage_', self._maxage)
+        default_maxage = kwargs.pop('maxage_', self._maxage)
 
         if not self._cache_enabled or no_cache or method.lower() != 'get':
-            # completely bypass cache lookup and conditional request
+            # completely bypass cache lookup and validation
             return super().request(method, url, params=params, headers=headers, **kwargs)
 
         key = (method,
@@ -357,8 +421,14 @@ class CachingSession(VersionedAPISession):
             return res
 
         if not refresh and (meta := self._store.get(key_meta)):
-            maxage = meta.get('maxage', suggested_maxage) or 0
-
+            # respect max-age from response cache-control
+            maxage = meta.get('maxage')
+            if maxage is None:
+                # but if cache-control was absent, client gets do decide
+                maxage = default_maxage
+
+            # note: by deferring this lookup, we could save a few ms in case
+            # when upstream content was modified (but that doesn't happen often)
             content = self._store.get(key_data)
 
             if epochnow() - meta['created'] < maxage:
@@ -372,26 +442,21 @@ class CachingSession(VersionedAPISession):
             res = super().request(method, url, params=params, headers=headers, **kwargs)
 
             if res.status_code == requests.codes['not_modified']:
+                # see 4.3.4. in rfc9111 (http caching)
+                # we know SAPI only uses weak validators
                 logger.debug('resource "not modified", using cached value')
-                meta['created'] = epochnow()
-                self._store[key_meta] = meta
+                self._update_cache(res, key_meta=key_meta, only_meta=True)
                 return make_response(content)
 
             else:
                 logger.debug('resource modified, updating cache')
-                meta = dict(created=epochnow(),
-                            etag=res.headers.get('etag'))
-                self._store[key_meta] = meta
-                self._store[key_data] = res.content
+                self._update_cache(res, key_meta=key_meta, key_data=key_data)
                 return res
 
         else:
             logger.debug('cache miss, updating cache')
             res = super().request(method, url, params=params, headers=headers, **kwargs)
-            meta = dict(created=epochnow(),
-                        etag=res.headers.get('etag'))
-            self._store[key_meta] = meta
-            self._store[key_data] = res.content
+            self._update_cache(res, key_meta=key_meta, key_data=key_data)
             return res
 
 
diff --git a/dwave/cloud/client/base.py b/dwave/cloud/client/base.py
index 4bc4ef6..47c9871 100644
--- a/dwave/cloud/client/base.py
+++ b/dwave/cloud/client/base.py
@@ -343,7 +343,7 @@ class Client(object):
     # Downloaded solver definition cache config
     _DEFAULT_SOLVERS_CACHE_CONFIG = dict(
         enabled=True,
-        maxage=900,     # 15 min heuristic maxage (TODO: API cache-control to override it)
+        maxage=3600,    # 1 hour heuristic maxage (cache-control in response overrides it)
     )
 
     # Downloaded region metadata cache maxage [sec]
diff --git a/releasenotes/notes/respect-cache-control-in-api-response-27dbd1a7f300648d.yaml b/releasenotes/notes/respect-cache-control-in-api-response-27dbd1a7f300648d.yaml
new file mode 100644
index 0000000..50fbb35
--- /dev/null
+++ b/releasenotes/notes/respect-cache-control-in-api-response-27dbd1a7f300648d.yaml
@@ -0,0 +1,9 @@
+---
+features:
+  - |
+    Parse ``Cache-Control`` header field if present in API response, and use it
+    to guide local caching policy.
+
+    This means ``dwave.cloud.api.DWaveAPIClient`` and ``dwave.cloud.api.resource``
+    classes now respect origin server response caching policy.
+    See `#646 <https://github.com/dwavesystems/dwave-cloud-client/issues/646>`_.


Test patch:
diff --git a/tests/api/test_api_clients.py b/tests/api/test_api_clients.py
index ed670ad..1f9a0d0 100644
--- a/tests/api/test_api_clients.py
+++ b/tests/api/test_api_clients.py
@@ -13,6 +13,7 @@
 # limitations under the License.
 
 import json
+import time
 import uuid
 import unittest
 
@@ -354,7 +355,7 @@ class TestResponseCaching(unittest.TestCase):
                 self.assertIsInstance(client.session._store, dict)
 
     @requests_mock.Mocker()
-    def test_caching(self, m):
+    def test_caching_no_cache_control(self, m):
         endpoint = 'https://mock'
 
         path_a = 'path-a'
@@ -369,11 +370,15 @@ class TestResponseCaching(unittest.TestCase):
 
         # mock: resource not modified
         m.get(f"{endpoint}/{path_a}", json=data_a, headers={'ETag': etag_a})
-        m.get(f"{endpoint}/{path_a}", request_headers={'If-None-Match': etag_a}, status_code=304)
+        m.get(f"{endpoint}/{path_a}",
+              request_headers={'If-None-Match': etag_a}, status_code=304,
+              headers={'ETag': etag_a})
 
         # mock: resource modified between requests
         m.get(f"{endpoint}/{path_b}", json=data_b, headers={'ETag': etag_b})
-        m.get(f"{endpoint}/{path_b}", request_headers={'If-None-Match': etag_b}, json=data_bb, headers={'ETag': etag_bb})
+        m.get(f"{endpoint}/{path_b}",
+              request_headers={'If-None-Match': etag_b}, json=data_bb,
+              headers={'ETag': etag_bb})
 
         store = {}
 
@@ -433,3 +438,121 @@ class TestResponseCaching(unittest.TestCase):
                 self.assertEqual(r.json(), data_a)
 
                 self.assertTrue(m.called)
+
+    @requests_mock.Mocker()
+    def test_cache_control_maxage(self, m):
+        # verify max-age from cache-control header is respected
+
+        endpoint = 'https://mock'
+
+        path = 'path'
+        etag = 'etag'
+        data = {"a": 1}
+
+        maxage = 10
+
+        # mock: resource not modified
+        m.get(f"{endpoint}/{path}", json=data,
+              headers={'ETag': etag, 'Cache-Control': f'public, max-age={maxage}'})
+        m.get(f"{endpoint}/{path}",
+              request_headers={'If-None-Match': etag}, status_code=304,
+              headers={'ETag': etag, 'Cache-Control': f'public, max-age={maxage}'})
+
+        store = {}
+
+        with DWaveAPIClient(endpoint=endpoint, cache=dict(store=store), history_size=1) as client:
+
+            with self.subTest("cache miss"):
+                self.assertEqual(len(store), 0)
+
+                r = client.session.get(path)
+                self.assertEqual(r.json(), data)
+
+                self.assertTrue(m.called)
+                self.assertEqual(len(store), 2)     # data + meta
+
+            with self.subTest("cache hit"):
+                m.reset_mock()
+
+                with unittest.mock.patch('dwave.cloud.api.client.epochnow',
+                                         lambda: time.time() + maxage - 1):
+                    r = client.session.get(path)
+                self.assertEqual(r.json(), data)
+
+                # if mock not called, data came from cache
+                self.assertFalse(m.called)
+
+            with self.subTest("cache expired, validated"):
+                m.reset_mock()
+
+                with unittest.mock.patch('dwave.cloud.api.client.epochnow',
+                                         lambda: time.time() + maxage + 1):
+                    r = client.session.get(path)
+                self.assertEqual(r.json(), data)
+
+                self.assertTrue(m.called)
+                self.assertEqual(client.session.history[-1].response.status_code, 304)
+
+    @requests_mock.Mocker()
+    def test_cache_control_no_store(self, m):
+        # verify we don't cache when server forbids caching
+
+        endpoint = 'https://mock'
+
+        path = 'path'
+        etag = 'etag'
+        data = {"a": 1}
+
+        m.get(f"{endpoint}/{path}", json=data,
+              headers={'ETag': etag, 'Cache-Control': 'no-store'})
+
+        store = {}
+
+        with DWaveAPIClient(endpoint=endpoint, cache=dict(store=store)) as client:
+
+            with self.subTest("cache miss"):
+                self.assertEqual(len(store), 0)
+
+                r = client.session.get(path)
+                self.assertEqual(r.json(), data)
+
+                self.assertTrue(m.called)
+                self.assertEqual(len(store), 0)
+
+    @requests_mock.Mocker()
+    def test_cache_control_no_cache(self, m):
+        # verify we cache but always validate when cache-control is no-cache
+
+        endpoint = 'https://mock'
+
+        path = 'path'
+        etag = 'etag'
+        data = {"a": 1}
+
+        m.get(f"{endpoint}/{path}", json=data,
+              headers={'ETag': etag, 'Cache-Control': f'no-cache, max-age=100'})
+        m.get(f"{endpoint}/{path}",
+              request_headers={'If-None-Match': etag}, status_code=304,
+              headers={'ETag': etag, 'Cache-Control': f'no-cache, max-age=100'})
+
+        store = {}
+
+        with DWaveAPIClient(endpoint=endpoint, cache=dict(store=store), history_size=1) as client:
+
+            with self.subTest("cache miss"):
+                self.assertEqual(len(store), 0)
+
+                r = client.session.get(path)
+                self.assertEqual(r.json(), data)
+
+                self.assertTrue(m.called)
+                self.assertEqual(len(store), 2)
+
+            with self.subTest("cache validated"):
+                m.reset_mock()
+
+                r = client.session.get(path)
+                self.assertEqual(r.json(), data)
+
+                self.assertTrue(m.called)
+                self.assertEqual(client.session.history[-1].response.status_code, 304)