Parse Overdrive book list pages into a model (PP-4938) - #3704
Parse Overdrive book list pages into a model (PP-4938)#3704jonathangreen wants to merge 7 commits into
Conversation
|
Claude finished @jonathangreen's task in 5m 43s —— View job SummaryThis is a clean piece of the HTTP/business-logic split: DetailsMinor:
|
Greptile SummaryThe PR introduces a typed envelope for OverDrive product and event feeds while leaving individual product documents unmodelled.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/palace/manager/integration/license/overdrive/model.py | Adds the BookListPage and FeedLink models, including optional envelope fields and safe relation lookup. |
| src/palace/manager/integration/license/overdrive/requests.py | Centralizes feed URL construction, fetching, HTTP-status checks, and model validation across synchronous and asynchronous clients. |
| src/palace/manager/integration/license/overdrive/api.py | Migrates inventory pagination and collection-size checks from raw response dictionaries to parsed pages. |
| src/palace/manager/integration/license/overdrive/representation.py | Narrows availability extraction to the raw product list now supplied by BookListPage. |
| tests/manager/integration/license/overdrive/test_requests.py | Covers URL escaping, response-status handling, validation failures, and asynchronous empty-page behavior. |
| tests/manager/integration/license/overdrive/test_model.py | Covers feed parsing, omitted optional fields, tolerant feed links, and safe pagination URLs. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
API[OverdriveAPI] --> URLs[OverdriveClientRequests URL builders]
URLs --> Feed[OverDrive product or events feed]
Feed --> Parser[Shared page parser]
Parser --> Page[BookListPage]
Page --> Products[Raw product documents]
Page --> Links[Safe pagination link]
Products --> Extractor[OverdriveRepresentationExtractor]
Links --> Feed
Reviews (9): Last reviewed commit: "Drop the relation parameter nothing pass..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/overdrive-async-requests #3704 +/- ##
===============================================================
Coverage 93.59% 93.60%
===============================================================
Files 514 514
Lines 47083 47091 +8
Branches 6420 6419 -1
===============================================================
+ Hits 44069 44080 +11
+ Misses 1948 1947 -1
+ Partials 1066 1064 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dbernstein
left a comment
dbernstein
left a comment
There was a problem hiding this comment.
A very welcome update.
14a2a1c to
54ae0e2
Compare
Book list and events feeds were handled as raw dicts, so callers had to know the envelope's shape and reach for the extractor just to find the next-page link. BookListPage models the envelope, and the client request layer builds the feed URLs and returns parsed pages. The products themselves stay as raw documents. The extractor reads far more of the Overdrive product schema than we would want to model here, and typing it is a much larger change than this one. Overdrive omits totalItems on the product feed, so the collection-size self test now says so rather than raising a KeyError.
Delete the extractor's link lookup and the async class's copy of it. BookListPage.link_safe is the one way to find a page's link now, so the async path parses the page it fetches rather than reading the envelope by hand, which is what this change is for. Take the products directly in availability_link_list, rather than rebuilding the envelope the caller just parsed so the method can look for a key it is guaranteed to find. Cover the collection size the feed does not report, which is the one behavior this changes, and give the last-page test a link shaped like the ones Overdrive actually sends: the model requires a type, and the payload had only an href.
raw_get hands a 404 back rather than raising it, and every field of a book list page is optional, so an error document validated into a page with no titles. The collection size self test used to fail loudly on that, because it read totalItems straight out of the body; after the model it reported success and said Overdrive gave no size, so an unreachable feed read as a passing test. Check the status, and translate a page we cannot parse rather than letting a pydantic error escape. Log the body, not the parsed page. The model drops every key it does not declare, which is exactly the content that would explain a response we did not expect.
Both the synchronous fetch and the import workers now validate the same model, so they share the code that turns a body into a page. An import worker was getting a bare pydantic error where the synchronous path reported an Overdrive one with the url and body attached. Cover the feed URLs, whose escaping is the part of them most likely to break quietly: both carry a colon in a query value that Overdrive will not accept unescaped. Build the response data once in book_list_page rather than spelling out the same six fields in each of its two error paths.
The status check only covered the synchronous fetch. The async client allows a 404 through rather than raising it, and an error document parses into a page with no products, so an import reported a missing 'products' key instead of the status that explained it. The check belongs in the parsing both paths share. Make Link.type optional. Nothing reads it, but requiring it meant one link without it failed a whole page, aborting a crawl or an import over a field we ignore. The tests here were relying on that to produce an invalid page, which says how easy it was to trip. Build the response data only when one of the failures happens. It decodes the body to text, and these pages run to megabytes on a crawl where neither failure normally occurs.
Relaxing Link so a page would not fail over its type was too broad: the contentlink of a fulfillment response is a Link, and its type becomes the media type we hand to the delivery mechanism, so a link without one went from a clear error to a silently missing content type. Only the feed envelope needs the tolerance, so it gets a FeedLink of its own, and Link goes back to requiring the field its reader depends on. Move the page parsing down to the two client-context layers that use it, rather than the base the patron context also inherits, and type the headers it takes.
Both callers follow the next link, so rel_to_follow existed only for the test that passed it something else. The same commit drops extractor_class for the same reason. Take the type back off the feed link in the async test. It was there because the shared Link model demanded it, and the envelope has its own link now that does not.
54ae0e2 to
fe09b94
Compare
2 participants
Description
Adds a
BookListPagemodel for the envelope of a product or events feed, and moves the feed URL construction and fetching intoOverdriveClientRequestsasall_products_url,events_urlandbook_list_page.The products inside the page stay as raw documents.
Motivation and Context
Part of separating the Overdrive integration's HTTP concerns from its business logic (PP-4938).
Book list and events feeds were handled as raw dicts, so callers had to know the envelope's shape and reach into
OverdriveRepresentationExtractorjust to find the next-page link.BookListPage.link_safe(rel)replaces that, and the request layer hands back a parsed page rather than a body to interpret.The products are deliberately not modelled.
OverdriveRepresentationExtractorreads far more of the Overdrive product schema than we would want to declare here, and typing it is a much larger change than this one — one worth doing separately, if at all.Behavior change
Overdrive omits
totalItemson the product feed. The collection-size self test read it unconditionally and so raised aKeyError; it now reports that Overdrive did not give a size.total_itemsis modelled as optional for that reason, as areproducts, which Overdrive omits entirely when a collection or page has no titles.How Has This Been Tested?
TestBookListPagecovers the envelope: a full page, the optional fields Overdrive omits, andlink_safereturningNonefor a relation the page does not carry.The API tests move to asserting on the parsed page rather than on a body, and the self test gains a case for a feed with no
totalItems.6023 passed.
mypyclean.Checklist