Move async book-info fetching into its own request class (PP-4938) - #3683
Move async book-info fetching into its own request class (PP-4938)#3683jonathangreen wants to merge 8 commits into
Conversation
|
Claude finished @jonathangreen's task in 4m 57s —— View job Code Review
SummaryThe move is faithful: comparing the old DetailsMinor:
|
Greptile SummaryThe PR moves asynchronous OverDrive book-information fetching from the API class into a dedicated request class while preserving the importer’s paginated workflow.
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 dedicated asynchronous request class while retaining the prior page, relation-fetching, retry, and link-sanitization behavior. |
| src/palace/manager/integration/license/overdrive/api.py | Composes the new asynchronous request object and removes transport-specific methods from the API class. |
| src/palace/manager/integration/license/overdrive/importer.py | Redirects paginated book-information fetching through the new request object without changing importer flow. |
| src/palace/manager/celery/tasks/overdrive.py | Updates the relocated endpoint type import used by OverDrive task orchestration. |
| tests/manager/integration/license/overdrive/test_requests.py | Moves asynchronous transport coverage to the request-layer test suite and retains success, retry, malformed-response, 404, pagination, and empty-page cases. |
Sequence Diagram
sequenceDiagram
participant Worker as Celery import worker
participant Importer as OverdriveImporter
participant API as OverdriveAPI
participant Async as OverdriveAsyncRequests
participant OD as OverDrive
Worker->>Importer: Import collection page
Importer->>API: Build initial endpoint
Importer->>Async: fetch_book_info_list(endpoint)
Async->>OD: Fetch product page
par Linked metadata
Async->>OD: Fetch metadata
and Linked availability
Async->>OD: Fetch availability
end
Async-->>Importer: Products and next endpoint
Importer-->>Worker: Import result and continuation
Reviews (10): Last reviewed commit: "Say why the host template is module leve..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/overdrive-patron-requests #3683 +/- ##
================================================================
Coverage 93.57% 93.57%
================================================================
Files 514 514
Lines 46968 46980 +12
Branches 6409 6410 +1
================================================================
+ Hits 43951 43963 +12
- Misses 1949 1950 +1
+ Partials 1068 1067 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6911395 to
f9bbab8
Compare
The async page fetching bypassed the token and endpoint machinery it sits next to, so it belongs in the transport layer rather than on OverdriveAPI. It gets its own class instead of joining OverdriveClientRequests, because it shares almost none of that class's machinery: it runs on httpx rather than requests, with its own retry and backoff policy, and holds one client open across a whole page of products. What it does share is the bearer token, which it reaches through the client request object it is handed, so a collection still only ever holds one. BookInfoEndpoint moves along with it, and its importers are updated in the same change. The unused extractor_class parameter is dropped rather than carried along: no caller ever passed it, and the only thing it was used for was finding the next-page link, which is now a small private helper. That keeps the representation extractor, and the data layer behind it, out of the transport layer.
jonathangreen
changed the title
f9bbab8 to
b5e0571
Compare
The book info test queued the availability document ahead of the bibliographic one, but fetch_book_info_list asks for metadata first, so each document was attached to the other's key. Both assertions only checked truthiness, so the test passed while proving nothing. Queue them in the order they are requested and assert on a field that tells the two documents apart. Define the host template once at module level. It cannot go on the base class: the templates that build on it are evaluated in a subclass's class body, which does not resolve names from base classes. The async fixture no longer takes the sync http client, which nothing on it used.
Resolving the host from its own settings and taking the bearer token from the client request object are the two things this class newly does for itself, and neither was checked. The mock async client answers from a queue whatever the URL and headers are, so a wrong host or a missing Authorization header passed both here and in the celery test. Build all three request fixtures from one settings factory rather than three copies of the same five keyword arguments.
Nothing in the test tree stops a real network call, and async_http_client only patches when a test asks for it, so a test using this fixture without also naming that one would have gone to Overdrive. Take it as a fixture argument, the way the sync request fixture already takes its client.
The fixture holds the mock client so that using it always mocks the network, but the tests were still taking that client as a second parameter and queueing against it directly, leaving the attribute unread. Queue through the fixture, as the sync request tests do, and the extra parameter goes away.
client_requests and patron_requests can both be handed in, so the async one should be too rather than always being built in place. Mock the sync client in the async fixture as well. Its OverdriveClientRequests is real, and only the seeded token keeps it off the network.
A page with links but no next one is what ends the import loop, and it was the one branch of _page_link nothing reached: the empty collection test exits on the missing links key instead. The old code delegated this to the representation extractor, which has its own test, so the gap arrived with the move. Stop storing the sync mock on the async fixture. The fixture still depends on it, which is what keeps the real client it builds off the network, but nothing reads the attribute.
Only OverdriveClientRequests needs it at class-body scope, where an inherited attribute would not resolve. The async class reads it at runtime and would be fine either way. [skip claude]
1 participant
Description
Moves
fetch_book_info_listand its helpers, along with theBookInfoEndpointdataclass, out ofOverdriveAPIand into a newOverdriveAsyncRequests. The importer and the celery task are updated in the same change, so there is no temporary re-export.Also drops the
extractor_classparameter fromfetch_book_info_list.Motivation and Context
Part of separating the Overdrive integration's HTTP concerns from its business logic (PP-4938).
The async page fetching bypassed the token and endpoint machinery it sits next to, so it belongs in the transport layer rather than in the API class.
It gets its own class rather than joining
OverdriveClientRequests, because it shares almost none of that class's machinery. It runs onhttpxrather thanrequests, with its own retry and backoff policy, and it holds a single client open across a whole page of products. What it does share is the bearer token, which it reaches through theOverdriveClientRequestsit is handed:so a collection still only ever holds one token.
A later PR changes how we get the token, so that we can handle token refreshes on the async path, but since this change is mostly a mechanical move, I'm going to keep that to a separate PR.
The
extractor_classparameter is removed rather than carried along: no caller ever passed it, and the only thing it was used for was finding the next-page link. That is now a small private helper using_make_link_safedirectly. Keeping the parameter would have meant importingOverdriveRepresentationExtractorinto the transport layer, and with it the data layer and SQLAlchemy models behind it, which is exactly the coupling this series is removing. A later PR replaces the helper with a typed model method.How Has This Been Tested?
The async tests move from
test_api.pytotest_requests.pyasTestOverdriveAsyncRequests, and now build aBookInfoEndpointdirectly rather than going through the database-backedbook_info_initial_endpoint, so they exercise the transport layer without a session. They continue to useMockAsyncClientFixture, and cover the happy path, retry-then-fail on a server error, tolerated 404s on metadata and availability links, a malformed response missingproducts, and an empty collection.A new
overdrive_async_requestsfixture builds the class against a client context with a seeded token, which replaces the autouse token-seeding the old tests needed.5989 passed.
mypyclean.Checklist