Have Overdrive models describe requests instead of making them (PP-4938) - #3684
Have Overdrive models describe requests instead of making them (PP-4938)#3684jonathangreen wants to merge 5 commits into
Conversation
|
Claude finished @jonathangreen's task in 3m 38s —— View job SummarySolid, behavior-preserving refactor — the split between "describe the request" and "execute the request" is the right one, and moving DetailsNit:
|
Greptile SummaryThe PR separates OverDrive request description from HTTP execution while preserving action validation and retry behavior.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/palace/manager/integration/license/overdrive/model.py | Introduces immutable request specifications and converts action methods into request builders; the previous mutable-header concern is resolved by copying headers into a frozendict. |
| src/palace/manager/integration/license/overdrive/requests.py | Executes request specifications, builds fresh authorization-header mappings per attempt, and retains token-refresh retry behavior. |
| src/palace/manager/integration/license/overdrive/api.py | Migrates OverDrive circulation operations to construct and execute RequestSpec instances. |
| tests/manager/integration/license/overdrive/test_model.py | Adds value-based coverage for request construction, method normalization, frozen headers, and action validation. |
| tests/manager/integration/license/overdrive/test_requests.py | Verifies request-spec methods, bodies, headers, authorization merging, and existing retry behavior. |
Sequence Diagram
sequenceDiagram
participant API as OverdriveAPI
participant Model as OverDrive Model
participant Requests as Patron Request Layer
participant OD as OverDrive
API->>Model: Build RequestSpec
Model-->>API: Immutable request description
API->>Requests: patron_request(token, spec)
Requests->>Requests: Add Authorization header
Requests->>OD: Execute HTTP request
alt 401 response
Requests->>Requests: Refresh patron token
Requests->>OD: Retry immutable request spec
end
OD-->>Requests: Response
Requests-->>API: Response or validated model
Reviews (6): Last reviewed commit: "Document build_field_request" | 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-async-move #3684 +/- ##
==============================================================
- Coverage 93.57% 93.57% -0.01%
==============================================================
Files 514 514
Lines 46980 46979 -1
Branches 6410 6407 -3
==============================================================
- Hits 43963 43962 -1
Misses 1950 1950
Partials 1067 1067 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1cf1be6 to
9ae8dc1
Compare
Action and Checkout took a callable and performed the HTTP request themselves, which made response models into request initiators and split request construction across model.py and api.py. They now return a frozen RequestSpec describing the request, and the patron request layer executes it. The models keep all their hypermedia intelligence: action lookup, field validation against the action's declared fields, payload construction and URL templating are unchanged, and their errors are still raised before any request is made. This mirrors LinkTemplate.template, which already returned a URL rather than fetching it. Method casing on the wire is unaffected. RequestSpec carries the uppercase verb from the action, where patron_request used to lowercase it; requests itself uppercases the method before sending either way.
Freeze the headers a RequestSpec carries. The dataclass was frozen but the headers were a plain dict, so a caller could still change them after the fact, and every spec that had any was unhashable. Coercing in __post_init__ rather than only defaulting to frozendict covers build_field_request, which is the one path that actually sets headers. Return Self from RequestSpec.get, which is how from_response_data already spells the same thing, and drop the module-wide postponed annotations that the one forward reference needed. That import applies to every model in the file, and pydantic resolves those types. Rename Checkout.action to build_action_request. It returns a request to make, not the action it was built from, and Action.request was renamed to build_request in this same commit for that reason.
9ae8dc1 to
86dccd7
Compare
_lock_in_format catches InvalidFieldOptionError from build_action_request, so the field validation errors are part of that method's contract rather than an internal detail of Action.build_request. Name all three on both, including ExtraFieldsError, which build_request raises but did not mention. Spell Mapping one way. RequestSpec introduced the collections.abc import and build_field_request was the last user of the typing alias.
Specs are values now: tests compare them and the frozen dataclass hashes
them. requests uppercases the verb before sending, so RequestSpec("get")
and RequestSpec("GET") describe the same wire request, and they should not
compare unequal. Uppercase it alongside the headers, which lets
Action.build_request stop doing it for itself.
It is public and its two surprises are worth stating: the method defaults to POST, and an empty fields mapping still asks for a JSON content type while sending no body, which is what an action taking no arguments needs.
1 participant
Description
ActionandCheckouttook aPatronRequestCallableand performed the HTTP request themselves. They now return a frozenRequestSpecdescribing the request, and the patron request layer executes it.Action.request(make_request, ...)becomesAction.build_request(...),Checkout.action(name, make_request, ...)becomesCheckout.action(name, ...), andPatronRequestCallableand_overdrive_field_requestare removed.Motivation and Context
Part of separating the Overdrive integration's HTTP concerns from its business logic (PP-4938).
Response models that also initiate requests meant request construction was split across
model.pyandapi.py, and it was the last thing keeping HTTP inside the models. Separating "what request should I make" from "make this request" is the same split asrequests.PreparedRequestandSession.send.The models keep all of their hypermedia intelligence. Action lookup, validation against the action's declared fields, default values, camelCase mapping, payload construction and URL templating are unchanged, and
MissingRequiredFieldError,InvalidFieldOptionErrorandExtraFieldsErrorare still raised before any request is made. This also makesActionconsistent withLinkTemplate.template, which already returned a URL rather than fetching it.Method casing on the wire is unaffected.
RequestSpeccarries the uppercase verb from the action, wherepatron_requestused to lowercase it, butrequestsuppercases the method before sending either way. Some test assertions on the mock's recorded method change accordingly.How Has This Been Tested?
The
ActionandCheckouttests intest_model.pynow assert on the returnedRequestSpecinstead of on aMagicMock's call args, so they are pure value assertions with no mocks. The patron request tests assert that the spec's verb, body and headers are used verbatim, and that theAuthorizationheader is added on top of the spec's own headers.The business-level tests in
test_api.pyare unchanged apart from the method casing noted above, which is the evidence that the requests going over the wire are the same.5988 passed.
mypyclean.Checklist