Extract OverdrivePatronRequests transport layer (PP-4938) - #3682
Open
jonathangreen wants to merge 8 commits into
Open
Extract OverdrivePatronRequests transport layer (PP-4938)#3682jonathangreen wants to merge 8 commits into
jonathangreen wants to merge 8 commits into
Conversation
Contributor
|
Claude finished @jonathangreen's task in 6m 41s —— View job SummaryThis is a clean extraction. I traced every moved piece against the base branch: all six patron endpoint templates are remapped to DetailsNit:
|
Contributor
Greptile SummaryThe PR extracts OverDrive patron-authentication transport mechanics into
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/palace/manager/integration/license/overdrive/requests.py | Introduces the patron transport layer and restores the previously flagged token timeout, retry, validation-translation, and f-string requirements. |
| src/palace/manager/integration/license/overdrive/api.py | Delegates patron HTTP behavior while retaining database-backed token lookup, refresh, and persistence. |
| src/palace/manager/api/model/token.py | Configures OAuth token validation errors to omit sensitive input. |
| tests/manager/integration/license/overdrive/test_requests.py | Adds focused coverage for patron token acquisition, transport policy, error mapping, response parsing, and 401 retry behavior. |
| tests/manager/integration/license/overdrive/test_api.py | Retains API-level coverage for scope construction and persisted credential replacement. |
Sequence Diagram
sequenceDiagram
participant API as OverdriveAPI
participant Provider as PatronTokenProvider
participant Requests as OverdrivePatronRequests
participant OD as OverDrive
API->>Requests: patron_request(provider, endpoint)
Requests->>Provider: token()
Provider-->>Requests: persisted bearer token
Requests->>OD: patron request
alt Successful response
OD-->>Requests: 2xx response
Requests-->>API: Response or parsed model
else First 401
OD-->>Requests: 401
Requests->>Provider: "token(force_refresh=True)"
Provider->>Requests: refresh_patron_oauth_token(...)
Requests->>OD: password-grant token request
OD-->>Requests: OAuth token
Provider-->>Requests: newly persisted bearer token
Requests->>OD: retry patron request once
OD-->>Requests: response
Requests-->>API: Response or parsed model
end
Reviews (8): Last reviewed commit: "Cover the error code translation at the ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## chore/overdrive-client-requests #3682 +/- ##
================================================================
Coverage 93.57% 93.57%
================================================================
Files 514 514
Lines 46953 46968 +15
Branches 6406 6409 +3
================================================================
+ Hits 43934 43951 +17
+ Misses 1952 1949 -3
- Partials 1067 1068 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jonathangreen
force-pushed
the
chore/overdrive-patron-requests
branch
from
August 31, 2026 17:10
fc39819 to
403b06f
Compare
Move the Overdrive "Patron Authentication" HTTP mechanics into their own requests class, alongside the client-context one. It owns the patron endpoint templates, the password-grant token request made with the Palace Project credentials, and the request path with its refresh-once-on-401 behavior and error-code translation. The two auth contexts are separate classes because they share nothing but hosts and URL templating: one authenticates with the collection's configured key and secret, the other with the Palace credentials from the environment, and no business method mixes them in a single flow. Patron tokens live in the Credential table, so the requests class cannot own them the way the client-context class owns its token. Instead the API passes a PatronTokenProvider callable, which keeps the retry in the transport layer while the database work stays in the API layer.
OverdriveAPI._do_post set a 120 second timeout and the collection's configured max_retry_count. The patron request class did not carry either across, so the patron token request fell back to the global 20 second timeout and five retries, ignoring the integration setting. Move _do_get and _do_post onto BaseOverdriveRequests so both request contexts share one policy rather than each restating it. Also bring the token response validation inside the error handler. OAuthTokenResponse requires token_type and a positive expires_in, where the previous code only read access_token and expires_in off the JSON, so a 2xx body Overdrive would previously have been fine with now raises a bare ValidationError in the middle of a checkout. Add an API level test for the 401 path. The request layer reaches the patron token through a PatronTokenProvider callable, so the API is where that callable is joined up to the Credential row; the request layer test for the same 401 uses an in-memory provider and stays green if the database glue breaks.
patron_request calls the token provider again after forcing a refresh, so a 401'd request looked the Credential up three times: once for the initial token, once to refresh it, and once for the retry. Hold the credential in the closure instead, which brings that down to one lookup. It also makes the provider's contract honest. The refreshed token was reached by calling the provider a second time rather than by using what the forced refresh returned, so the protocol quietly required implementations to persist as a side effect; now the retry reads the same credential object the refresh wrote to. The callable is built per patron_request call, so the cached credential never outlives the request it belongs to. Use f-strings for the scope string and the Palace basic auth header, per the project convention. The sibling header in OverdriveClientRequests already builds its credentials this way.
jonathangreen
force-pushed
the
chore/overdrive-patron-requests
branch
from
September 1, 2026 14:52
403b06f to
d52c8c9
Compare
Cover the 401 retry's response_type forwarding. The code this PR replaced dropped it, so a patron request that asked for a model got a raw Response back whenever a token happened to expire mid-flight. That is a behavior fix in a PR that is otherwise a move, so it needs a test of its own. Say why patron API calls stay on the global HTTP timeout and retries. The token endpoints now take their policy from the base class, so the sibling that deliberately does not was starting to read like an oversight. Import Configuration from core.config, where overdrive_fulfillment_keys is actually defined. Reaching it through api.config pulled flask_babel, Crypto and the library settings models into the transport module for nothing. Drop the server nickname from OverdriveAPI, whose only reader moved to OverdrivePatronRequests, and an unused sample data fixture left behind by a test split.
The unusable token response branch logged the response body and pydantic's rendering of it. That body is a 2xx token document, so a schema change on Overdrive's side that left access_token intact would write a live bearer token to the log on every patron authentication, and carry it into the exception's debug message as well. Report the validation errors with the input excluded instead, and cover it with a test that fails if the token reaches either place. Interpolate the hold endpoint through the patron request object. release_hold was the one caller passing a patron template to the API's endpoint(), which forwards to the client context. The two objects derive the same hosts from the same settings today, but they are constructed independently, so the contexts should not be crossed. Drop CHECKOUT_ENDPOINT, which has no callers.
The previous commit built a redacted message but logged it with log.exception, which attaches the ValidationError and lets pydantic render the input that failed. The access token was in that traceback, so the redaction did nothing. Hide the input at the model instead. An OAuth token response always carries a live credential, and every caller of OAuthTokenResponse logs these errors or attaches them to an exception, so the client context and the OPDS and Boundless integrations were exposed the same way. Log the patron failure without exc_info as well, since the errors are already in the message and the traceback only adds the thing being kept out. The earlier test passed only because pydantic elides the middle of a long value and the token used was long enough to be caught by it. Use a short token and assert on the traceback and the input marker, so a leak fails the test rather than being hidden by formatting. State on PatronTokenProvider that a forced refresh has to be visible to later calls, which is what the retry after a 401 relies on.
patron_request threw away the refreshed token and asked the provider for it again, which only worked because OverdriveAPI's provider writes through to the Credential. Nothing made that a requirement, so the Protocol had to carry a docstring warning that a provider returning a fresh token without storing it would retry with the rejected one. Use the returned token and the requirement goes away, along with the recursion. exception_on_401 existed only to tell the second attempt apart from the first, so it drops off the signature and both overloads; it had no callers outside the recursion itself. Also cover the missing Palace credentials path, which turns a CannotLoadConfiguration into CannotFulfill and was the last uncovered branch in this file.
Turning an Overdrive error body into NoActiveLoan, AlreadyCheckedOut and the rest is the most patron-visible thing patron_request does, and it was reached only through the API's checkout and hold tests. Covering it where it happens is the split this branch is making everywhere else. The unmapped code case is worth having too: ErrorResponse requires a message key even though the value may be null, so a body carrying only an errorCode does not parse and arrives as a generic error with no code. Patch the request helpers on the base class in test_constructor_makes_no_requests. They live there now, so one patch per method covers every context, and the patron override the test used to patch no longer exists.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Moves the Overdrive "Patron Authentication" HTTP mechanics into their own request class alongside the client-context one added in the previous PR.
OverdrivePatronRequestsowns the patron endpoint templates, the password-grant token request made with the Palace Project credentials, and the request path with its refresh-once-on-401 behavior and error-code translation.OverdriveAPIkeeps apatron_requestdelegator with an unchanged signature, so business methods are untouched here._do_post,_do_patron_requestand_palace_context_basic_auth_headerare gone from the API class._do_get/_do_postnow live onBaseOverdriveRequests, so both request contexts share one timeout and retry policy rather than each restating it.Motivation and Context
Part of separating the Overdrive integration's HTTP concerns from its business logic (PP-4938).
The two authentication contexts are separate classes rather than one because they share nothing but hosts and URL templating. Client authentication uses the collection's configured key and secret; patron authentication uses the Palace credentials from the environment. Their endpoint sets are disjoint, and no business method mixes them in a single flow, so the API holds them as two attributes rather than threading both through one object.
Patron tokens are persisted in the
Credentialtable, so this class cannot own its token the way the client-context class owns its in-memory one. Rather than give the request layer a database session, the API passes aPatronTokenProvidercallable:That keeps the retry-on-401 in the transport layer, where the client-context class also does it, while the
Credentiallookup and persistence stay in the API layer._refresh_patron_oauth_tokenbecomes a thin adapter that calls the pure HTTP refresh and writes the result to the credential, still using the existing 0.9 expiry factor, so nothing about what is stored changes.Behavior changes
This is mostly a move, but three things do change:
response_type. The old code dropped it, so a patron request that asked for a model got a rawResponseback whenever a token happened to expire mid-request —get_loanwould return aResponseinstead of aCheckoutand fail on the first attribute access. Pinned by a test.OAuthTokenResponsesetshide_input_in_errors=True. A token response body carries a live credential, and every caller of this model logs its validation errors or attaches them to an exception, so a malformed 2xx could put a bearer token in the logs. This is a shared model, so the fix also covers the client-context, OPDS and Boundless token paths.patron_requestno longer takesexception_on_401. It retries with the token the forced refresh returns instead of asking the provider again, which turns the recursion into a loop and removes the parameter that only existed to tell the second attempt from the first. It had no callers outside the recursion.Two smaller corrections while the code was being moved:
release_holdwas the one caller interpolating a patron template through the API'sendpoint(), which forwards to the client context, andCHECKOUT_ENDPOINThad no callers and is dropped rather than carried into the new class.How Has This Been Tested?
Adds
TestOverdrivePatronRequestscovering the token payload with and without a PIN, the request policy (120s timeout and the collection'smax_retry_count), the error mapping toPatronAuthorizationFailedExceptionfor a real Overdrive 400 body and for a non-JSON response, an unusable 2xx token body, missing Palace credentials mapping toCannotFulfill, the error-code translation intoNoActiveLoanand into a genericOverdriveResponseException, the 401 path forcing a refresh and retrying, the retry still parsing into the requested model, a second 401 raising, and a validation failure raisingOverdriveValidationError.The patron-token tests that duplicated this at the API level moved there. What remains at the API level is what is genuinely API-level: that the scope is built from the collection's website ID and the patron library's ILS name, that the token is written to the credential, and that a 401 rewrites the stored
Credential— the one seam the request-layer test cannot cover, since it uses an in-memory provider.tests/manager/api/model/test_token.pygains a test asserting a validation error carries neither the token nor the input that produced it.Patron request tests never queue a client token, which is a small side benefit of the split.
5989 passed.
mypyclean.Checklist