Query updates #84

charlottekostelic · 2024-03-05T17:15:07Z

Edited to reflect changes:

Added option for users to add automatic retries to requests

added total_retries, backoff_factor, status_forcelist, and allowed_methods as args in WorldcatSession and MetadataSession
default number of retries is 0
added tests to all modules and live tests of retries to test_metadata_api.py

Somewhat related: there's an open enhancement request for requests asking to be able to refresh authentication header on retry. That functionality doesn't currently exist and it doesn't look like it's being worked on
psf/requests#5975

bookops_worldcat/query.py

klinga

@charlottekostelic in the description you mention a removal of a test for a stale token from test_query but I don't see such changes in this PR. We should be testing for a stale token in this module since the check if it is still valid happens only here. Stale token tests were somewhat redundant in the test_metadata_api and it's OK to skip them there.

Also (this may be complicated but useful), it would be smart to have a test in the test_query to test for retries (if they are being activated appropriately, especially on timeouts since these are common with the service).

charlottekostelic · 2024-03-06T14:22:00Z

@charlottekostelic in the description you mention a removal of a test for a stale token from test_query but I don't see such changes in this PR. We should be testing for a stale token in this module since the check if it is still valid happens only here. Stale token tests were somewhat redundant in the test_metadata_api and it's OK to skip them there.

I removed the stale token test and that lowered the test coverage (the if session.authorization.is_expired() block) so I added it back in but forgot to update the description.

charlottekostelic · 2024-03-06T19:52:06Z

@klinga I made a few changes:

Changed where retries are mounted so they are now in WorldcatSession rather than Query
Added RetryError as an exception for Query
Added additional tests

klinga

@charlottekostelic further changes are needed in _session, metadata_api and their tests.
Please see my comments for the details. Let me know if my explanations weren't clear.

bookops_worldcat/_session.py

bookops_worldcat/query.py

tests/test_session.py

tests/test_query.py

klinga

Apart from two instances of typing question I have this looks good.

bookops_worldcat/_session.py

klinga · 2024-03-07T19:18:13Z

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]] = [],


Does mypy agree?

klinga · 2024-03-07T19:25:42Z

tests/test_query.py

+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


👏 Awesome!

klinga · 2024-03-07T19:26:23Z

tests/test_session.py

+
+    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,
+            ]


Looks great!

@charlottekostelic

* move cov & pytest config to pyproject.toml * remove temp file * remove pytest-recording * add py311 * dependencies updates * drop py3.7 * copy main config * drop py3.7 * add requests to dev dependencies * fix missing dependencies * dependencies final update * version bump to 1.0.0 * adds all dependencies * add py3.12 * typing cleanup & tests refactoring * Authentication updates (#69) * ingnore E501 in tests * add types-requests to dev dependencies * scopes as str * scopes as str & types cleanup * dev dependencies moved to tool.poetry.group.dev.dependencies section * token_expires_at as datetime obj * token_expires_at as datetime * changed utcnow to now(timezone.utc) (#71) * Ocn-parsing-refactor (#72) * prep_oclc_numbe_strr refactor * oclcNumber stripped * verification refactor * utc fixes (#73) * fixed datetime type errors * moved datetime edits to _hasten_expiration_time * Errors-refactor (#74) * removed WorldcatSessionError * incorrect AttributeError replaced with TypeError * replaces WorldcatAuthorizationError with TypeError and ValueError for configuration * removed unused WorldcatAuthorizationError import * removed unused WorldcatRequestError import * added safe decoding for bytes-str * ignore F401 * None type added to possible timeout types * added type ingnore * removed unused InvalidOclcNumber import & typing fixes * changed endpoints in metadata api 2.0 (#77) * changed endpoints in metadata api 2.0 * fixed tests with typos * changed response_format default in get_full_bib * Changed search endpoints (#78) * changed search endpoints in metadata api 2.0 * fixed types * fixed spacing and indentation * type hint fixes and refactored test * Removed principalID and principalIDNS from token requests (#79) * removed unnecessary params from token requests * fixed docstring * MetadataSession cleanup (#80) * reordered methods in MetadataSession * removed obsolete 409 error handling from query.py * simplified changes * added new api endpoints (#81) * added new api endpoints * added tests * added test * added to doc string, fixed typos (#82) @charlottekostelic This is something that should be brought to users attention in the documentation. Will create an issue as a reminder. Besides that, looks good. Thanks! * 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 * Reordered metadata methods (#85) * renamed/reordered metadata_api methods * fixed optional/required args, added to doc strings * fixed default values to match API defaults * fixed error in live test * dev status update to 5, removes py3.7 & adds py3.11 & py3.12 (#87) * Update docs (#88) * reorganized docs, added mkdocs-material theme * added css for NYPL colors * reorganized docs, added examples * changed structure of docs, added to docs * changed snake case to camel case in args * typo fixes * added contributing.md, python versions for black * added 1.0 to changelog, migration section in docs * Added migration section to README * typo fixes, link fixes * added py.typed file * fixed links, made edits per PR 88 * added mkdocstrings, removed mkapi, doc edits (#91) * edited changelog * pyproject.toml edits * update unit-tests.yaml * fixed unit-tests.yaml * unit-tests.yaml indentation issues --------- Co-authored-by: klinga <klingaroo@gmail.com>

charlottekostelic added 3 commits February 29, 2024 09:21

work in progress

ec6a6de

added retries to query

7b00ec1

removed test with stale token

17cb327

charlottekostelic requested a review from klinga March 5, 2024 17:15

charlottekostelic marked this pull request as draft March 5, 2024 17:45

charlottekostelic added 2 commits March 5, 2024 12:47

added stale token test back in

dda7979

added stale token test back in

704e667

charlottekostelic marked this pull request as ready for review March 5, 2024 17:50

klinga reviewed Mar 6, 2024

View reviewed changes

bookops_worldcat/query.py Outdated Show resolved Hide resolved

klinga reviewed Mar 6, 2024

View reviewed changes

moved retries to _session module, added tests

91246a4

charlottekostelic requested a review from klinga March 6, 2024 20:16

klinga requested changes Mar 7, 2024

View reviewed changes

bookops_worldcat/_session.py Outdated Show resolved Hide resolved

bookops_worldcat/query.py Show resolved Hide resolved

tests/test_session.py Outdated Show resolved Hide resolved

tests/test_query.py Outdated Show resolved Hide resolved

charlottekostelic added 5 commits March 7, 2024 09:43

added retry status_forcelist tests

70b3a24

added custom adapter test

984a370

changed default retry behavior

21c0c3f

added another test

2543676

more testing

cd4e180

charlottekostelic requested a review from klinga March 7, 2024 17:25

charlottekostelic merged commit ae23a7f into releases/v1.0.0 Mar 7, 2024
6 checks passed

charlottekostelic deleted the query-updates branch March 7, 2024 19:24

klinga approved these changes Mar 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query updates #84

Query updates #84

charlottekostelic commented Mar 5, 2024 •

edited

Loading

klinga left a comment •

edited

Loading

charlottekostelic commented Mar 6, 2024

charlottekostelic commented Mar 6, 2024

klinga left a comment •

edited

Loading

klinga left a comment

klinga Mar 7, 2024

klinga Mar 7, 2024

klinga Mar 7, 2024

Query updates #84

Query updates #84

Conversation

charlottekostelic commented Mar 5, 2024 • edited Loading

klinga left a comment • edited Loading

Choose a reason for hiding this comment

charlottekostelic commented Mar 6, 2024

charlottekostelic commented Mar 6, 2024

klinga left a comment • edited Loading

Choose a reason for hiding this comment

klinga left a comment

Choose a reason for hiding this comment

klinga Mar 7, 2024

Choose a reason for hiding this comment

klinga Mar 7, 2024

Choose a reason for hiding this comment

klinga Mar 7, 2024

Choose a reason for hiding this comment

charlottekostelic commented Mar 5, 2024 •

edited

Loading

klinga left a comment •

edited

Loading

klinga left a comment •

edited

Loading