Repository: dwavesystems/dwave-cloud-client

Issue:
Refactor `get_regions()` to use cache from `CachingSession`
Drop the custom implementation done for `regions`/`get_regions()`, and use the caching support provided by low-level API's `CachingSession`, as implemented in #645.

Discussion:

Patch:
diff --git a/dwave/cloud/api/client.py b/dwave/cloud/api/client.py
index 366bd31..8160531 100644
--- a/dwave/cloud/api/client.py
+++ b/dwave/cloud/api/client.py
@@ -17,7 +17,9 @@ from __future__ import annotations
 import io
 import hashlib
 import logging
+import numbers
 import os
+import warnings
 from collections import deque, namedtuple
 from typing import Callable, Mapping, Optional, Tuple, TypedDict, Union, TYPE_CHECKING
 
@@ -285,7 +287,7 @@ class CachingSession(VersionedAPISession):
 
     class CacheConfig(TypedDict, total=False):
         enabled: bool
-        maxage: int
+        maxage: float
         store: Union[Mapping, Callable[[], Mapping]]
 
     # default cache config
@@ -315,8 +317,9 @@ class CachingSession(VersionedAPISession):
             logger.debug("[%s] cache disabled.", type(self).__name__)
             return
 
-        if not isinstance(_maxage := config.get('maxage'), int) or _maxage < 0:
-            raise ValueError("A non-negative integer required for 'maxage'.")
+        _maxage = config.get('maxage')
+        if not isinstance(_maxage, numbers.Real) or _maxage < 0:
+            raise ValueError("A non-negative real value required for 'maxage'.")
         self._maxage = _maxage
 
         self._store = config['store']
@@ -399,7 +402,14 @@ class CachingSession(VersionedAPISession):
         # read CachingSession-specific params
         refresh = kwargs.pop('refresh_', False)
         no_cache = kwargs.pop('no_cache_', False)
-        default_maxage = kwargs.pop('maxage_', self._maxage)
+        default_maxage = kwargs.pop('maxage_', None)
+        if default_maxage is None:
+            default_maxage = self._maxage
+        elif not isinstance(default_maxage, numbers.Real) or default_maxage < 0:
+           warnings.warn(
+               f"non-negative real value required for 'maxage_'; ignoring {default_maxage}",
+               UserWarning, stacklevel=2)
+           default_maxage = self._maxage
 
         if not self._cache_enabled or no_cache or method.lower() != 'get':
             # completely bypass cache lookup and validation
@@ -435,9 +445,10 @@ class CachingSession(VersionedAPISession):
                 logger.debug('cache hit within maxage')
                 return make_response(content)
 
-            # conditional request
-            headers = {} if headers is None else headers.copy()
-            headers['If-None-Match'] = meta['etag']
+            # validate with conditional request if possible
+            if etag := meta.get('etag'):
+                headers = {} if headers is None else headers.copy()
+                headers['If-None-Match'] = etag
 
             res = super().request(method, url, params=params, headers=headers, **kwargs)
 
diff --git a/dwave/cloud/api/resources.py b/dwave/cloud/api/resources.py
index a0487d7..921d43f 100644
--- a/dwave/cloud/api/resources.py
+++ b/dwave/cloud/api/resources.py
@@ -106,16 +106,16 @@ class Regions(ResourceBase):
     client_class = MetadataAPIClient
 
     @accepts(media_type='application/vnd.dwave.metadata.regions+json', version='~=1.0')
-    def list_regions(self) -> List[models.Region]:
+    def list_regions(self, **kwargs) -> List[models.Region]:
         path = ''
-        response = self.session.get(path)
+        response = self.session.get(path, **kwargs)
         regions = response.json()
         return TypeAdapter(List[models.Region]).validate_python(regions)
 
     @accepts(media_type='application/vnd.dwave.metadata.region+json', version='~=1.0')
-    def get_region(self, code: str) -> models.Region:
+    def get_region(self, code: str, **kwargs) -> models.Region:
         path = '{}'.format(code)
-        response = self.session.get(path)
+        response = self.session.get(path, **kwargs)
         region = response.json()
         return TypeAdapter(models.Region).validate_python(region)
 
diff --git a/dwave/cloud/client/base.py b/dwave/cloud/client/base.py
index 4aefa9d..7c64164 100644
--- a/dwave/cloud/client/base.py
+++ b/dwave/cloud/client/base.py
@@ -629,9 +629,13 @@ class Client(object):
                             self._poll_workers, self._load_workers):
             worker.join()
 
-        # Close the main thread's session
+        # Close the main thread's sessions
         self.session.close()
 
+        solvers_session = getattr(self, '_solvers_session', None)
+        if solvers_session:
+            solvers_session.close()
+
     def __enter__(self):
         """Let connections be used in with blocks."""
         return self
diff --git a/dwave/cloud/regions.py b/dwave/cloud/regions.py
index 5e49ae4..3aa10ab 100644
--- a/dwave/cloud/regions.py
+++ b/dwave/cloud/regions.py
@@ -14,37 +14,35 @@
 
 import logging
 import re
-from functools import partial
-from typing import Any, Dict, List, Optional, Union
+from typing import Dict, List, Optional, Union
 from urllib.parse import urlsplit
 
-from pydantic import TypeAdapter
-
 from dwave.cloud import api
 from dwave.cloud.api.constants import (
     DEFAULT_REGION, DEFAULT_SOLVER_API_ENDPOINT, DEFAULT_LEAP_API_ENDPOINT,
     DEFAULT_METADATA_API_ENDPOINT)
 from dwave.cloud.config.models import ClientConfig, validate_config_v1
-from dwave.cloud.utils.decorators import cached
 
 __all__ = ['get_regions']
 
 logger = logging.getLogger(__name__)
 
-_REGIONS_CACHE_MAXAGE = 7 * 86400   # 7 days
+_DEFAULT_REGIONS_CACHE_CONFIG = dict(
+    enabled=True,
+    maxage=7 * 86400    # 7 days
+)
 
 
-@cached.ondisk(maxage=_REGIONS_CACHE_MAXAGE, key='cache_key', bucket='regions')
-def _fetch_available_regions(cache_key: Any, config: ClientConfig) -> Dict[str, str]:
+def _fetch_available_regions(config: ClientConfig, **kwargs) -> Dict[str, str]:
     logger.debug("Fetching available regions from the Metadata API at %r",
                  config.metadata_api_endpoint)
 
-    with api.Regions.from_config(config) as regions:
-        regions = regions.list_regions()
-        data = TypeAdapter(Any).dump_python(regions)
+    with api.Regions.from_config(
+            config, cache=_DEFAULT_REGIONS_CACHE_CONFIG) as regions:
+        regions = regions.list_regions(**kwargs)
 
-    logger.debug("Received region metadata: %r", data)
-    return data
+    logger.debug("Received region metadata: %r", regions)
+    return regions
 
 
 def get_regions(config: Optional[Union[ClientConfig, str, dict]] = None,
@@ -66,7 +64,8 @@ def get_regions(config: Optional[Union[ClientConfig, str, dict]] = None,
         refresh:
             Force regions cache refresh.
         maxage:
-            Maximum allowed age, in seconds, of cached regions metadata.
+            Default maximum allowed age, in seconds, of cached regions metadata.
+            Overridden by Metadata API cache control.
         no_cache:
             Do not use cache.
 
@@ -77,6 +76,10 @@ def get_regions(config: Optional[Union[ClientConfig, str, dict]] = None,
     .. versionadded:: 0.11.0
         Added :func:`.get_regions`.
 
+    .. versionchanged:: 0.12.2
+        ``maxage`` parameter is now just a default value for the maximum allowed
+        cache age, and it's typically overridden by regions API's cache control.
+
     """
     if isinstance(config, str):
         config = ClientConfig(metadata_api_endpoint=config)
@@ -87,27 +90,15 @@ def get_regions(config: Optional[Union[ClientConfig, str, dict]] = None,
     elif not isinstance(config, ClientConfig):
         raise TypeError(f"'config' type {type(config).__name__!r} not supported")
 
-    # make sure we don't cache by noise in config
-    cache_key = (config.metadata_api_endpoint, config.cert, config.headers,
-                 config.proxy, config.permissive_ssl,
-                 config.request_retry, config.request_timeout)
-
-    fetch = partial(_fetch_available_regions,
-        cache_key=cache_key, config=config, refresh_=refresh, maxage_=maxage)
-
-    if no_cache:
-        fetch = cached.disabled()(fetch)
-
     try:
-        obj = fetch()
-    except api.exceptions.RequestError as exc:
+        return _fetch_available_regions(
+            config=config, refresh_=refresh, maxage_=maxage, no_cache_=no_cache)
+
+    except api.exceptions.RequestError:
         logger.debug("Metadata API unavailable", exc_info=True)
         raise ValueError(
             f"Metadata API unavailable at {config.metadata_api_endpoint!r}")
 
-    regions = TypeAdapter(List[api.models.Region]).validate_python(obj)
-    return regions
-
 
 def _infer_leap_api_endpoint(solver_api_endpoint: str,
                              region_code: Optional[str] = None) -> str:
diff --git a/releasenotes/notes/get_regions-use-cachingsession-8fd9d84b1bbaa53d.yaml b/releasenotes/notes/get_regions-use-cachingsession-8fd9d84b1bbaa53d.yaml
new file mode 100644
index 0000000..2cf7096
--- /dev/null
+++ b/releasenotes/notes/get_regions-use-cachingsession-8fd9d84b1bbaa53d.yaml
@@ -0,0 +1,6 @@
+---
+features:
+  - |
+    Enable regions metadata API cache control in ``dwave.cloud.regions.get_regions()``
+    by switching to ``api.Regions`` built-in caching session.
+    See `#647 <https://github.com/dwavesystems/dwave-cloud-client/issues/647>`_.


Test patch:
diff --git a/tests/api/test_api_clients.py b/tests/api/test_api_clients.py
index 1f9a0d0..a1df6aa 100644
--- a/tests/api/test_api_clients.py
+++ b/tests/api/test_api_clients.py
@@ -20,6 +20,7 @@ import unittest
 import diskcache
 import requests
 import requests_mock
+from parameterized import parameterized
 
 from dwave.cloud.api import exceptions, constants
 from dwave.cloud.api.client import (
@@ -354,8 +355,28 @@ class TestResponseCaching(unittest.TestCase):
                 self.assertEqual(client.session._maxage, 5)
                 self.assertIsInstance(client.session._store, dict)
 
+    @parameterized.expand([
+        (None, ),
+        (2j, ),
+        (-1.0, ),
+    ])
+    def test_invalid_maxage(self, maxage):
+        with self.assertRaises(ValueError):
+            DWaveAPIClient(
+                endpoint='https://mock', cache=dict(maxage=maxage, store={}))
+
+    @parameterized.expand([
+        (0, ),
+        (1, ),
+        (2.0, ),
+    ])
+    def test_valid_maxage(self, maxage):
+        with DWaveAPIClient(endpoint='https://mock',
+                            cache=dict(maxage=maxage, store={})) as client:
+            self.assertEqual(client.session._maxage, maxage)
+
     @requests_mock.Mocker()
-    def test_caching_no_cache_control(self, m):
+    def test_conditional_requests_with_no_cache_control(self, m):
         endpoint = 'https://mock'
 
         path_a = 'path-a'
@@ -556,3 +577,68 @@ class TestResponseCaching(unittest.TestCase):
 
                 self.assertTrue(m.called)
                 self.assertEqual(client.session.history[-1].response.status_code, 304)
+
+    @requests_mock.Mocker()
+    def test_time_based_validation_only(self, m):
+        # when etag is not available, cache validation is based on time/maxage
+
+        endpoint = 'https://mock'
+        path = 'path'
+        data = {"a": 1}
+
+        m.get(f"{endpoint}/{path}", json=data)
+
+        store = {}
+        maxage = 10
+
+        with DWaveAPIClient(endpoint=endpoint,
+                            cache=dict(store=store, maxage=maxage)) 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"):
+                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)
+
+            with self.subTest("ignored maxage=None"):
+                m.reset_mock()
+                store.clear()
+
+                r = client.session.get(path, maxage_=None)
+                self.assertEqual(r.json(), data)
+
+                self.assertTrue(m.called)
+
+            with self.subTest("warn about invalid maxage"):
+                m.reset_mock()
+                store.clear()
+
+                with self.assertWarns(UserWarning):
+                    r = client.session.get(path, maxage_=-1)
+                self.assertEqual(r.json(), data)
+
+                self.assertTrue(m.called)
diff --git a/tests/test_regions.py b/tests/test_regions.py
index b6af53d..6e084be 100644
--- a/tests/test_regions.py
+++ b/tests/test_regions.py
@@ -13,11 +13,12 @@
 # limitations under the License.
 
 import unittest
-from secrets import token_hex
 from unittest import mock
-import contextlib
+from typing import List
 
+import requests_mock
 from parameterized import parameterized
+from pydantic import TypeAdapter
 
 from dwave.cloud.api.models import Region
 from dwave.cloud.api import constants
@@ -63,50 +64,29 @@ class GetRegionsInit(unittest.TestCase):
 class GetRegionsFunctionality(unittest.TestCase):
 
     def test_config(self):
-        mock_regions = [
-            Region(code='A', name='Region A', endpoint='http://a/'),
-            Region(code='B', name='Region B', endpoint='http://b/'),
-        ]
-        # use a random mock metadata api to avoid cache contamination with mock data
-        # XXX: alternatively, we flush the cache after this test
-        mock_metadata_api_endpoint = f'https://example.com/meta/{token_hex(8)}/'
-
-        # mock `api.Regions.from_config().list_regions()` call
-        @contextlib.contextmanager
-        def mock_regions_from_config(*args, **kwargs):
-            class mock_regions_obj:
-                def list_regions(_):
-                    return mock_regions
-            yield mock_regions_obj()
+        # get_regions() caching params are passed down to CachingSession
 
-        with mock.patch("dwave.cloud.regions.api.Regions.from_config",
-                        side_effect=mock_regions_from_config) as m:
+        with mock.patch("dwave.cloud.regions.api.Regions.from_config") as m:
+            ls = m.return_value.__enter__.return_value.list_regions
 
             with self.subTest('default call'):
-                ret = get_regions(mock_metadata_api_endpoint)
-                m.assert_called()
-                self.assertEqual(ret, mock_regions)
-
-            with self.subTest('caching'):
-                m.reset_mock()
-                ret = get_regions(mock_metadata_api_endpoint)
-                m.assert_not_called()
-                self.assertEqual(ret, mock_regions)
+                get_regions()
+                ls.assert_called()
 
             with self.subTest('cache refresh'):
                 m.reset_mock()
-                ret = get_regions(mock_metadata_api_endpoint, refresh=True)
-                m.assert_called()
+                get_regions(refresh=True)
+                self.assertTrue(ls.call_args.kwargs.get('refresh_'))
 
-            with self.subTest('cache refresh'):
+            with self.subTest('cache maxage'):
                 m.reset_mock()
-                ret = get_regions(mock_metadata_api_endpoint, maxage=0)
-                m.assert_called()
+                get_regions(maxage=0)
+                self.assertEqual(ls.call_args.kwargs.get('maxage_'), 0)
 
             with self.subTest('cache bypass'):
                 m.reset_mock()
-                ret = get_regions(mock_metadata_api_endpoint, no_cache=True)
-                m.assert_called()
+                get_regions(no_cache=True)
+                self.assertTrue(ls.call_args.kwargs.get('no_cache_'))
 
     @mock.patch("dwave.cloud.regions._fetch_available_regions",
                 side_effect=ResourceAccessForbiddenError)
@@ -114,6 +94,23 @@ class GetRegionsFunctionality(unittest.TestCase):
         with self.assertRaises(ValueError):
             get_regions()
 
+    @requests_mock.Mocker()
+    def test_end_to_end_mocked(self, m):
+
+        mock_endpoint = 'https://mock'
+
+        mock_regions = [
+            Region(code='A', name='Region A', endpoint='http://a/'),
+            Region(code='B', name='Region B', endpoint='http://b/'),
+        ]
+        mock_json = TypeAdapter(List[Region]).dump_python(mock_regions)
+
+        m.get(f"{mock_endpoint}/regions/", json=mock_json,
+              headers={'Content-Type': 'application/vnd.dwave.metadata.regions+json; version=1.0'})
+
+        regions = get_regions(mock_endpoint)
+        self.assertEqual(regions, mock_regions)
+
 
 class ResolveEndpointsMocked(unittest.TestCase):