Query updates (#84)

* work in progress

* added retries to query

* removed test with stale token

* added stale token test back in

* added stale token test back in

* moved retries to _session module, added tests

* added retry status_forcelist tests

* added custom adapter test

* changed default retry behavior

* added another test

* more testing

bookops_worldcat/_session.py

@@ -5,9 +5,10 @@
 and others.
 """
 
-from typing import Optional, Tuple, Union
+from typing import Optional, Tuple, Union, List
 
 import requests
+from urllib3.util import Retry
 
 from . import __title__, __version__
 from .authorize import WorldcatAccessToken
@@ -24,6 +25,10 @@ def __init__(
             5,
             5,
         ),
+        total_retries: int = 0,
+        backoff_factor: float = 0,
+        status_forcelist: Optional[List[int]] = [],
+        allowed_methods: Optional[List[str]] = None,
     ) -> None:
         """
         Args:
@@ -32,9 +37,26 @@ def __init__(
                                     request in the session
             timeout:                how long to wait for server to send data
                                     before giving up
+            total_retries:          optional number of times to retry a request that
+                                    failed or timed out. if total_retries argument is
+                                    not passed, any arguments passed to
+                                    backoff_factor, status_forcelist, and
+                                    allowed_methods will be ignored. default is 0
+            backoff_factor:         if total_retries is not 0, the backoff
+                                    factor as a float to use to calculate amount of
+                                    time session will sleep before attempting request
+                                    again. default is 0
+            status_forcelist:       if total_retries is not 0, a list of HTTP
+                                    status codes to automatically retry requests on.
+                                    if not specified, all failed requests will be
+                                    retried up to number of total_retries.
+                                    example: [500, 502, 503, 504]
+            allowed_methods:        if total_retries is not 0, set of HTTP methods that
+                                    requests should be retried on. if not specified,
+                                    requests using any HTTP method verbs will be
+                                    retried. example: ["GET", "POST"]
         """
         super().__init__()
-
         self.authorization = authorization
 
         if not isinstance(self.authorization, WorldcatAccessToken):
@@ -51,6 +73,16 @@ def __init__(
 
         self.timeout = timeout
 
+        # if user provides retry args, create Retry object and mount adapter to session
+        if total_retries != 0:
+            retries = Retry(
+                total=total_retries,
+                backoff_factor=backoff_factor,
+                status_forcelist=status_forcelist,
+                allowed_methods=allowed_methods,
+            )
+            self.mount("https://", requests.adapters.HTTPAdapter(max_retries=retries))
+
         self._update_authorization()
 
     def _get_new_access_token(self) -> None:

bookops_worldcat/metadata_api.py

@@ -24,6 +24,10 @@ def __init__(
         authorization: WorldcatAccessToken,
         agent: Optional[str] = None,
         timeout: Union[int, float, Tuple[int, int], Tuple[float, float], None] = None,
+        total_retries: int = 0,
+        backoff_factor: float = 0,
+        status_forcelist: Optional[List[int]] = [],
+        allowed_methods: Optional[List[str]] = None,
     ) -> None:
         """
         Args:
@@ -32,8 +36,34 @@ def __init__(
                                     header; usage strongly encouraged
             timeout:                how long to wait for server to send data before
                                     giving up; default value is 5 seconds
-        """
-        super().__init__(authorization, agent=agent, timeout=timeout)
+            total_retries:          optional number of times to retry a request that
+                                    failed or timed out. if total_retries argument is
+                                    not passed, any arguments passed to
+                                    backoff_factor, status_forcelist, and
+                                    allowed_methods will be ignored. default is 0
+            backoff_factor:         if total_retries is not 0, the backoff
+                                    factor as a float to use to calculate amount of
+                                    time session will sleep before attempting request
+                                    again. default is 0
+            status_forcelist:       if total_retries is not 0, a list of HTTP
+                                    status codes to automatically retry requests on.
+                                    if not specified, all failed requests will be
+                                    retried up to number of total_retries.
+                                    example: [500, 502, 503, 504]
+            allowed_methods:        if total_retries is not 0, set of HTTP methods that
+                                    requests should be retried on. if not specified,
+                                    requests using any HTTP method verbs will be
+                                    retried. example: ["GET", "POST"]
+        """
+        super().__init__(
+            authorization,
+            agent=agent,
+            timeout=timeout,
+            total_retries=total_retries,
+            backoff_factor=backoff_factor,
+            status_forcelist=status_forcelist,
+            allowed_methods=allowed_methods,
+        )
 
     def _split_into_legal_volume(
         self, oclc_numbers: List[str] = [], n: int = 50
@@ -207,7 +237,7 @@ def get_institution_holdings(
         self,
         oclcNumbers: Union[str, List[Union[str, int]]],
         hooks: Optional[Dict[str, Callable]] = None,
-    ) -> List[Optional[Response]]:
+    ) -> List[Response]:
         """
         Retrieves Worlcat holdings status of a record with provided OCLC number.
         The service automatically recognizes institution based on the issued access

bookops_worldcat/query.py

@@ -8,8 +8,7 @@
 import sys
 
 from requests.models import PreparedRequest
-from requests.exceptions import ConnectionError, HTTPError, Timeout
-
+from requests.exceptions import ConnectionError, HTTPError, Timeout, RetryError
 from .errors import WorldcatRequestError
 
 
@@ -19,12 +18,12 @@
 
 class Query:
     """
-    Sends a request to OClC service and unifies received exceptions
+    Sends a request to OCLC service and unifies exceptions.
     Query object handles refreshing expired token before request is
     made to the web service.
 
     `Query.response` attribute is `requests.Response` instance that
-    can be parsed to exctract received information from the web service.
+    can be parsed to extract information received from the web service.
     """
 
     def __init__(
@@ -53,8 +52,6 @@ def __init__(
         if session.authorization.is_expired():
             session._get_new_access_token()
 
-        self.response = None
-
         try:
             self.response = session.send(prepared_request, timeout=timeout)
             self.response.raise_for_status()
@@ -64,7 +61,8 @@ def __init__(
                 f"{exc}. Server response: "  # type: ignore
                 f"{self.response.content.decode('utf-8')}"
             )
-        except (Timeout, ConnectionError):
+        except (Timeout, ConnectionError, RetryError):
             raise WorldcatRequestError(f"Connection Error: {sys.exc_info()[0]}")
-        except:
+
+        except Exception:
             raise WorldcatRequestError(f"Unexpected request error: {sys.exc_info()[0]}")

tests/conftest.py

@@ -122,6 +122,11 @@ def __init__(self, *args, **kwargs):
         raise requests.exceptions.ConnectionError
 
 
+class MockRetryError:
+    def __init__(self, *args, **kwargs):
+        raise requests.exceptions.RetryError
+
+
 class MockHTTPSessionResponse(Response):
     def __init__(self, http_code):
         self.status_code = http_code
@@ -200,6 +205,13 @@ def mock_connection_error(monkeypatch):
     monkeypatch.setattr("requests.Session.send", MockConnectionError)
 
 
+@pytest.fixture
+def mock_retry_error(monkeypatch):
+    monkeypatch.setattr("requests.post", MockRetryError)
+    monkeypatch.setattr("requests.get", MockRetryError)
+    monkeypatch.setattr("requests.Session.send", MockRetryError)
+
+
 @pytest.fixture
 def mock_token(mock_credentials, mock_successful_post_token_response):
     return WorldcatAccessToken(**mock_credentials)
@@ -211,6 +223,18 @@ def stub_session(mock_token):
         yield session
 
 
+@pytest.fixture
+def stub_retry_session(mock_token):
+    with MetadataSession(
+        authorization=mock_token,
+        total_retries=3,
+        backoff_factor=0.5,
+        status_forcelist=[500, 502, 503, 504],
+        allowed_methods=["GET", "POST", "PUT"],
+    ) as session:
+        yield session
+
+
 @pytest.fixture
 def mock_400_response(monkeypatch):
     def mock_api_response(*args, **kwargs):

tests/test_metadata_api.py

@@ -1194,3 +1194,37 @@ def test_validate_bib(self, live_keys, stub_marc21):
                 == "https://metadata.api.oclc.org/worldcat/manage/bibs/validate/validateFull"
             )
             assert sorted(response.json().keys()) == sorted(["httpStatus", "status"])
+
+    def test_default_retries(self, live_keys, stub_marc21):
+        token = WorldcatAccessToken(
+            key=os.getenv("WCKey"),
+            secret=os.getenv("WCSecret"),
+            scopes=os.getenv("WCScopes"),
+        )
+
+        with MetadataSession(authorization=token) as session:
+            with pytest.raises(WorldcatRequestError) as exc:
+                session.validate_bib(stub_marc21, recordFormat="foo/bar")
+            assert "406 Client Error: Not Acceptable for url: " in (str(exc.value))
+            assert session.adapters["https://"].max_retries.total == 0
+
+    def test_custom_retries(self, live_keys, stub_marc21):
+        token = WorldcatAccessToken(
+            key=os.getenv("WCKey"),
+            secret=os.getenv("WCSecret"),
+            scopes=os.getenv("WCScopes"),
+        )
+
+        with MetadataSession(
+            authorization=token,
+            total_retries=3,
+            backoff_factor=0.5,
+            status_forcelist=[406],
+            allowed_methods=["GET", "POST"],
+        ) as session:
+            with pytest.raises(WorldcatRequestError) as exc:
+                session.validate_bib(stub_marc21, recordFormat="foo/bar")
+            assert "Connection Error: <class 'requests.exceptions.RetryError'>" in (
+                str(exc.value)
+            )
+            assert session.adapters["https://"].max_retries.total == 3

tests/test_query.py

@@ -21,7 +21,7 @@ def test_query_live(live_keys):
     )
     with MetadataSession(authorization=token) as session:
         header = {"Accept": "application/json"}
-        url = "https://americas.metadata.api.oclc.org/worldcat/search/v1/brief-bibs/41266045"
+        url = "https://metadata.api.oclc.org/worldcat/search/brief-bibs/41266045"
         req = Request(
             "GET",
             url,
@@ -95,9 +95,7 @@ def test_query_http_207_response(stub_session, mock_session_response):
 @pytest.mark.http_code(404)
 def test_query_http_404_response(stub_session, mock_session_response):
     header = {"Accept": "application/json"}
-    url = (
-        "https://americas.metadata.api.oclc.org/worldcat/search/v1/brief-bibs/41266045"
-    )
+    url = "https://metadata.api.oclc.org/worldcat/search/brief-bibs/41266045"
     req = Request("GET", url, headers=header, hooks=None)
     prepped = stub_session.prepare_request(req)
 
@@ -143,10 +141,41 @@ def test_query_connection_exception(stub_session, mock_connection_error):
     )
 
 
+def test_query_retry_exception(stub_session, mock_retry_error):
+    req = Request("GET", "https://foo.org")
+    prepped = stub_session.prepare_request(req)
+    with pytest.raises(WorldcatRequestError) as exc:
+        Query(stub_session, prepped)
+
+    assert "Connection Error: <class 'requests.exceptions.RetryError'>" in str(
+        exc.value
+    )
+
+
 def test_query_unexpected_exception(stub_session, mock_unexpected_error):
     req = Request("GET", "https://foo.org")
     prepped = stub_session.prepare_request(req)
     with pytest.raises(WorldcatRequestError) as exc:
         Query(stub_session, prepped)
 
     assert "Unexpected request error: <class 'Exception'>" in str(exc.value)
+
+
+def test_query_timeout_retry(stub_retry_session, caplog):
+    req = Request("GET", "https://foo.org")
+    prepped = stub_retry_session.prepare_request(req)
+    with pytest.raises(WorldcatRequestError):
+        Query(stub_retry_session, prepped)
+
+    assert "Retry(total=0, " in caplog.records[2].message
+    assert "Retry(total=1, " in caplog.records[1].message
+    assert "Retry(total=2, " in caplog.records[0].message
+
+
+def test_query_timeout_no_retry(stub_session, caplog):
+    req = Request("GET", "https://foo.org")
+    prepped = stub_session.prepare_request(req)
+    with pytest.raises(WorldcatRequestError):
+        Query(stub_session, prepped)
+
+    assert "Retry" not in caplog.records

tests/test_session.py

@@ -38,3 +38,22 @@ def test_default_timeout(self, mock_token):
     def test_custom_timeout(self, mock_token):
         with WorldcatSession(mock_token, timeout=1) as session:
             assert session.timeout == 1
+
+    def test_default_adapter(self, mock_token):
+        with WorldcatSession(mock_token) as session:
+            assert session.adapters["https://"].max_retries.total == 0
+
+    def test_adapter_retries(self, mock_token):
+        with WorldcatSession(
+            authorization=mock_token,
+            total_retries=3,
+            backoff_factor=0.5,
+            status_forcelist=[500, 502, 503, 504],
+            allowed_methods=["GET", "POST", "PUT"],
+        ) as session:
+            assert session.adapters["https://"].max_retries.status_forcelist == [
+                500,
+                502,
+                503,
+                504,
+            ]