Have Overdrive models describe requests instead of making them (PP-4938)

src/palace/manager/integration/license/overdrive/api.py

@@ -87,8 +87,8 @@
     Holds as HoldsResponse,
     LibraryResponse,
     PatronInformation,
-    PatronRequestCallable,
-    _overdrive_field_request,
+    RequestSpec,
+    build_field_request,
 )
 from palace.manager.integration.license.overdrive.representation import (
     OverdriveRepresentationExtractor,
@@ -605,10 +605,7 @@ def patron_request(
         self,
         patron: Patron,
         pin: str | None,
-        url: str,
-        extra_headers: dict[str, str] | None = ...,
-        data: str | None = ...,
-        method: str | None = ...,
+        request: RequestSpec,
         response_type: None = ...,
     ) -> Response: ...
 
@@ -617,21 +614,15 @@ def patron_request[TOverdriveModel: BaseOverdriveModel](
         self,
         patron: Patron,
         pin: str | None,
-        url: str,
-        extra_headers: dict[str, str] | None = ...,
-        data: str | None = ...,
-        method: str | None = ...,
+        request: RequestSpec,
         response_type: type[TOverdriveModel] = ...,
     ) -> TOverdriveModel: ...
 
     def patron_request[TOverdriveModel: BaseOverdriveModel](
         self,
         patron: Patron,
         pin: str | None,
-        url: str,
-        extra_headers: dict[str, str] | None = None,
-        data: str | None = None,
-        method: str | None = None,
+        request: RequestSpec,
         response_type: type[TOverdriveModel] | None = None,
     ) -> Response | TOverdriveModel:
         """
@@ -644,10 +635,7 @@ def patron_request[TOverdriveModel: BaseOverdriveModel](
         """
         return self.patron_requests.patron_request(
             self._patron_token_provider(patron, pin),
-            url,
-            extra_headers=extra_headers,
-            data=data,
-            method=method,
+            request,
             response_type=response_type,
         )
 
@@ -754,13 +742,14 @@ def checkout(
 
         already_checked_out = False
         try:
-            make_request: PatronRequestCallable[Checkout] = partial(
-                self.patron_request, patron, pin, response_type=Checkout
-            )
-            checkout = _overdrive_field_request(
-                make_request,
-                self.patron_requests.CHECKOUTS_ENDPOINT,
-                {"reserveId": overdrive_id},
+            checkout = self.patron_request(
+                patron,
+                pin,
+                build_field_request(
+                    self.patron_requests.CHECKOUTS_ENDPOINT,
+                    {"reserveId": overdrive_id},
+                ),
+                response_type=Checkout,
             )
         except OverdriveResponseException as e:
             code = e.error_message
@@ -819,8 +808,9 @@ def checkout(
                 do_early_return = not already_checked_out and existing_hold is None
 
                 if do_early_return:
-                    make_request = partial(self.patron_request, patron, pin)
-                    checkout.action("early_return", make_request)
+                    self.patron_request(
+                        patron, pin, checkout.build_action_request("early_return")
+                    )
 
                 # If this was a hold, we remove the hold record from the database before
                 # we raise the exception, since the hold has been converted to a checkout.
@@ -865,8 +855,7 @@ def checkin(
         # First we get the loan for this patron.
         try:
             loan = self.get_loan(patron, pin, licensepool.identifier.identifier)
-            make_request = partial(self.patron_request, patron, pin)
-            loan.action("early_return", make_request)
+            self.patron_request(patron, pin, loan.build_action_request("early_return"))
         except NoActiveLoan:
             # The loan is already gone, no need to return it. This exception gets
             # handled higher up the stack.
@@ -891,7 +880,9 @@ def get_loan(self, patron: Patron, pin: str | None, overdrive_id: str) -> Checko
         :return: Information about the loan.
         """
         url = f"{self.patron_requests.CHECKOUTS_ENDPOINT}/{overdrive_id.upper()}"
-        return self.patron_request(patron, pin, url, response_type=Checkout)
+        return self.patron_request(
+            patron, pin, RequestSpec.get(url), response_type=Checkout
+        )
 
     def fulfill(
         self,
@@ -930,7 +921,7 @@ def _contentlink_fulfillment(
             odreadauthurl=fulfill_url,
         )
         download_response = self.patron_request(
-            patron, pin, download_link, response_type=Format
+            patron, pin, RequestSpec.get(download_link), response_type=Format
         )
         result = download_response.links["contentlink"]
         url = result.href
@@ -1012,11 +1003,13 @@ def _get_fulfill_format_information(
     def _lock_in_format(
         self, patron: Patron, pin: str | None, format_type: str, loan: Checkout
     ) -> Format:
-        make_request: PatronRequestCallable[Format] = partial(
-            self.patron_request, patron, pin, response_type=Format
-        )
         try:
-            format_data = loan.action("format", make_request, format_type=format_type)
+            format_data = self.patron_request(
+                patron,
+                pin,
+                loan.build_action_request("format", format_type=format_type),
+                response_type=Format,
+            )
         except InvalidFieldOptionError:
             raise FormatNotAvailable(
                 "This book is not available in the format you requested."
@@ -1038,15 +1031,15 @@ def get_patron_checkouts(self, patron: Patron, pin: str | None) -> Checkouts:
         return self.patron_request(
             patron,
             pin,
-            self.patron_requests.CHECKOUTS_ENDPOINT,
+            RequestSpec.get(self.patron_requests.CHECKOUTS_ENDPOINT),
             response_type=Checkouts,
         )
 
     def get_patron_holds(self, patron: Patron, pin: str | None) -> HoldsResponse:
         return self.patron_request(
             patron,
             pin,
-            self.patron_requests.HOLDS_ENDPOINT,
+            RequestSpec.get(self.patron_requests.HOLDS_ENDPOINT),
             response_type=HoldsResponse,
         )
 
@@ -1214,7 +1207,7 @@ def default_notification_email_address(
             patron_information = self.patron_request(
                 patron,
                 pin,
-                self.patron_requests.PATRON_INFORMATION_ENDPOINT,
+                RequestSpec.get(self.patron_requests.PATRON_INFORMATION_ENDPOINT),
                 response_type=PatronInformation,
             )
             address = patron_information.last_hold_email
@@ -1258,13 +1251,11 @@ def place_hold(
             form_fields["ignoreHoldEmail"] = True
 
         try:
-            make_request: PatronRequestCallable[HoldResponse] = partial(
-                self.patron_request, patron, pin, response_type=HoldResponse
-            )
-            hold = _overdrive_field_request(
-                make_request,
-                self.patron_requests.HOLDS_ENDPOINT,
-                form_fields,
+            hold = self.patron_request(
+                patron,
+                pin,
+                build_field_request(self.patron_requests.HOLDS_ENDPOINT, form_fields),
+                response_type=HoldResponse,
             )
         except OverdriveResponseException as e:
             raise CannotHold(e.error_code) from e
@@ -1288,7 +1279,7 @@ def release_hold(
             product_id=licensepool.identifier.identifier,
         )
         try:
-            self.patron_request(patron, pin, url, method="DELETE")
+            self.patron_request(patron, pin, RequestSpec("DELETE", url))
         except OverdriveResponseException as e:
             response = e.response
             if (

src/palace/manager/integration/license/overdrive/model.py

@@ -1,11 +1,14 @@
 import json
 import re
 import typing
+from collections.abc import Mapping
+from dataclasses import dataclass
 from enum import StrEnum
 from functools import cached_property
-from typing import Protocol, Self, overload
+from typing import Self, overload
 from urllib.parse import quote_plus
 
+from frozendict import frozendict
 from pydantic import (
     AliasChoices,
     AwareDatetime,
@@ -190,26 +193,52 @@ class ActionField(BaseOverdriveModel):
     optional: bool = False
 
 
-class PatronRequestCallable[T](
-    Protocol,
-):
-    def __call__(
-        self,
-        *,
-        url: str,
-        extra_headers: dict[str, str] | None = None,
-        data: str | None = None,
-        method: str | None = None,
-    ) -> T: ...
+@dataclass(frozen=True)
+class RequestSpec:
+    """A prepared Overdrive API request.
 
+    Models describe the request to make -- the hypermedia action or link
+    tells us the URL, method and payload -- and the request layer executes
+    it. This keeps response models free of transport concerns while leaving
+    the navigation knowledge where the API document puts it.
+    """
+
+    method: str
+    url: str
+    data: str | None = None
+    headers: Mapping[str, str] = frozendict()
+
+    def __post_init__(self) -> None:
+        # Two specs that describe the same wire request should compare equal,
+        # and requests uppercases the verb regardless, so normalize it here.
+        object.__setattr__(self, "method", self.method.upper())
+        # Freeze whatever the caller passed, so that the headers are covered
+        # by the frozen contract rather than only the field holding them.
+        object.__setattr__(self, "headers", frozendict(self.headers))
+
+    @classmethod
+    def get(cls, url: str) -> Self:
+        """A plain GET of the given URL."""
+        return cls("GET", url)
 
-def _overdrive_field_request[T](
-    make_request: PatronRequestCallable[T],
+
+def build_field_request(
     url: str,
-    fields: typing.Mapping[str, str | bool | int],
+    fields: Mapping[str, str | bool | int],
     *,
-    method: str | None = None,
-) -> T:
+    method: str = "POST",
+) -> RequestSpec:
+    """Describe a request whose body is an Overdrive "fields" document.
+
+    :param url: The URL to request.
+    :param fields: The field names and values to send. An empty mapping
+        produces a request with no body, though still with the JSON content
+        type, which is what an action taking no arguments needs.
+    :param method: The HTTP method, defaulting to POST because that is what
+        every Overdrive action that carries fields uses.
+
+    :return: The request to make.
+    """
     if fields:
         data = json.dumps(
             {
@@ -221,13 +250,11 @@ def _overdrive_field_request[T](
     else:
         data = None
 
-    headers = {"Content-Type": "application/json"}
-
-    return make_request(
+    return RequestSpec(
         method=method,
         url=url,
         data=data,
-        extra_headers=headers,
+        headers={"Content-Type": "application/json"},
     )
 
 
@@ -262,22 +289,22 @@ def get_field(self, name: str, raising: bool = False) -> ActionField | None:
             raise NotFoundError(camel_name, "field", {f.name for f in self.fields})
         return None
 
-    def request[T](self, make_request: PatronRequestCallable[T], **kwargs: str) -> T:
+    def build_request(self, **kwargs: str) -> RequestSpec:
         """
-        Make a HTTP request with the parameters and method specified in the action.
+        Describe the HTTP request with the parameters and method specified in the action.
 
         The request data is constructed from the fields in the action, in the format
-        that Overdrive expects.
+        that Overdrive expects. The returned spec is executed by the request layer.
 
-        :param make_request: The callable used to make the HTTP request.
         :param kwargs: The values to provide in the request for fields in the action.
                        These can be either in camelCase or snake_case. snake_case is
                        converted to camelCase before being used.
 
         :raises MissingRequiredFieldError: If a required field is missing.
         :raises InvalidFieldOptionError: If a field has a value that is not in its options.
+        :raises ExtraFieldsError: If a supplied field is not one the action declares.
 
-        :return: The response from the HTTP request.
+        :return: The request to make.
         """
 
         camel_kwargs = {to_camel(k): v for k, v in kwargs.items()}
@@ -299,9 +326,8 @@ def request[T](self, make_request: PatronRequestCallable[T], **kwargs: str) -> T
         if camel_kwargs:
             raise ExtraFieldsError(camel_kwargs.keys())
 
-        return _overdrive_field_request(
-            make_request,
-            method=self.method.upper(),
+        return build_field_request(
+            method=self.method,
             url=self.href,
             fields=field_data,
         )
@@ -410,23 +436,28 @@ def available_formats(self) -> set[str]:
 
         return formats
 
-    def action[T](
-        self, name: str, make_request: PatronRequestCallable[T], **kwargs: str
-    ) -> T:
+    def build_action_request(self, name: str, **kwargs: str) -> RequestSpec:
         """
-        Make a HTTP request to the action with the specified name.
+        Describe the request to the action with the specified name.
 
         :param name: The name of the action to request, in snake_case or camelCase.
-        :param make_request: The callable used to make the HTTP request.
         :param kwargs: The values to provide in the request for fields in the action.
 
-        :return: The response from the HTTP request as returned by make_request.
+        :raises NotFoundError: If the checkout has no action with that name.
+        :raises MissingRequiredFieldError: If a field the action requires was
+            not supplied.
+        :raises InvalidFieldOptionError: If a supplied value is not one of the
+            options the action declares for that field.
+        :raises ExtraFieldsError: If a supplied field is not one the action
+            declares.
+
+        :return: The request to make.
         """
 
         camel_name = to_camel(name)
         if camel_name not in self.actions:
             raise NotFoundError(camel_name, "action", self.actions.keys())
-        return self.actions[camel_name].request(make_request, **kwargs)
+        return self.actions[camel_name].build_request(**kwargs)
 
 
 class Checkouts(BaseOverdriveModel):