diff --git a/src/palace/manager/api/axis.py b/src/palace/manager/api/axis.py index c3bbc8d80..e2ec3a9f3 100644 --- a/src/palace/manager/api/axis.py +++ b/src/palace/manager/api/axis.py @@ -31,15 +31,15 @@ from palace.manager.api.admin.validator import Validator from palace.manager.api.circulation import ( - APIAwareFulfillmentInfo, BaseCirculationAPI, BaseCirculationApiSettings, BaseCirculationLoanSettings, CirculationInternalFormatsMixin, - FulfillmentInfo, + Fulfillment, HoldInfo, LoanInfo, PatronActivityCirculationAPI, + UrlFulfillment, ) from palace.manager.api.circulation_exceptions import ( AlreadyCheckedOut, @@ -81,7 +81,6 @@ IdentifierSweepMonitor, TimelineMonitor, ) -from palace.manager.core.problem_details import INTEGRATION_ERROR from palace.manager.integration.settings import ( ConfigurationFormItem, ConfigurationFormItemType, @@ -105,9 +104,8 @@ from palace.manager.sqlalchemy.model.resource import Hyperlink, Representation from palace.manager.util.datetime_helpers import datetime_utc, strptime_utc, utc_now from palace.manager.util.flask_util import Response -from palace.manager.util.http import HTTP, RequestNetworkException +from palace.manager.util.http import HTTP, RemoteIntegrationException from palace.manager.util.log import LoggerMixin -from palace.manager.util.problem_detail import ProblemDetail from palace.manager.util.xmlparser import XMLProcessor @@ -478,7 +476,7 @@ def fulfill( pin: str, licensepool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: + ) -> Fulfillment: """Fulfill a patron's request for a specific book.""" identifier = licensepool.identifier # This should include only one 'activity'. @@ -498,8 +496,8 @@ def fulfill( continue # We've found the remote loan corresponding to this # license pool. - fulfillment = loan.fulfillment_info - if not fulfillment or not isinstance(fulfillment, FulfillmentInfo): + fulfillment = loan.fulfillment + if not fulfillment or not isinstance(fulfillment, Fulfillment): raise CannotFulfill() return fulfillment # If we made it to this point, the patron does not have this @@ -1614,26 +1612,14 @@ def process_one( or "" ) - # Arguments common to FulfillmentInfo and - # Axis360FulfillmentInfo. - kwargs = dict( - data_source_name=DataSource.AXIS_360, - identifier_type=self.id_type, - identifier=axis_identifier, - ) - - fulfillment: FulfillmentInfo | None + fulfillment: Fulfillment | None if download_url and self.internal_format != self.api.AXISNOW: # The patron wants a direct link to the book, which we can deliver # immediately, without making any more API requests. - fulfillment = Axis360AcsFulfillmentInfo( - collection=self.collection, + fulfillment = Axis360AcsFulfillment( content_link=html.unescape(download_url), content_type=DeliveryMechanism.ADOBE_DRM, - content=None, - content_expires=None, verify=self.api.verify_certificate, - **kwargs, ) elif transaction_id: # We will eventually need to make a request to the @@ -1648,9 +1634,13 @@ def process_one( # need to make a second request to get the audiobook # metadata. # - # Axis360FulfillmentInfo can handle both cases. - fulfillment = Axis360FulfillmentInfo( - api=self.api, key=transaction_id, **kwargs + # Axis360Fulfillment can handle both cases. + fulfillment = Axis360Fulfillment( + data_source_name=DataSource.AXIS_360, + identifier_type=self.id_type, + identifier=axis_identifier, + api=self.api, + key=transaction_id, ) else: # We're out of luck -- we can't fulfill this loan. @@ -1662,7 +1652,7 @@ def process_one( identifier=axis_identifier, start_date=start_date, end_date=end_date, - fulfillment_info=fulfillment, + fulfillment=fulfillment, ) elif reserved: @@ -1914,35 +1904,70 @@ def __str__(self) -> str: return json.dumps(data, sort_keys=True) -class Axis360FulfillmentInfo(APIAwareFulfillmentInfo, LoggerMixin): - """An Axis 360-specific FulfillmentInfo implementation for audiobooks +class Axis360Fulfillment(Fulfillment, LoggerMixin): + """An Axis 360-specific Fulfillment implementation for audiobooks and books served through AxisNow. - We use these instead of normal FulfillmentInfo objects because - putting all this information into FulfillmentInfo would require + We use these instead of normal Fulfillment objects because + putting all this information into Fulfillment would require one or two extra HTTP requests, and there's often no need to make those requests. """ - def do_fetch(self) -> None: + def __init__( + self, + api: Axis360API, + data_source_name: str, + identifier_type: str, + identifier: str, + key: str, + ): + """Constructor. + + :param api: An Axis360API instance, in case the parsing of + a fulfillment document triggers additional API requests. + + :param key: The transaction ID that will be used to fulfill + the request. + """ + self.data_source_name = data_source_name + self.identifier_type = identifier_type + self.identifier = identifier + self.api = api + self.key = key + + self.content_type: str | None = None + self.content: str | None = None + + def license_pool(self, _db: Session) -> LicensePool: + """Find the LicensePool model object corresponding to this object.""" + collection = self.api.collection + pool, is_new = LicensePool.for_foreign_id( + _db, + self.data_source_name, + self.identifier_type, + self.identifier, + collection=collection, + ) + return pool + + def do_fetch(self) -> tuple[str, str]: _db = self.api._db license_pool = self.license_pool(_db) transaction_id = self.key - if not isinstance(self.api, Axis360API): - self.log.error( - f"Called with wrong API type {self.api.__class__.__name__} should be {Axis360API.__name__}" - ) - raise ValueError("Axis360FulfillmentInfo can only be used with Axis360API") response = self.api.get_fulfillment_info(transaction_id) parser = Axis360FulfillmentInfoResponseParser(self.api) manifest, expires = parser.parse(response.content, license_pool=license_pool) - self._content = str(manifest) - self._content_type = manifest.MEDIA_TYPE - self._content_expires = expires + return str(manifest), manifest.MEDIA_TYPE + def response(self) -> Response: + if self.content is None: + self.content, self.content_type = self.do_fetch() + return Response(response=self.content, content_type=self.content_type) -class Axis360AcsFulfillmentInfo(FulfillmentInfo, LoggerMixin): - """This implements a Axis 360 specific FulfillmentInfo for ACS content + +class Axis360AcsFulfillment(UrlFulfillment, LoggerMixin): + """This implements a Axis 360 specific Fulfillment for ACS content served through AxisNow. The AxisNow API gives us a link that we can use to get the ACSM file that we serve to the mobile apps. @@ -1967,27 +1992,22 @@ class Axis360AcsFulfillmentInfo(FulfillmentInfo, LoggerMixin): and closed the ticket. https://github.com/urllib3/urllib3/issues/1677 - This FulfillmentInfo implementation uses the built in Python urllib + This Fulfillment implementation uses the built in Python urllib implementation instead of requests (and urllib3) to make this request to the Axis 360 API, sidestepping the problem, but taking a different code path than most of our external HTTP requests. """ - def __init__(self, verify: bool, **kwargs: Any) -> None: - super().__init__(**kwargs) - self.verify: bool = verify - - def problem_detail_document(self, error_details: str) -> ProblemDetail: - service_name = urlparse(self.content_link).netloc - self.log.warning(error_details) - return INTEGRATION_ERROR.detailed( - _(RequestNetworkException.detail, service=service_name), - title=RequestNetworkException.title, - debug_message=error_details, - ) + def __init__( + self, + content_link: str, + verify: bool, + content_type: str = DeliveryMechanism.ADOBE_DRM, + ) -> None: + super().__init__(content_link, content_type) + self.verify = verify - @property - def as_response(self) -> Response | ProblemDetail: + def response(self) -> Response: service_name = urlparse(str(self.content_link)).netloc try: if self.verify: @@ -1999,10 +2019,6 @@ def as_response(self) -> Response | ProblemDetail: else: # Default context does no ssl verification ssl_context = ssl.SSLContext() - if self.content_link is None: - return self.problem_detail_document( - f"No content link provided for {service_name}" - ) req = urllib.request.Request(self.content_link) with urllib.request.urlopen( req, timeout=20, context=ssl_context @@ -2012,21 +2028,19 @@ def as_response(self) -> Response | ProblemDetail: headers = response.headers # Mimic the behavior of the HTTP.request_with_timeout class and - # wrap the exceptions thrown by urllib and ssl returning a ProblemDetail document. + # wrap the exceptions thrown by urllib and ssl by raising a RemoteIntegrationException except urllib.error.HTTPError as e: - return self.problem_detail_document( - "The server received a bad status code ({}) while contacting {}".format( - e.code, service_name - ) - ) - except TimeoutError: - return self.problem_detail_document( - f"Error connecting to {service_name}. Timeout occurred." - ) + message = f"The server received a bad status code ({e.code}) while contacting {service_name}" + self.log.warning(message) + raise RemoteIntegrationException(service_name, message) from e + except TimeoutError as e: + message = f"Error connecting to {service_name}. Timeout occurred." + self.log.warning(message) + raise RemoteIntegrationException(service_name, message) from e except (urllib.error.URLError, ssl.SSLError) as e: reason = getattr(e, "reason", e.__class__.__name__) - return self.problem_detail_document( - f"Error connecting to {service_name}. {reason}." - ) + message = f"Error connecting to {service_name}. {reason}." + self.log.warning(message) + raise RemoteIntegrationException(service_name, message) from e return Response(response=content, status=status, headers=headers) diff --git a/src/palace/manager/api/bibliotheca.py b/src/palace/manager/api/bibliotheca.py index f0c691691..be8b1c2fa 100644 --- a/src/palace/manager/api/bibliotheca.py +++ b/src/palace/manager/api/bibliotheca.py @@ -26,7 +26,7 @@ BaseCirculationAPI, BaseCirculationApiSettings, BaseCirculationLoanSettings, - FulfillmentInfo, + DirectFulfillment, HoldInfo, LoanInfo, PatronActivityCirculationAPI, @@ -488,7 +488,7 @@ def fulfill( password: str, pool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: + ) -> DirectFulfillment: """Get the actual resource file to the patron.""" if ( delivery_mechanism.delivery_mechanism.drm_scheme @@ -513,15 +513,9 @@ def fulfill( response.content, exc_info=e, ) - return FulfillmentInfo( - pool.collection, - DataSource.BIBLIOTHECA, - pool.identifier.type, - pool.identifier.identifier, - content_link=None, - content_type=content_type or response.headers.get("Content-Type"), + return DirectFulfillment( content=content, - content_expires=None, + content_type=content_type or response.headers.get("Content-Type"), ) def get_fulfillment_file(self, patron_id, bibliotheca_id): diff --git a/src/palace/manager/api/circulation.py b/src/palace/manager/api/circulation.py index c9da7c6ff..c26151993 100644 --- a/src/palace/manager/api/circulation.py +++ b/src/palace/manager/api/circulation.py @@ -5,9 +5,10 @@ import logging from abc import ABC, abstractmethod from collections.abc import Iterable, Mapping -from typing import Any, Literal, TypeVar +from typing import Literal, TypeVar import flask +import requests from flask import Response from flask_babel import lazy_gettext as _ from pydantic import PositiveInt @@ -57,8 +58,8 @@ from palace.manager.sqlalchemy.model.resource import Resource from palace.manager.sqlalchemy.util import get_one from palace.manager.util.datetime_helpers import utc_now +from palace.manager.util.http import HTTP, BadResponseException from palace.manager.util.log import LoggerMixin -from palace.manager.util.problem_detail import ProblemDetail class CirculationInfo: @@ -214,213 +215,116 @@ def apply( return lpdm -class FulfillmentInfo(CirculationInfo): - """A record of a technique that can be used *right now* to fulfill - a loan. +class Fulfillment(ABC): + """ + Represents a method of fulfilling a loan. """ - def __init__( - self, - collection: Collection | int | None, - data_source_name: str | DataSource | None, - identifier_type: str | None, - identifier: str | None, - content_link: str | None, - content_type: str | None, - content: str | None, - content_expires: datetime.datetime | None, - content_link_redirect: bool = False, - ) -> None: - """Constructor. - - One and only one of `content_link` and `content` should be - provided. - - :param collection: A Collection object explaining which Collection - the loan is found in. - :param identifier_type: A possible value for Identifier.type indicating - a type of identifier such as ISBN. - :param identifier: A possible value for Identifier.identifier containing - the identifier used to designate the item beinf fulfilled. - :param content_link: A "next step" URL towards fulfilling the - work. This may be a link to an ACSM file, a - streaming-content web application, a direct download, etc. - :param content_type: Final media type of the content, once acquired. - E.g. EPUB_MEDIA_TYPE or - Representation.TEXT_HTML_MEDIA_TYPE + DeliveryMechanism.STREAMING_PROFILE - :param content: "Next step" content to be served. This may be - the actual content of the item on loan (in which case its - is of the type mentioned in `content_type`) or an - intermediate document such as an ACSM file or audiobook - manifest (in which case its media type will differ from - `content_type`). - :param content_expires: A time after which the "next step" - link or content will no longer be usable. - :param content_link_redirect: Force the API layer to redirect the client to - the content_link + @abstractmethod + def response(self) -> Response: """ - super().__init__(collection, data_source_name, identifier_type, identifier) - self._content_link = content_link - self._content_type = content_type - self._content = content - self._content_expires = content_expires - self.content_link_redirect = content_link_redirect + Return a Flask Response object that can be used to fulfill a loan. + """ + ... - def __repr__(self) -> str: - if self.content: - blength = len(self.content) - else: - blength = 0 - return ( - "" - % ( - self.content_link, - self.content_type, - blength, - self.fd(self.content_expires), - self.content_link_redirect, - ) - ) - @property - def as_response(self) -> Response | ProblemDetail | None: - """Bypass the normal process of creating a Flask Response. +class UrlFulfillment(Fulfillment, ABC): + """ + Represents a method of fulfilling a loan that has a URL to an external resource. + """ - :return: A Response object, or None if you're okay with the - normal process. - """ - return None + def __init__(self, content_link: str, content_type: str | None = None) -> None: + self.content_link = content_link + self.content_type = content_type - @property - def content_link(self) -> str | None: - return self._content_link + def __repr__(self) -> str: + repr_data = [f"content_link: {self.content_link}"] + if self.content_type: + repr_data.append(f"content_type: {self.content_type}") + return f"<{self.__class__.__name__}: {', '.join(repr_data)}>" - @content_link.setter - def content_link(self, value: str | None) -> None: - self._content_link = value - @property - def content_type(self) -> str | None: - return self._content_type +class DirectFulfillment(Fulfillment): + """ + Represents a method of fulfilling a loan by directly serving some content + that we know about locally. + """ - @content_type.setter - def content_type(self, value: str | None) -> None: - self._content_type = value + def __init__(self, content: str, content_type: str | None) -> None: + self.content = content + self.content_type = content_type - @property - def content(self) -> str | None: - return self._content + def response(self) -> Response: + return Response(self.content, content_type=self.content_type) - @content.setter - def content(self, value: str | None) -> None: - self._content = value + def __repr__(self) -> str: + length = len(self.content) + return f"<{self.__class__.__name__}: content_type: {self.content_type}, content: {length} bytes>" - @property - def content_expires(self) -> datetime.datetime | None: - return self._content_expires - @content_expires.setter - def content_expires(self, value: datetime.datetime | None) -> None: - self._content_expires = value +class RedirectFulfillment(UrlFulfillment): + """ + Fulfill a loan by redirecting the client to a URL. + """ + + def response(self) -> Response: + return Response( + f"Redirecting to {self.content_link} ...", + status=302, + headers={"Location": self.content_link}, + content_type="text/plain", + ) -class APIAwareFulfillmentInfo(FulfillmentInfo, ABC): - """This that acts like FulfillmentInfo but is prepared to make an API - request on demand to get data, rather than having all the data - ready right now. +class FetchFulfillment(UrlFulfillment, LoggerMixin): + """ + Fulfill a loan by fetching a URL and returning the content. This should be + avoided for large files, since it will be slow and unreliable as well as + blocking the server. - This class is useful in situations where generating a full - FulfillmentInfo object would be costly. We only want to incur that - cost when the patron wants to fulfill this title and is not just - looking at their loans. + In some cases for small files like ACSM or LCPL files, the server may be + the only entity that can fetch the file, so we fetch it and then return it + to the client. """ def __init__( self, - api: CirculationApiType, - data_source_name: str | None, - identifier_type: str | None, - identifier: str | None, - key: Any, + content_link: str, + content_type: str | None = None, + *, + include_headers: dict[str, str] | None = None, + allowed_response_codes: list[str | int] | None = None, ) -> None: - """Constructor. - - :param api: An object that knows how to make API requests. - :param data_source_name: The name of the data source that's - offering to fulfill a book. - :param identifier: The Identifier of the book being fulfilled. - :param key: Any special data, such as a license key, which must - be used to fulfill the book. - """ - super().__init__( - api.collection, - data_source_name, - identifier_type, - identifier, - None, - None, - None, - None, + super().__init__(content_link, content_type) + self.include_headers = include_headers or {} + self.allowed_response_codes = allowed_response_codes or [] + + def get(self, url: str) -> requests.Response: + return HTTP.get_with_timeout( + url, + headers=self.include_headers, + allowed_response_codes=self.allowed_response_codes, + allow_redirects=True, ) - self.api = api - self.key = key - self._fetched = False - self.content_link_redirect = False - - def fetch(self) -> None: - """It's time to tell the API that we want to fulfill this book.""" - if self._fetched: - # We already sent the API request.. - return None - self.do_fetch() - self._fetched = True - - @abstractmethod - def do_fetch(self) -> None: - """Actually make the API request. - - When implemented, this method must set values for some or all - of _content_link, _content_type, _content, and - _content_expires. - """ - ... - - @property - def content_link(self) -> str | None: - self.fetch() - return self._content_link - - @content_link.setter - def content_link(self, value: str | None) -> None: - raise NotImplementedError() - - @property - def content_type(self) -> str | None: - self.fetch() - return self._content_type - - @content_type.setter - def content_type(self, value: str | None) -> None: - raise NotImplementedError() - - @property - def content(self) -> str | None: - self.fetch() - return self._content + def response(self) -> Response: + try: + response = self.get(self.content_link) + except BadResponseException as ex: + response = ex.response + self.log.exception( + f"Error fulfilling loan. Bad response from: {self.content_link}. " + f"Status code: {response.status_code}. " + f"Response: {response.text}." + ) + raise - @content.setter - def content(self, value: str | None) -> None: - raise NotImplementedError() + headers = dict(response.headers) - @property - def content_expires(self) -> datetime.datetime | None: - self.fetch() - return self._content_expires + if self.content_type: + headers["Content-Type"] = self.content_type - @content_expires.setter - def content_expires(self, value: datetime.datetime | None) -> None: - raise NotImplementedError() + return Response(response.content, status=response.status_code, headers=headers) class LoanInfo(CirculationInfo): @@ -434,7 +338,7 @@ def __init__( identifier: str | None, start_date: datetime.datetime | None, end_date: datetime.datetime | None, - fulfillment_info: FulfillmentInfo | None = None, + fulfillment: Fulfillment | None = None, external_identifier: str | None = None, locked_to: DeliveryMechanismInfo | None = None, ): @@ -442,7 +346,7 @@ def __init__( :param start_date: A datetime reflecting when the patron borrowed the book. :param end_date: A datetime reflecting when the checked-out book is due. - :param fulfillment_info: A FulfillmentInfo object representing an + :param fulfillment: A Fulfillment object representing an active attempt to fulfill the loan. :param locked_to: A DeliveryMechanismInfo object representing the delivery mechanism to which this loan is 'locked'. @@ -450,13 +354,13 @@ def __init__( super().__init__(collection, data_source_name, identifier_type, identifier) self.start_date = start_date self.end_date = end_date - self.fulfillment_info = fulfillment_info + self.fulfillment = fulfillment self.locked_to = locked_to self.external_identifier = external_identifier def __repr__(self) -> str: - if self.fulfillment_info: - fulfillment = " Fulfilled by: " + repr(self.fulfillment_info) + if self.fulfillment: + fulfillment = " Fulfilled by: " + repr(self.fulfillment) else: fulfillment = "" f = "%Y/%m/%d" @@ -707,7 +611,7 @@ def fulfill( pin: str, licensepool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: + ) -> Fulfillment: """Get the actual resource file to the patron.""" ... @@ -1395,7 +1299,7 @@ def fulfill( pin: str, licensepool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: + ) -> Fulfillment: """Fulfil a book that a patron has previously checked out. :param delivery_mechanism: A LicensePoolDeliveryMechanism @@ -1404,10 +1308,9 @@ def fulfill( mechanism, this parameter is ignored and the previously used mechanism takes precedence. - :return: A FulfillmentInfo object. + :return: A Fulfillment object. """ - fulfillment: FulfillmentInfo loan = get_one( self._db, Loan, @@ -1442,7 +1345,7 @@ def fulfill( licensepool, delivery_mechanism=delivery_mechanism, ) - if not fulfillment or not (fulfillment.content_link or fulfillment.content): + if not fulfillment: raise NoAcceptableFormat() # Send out an analytics event to record the fact that diff --git a/src/palace/manager/api/controller/loan.py b/src/palace/manager/api/controller/loan.py index f7bfd3973..d03bc446c 100644 --- a/src/palace/manager/api/controller/loan.py +++ b/src/palace/manager/api/controller/loan.py @@ -1,14 +1,13 @@ from __future__ import annotations -from typing import Any - import flask -from flask import Response, redirect +from flask import Response from flask_babel import lazy_gettext as _ from lxml import etree from pydantic import parse_obj_as from werkzeug import Response as wkResponse +from palace.manager.api.circulation import UrlFulfillment from palace.manager.api.circulation_exceptions import ( CirculationException, RemoteInitiatedServerError, @@ -28,19 +27,15 @@ from palace.manager.core.problem_details import INTERNAL_SERVER_ERROR from palace.manager.feed.acquisition import OPDSAcquisitionFeed from palace.manager.service.redis.models.patron_activity import PatronActivity -from palace.manager.sqlalchemy.model.datasource import DataSource from palace.manager.sqlalchemy.model.library import Library from palace.manager.sqlalchemy.model.licensing import ( - DeliveryMechanism, LicensePool, LicensePoolDeliveryMechanism, ) from palace.manager.sqlalchemy.model.patron import Hold, Loan, Patron -from palace.manager.sqlalchemy.model.resource import Representation from palace.manager.util.flask_util import OPDSEntryResponse -from palace.manager.util.http import RemoteIntegrationException from palace.manager.util.opds_writer import OPDSFeed -from palace.manager.util.problem_detail import ProblemDetail +from palace.manager.util.problem_detail import BaseProblemDetailException, ProblemDetail class LoanController(CirculationManagerController): @@ -273,7 +268,6 @@ def fulfill( self, license_pool_id: int, mechanism_id: int | None = None, - do_get: Any | None = None, ) -> wkResponse | ProblemDetail: """Fulfill a book that has already been checked out, or which can be fulfilled with no active loan. @@ -286,8 +280,6 @@ def fulfill( :param license_pool_id: Database ID of a LicensePool. :param mechanism_id: Database ID of a DeliveryMechanism. """ - do_get = do_get or Representation.simple_http_get - # Unlike most controller methods, this one has different # behavior whether or not the patron is authenticated. This is # why we're about to do something we don't usually do--call @@ -368,22 +360,12 @@ def fulfill( except (CirculationException, RemoteInitiatedServerError) as e: return e.problem_detail - # A subclass of FulfillmentInfo may want to bypass the whole - # response creation process. - response = fulfillment.as_response - if response is not None: - return response - - headers = dict() - encoding_header = dict() - if ( - fulfillment.data_source_name == DataSource.ENKI - and mechanism.delivery_mechanism.drm_scheme_media_type - == DeliveryMechanism.NO_DRM + # TODO: This should really be turned into its own Fulfillment class, + # so each integration can choose when to return a feed response like + # this, and when to return a direct response. + if mechanism.delivery_mechanism.is_streaming and isinstance( + fulfillment, UrlFulfillment ): - encoding_header["Accept-Encoding"] = "deflate" - - if mechanism.delivery_mechanism.is_streaming: # If this is a streaming delivery mechanism, create an OPDS entry # with a fulfillment link to the streaming reader url. feed = OPDSAcquisitionFeed.single_entry_loans_feed( @@ -397,38 +379,16 @@ def fulfill( return feed else: content = etree.tostring(feed) - status_code = 200 - headers["Content-Type"] = OPDSFeed.ACQUISITION_FEED_TYPE - elif fulfillment.content_link_redirect is True and fulfillment.content_link: - # The fulfillment API has asked us to not be a proxy and instead redirect the client directly - return redirect(fulfillment.content_link) - else: - content = fulfillment.content - if fulfillment.content_link: - # If we have a link to the content on a remote server, web clients may not - # be able to access it if the remote server does not support CORS requests. - - # If the pool is open access though, the web client can link directly to the - # file to download it, so it's safe to redirect. - if requested_license_pool.open_access: - return redirect(fulfillment.content_link) - - # Otherwise, we need to fetch the content and return it instead - # of redirecting to it, since it may be downloaded through an - # indirect acquisition link. - try: - status_code, resp_headers, content = do_get( - fulfillment.content_link, headers=encoding_header - ) - headers = dict(resp_headers) - except RemoteIntegrationException as e: - return e.problem_detail - else: - status_code = 200 - if fulfillment.content_type: - headers["Content-Type"] = fulfillment.content_type + return Response( + response=content, + status=200, + content_type=OPDSFeed.ACQUISITION_FEED_TYPE, + ) - return Response(response=content, status=status_code, headers=headers) + try: + return fulfillment.response() + except BaseProblemDetailException as e: + return e.problem_detail def can_fulfill_without_loan( self, diff --git a/src/palace/manager/api/enki.py b/src/palace/manager/api/enki.py index 2bf2e8527..6aaa89e0a 100644 --- a/src/palace/manager/api/enki.py +++ b/src/palace/manager/api/enki.py @@ -15,7 +15,7 @@ from palace.manager.api.circulation import ( BaseCirculationAPI, BaseCirculationApiSettings, - FulfillmentInfo, + FetchFulfillment, HoldInfo, LoanInfo, PatronActivityCirculationAPI, @@ -473,7 +473,7 @@ def fulfill( pin: str, licensepool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: + ) -> FetchFulfillment: """Get the actual resource file to the patron.""" book_id = licensepool.identifier.identifier enki_library_id = self.enki_library_id(patron.library) @@ -509,15 +509,19 @@ def fulfill( drm_type = mechanism.drm_scheme break - return FulfillmentInfo( - licensepool.collection, - licensepool.data_source.name, - licensepool.identifier.type, - licensepool.identifier.identifier, + # TODO: I think in this case we should be returning a RedirectFulfillment + # instead of a FetchFulfillment. I think we are setting the "Accept-Encoding" + # header here to minimize the size of the response, but I think we would be + # better off just returning the URL and letting the client handle the request. + # However I need to test this case, so I'm leaving it as is for now. + headers = {} + if drm_type == DeliveryMechanism.NO_DRM: + headers["Accept-Encoding"] = "deflate" + + return FetchFulfillment( content_link=url, content_type=drm_type, - content=None, - content_expires=expires, + include_headers=headers, ) def parse_fulfill_result( @@ -587,7 +591,7 @@ def parse_patron_loans(self, checkout_data: Mapping[str, Any]) -> LoanInfo: enki_id, start_date=start_date, end_date=end_date, - fulfillment_info=None, + fulfillment=None, ) def parse_patron_holds(self, hold_data: Mapping[str, Any]) -> HoldInfo | None: diff --git a/src/palace/manager/api/odl/api.py b/src/palace/manager/api/odl/api.py index 4dda6f21f..4c247a925 100644 --- a/src/palace/manager/api/odl/api.py +++ b/src/palace/manager/api/odl/api.py @@ -16,10 +16,13 @@ from palace.manager.api.circulation import ( BaseCirculationAPI, - FulfillmentInfo, + DirectFulfillment, + FetchFulfillment, + Fulfillment, HoldInfo, LoanInfo, PatronActivityCirculationAPI, + RedirectFulfillment, ) from palace.manager.api.circulation_exceptions import ( AlreadyCheckedOut, @@ -468,7 +471,7 @@ def fulfill( pin: str, licensepool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: + ) -> Fulfillment: """Get the actual resource file to the patron.""" _db = Session.object_session(patron) @@ -514,23 +517,14 @@ def _find_content_link_and_type( def _unlimited_access_fulfill( self, loan: Loan, delivery_mechanism: LicensePoolDeliveryMechanism - ) -> FulfillmentInfo: + ) -> Fulfillment: licensepool = loan.license_pool fulfillment = self._find_matching_delivery_mechanism( delivery_mechanism.delivery_mechanism, licensepool ) content_link = fulfillment.resource.representation.public_url content_type = fulfillment.resource.representation.media_type - return FulfillmentInfo( - licensepool.collection, - licensepool.data_source.name, - licensepool.identifier.type, - licensepool.identifier.identifier, - content_link, - content_type, - None, - None, - ) + return RedirectFulfillment(content_link, content_type) def _find_matching_delivery_mechanism( self, requested_delivery_mechanism: DeliveryMechanism, licensepool: LicensePool @@ -549,7 +543,7 @@ def _find_matching_delivery_mechanism( def _lcp_fulfill( self, loan: Loan, delivery_mechanism: LicensePoolDeliveryMechanism - ) -> FulfillmentInfo: + ) -> Fulfillment: doc = self.get_license_status_document(loan) status = doc.get("status") @@ -570,20 +564,14 @@ def _lcp_fulfill( links, delivery_mechanism.delivery_mechanism.drm_scheme ) - return FulfillmentInfo( - loan.license_pool.collection, - loan.license_pool.data_source.name, - loan.license_pool.identifier.type, - loan.license_pool.identifier.identifier, - content_link, - content_type, - None, - expires, - ) + if content_link is None or content_type is None: + raise CannotFulfill() + + return FetchFulfillment(content_link, content_type) def _bearer_token_fulfill( self, loan: Loan, delivery_mechanism: LicensePoolDeliveryMechanism - ) -> FulfillmentInfo: + ) -> Fulfillment: licensepool = loan.license_pool fulfillment_mechanism = self._find_matching_delivery_mechanism( delivery_mechanism.delivery_mechanism, licensepool @@ -609,22 +597,16 @@ def _bearer_token_fulfill( location=fulfillment_mechanism.resource.url, ) - return FulfillmentInfo( - licensepool.collection, - licensepool.data_source.name, - licensepool.identifier.type, - licensepool.identifier.identifier, - content_link=None, + return DirectFulfillment( content_type=DeliveryMechanism.BEARER_TOKEN, content=json.dumps(token_document), - content_expires=self._session_token.expires, ) def _fulfill( self, loan: Loan, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: + ) -> Fulfillment: if loan.license_pool.open_access or loan.license_pool.unlimited_access: if ( delivery_mechanism.delivery_mechanism.drm_scheme diff --git a/src/palace/manager/api/opds_for_distributors.py b/src/palace/manager/api/opds_for_distributors.py index b0ef9ca29..30505c363 100644 --- a/src/palace/manager/api/opds_for_distributors.py +++ b/src/palace/manager/api/opds_for_distributors.py @@ -9,7 +9,11 @@ from flask_babel import lazy_gettext as _ from sqlalchemy.orm import Session -from palace.manager.api.circulation import BaseCirculationAPI, FulfillmentInfo, LoanInfo +from palace.manager.api.circulation import ( + BaseCirculationAPI, + DirectFulfillment, + LoanInfo, +) from palace.manager.api.circulation_exceptions import ( CannotFulfill, DeliveryMechanismError, @@ -286,10 +290,10 @@ def fulfill( pin: str, licensepool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: + ) -> DirectFulfillment: """Retrieve a bearer token that can be used to download the book. - :return: a FulfillmentInfo object. + :return: a DirectFulfillment object. """ if ( delivery_mechanism.delivery_mechanism.drm_scheme @@ -339,15 +343,9 @@ def fulfill( location=url, ) - return FulfillmentInfo( - licensepool.collection, - licensepool.data_source.name, - licensepool.identifier.type, - licensepool.identifier.identifier, - content_link=None, - content_type=DeliveryMechanism.BEARER_TOKEN, + return DirectFulfillment( content=json.dumps(token_document), - content_expires=credential.expires, + content_type=DeliveryMechanism.BEARER_TOKEN, ) def release_hold(self, patron: Patron, pin: str, licensepool: LicensePool) -> None: diff --git a/src/palace/manager/api/overdrive.py b/src/palace/manager/api/overdrive.py index 34e61b5a1..8913151e7 100644 --- a/src/palace/manager/api/overdrive.py +++ b/src/palace/manager/api/overdrive.py @@ -35,10 +35,12 @@ BaseCirculationEbookLoanSettings, CirculationInternalFormatsMixin, DeliveryMechanismInfo, - FulfillmentInfo, + FetchFulfillment, + Fulfillment, HoldInfo, LoanInfo, PatronActivityCirculationAPI, + RedirectFulfillment, ) from palace.manager.api.circulation_exceptions import ( CannotFulfill, @@ -1224,7 +1226,7 @@ def fulfill( pin: str, licensepool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: + ) -> Fulfillment: """Get the actual resource file to the patron.""" internal_format = self.internal_format(delivery_mechanism) if licensepool.identifier.identifier is None: @@ -1236,9 +1238,9 @@ def fulfill( result = self.get_fulfillment_link( patron, pin, licensepool.identifier.identifier, internal_format ) - if isinstance(result, FulfillmentInfo): + if isinstance(result, Fulfillment): # The fulfillment process was short-circuited, probably - # by the creation of an OverdriveManifestFulfillmentInfo. + # by the creation of an OverdriveManifestFulfillment. return result url, media_type = result @@ -1269,21 +1271,20 @@ def fulfill( "ebook-pdf-open", ] - return FulfillmentInfo( - licensepool.collection, - licensepool.data_source.name, - licensepool.identifier.type, - licensepool.identifier.identifier, - content_link=url, - content_type=media_type, - content=None, - content_expires=None, - content_link_redirect=fulfillment_force_redirect, - ) + if fulfillment_force_redirect: + return RedirectFulfillment( + content_link=url, + content_type=media_type, + ) + else: + return FetchFulfillment( + content_link=url, + content_type=media_type, + ) def get_fulfillment_link( self, patron: Patron, pin: str | None, overdrive_id: str, format_type: str - ) -> OverdriveManifestFulfillmentInfo | tuple[str, str]: + ) -> OverdriveManifestFulfillment | tuple[str, str]: """Get the link to the ACSM or manifest for an existing loan.""" try: loan = self.get_loan(patron, pin, overdrive_id) @@ -1344,10 +1345,11 @@ def get_fulfillment_link( pin, is_fulfillment=True, ).credential - return OverdriveManifestFulfillmentInfo( - self.collection, + # The credential should never be None, but mypy doesn't know that, so + # we assert to be safe. + assert fulfillment_access_token is not None + return OverdriveManifestFulfillment( download_link, - overdrive_id, scope_string, fulfillment_access_token, ) @@ -3150,36 +3152,17 @@ def explain_advantage_collection(self, collection): break -class OverdriveManifestFulfillmentInfo(FulfillmentInfo): - def __init__( - self, collection, content_link, overdrive_identifier, scope_string, access_token - ): - """Constructor. - - Most of the arguments to the superconstructor can be assumed, - and none of them matter all that much, since this class - overrides the normal process by which a FulfillmentInfo becomes - a Flask response. - """ - super().__init__( - collection=collection, - data_source_name=DataSource.OVERDRIVE, - identifier_type=Identifier.OVERDRIVE_ID, - identifier=overdrive_identifier, - content_link=content_link, - content_type=None, - content=None, - content_expires=None, - ) +class OverdriveManifestFulfillment(RedirectFulfillment): + def __init__(self, content_link: str, scope_string: str, access_token: str) -> None: + super().__init__(content_link) self.scope_string = scope_string self.access_token = access_token - @property - def as_response(self): + def response(self) -> flask.Response: headers = { "Location": self.content_link, "X-Overdrive-Scope": self.scope_string, "X-Overdrive-Patron-Authorization": f"Bearer {self.access_token}", - "Content-Type": self.content_type or "text/plain", + "Content-Type": "text/plain", } return flask.Response("", 302, headers) diff --git a/src/palace/manager/core/opds2_import.py b/src/palace/manager/core/opds2_import.py index cf13d7427..e8f0fdb50 100644 --- a/src/palace/manager/core/opds2_import.py +++ b/src/palace/manager/core/opds2_import.py @@ -23,7 +23,7 @@ ) from webpub_manifest_parser.utils import encode, first_or_default -from palace.manager.api.circulation import FulfillmentInfo +from palace.manager.api.circulation import RedirectFulfillment from palace.manager.api.circulation_exceptions import CannotFulfill from palace.manager.core.coverage import CoverageFailure from palace.manager.core.metadata_layer import ( @@ -220,14 +220,11 @@ def get_authentication_token( return token def fulfill_token_auth( - self, patron: Patron, licensepool: LicensePool, fulfillment: FulfillmentInfo - ) -> FulfillmentInfo: - if not fulfillment.content_link: - self.log.warning( - "No content link found in fulfillment, unable to fulfill via OPDS2 token auth." - ) - return fulfillment - + self, + patron: Patron, + licensepool: LicensePool, + fulfillment: RedirectFulfillment, + ) -> RedirectFulfillment: templated = URITemplate(fulfillment.content_link) if "authentication_token" not in templated.variable_names: self.log.warning( @@ -245,7 +242,6 @@ def fulfill_token_auth( patron, licensepool.data_source, self.token_auth_configuration ) fulfillment.content_link = templated.expand(authentication_token=token) - fulfillment.content_link_redirect = True return fulfillment def fulfill( @@ -254,13 +250,11 @@ def fulfill( pin: str, licensepool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: - fufillment_info = super().fulfill(patron, pin, licensepool, delivery_mechanism) + ) -> RedirectFulfillment: + fulfillment = super().fulfill(patron, pin, licensepool, delivery_mechanism) if self.token_auth_configuration: - fufillment_info = self.fulfill_token_auth( - patron, licensepool, fufillment_info - ) - return fufillment_info + fulfillment = self.fulfill_token_auth(patron, licensepool, fulfillment) + return fulfillment class OPDS2Importer(BaseOPDSImporter[OPDS2ImporterSettings]): diff --git a/src/palace/manager/core/opds_import.py b/src/palace/manager/core/opds_import.py index 3b93bd160..a5f7b7c3c 100644 --- a/src/palace/manager/core/opds_import.py +++ b/src/palace/manager/core/opds_import.py @@ -25,9 +25,9 @@ from palace.manager.api.circulation import ( BaseCirculationAPI, BaseCirculationApiSettings, - FulfillmentInfo, HoldInfo, LoanInfo, + RedirectFulfillment, ) from palace.manager.api.circulation_exceptions import ( CurrentlyAvailable, @@ -248,8 +248,8 @@ def update_availability(self, licensepool: LicensePool) -> None: pass def fulfill_saml_wayfless( - self, template: str, patron: Patron, fulfillment: FulfillmentInfo - ) -> FulfillmentInfo: + self, template: str, patron: Patron, fulfillment: RedirectFulfillment + ) -> RedirectFulfillment: self.log.debug(f"WAYFless acquisition link template: {template}") db = Session.object_session(patron) @@ -277,17 +277,10 @@ def fulfill_saml_wayfless( SAMLWAYFlessConstants.IDP_PLACEHOLDER, urllib.parse.quote(saml_subject.idp, safe=""), ) - if fulfillment.content_link is None: - self.log.warning( - f"Fulfillment {fulfillment} has no content link, unable to transform it" - ) - content_link = "" - else: - content_link = fulfillment.content_link acquisition_link = acquisition_link.replace( SAMLWAYFlessConstants.ACQUISITION_LINK_PLACEHOLDER, - urllib.parse.quote(content_link, safe=""), + urllib.parse.quote(fulfillment.content_link, safe=""), ) self.log.debug( @@ -303,7 +296,7 @@ def fulfill( pin: str, licensepool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: + ) -> RedirectFulfillment: requested_mechanism = delivery_mechanism.delivery_mechanism fulfillment = None for lpdm in licensepool.delivery_mechanisms: @@ -330,23 +323,17 @@ def fulfill( content_link = rep.public_url media_type = rep.media_type - fulfillment_info = FulfillmentInfo( - licensepool.collection, - licensepool.data_source.name, - identifier_type=licensepool.identifier.type, - identifier=licensepool.identifier.identifier, + fulfillment_strategy = RedirectFulfillment( content_link=content_link, content_type=media_type, - content=None, - content_expires=None, ) if self.saml_wayfless_url_template: - fulfillment_info = self.fulfill_saml_wayfless( - self.saml_wayfless_url_template, patron, fulfillment_info + fulfillment_strategy = self.fulfill_saml_wayfless( + self.saml_wayfless_url_template, patron, fulfillment_strategy ) - return fulfillment_info + return fulfillment_strategy def checkout( self, diff --git a/src/palace/manager/feed/acquisition.py b/src/palace/manager/feed/acquisition.py index 7579634b9..f4db8b6c3 100644 --- a/src/palace/manager/feed/acquisition.py +++ b/src/palace/manager/feed/acquisition.py @@ -44,7 +44,7 @@ from palace.manager.util.problem_detail import ProblemDetail if TYPE_CHECKING: - from palace.manager.api.circulation import CirculationAPI, FulfillmentInfo + from palace.manager.api.circulation import CirculationAPI, UrlFulfillment from palace.manager.sqlalchemy.model.lane import WorkList @@ -525,7 +525,7 @@ def single_entry_loans_feed( circulation: Any, item: LicensePool | Loan | Hold | None, annotator: LibraryAnnotator | None = None, - fulfillment: FulfillmentInfo | None = None, + fulfillment: UrlFulfillment | None = None, **response_kwargs: Any, ) -> OPDSEntryResponse | ProblemDetail | None: """A single entry as a standalone feed specific to a patron""" diff --git a/src/palace/manager/feed/annotator/circulation.py b/src/palace/manager/feed/annotator/circulation.py index 477fe4079..fad5e1559 100644 --- a/src/palace/manager/feed/annotator/circulation.py +++ b/src/palace/manager/feed/annotator/circulation.py @@ -15,7 +15,11 @@ from palace.manager.api.adobe_vendor_id import AuthdataUtility from palace.manager.api.annotations import AnnotationWriter -from palace.manager.api.circulation import BaseCirculationAPI, CirculationAPI +from palace.manager.api.circulation import ( + BaseCirculationAPI, + CirculationAPI, + UrlFulfillment, +) from palace.manager.api.config import Configuration from palace.manager.api.lanes import DynamicLane from palace.manager.api.metadata.novelist import NoveListAPI @@ -194,7 +198,7 @@ def __init__( lane: WorkList | None, active_loans_by_work: dict[Work, Loan] | None = None, active_holds_by_work: dict[Work, Hold] | None = None, - active_fulfillments_by_work: dict[Work, Any] | None = None, + active_fulfillments_by_work: dict[Work, UrlFulfillment] | None = None, hidden_content_types: list[str] | None = None, analytics: Analytics = Provide[Services.analytics.analytics], ) -> None: @@ -404,7 +408,7 @@ def acquisition_links( active_license_pool: LicensePool | None, active_loan: Loan | None, active_hold: Hold | None, - active_fulfillment: Any | None, + active_fulfillment: UrlFulfillment | None, identifier: Identifier, can_hold: bool = True, can_revoke_hold: bool = True, @@ -502,7 +506,10 @@ def acquisition_links( url = active_fulfillment.content_link rel = OPDSFeed.ACQUISITION_REL link_tag = self.acquisition_link( - rel=rel, href=url, types=[type], active_loan=active_loan + rel=rel, + href=url, + types=[type] if type else None, + active_loan=active_loan, ) fulfill_links.append(link_tag) @@ -695,7 +702,7 @@ def __init__( patron: Patron | None = None, active_loans_by_work: dict[Work, Loan] | None = None, active_holds_by_work: dict[Work, Hold] | None = None, - active_fulfillments_by_work: dict[Work, Any] | None = None, + active_fulfillments_by_work: dict[Work, UrlFulfillment] | None = None, facet_view: str = "feed", top_level_title: str = "All Books", library_identifies_patrons: bool = True, diff --git a/tests/manager/api/controller/test_loan.py b/tests/manager/api/controller/test_loan.py index cf7462da2..e4474ce8a 100644 --- a/tests/manager/api/controller/test_loan.py +++ b/tests/manager/api/controller/test_loan.py @@ -10,14 +10,17 @@ from flask import url_for from werkzeug import Response as wkResponse -from palace.manager.api.axis import Axis360API, Axis360FulfillmentInfo +from palace.manager.api.axis import Axis360API, Axis360Fulfillment from palace.manager.api.bibliotheca import BibliothecaAPI from palace.manager.api.circulation import ( BaseCirculationAPI, CirculationAPI, - FulfillmentInfo, + DirectFulfillment, + FetchFulfillment, + Fulfillment, HoldInfo, LoanInfo, + RedirectFulfillment, ) from palace.manager.api.circulation_exceptions import ( AlreadyOnHold, @@ -69,7 +72,7 @@ from tests.fixtures.redis import RedisFixture from tests.fixtures.services import ServicesFixture from tests.mocks.circulation import MockPatronActivityCirculationAPI -from tests.mocks.mock import MockRepresentationHTTPClient +from tests.mocks.mock import MockHTTPClient class LoanFixture(CirculationControllerFixture): @@ -389,18 +392,12 @@ def test_borrow_success( assert loan_fixture.mech1.resource.representation.url is not None # Now let's try to fulfill the loan using the first delivery mechanism. - fulfillment = FulfillmentInfo( - loan_fixture.pool.collection, - loan_fixture.pool.data_source, - loan_fixture.pool.identifier.type, - loan_fixture.pool.identifier.identifier, + redirect = RedirectFulfillment( content_link=fulfillable_mechanism.resource.representation.public_url, content_type=fulfillable_mechanism.resource.representation.media_type, - content=None, - content_expires=None, ) loan_fixture.manager.d_circulation.queue_fulfill( - loan_fixture.pool, fulfillment + loan_fixture.pool, redirect ) fulfill_response = loan_fixture.manager.loans.fulfill( @@ -423,29 +420,23 @@ def test_borrow_success( # external request to obtain the book. loan_fixture.pool.open_access = False - http = MockRepresentationHTTPClient() + http = MockHTTPClient() - fulfillment = FulfillmentInfo( - loan_fixture.pool.collection, - loan_fixture.pool.data_source, - loan_fixture.pool.identifier.type, - loan_fixture.pool.identifier.identifier, + assert fulfillable_mechanism.resource.url is not None + fetch = FetchFulfillment( content_link=fulfillable_mechanism.resource.url, content_type=fulfillable_mechanism.resource.representation.media_type, - content=None, - content_expires=None, ) # Now that we've set a mechanism, we can fulfill the loan # again without specifying a mechanism. - loan_fixture.manager.d_circulation.queue_fulfill( - loan_fixture.pool, fulfillment - ) + loan_fixture.manager.d_circulation.queue_fulfill(loan_fixture.pool, fetch) http.queue_response(200, content="I am an ACSM file") - fulfill_response = loan_fixture.manager.loans.fulfill( - loan_fixture.pool_id, do_get=http.do_get - ) + with http.patch(): + fulfill_response = loan_fixture.manager.loans.fulfill( + loan_fixture.pool_id + ) assert isinstance(fulfill_response, wkResponse) assert 200 == fulfill_response.status_code assert "I am an ACSM file" == fulfill_response.get_data(as_text=True) @@ -465,16 +456,16 @@ def test_borrow_success( ) # If the remote server fails, we get a problem detail. - def doomed_get(url, headers, **kwargs): - raise RemoteIntegrationException("fulfill service", "Error!") + doomed_fulfillment = create_autospec(Fulfillment) + doomed_fulfillment.response.side_effect = RemoteIntegrationException( + "fulfill service", "Error!" + ) loan_fixture.manager.d_circulation.queue_fulfill( - loan_fixture.pool, fulfillment + loan_fixture.pool, doomed_fulfillment ) - fulfill_response = loan_fixture.manager.loans.fulfill( - loan_fixture.pool_id, do_get=doomed_get - ) + fulfill_response = loan_fixture.manager.loans.fulfill(loan_fixture.pool_id) assert isinstance(fulfill_response, ProblemDetail) assert 502 == fulfill_response.status_code @@ -559,16 +550,10 @@ def test_borrow_and_fulfill_with_streaming_delivery_mechanism( # Now let's try to fulfill the loan using the streaming mechanism. loan_fixture.manager.d_circulation.queue_fulfill( pool, - FulfillmentInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, + RedirectFulfillment( "http://streaming-content-link", Representation.TEXT_HTML_MEDIA_TYPE + DeliveryMechanism.STREAMING_PROFILE, - None, - None, ), ) fulfill_response = loan_fixture.manager.loans.fulfill( @@ -602,25 +587,20 @@ def test_borrow_and_fulfill_with_streaming_delivery_mechanism( assert None == loan.fulfillment # We can still use the other mechanism too. - http = MockRepresentationHTTPClient() + http = MockHTTPClient() http.queue_response(200, content="I am an ACSM file") loan_fixture.manager.d_circulation.queue_fulfill( pool, - FulfillmentInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, + FetchFulfillment( "http://other-content-link", Representation.TEXT_HTML_MEDIA_TYPE, - None, - None, ), ) - fulfill_response = loan_fixture.manager.loans.fulfill( - pool.id, mech1.delivery_mechanism.id, do_get=http.do_get - ) + with http.patch(): + fulfill_response = loan_fixture.manager.loans.fulfill( + pool.id, mech1.delivery_mechanism.id + ) assert isinstance(fulfill_response, wkResponse) assert 200 == fulfill_response.status_code @@ -630,16 +610,10 @@ def test_borrow_and_fulfill_with_streaming_delivery_mechanism( # But we can still fulfill the streaming mechanism again. loan_fixture.manager.d_circulation.queue_fulfill( pool, - FulfillmentInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, + RedirectFulfillment( "http://streaming-content-link", Representation.TEXT_HTML_MEDIA_TYPE + DeliveryMechanism.STREAMING_PROFILE, - None, - None, ), ) @@ -889,37 +863,31 @@ def test_fulfill(self, loan_fixture: LoanFixture): ) @pytest.mark.parametrize( - "as_response_value", + "response_value", [ Response(status=200, response="Here's your response"), Response(status=401, response="Error"), Response(status=500, response="Fault"), ], ) - def test_fulfill_returns_fulfillment_info_implementing_as_response( - self, as_response_value, loan_fixture: LoanFixture + def test_fulfill_returns_fulfillment( + self, response_value: Response, loan_fixture: LoanFixture ): - # If CirculationAPI.fulfill returns a FulfillmentInfo that - # defines as_response, the result of as_response is returned - # directly and the normal process of converting a FulfillmentInfo - # to a Flask response is skipped. - class MockFulfillmentInfo(FulfillmentInfo): - @property - def as_response(self): - return as_response_value + # When CirculationAPI.fulfill returns a Fulfillment, we + # simply return the result of Fulfillment.response() + class MockFulfillment(Fulfillment): + def __init__(self): + self.response_called = False + + def response(self) -> Response: + self.response_called = True + return response_value + + fulfillment = MockFulfillment() class MockCirculationAPI: def fulfill(self, *args, **kwargs): - return MockFulfillmentInfo( - loan_fixture.db.default_collection(), - None, - None, - None, - None, - None, - None, - None, - ) + return fulfillment controller = loan_fixture.manager.loans mock = MockCirculationAPI() @@ -937,9 +905,9 @@ def fulfill(self, *args, **kwargs): loan_fixture.pool_id, loan_fixture.mech2.delivery_mechanism.id ) - # The result of MockFulfillmentInfo.as_response was + # The result of MockFulfillment.response was # returned directly. - assert as_response_value == result + assert response_value == result def test_fulfill_without_active_loan(self, loan_fixture: LoanFixture): controller = loan_fixture.manager.loans @@ -982,7 +950,7 @@ def mock_authenticated_patron(): # However, if can_fulfill_without_loan returns True, then # fulfill() will be called. If fulfill() returns a - # FulfillmentInfo, then the title is fulfilled, with no loan + # Fulfillment, then the title is fulfilled, with no loan # having been created. # # To that end, we'll mock can_fulfill_without_loan and fulfill. @@ -990,15 +958,9 @@ def mock_can_fulfill_without_loan(*args, **kwargs): return True def mock_fulfill(*args, **kwargs): - return FulfillmentInfo( - loan_fixture.collection, - loan_fixture.pool.data_source.name, - loan_fixture.pool.identifier.type, - loan_fixture.pool.identifier.identifier, - None, - "text/html", - "here's your book", - utc_now(), + return DirectFulfillment( + content_type="text/html", + content="here's your book", ) # Now we're able to fulfill the book even without @@ -1027,9 +989,7 @@ def test_fulfill_without_single_item_feed(self, loan_fixture: LoanFixture): with patch( "palace.manager.api.controller.opds_feed.OPDSAcquisitionFeed.single_entry_loans_feed" ) as feed, patch.object(circulation, "fulfill") as fulfill: - # Complex setup - # The fulfillmentInfo should not be have response type - fulfill.return_value.as_response = None + fulfill.return_value = MagicMock(spec=RedirectFulfillment) # The single_item_feed must return this error feed.return_value = NOT_FOUND_ON_REMOTE # The content type needs to be streaming @@ -1068,16 +1028,9 @@ def test_no_drm_fulfill(self, loan_fixture: LoanFixture): # Mock out the flow api = MagicMock(spec=BaseCirculationAPI) - api.fulfill.return_value = FulfillmentInfo( - loan_fixture.db.default_collection(), - DataSource.OVERDRIVE, - "overdrive", - pool.identifier.identifier, + api.fulfill.return_value = RedirectFulfillment( "https://example.org/redirect_to_epub", MediaTypes.EPUB_MEDIA_TYPE, - "", - None, - content_link_redirect=True, ) controller.can_fulfill_without_loan = MagicMock(return_value=False) controller.authenticated_patron_from_request = MagicMock(return_value=patron) @@ -1099,8 +1052,12 @@ def test_no_drm_fulfill(self, loan_fixture: LoanFixture): api = MagicMock(spec=Axis360API) api.collection = loan_fixture.db.default_collection() api._db = loan_fixture.db.session - axis360_ff = Axis360FulfillmentInfo( - api, DataSource.AXIS_360, "Axis 360 ID", "xxxxxx", "xxxxxx" + axis360_ff = Axis360Fulfillment( + api=api, + data_source_name=DataSource.AXIS_360, + identifier_type="Axis 360 ID", + identifier="xxxxxx", + key="xxxxxx", ) api.get_fulfillment_info.return_value = MagicMock( content={ diff --git a/tests/manager/api/controller/test_work.py b/tests/manager/api/controller/test_work.py index ad4aaca68..df76536d5 100644 --- a/tests/manager/api/controller/test_work.py +++ b/tests/manager/api/controller/test_work.py @@ -10,7 +10,7 @@ import pytest from flask import url_for -from palace.manager.api.circulation import FulfillmentInfo, LoanInfo +from palace.manager.api.circulation import Fulfillment, LoanInfo from palace.manager.api.lanes import ( ContributorFacets, ContributorLane, @@ -29,7 +29,6 @@ from palace.manager.feed.annotator.circulation import LibraryAnnotator from palace.manager.feed.types import WorkEntry from palace.manager.search.external_search import SortKeyPagination -from palace.manager.sqlalchemy.constants import MediaTypes from palace.manager.sqlalchemy.model.datasource import DataSource from palace.manager.sqlalchemy.model.edition import Edition from palace.manager.sqlalchemy.model.identifier import Identifier @@ -434,17 +433,9 @@ def test_permalink_returns_fulfillment_links_for_authenticated_patrons_with_fulf ) work_fixture.manager.d_circulation.queue_checkout(pool, loan_info) - fulfillment = FulfillmentInfo( - pool.collection, - pool.data_source, - pool.identifier.type, - pool.identifier.identifier, - content_link=content_link, - content_type=MediaTypes.EPUB_MEDIA_TYPE, - content=None, - content_expires=None, + work_fixture.manager.d_circulation.queue_fulfill( + pool, create_autospec(Fulfillment) ) - work_fixture.manager.d_circulation.queue_fulfill(pool, fulfillment) # Both patrons have loans: # - the first patron's loan and fulfillment will be created via API. @@ -457,13 +448,12 @@ def test_permalink_returns_fulfillment_links_for_authenticated_patrons_with_fulf work_fixture.manager.loans.fulfill( pool.id, delivery_mechanism.delivery_mechanism.id, - do_get=MagicMock(return_value=(200, {}, b"")), ) patron1_loan = pool.loans[0] # We have to create a Resource object manually # to assign a URL to the fulfillment that will be used to generate an acquisition link. - patron1_loan.fulfillment.resource = Resource(url=fulfillment.content_link) + patron1_loan.fulfillment.resource = Resource(url=content_link) patron2_loan, _ = pool.loan_to(patron_2) diff --git a/tests/manager/api/odl/test_api.py b/tests/manager/api/odl/test_api.py index a54875221..f42ec34a9 100644 --- a/tests/manager/api/odl/test_api.py +++ b/tests/manager/api/odl/test_api.py @@ -12,7 +12,13 @@ import pytest from freezegun import freeze_time -from palace.manager.api.circulation import HoldInfo, LoanInfo +from palace.manager.api.circulation import ( + DirectFulfillment, + FetchFulfillment, + HoldInfo, + LoanInfo, + RedirectFulfillment, +) from palace.manager.api.circulation_exceptions import ( AlreadyCheckedOut, AlreadyOnHold, @@ -885,23 +891,7 @@ def test_fulfill_success( opds2_with_odl_api_fixture.pool, lpdm, ) - - assert opds2_with_odl_api_fixture.collection == fulfillment.collection( - db.session - ) - assert ( - opds2_with_odl_api_fixture.pool.data_source.name - == fulfillment.data_source_name - ) - assert ( - opds2_with_odl_api_fixture.pool.identifier.type - == fulfillment.identifier_type - ) - assert ( - opds2_with_odl_api_fixture.pool.identifier.identifier - == fulfillment.identifier - ) - assert datetime_utc(2017, 10, 21, 11, 12, 13) == fulfillment.content_expires + assert isinstance(fulfillment, FetchFulfillment) assert correct_link == fulfillment.content_link assert correct_type == fulfillment.content_type @@ -936,16 +926,7 @@ def test_fulfill_open_access( opds2_with_odl_api_fixture.patron, "pin", pool, lpdm ) - assert opds2_with_odl_api_fixture.collection == fulfillment.collection( - db.session - ) - assert ( - opds2_with_odl_api_fixture.pool.data_source.name - == fulfillment.data_source_name - ) - assert fulfillment.identifier_type == pool.identifier.type - assert fulfillment.identifier == pool.identifier.identifier - assert fulfillment.content_expires is None + assert isinstance(fulfillment, RedirectFulfillment) assert fulfillment.content_link == pool.open_access_download_url assert fulfillment.content_type == lpdm.delivery_mechanism.content_type @@ -983,24 +964,11 @@ def test_fulfill_bearer_token( opds2_with_odl_api_fixture.patron, "pin", pool, lpdm ) - assert opds2_with_odl_api_fixture.collection == fulfillment.collection( - db.session - ) - assert ( - opds2_with_odl_api_fixture.pool.data_source.name - == fulfillment.data_source_name - ) - assert fulfillment.identifier_type == pool.identifier.type - assert fulfillment.identifier == pool.identifier.identifier - assert opds2_with_odl_api_fixture.api._session_token is not None - assert ( - fulfillment.content_expires - == opds2_with_odl_api_fixture.api._session_token.expires - ) - assert fulfillment.content_link is None + assert isinstance(fulfillment, DirectFulfillment) assert fulfillment.content_type == DeliveryMechanism.BEARER_TOKEN assert fulfillment.content is not None token_doc = json.loads(fulfillment.content) + assert opds2_with_odl_api_fixture.api._session_token is not None assert ( token_doc.get("access_token") == opds2_with_odl_api_fixture.api._session_token.token @@ -1018,13 +986,31 @@ def test_fulfill_bearer_token( fulfillment_2 = opds2_with_odl_api_fixture.api.fulfill( opds2_with_odl_api_fixture.patron, "pin", pool, lpdm ) + assert isinstance(fulfillment_2, DirectFulfillment) assert fulfillment_2.content == fulfillment.content assert opds2_with_odl_api_fixture.api.refresh_token_calls == 1 + @pytest.mark.parametrize( + "status_document, updated_availability", + [ + ({"status": "revoked"}, True), + ({"status": "cancelled"}, True), + ( + { + "status": "active", + "potential_rights": {"end": "2017-10-21T11:12:13Z"}, + "links": [], + }, + False, + ), + ], + ) def test_fulfill_cannot_fulfill( self, db: DatabaseTransactionFixture, opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, + status_document: dict[str, Any], + updated_availability: bool, ) -> None: opds2_with_odl_api_fixture.setup_license(concurrency=7, available=7) opds2_with_odl_api_fixture.checkout() @@ -1032,27 +1018,23 @@ def test_fulfill_cannot_fulfill( assert 1 == db.session.query(Loan).count() assert 6 == opds2_with_odl_api_fixture.pool.licenses_available - lsd = json.dumps( - { - "status": "revoked", - } - ) + lsd = json.dumps(status_document) opds2_with_odl_api_fixture.mock_http.queue_response(200, content=lsd) - pytest.raises( - CannotFulfill, - opds2_with_odl_api_fixture.api.fulfill, - opds2_with_odl_api_fixture.patron, - "pin", - opds2_with_odl_api_fixture.pool, - Representation.EPUB_MEDIA_TYPE, - ) + with pytest.raises(CannotFulfill): + opds2_with_odl_api_fixture.api.fulfill( + opds2_with_odl_api_fixture.patron, + "pin", + opds2_with_odl_api_fixture.pool, + MagicMock(), + ) - # The pool's availability has been updated and the local - # loan has been deleted, since we found out the loan is - # no longer active. - assert 7 == opds2_with_odl_api_fixture.pool.licenses_available - assert 0 == db.session.query(Loan).count() + if updated_availability: + # The pool's availability has been updated and the local + # loan has been deleted, since we found out the loan is + # no longer active. + assert 7 == opds2_with_odl_api_fixture.pool.licenses_available + assert 0 == db.session.query(Loan).count() def _holdinfo_from_hold(self, hold: Hold) -> HoldInfo: pool: LicensePool = hold.license_pool diff --git a/tests/manager/api/test_axis.py b/tests/manager/api/test_axis.py index 504b211d9..c45f4632b 100644 --- a/tests/manager/api/test_axis.py +++ b/tests/manager/api/test_axis.py @@ -5,6 +5,7 @@ import socket import ssl import urllib +from contextlib import contextmanager from functools import partial from typing import TYPE_CHECKING, cast from unittest.mock import MagicMock, Mock, PropertyMock, patch @@ -15,12 +16,12 @@ from palace.manager.api.axis import ( AudiobookMetadataParser, AvailabilityResponseParser, - Axis360AcsFulfillmentInfo, + Axis360AcsFulfillment, Axis360API, Axis360APIConstants, Axis360BibliographicCoverageProvider, Axis360CirculationMonitor, - Axis360FulfillmentInfo, + Axis360Fulfillment, Axis360FulfillmentInfoResponseParser, Axis360Settings, AxisCollectionReaper, @@ -33,7 +34,7 @@ JSONResponseParser, StatusResponseParser, ) -from palace.manager.api.circulation import FulfillmentInfo, HoldInfo, LoanInfo +from palace.manager.api.circulation import HoldInfo, LoanInfo, UrlFulfillment from palace.manager.api.circulation_exceptions import ( AlreadyCheckedOut, AlreadyOnHold, @@ -70,7 +71,10 @@ from palace.manager.util.datetime_helpers import datetime_utc, utc_now from palace.manager.util.flask_util import Response from palace.manager.util.http import RemoteIntegrationException -from palace.manager.util.problem_detail import ProblemDetail, ProblemDetailException +from palace.manager.util.problem_detail import ( + BaseProblemDetailException, + ProblemDetailException, +) from tests.fixtures.files import FilesFixture from tests.fixtures.library import LibraryFixture from tests.mocks.analytics_provider import MockAnalyticsProvider @@ -481,29 +485,28 @@ def test_fulfill(self, axis360: Axis360Fixture): pytest.raises(NoActiveLoan, fulfill) # If an ebook is checked out and we're not asking for it to be - # fulfilled through AxisNow, we get a regular FulfillmentInfo + # fulfilled through AxisNow, we get a Axis360AcsFulfillment # object with a content link. data = axis360.sample_data("availability_with_loan_and_hold.xml") axis360.api.queue_response(200, content=data) fulfillment = fulfill() - assert isinstance(fulfillment, FulfillmentInfo) - assert not isinstance(fulfillment, Axis360FulfillmentInfo) + assert isinstance(fulfillment, Axis360AcsFulfillment) + assert not isinstance(fulfillment, Axis360Fulfillment) assert DeliveryMechanism.ADOBE_DRM == fulfillment.content_type assert "http://fulfillment/" == fulfillment.content_link - assert None == fulfillment.content - # If we ask for AxisNow format, we get an Axis360FulfillmentInfo + # If we ask for AxisNow format, we get an Axis360Fulfillment # containing an AxisNow manifest document. data = axis360.sample_data("availability_with_axisnow_fulfillment.xml") data = data.replace(b"0016820953", pool.identifier.identifier.encode("utf8")) axis360.api.queue_response(200, content=data) delivery_mechanism.drm_scheme = DeliveryMechanism.AXISNOW_DRM fulfillment = fulfill() - assert isinstance(fulfillment, Axis360FulfillmentInfo) + assert isinstance(fulfillment, Axis360Fulfillment) - # Looking up the details of the Axis360FulfillmentInfo will + # Looking up the details of the Axis360Fulfillment will # trigger another API request, so we won't do that; that's - # tested in TestAxis360FulfillmentInfo. + # tested in TestAxis360Fulfillment. # If the title is checked out but Axis provides no fulfillment # info, the exception is CannotFulfill. @@ -512,7 +515,8 @@ def test_fulfill(self, axis360: Axis360Fixture): axis360.api.queue_response(200, content=data) pytest.raises(CannotFulfill, fulfill) - # If we ask to fulfill an audiobook, we get an AudiobookFulfillmentInfo. + # If we ask to fulfill an audiobook, we get an Axis360Fulfillment, since + # it can handle both cases. # # Change our test LicensePool's identifier to match the data we're about # to load into the API. @@ -523,7 +527,7 @@ def test_fulfill(self, axis360: Axis360Fixture): axis360.api.queue_response(200, content=data) delivery_mechanism.drm_scheme = DeliveryMechanism.FINDAWAY_DRM fulfillment = fulfill() - assert isinstance(fulfillment, Axis360FulfillmentInfo) + assert isinstance(fulfillment, Axis360Fulfillment) def test_patron_activity(self, axis360: Axis360Fixture): """Test the method that locates all current activity @@ -1459,10 +1463,10 @@ def test_parse_checkout_success(self, axis360parsers: Axis360FixturePlusParsers) assert Identifier.AXIS_360_ID == parsed.identifier_type assert datetime_utc(2015, 8, 11, 18, 57, 42) == parsed.end_date - # There is no FulfillmentInfo associated with the LoanInfo, + # There is no Fulfillment associated with the LoanInfo, # because we don't need it (checkout and fulfillment are # separate steps). - assert parsed.fulfillment_info == None + assert parsed.fulfillment == None def test_parse_already_checked_out(self, axis360parsers: Axis360FixturePlusParsers): data = axis360parsers.sample_data("already_checked_out.xml") @@ -1530,8 +1534,8 @@ def test_parse_loan_and_hold(self, axis360parsers: Axis360FixturePlusParsers): assert axis360parsers.api.collection.id == loan.collection_id assert "0015176429" == loan.identifier - assert loan.fulfillment_info is not None - assert "http://fulfillment/" == loan.fulfillment_info.content_link + assert isinstance(loan.fulfillment, UrlFulfillment) + assert "http://fulfillment/" == loan.fulfillment.content_link assert datetime_utc(2015, 8, 12, 17, 40, 27) == loan.end_date assert axis360parsers.api.collection.id == reserved.collection_id @@ -1550,7 +1554,7 @@ def test_parse_loan_no_availability( assert axis360parsers.api.collection is not None assert axis360parsers.api.collection.id == loan.collection_id assert "0015176429" == loan.identifier - assert None == loan.fulfillment_info + assert None == loan.fulfillment assert datetime_utc(2015, 8, 12, 17, 40, 27) == loan.end_date def test_parse_audiobook_availability( @@ -1560,16 +1564,16 @@ def test_parse_audiobook_availability( parser = AvailabilityResponseParser(axis360parsers.api) [loan] = list(parser.process_all(data)) assert isinstance(loan, LoanInfo) - fulfillment = loan.fulfillment_info - assert isinstance(fulfillment, Axis360FulfillmentInfo) + fulfillment = loan.fulfillment + assert isinstance(fulfillment, Axis360Fulfillment) # The transaction ID is stored as the .key. If we actually # need to make a manifest for this book, the key will be used - # in two more API requests. (See TestAudiobookFulfillmentInfo + # in two more API requests. (See TestAxis360Fulfillment # for that.) assert "C3F71F8D-1883-2B34-061F-96570678AEB0" == fulfillment.key - # The API object is present in the FulfillmentInfo and ready to go. + # The API object is present in the Fulfillment and ready to go. assert axis360parsers.api == fulfillment.api def test_parse_ebook_availability(self, axis360parsers: Axis360FixturePlusParsers): @@ -1581,13 +1585,13 @@ def test_parse_ebook_availability(self, axis360parsers: Axis360FixturePlusParser epub_parser = AvailabilityResponseParser(axis360parsers.api, "ePub") [availability] = list(epub_parser.process_all(data)) assert isinstance(availability, LoanInfo) - fulfillment = availability.fulfillment_info + fulfillment = availability.fulfillment # This particular file has a downloadUrl ready to go, so we - # get a standard FulfillmentInfo object with that downloadUrl + # get a standard Axis360AcsFulfillment object with that downloadUrl # as its content_link. - assert isinstance(fulfillment, FulfillmentInfo) - assert not isinstance(fulfillment, Axis360FulfillmentInfo) + assert isinstance(fulfillment, Axis360AcsFulfillment) + assert not isinstance(fulfillment, Axis360Fulfillment) assert ( "http://adobe.acsm/?src=library&transactionId=2a34598b-12af-41e4-a926-af5e42da7fe5&isbn=9780763654573&format=F2" == fulfillment.content_link @@ -1602,11 +1606,11 @@ def test_parse_ebook_availability(self, axis360parsers: Axis360FixturePlusParser ) [availability] = list(axisnow_parser.process_all(data)) assert isinstance(availability, LoanInfo) - fulfillment = availability.fulfillment_info - assert isinstance(fulfillment, Axis360FulfillmentInfo) + fulfillment = availability.fulfillment + assert isinstance(fulfillment, Axis360Fulfillment) assert "6670197A-D264-447A-86C7-E4CB829C0236" == fulfillment.key - # The API object is present in the FulfillmentInfo and ready to go + # The API object is present in the Fulfillment and ready to go # make that extra request. assert axis360parsers.api == fulfillment.api @@ -1856,8 +1860,8 @@ def test__extract_spine_item(self, axis360: Axis360Fixture): assert Representation.MP3_MEDIA_TYPE == item.media_type -class TestAxis360FulfillmentInfo: - """An Axis360FulfillmentInfo can fulfill a title whether it's an ebook +class TestAxis360Fulfillment: + """An Axis360Fulfillment can fulfill a title whether it's an ebook (fulfilled through AxisNow) or an audiobook (fulfilled through Findaway). """ @@ -1876,22 +1880,24 @@ def test_fetch_audiobook(self, axis360: Axis360Fixture): # Setup. edition, pool = axis360.db.edition(with_license_pool=True) identifier = pool.identifier - fulfillment = Axis360FulfillmentInfo( + fulfillment = Axis360Fulfillment( axis360.api, pool.data_source.name, identifier.type, identifier.identifier, "transaction_id", ) - assert None == fulfillment._content_type + assert fulfillment.content_type is None + assert fulfillment.content is None # Turn the crank. - fulfillment.fetch() + fulfillment.response() - # The Axis360FulfillmentInfo now contains a Findaway manifest + # The Axis360Fulfillment now contains a Findaway manifest # document. - assert DeliveryMechanism.FINDAWAY_DRM == fulfillment.content_type - assert isinstance(fulfillment.content, str) + assert fulfillment.content_type == DeliveryMechanism.FINDAWAY_DRM + assert fulfillment.content is not None + assert isinstance(fulfillment.content, str) # type: ignore[unreachable] # The manifest document combines information from the # fulfillment document and the metadata document. @@ -1901,10 +1907,6 @@ def test_fetch_audiobook(self, axis360: Axis360Fixture): ): assert required in fulfillment.content - # The content expiration date also comes from the fulfillment - # document. - assert datetime_utc(2018, 9, 29, 18, 34) == fulfillment.content_expires - def test_fetch_ebook(self, axis360: Axis360Fixture): # When no Findaway information is present in the response from # the fulfillment API, information from the request is @@ -1916,30 +1918,27 @@ def test_fetch_ebook(self, axis360: Axis360Fixture): # Setup. edition, pool = axis360.db.edition(with_license_pool=True) identifier = pool.identifier - fulfillment = Axis360FulfillmentInfo( + fulfillment = Axis360Fulfillment( axis360.api, pool.data_source.name, identifier.type, identifier.identifier, "transaction_id", ) - assert None == fulfillment._content_type + assert fulfillment.content_type is None + assert fulfillment.content is None # Turn the crank. - fulfillment.fetch() + fulfillment.response() - # The Axis360FulfillmentInfo now contains an AxisNow manifest + # The Axis360Fulfillment now contains an AxisNow manifest # document derived from the fulfillment document. - assert DeliveryMechanism.AXISNOW_DRM == fulfillment.content_type + assert fulfillment.content_type == DeliveryMechanism.AXISNOW_DRM assert ( - '{"book_vault_uuid": "1c11c31f-81c2-41bb-9179-491114c3f121", "isbn": "9780547351551"}' - == fulfillment.content + fulfillment.content + == '{"book_vault_uuid": "1c11c31f-81c2-41bb-9179-491114c3f121", "isbn": "9780547351551"}' ) - # The content expiration date also comes from the fulfillment - # document. - assert datetime_utc(2018, 9, 29, 18, 34) == fulfillment.content_expires - class TestAxisNowManifest: """Test the simple data format used to communicate an entry point into @@ -2053,9 +2052,19 @@ def test_transient_failure_if_requested_book_not_mentioned( assert [] == identifier.primarily_identifies -class TestAxis360AcsFulfillmentInfo: - @pytest.fixture - def mock_request(self): +class Axis360AcsFulfillmentFixture: + def __init__(self, mock_urlopen: MagicMock): + self.fulfillment_info = partial( + Axis360AcsFulfillment, + content_link="https://fake.url", + verify=False, + ) + self.mock_request = self.create_mock_request() + self.mock_urlopen = mock_urlopen + self.mock_urlopen.return_value = self.mock_request + + @staticmethod + def create_mock_request() -> MagicMock: # Create a mock request object that we can use in the tests response = MagicMock(return_value="") type(response).headers = PropertyMock(return_value=[]) @@ -2065,54 +2074,38 @@ def mock_request(self): mock_request.__exit__.return_value = None return mock_request - @pytest.fixture - def fulfillment_info(self): - # A partial of the Axis360AcsFulfillmentInfo object to make it easier to - # create these objects in our tests by supplying default parameters - params = { - "collection": 0, - "content_type": None, - "content": None, - "content_expires": None, - "data_source_name": None, - "identifier_type": None, - "identifier": None, - "verify": None, - "content_link": "https://fake.url", - } - return partial(Axis360AcsFulfillmentInfo, **params) - - @pytest.fixture - def patch_urllib_urlopen(self, monkeypatch): - # Monkeypatch the urllib.request.urlopen function to whatever is passed into - # this function. - def patch_urlopen(mock): - monkeypatch.setattr(urllib.request, "urlopen", mock) - - return patch_urlopen + @classmethod + @contextmanager + def fixture(self): + with patch("urllib.request.urlopen") as mock_urlopen: + yield Axis360AcsFulfillmentFixture(mock_urlopen) + + +@pytest.fixture +def axis360_acs_fulfillment_fixture(): + with Axis360AcsFulfillmentFixture.fixture() as fixture: + yield fixture + +class TestAxis360AcsFulfillment: def test_url_encoding_not_capitalized( - self, patch_urllib_urlopen, mock_request, fulfillment_info + self, axis360_acs_fulfillment_fixture: Axis360AcsFulfillmentFixture ): # Mock the urllopen function to make sure that the URL is not actually requested # then make sure that when the request is built the %3a character encoded in the # string is not uppercased to be %3A. - called_url = None - - def mock_urlopen(url, **kwargs): - nonlocal called_url - called_url = url - return mock_request - patch_urllib_urlopen(mock_urlopen) - fulfillment = fulfillment_info( + fulfillment = axis360_acs_fulfillment_fixture.fulfillment_info( content_link="https://test.com/?param=%3atest123" ) - response = fulfillment.as_response + response = fulfillment.response() + axis360_acs_fulfillment_fixture.mock_urlopen.assert_called() + called_url = axis360_acs_fulfillment_fixture.mock_urlopen.call_args[0][0] assert called_url is not None assert called_url.selector == "/?param=%3atest123" assert called_url.host == "test.com" assert type(response) == Response + mock_request = axis360_acs_fulfillment_fixture.mock_request mock_request.__enter__.assert_called() mock_request.__enter__.return_value.read.assert_called() assert "status" in dir(mock_request.__enter__.return_value) @@ -2129,16 +2122,18 @@ def mock_urlopen(url, **kwargs): ], ids=lambda val: val.__class__.__name__, ) - def test_exception_returns_problem_detail( - self, patch_urllib_urlopen, fulfillment_info, exception + def test_exception_raises_problem_detail_exception( + self, + axis360_acs_fulfillment_fixture: Axis360AcsFulfillmentFixture, + exception: Exception, ): # Check that when the urlopen function throws an exception, we catch the exception and # we turn it into a problem detail to be returned to the client. This mimics the behavior # of the http utils function that we are bypassing with this fulfillment method. - patch_urllib_urlopen(Mock(side_effect=exception)) - fulfillment = fulfillment_info() - response = fulfillment.as_response - assert type(response) == ProblemDetail + axis360_acs_fulfillment_fixture.mock_urlopen.side_effect = exception + fulfillment = axis360_acs_fulfillment_fixture.fulfillment_info() + with pytest.raises(BaseProblemDetailException): + fulfillment.response() @pytest.mark.parametrize( ("verify", "verify_mode", "check_hostname"), @@ -2146,22 +2141,18 @@ def test_exception_returns_problem_detail( ) def test_verify_ssl( self, - patch_urllib_urlopen, - fulfillment_info, - verify, - verify_mode, - check_hostname, - mock_request, + axis360_acs_fulfillment_fixture: Axis360AcsFulfillmentFixture, + verify: bool, + verify_mode: ssl.VerifyMode, + check_hostname: bool, ): # Make sure that when the verify parameter of the fulfillment method is set we use the # correct SSL context to either verify or not verify the ssl certificate for the # URL we are fetching. - mock = MagicMock(return_value=mock_request) - patch_urllib_urlopen(mock) - fulfillment = fulfillment_info(verify=verify) - response = fulfillment.as_response - mock.assert_called() - assert "context" in mock.call_args[1] - context = mock.call_args[1]["context"] + fulfillment = axis360_acs_fulfillment_fixture.fulfillment_info(verify=verify) + fulfillment.response() + axis360_acs_fulfillment_fixture.mock_urlopen.assert_called() + assert "context" in axis360_acs_fulfillment_fixture.mock_urlopen.call_args[1] + context = axis360_acs_fulfillment_fixture.mock_urlopen.call_args[1]["context"] assert context.verify_mode == verify_mode assert context.check_hostname == check_hostname diff --git a/tests/manager/api/test_bibliotheca.py b/tests/manager/api/test_bibliotheca.py index a6215dde1..aa34d234d 100644 --- a/tests/manager/api/test_bibliotheca.py +++ b/tests/manager/api/test_bibliotheca.py @@ -27,7 +27,7 @@ ) from palace.manager.api.circulation import ( CirculationAPI, - FulfillmentInfo, + Fulfillment, HoldInfo, LoanInfo, ) @@ -561,11 +561,8 @@ def test_fulfill(self, bibliotheca_fixture: BibliothecaAPITestFixture): fulfillment = bibliotheca_fixture.api.fulfill( patron, "password", pool, delivery_mechanism=delivery_mechanism ) - assert isinstance(fulfillment, FulfillmentInfo) + assert isinstance(fulfillment, Fulfillment) assert b"this is an ACSM" == fulfillment.content - assert pool.identifier.identifier == fulfillment.identifier - assert pool.identifier.type == fulfillment.identifier_type - assert pool.data_source.name == fulfillment.data_source_name # The media type reported by the server is passed through. assert "presumably/an-acsm" == fulfillment.content_type @@ -583,7 +580,7 @@ def test_fulfill(self, bibliotheca_fixture: BibliothecaAPITestFixture): fulfillment = bibliotheca_fixture.api.fulfill( patron, "password", pool, delivery_mechanism=delivery_mechanism ) - assert isinstance(fulfillment, FulfillmentInfo) + assert isinstance(fulfillment, Fulfillment) # Here, the media type reported by the server is not passed # through; it's replaced by a more specific media type @@ -614,10 +611,10 @@ def test_fulfill(self, bibliotheca_fixture: BibliothecaAPITestFixture): fulfillment = bibliotheca_fixture.api.fulfill( patron, "password", pool, delivery_mechanism=delivery_mechanism ) - assert isinstance(fulfillment, FulfillmentInfo) + assert isinstance(fulfillment, Fulfillment) # The (apparently) bad document is just passed on to the - # client as part of the FulfillmentInfo, in the hopes that the + # client as part of the Fulfillment, in the hopes that the # client will know what to do with it. assert bad_media_type == fulfillment.content_type assert bad_content == fulfillment.content diff --git a/tests/manager/api/test_circulation.py b/tests/manager/api/test_circulation.py new file mode 100644 index 000000000..e52515f9a --- /dev/null +++ b/tests/manager/api/test_circulation.py @@ -0,0 +1,146 @@ +import pytest +from flask import Response + +from palace.manager.api.circulation import ( + DirectFulfillment, + FetchFulfillment, + RedirectFulfillment, +) +from palace.manager.util.http import BadResponseException +from tests.mocks.mock import MockHTTPClient + + +class TestDirectFulfillment: + def test_response(self) -> None: + fulfillment = DirectFulfillment("This is some content.", "text/plain") + response = fulfillment.response() + assert isinstance(response, Response) + assert response.status_code == 200 + assert response.get_data(as_text=True) == "This is some content." + assert response.content_type == "text/plain" + + def test__repr__(self) -> None: + fulfillment = DirectFulfillment("test", "foo/bar") + assert ( + fulfillment.__repr__() + == "" + ) + + +class TestRedirectFulfillment: + def test_response(self) -> None: + fulfillment = RedirectFulfillment("http://some.location", "foo/bar") + response = fulfillment.response() + assert isinstance(response, Response) + assert response.status_code == 302 + assert response.headers["Location"] == "http://some.location" + assert response.content_type != "foo/bar" + assert response.content_type == "text/plain" + + def test__repr__(self) -> None: + fulfillment = RedirectFulfillment("http://some.location") + assert ( + fulfillment.__repr__() + == "" + ) + + fulfillment = RedirectFulfillment("http://some.location", "foo/bar") + assert ( + fulfillment.__repr__() + == "" + ) + + +class TestFetchFulfillment: + def test_fetch_fulfillment(self) -> None: + http = MockHTTPClient() + http.queue_response( + 204, content="This is some content.", media_type="application/xyz" + ) + fulfillment = FetchFulfillment("http://some.location", "foo/bar") + with http.patch(): + response = fulfillment.response() + assert isinstance(response, Response) + # The external requests status code is passed through. + assert response.status_code == 204 + # As is its content. + assert response.get_data(as_text=True) == "This is some content." + # Any content type set on the fulfillment, overrides the content type from the request. + assert response.content_type == "foo/bar" + assert http.requests == ["http://some.location"] + + # If no content type is set on the fulfillment, the content type from the request is used. + http = MockHTTPClient() + http.queue_response(200, content="Other content.", media_type="application/xyz") + fulfillment = FetchFulfillment("http://some.other.location") + with http.patch(): + response = fulfillment.response() + assert isinstance(response, Response) + assert response.status_code == 200 + assert response.get_data(as_text=True) == "Other content." + assert response.content_type == "application/xyz" + assert http.requests == ["http://some.other.location"] + [(args, kwargs)] = http.requests_args + assert kwargs["allow_redirects"] is True + + def test_fetch_fulfillment_include_headers(self) -> None: + # If include_headers is set, the headers are set when the fetch is made, but + # not included in the response. + http = MockHTTPClient() + http.queue_response( + 204, content="This is some content.", media_type="application/xyz" + ) + fulfillment = FetchFulfillment( + "http://some.location", "foo/bar", include_headers={"X-Test": "test"} + ) + with http.patch(): + response = fulfillment.response() + assert isinstance(response, Response) + assert response.status_code == 204 + assert response.get_data(as_text=True) == "This is some content." + assert response.content_type == "foo/bar" + assert "X-Test" not in response.headers + assert http.requests == ["http://some.location"] + [(args, kwargs)] = http.requests_args + assert kwargs["headers"]["X-Test"] == "test" + + def test_fetch_fulfillment_allowed_response_codes( + self, caplog: pytest.LogCaptureFixture + ) -> None: + http = MockHTTPClient() + http.queue_response( + 403, + content='{"type":"http://opds-spec.org/odl/error/checkout/expired",' + '"title":"the license has expired","detail":"["loan_term_limit_reached"]","status":403}', + media_type="application/api-problem+json", + ) + fulfillment = FetchFulfillment( + "http://some.location", allowed_response_codes=["2xx"] + ) + with ( + http.patch(), + pytest.raises(BadResponseException) as excinfo, + ): + fulfillment.response() + + assert ( + excinfo.value.problem_detail.detail + == "The server made a request to some.location, and got an unexpected or invalid response." + ) + assert ( + "Error fulfilling loan. Bad response from: http://some.location." + in caplog.text + ) + + def test__repr__(self) -> None: + fulfillment = FetchFulfillment("http://some.location") + assert ( + fulfillment.__repr__() + == "" + ) + + fulfillment = FetchFulfillment("http://some.location", "foo/bar") + assert ( + fulfillment.__repr__() + == "" + ) diff --git a/tests/manager/api/test_circulationapi.py b/tests/manager/api/test_circulationapi.py index 9510b9025..3875f6930 100644 --- a/tests/manager/api/test_circulationapi.py +++ b/tests/manager/api/test_circulationapi.py @@ -9,12 +9,10 @@ from flask import Flask from palace.manager.api.circulation import ( - APIAwareFulfillmentInfo, BaseCirculationAPI, CirculationAPI, CirculationInfo, DeliveryMechanismInfo, - FulfillmentInfo, HoldInfo, LoanInfo, ) @@ -1349,154 +1347,3 @@ def test_apply(self, db: DatabaseTransactionFixture): # The LicensePool is now considered an open-access LicensePool, # since it has an open-access delivery mechanism. assert True == pool.open_access - - -class TestFulfillmentInfo: - def test_as_response(self, db: DatabaseTransactionFixture): - # The default behavior of as_response is to do nothing - # and let controller code turn the FulfillmentInfo - # into a Flask Response. - info = FulfillmentInfo( - db.default_collection(), None, None, None, None, None, None, None - ) - assert None == info.as_response - - -class APIAwareFulfillmentFixture: - def __init__(self, db: DatabaseTransactionFixture): - self.db = db - self.collection = db.default_collection() - - # Create a bunch of mock objects which will be used to initialize - # the instance variables of MockAPIAwareFulfillmentInfo objects. - self.mock_data_source_name = MagicMock() - self.mock_identifier_type = MagicMock() - self.mock_identifier = MagicMock() - self.mock_key = MagicMock() - - -@pytest.fixture(scope="function") -def api_aware_fulfillment_fixture( - db: DatabaseTransactionFixture, -) -> APIAwareFulfillmentFixture: - return APIAwareFulfillmentFixture(db) - - -class TestAPIAwareFulfillmentInfo: - # The APIAwareFulfillmentInfo class has the same properties as a - # regular FulfillmentInfo -- content_link and so on -- but their - # values are filled dynamically the first time one of them is - # accessed, by calling the do_fetch() method. - - class MockAPIAwareFulfillmentInfo(APIAwareFulfillmentInfo): - """An APIAwareFulfillmentInfo that implements do_fetch() by delegating - to its API object. - """ - - def do_fetch(self): - return self.api.do_fetch() - - class MockAPI: - """An API class that sets a flag when do_fetch() - is called. - """ - - def __init__(self, collection): - self.collection = collection - self.fetch_happened = False - - def do_fetch(self): - self.fetch_happened = True - - def make_info( - self, api_aware_fulfillment_fixture: APIAwareFulfillmentFixture, api=None - ): - # Create a MockAPIAwareFulfillmentInfo with - # well-known mock values for its properties. - return self.MockAPIAwareFulfillmentInfo( - api, - api_aware_fulfillment_fixture.mock_data_source_name, - api_aware_fulfillment_fixture.mock_identifier_type, - api_aware_fulfillment_fixture.mock_identifier, - api_aware_fulfillment_fixture.mock_key, - ) - - def test_constructor( - self, api_aware_fulfillment_fixture: APIAwareFulfillmentFixture - ): - data = api_aware_fulfillment_fixture - - # The constructor sets the instance variables appropriately, - # but does not call do_fetch() or set any of the variables - # that imply do_fetch() has happened. - - # Create a MockAPI - api = self.MockAPI(data.collection) - - # Create an APIAwareFulfillmentInfo based on that API. - info = self.make_info(api_aware_fulfillment_fixture, api) - assert api == info.api - assert data.mock_key == info.key - assert data.collection == api.collection - assert api.collection == info.collection(data.db.session) - assert data.mock_data_source_name == info.data_source_name - assert data.mock_identifier_type == info.identifier_type - assert data.mock_identifier == info.identifier - - # The fetch has not happened. - assert False == api.fetch_happened - assert None == info._content_link - assert None == info._content_type - assert None == info._content - assert None == info._content_expires - - def test_fetch(self, api_aware_fulfillment_fixture: APIAwareFulfillmentFixture): - data = api_aware_fulfillment_fixture - - # Verify that fetch() calls api.do_fetch() - api = self.MockAPI(data.collection) - info = self.make_info(api_aware_fulfillment_fixture, api) - assert False == info._fetched - assert False == api.fetch_happened - info.fetch() - assert True == info._fetched - assert True == api.fetch_happened - - # We don't check that values like _content_link were set, - # because our implementation of do_fetch() doesn't set any of - # them. Different implementations may set different subsets - # of these values. - - def test_properties_fetch_on_demand( - self, api_aware_fulfillment_fixture: APIAwareFulfillmentFixture - ): - data = api_aware_fulfillment_fixture - - # Verify that accessing each of the properties calls fetch() - # if it hasn't been called already. - api = self.MockAPI(data.collection) - info = self.make_info(api_aware_fulfillment_fixture, api) - assert False == info._fetched - info.content_link - assert True == info._fetched - - info = self.make_info(api_aware_fulfillment_fixture, api) - assert False == info._fetched - info.content_type - assert True == info._fetched - - info = self.make_info(api_aware_fulfillment_fixture, api) - assert False == info._fetched - info.content - assert True == info._fetched - - info = self.make_info(api_aware_fulfillment_fixture, api) - assert False == info._fetched - info.content_expires - assert True == info._fetched - - # Once the data has been fetched, accessing one of the properties - # doesn't call fetch() again. - info.fetch_happened = False - info.content_expires - assert False == info.fetch_happened diff --git a/tests/manager/api/test_enki.py b/tests/manager/api/test_enki.py index a15591f20..c950dae9b 100644 --- a/tests/manager/api/test_enki.py +++ b/tests/manager/api/test_enki.py @@ -7,7 +7,7 @@ import pytest -from palace.manager.api.circulation import FulfillmentInfo, LoanInfo +from palace.manager.api.circulation import FetchFulfillment, LoanInfo from palace.manager.api.circulation_exceptions import ( NoAvailableCopies, PatronAuthorizationFailedException, @@ -542,16 +542,13 @@ def test_fulfill_success(self, enki_test_fixture: EnkiTestFixure): # patron's library was used as the 'lib' parameter. assert "c" == params["lib"] - # A FulfillmentInfo for the loan was returned. - assert isinstance(fulfillment, FulfillmentInfo) - assert fulfillment.identifier == pool.identifier.identifier - assert fulfillment.collection_id == pool.collection.id - assert DeliveryMechanism.ADOBE_DRM == fulfillment.content_type + # A Fulfillment for the loan was returned. + assert isinstance(fulfillment, FetchFulfillment) + assert fulfillment.content_type == DeliveryMechanism.ADOBE_DRM assert fulfillment.content_link is not None assert fulfillment.content_link.startswith( "http://afs.enkilibrary.org/fulfillment/URLLink.acsm" ) - assert fulfillment.content_expires == datetime_utc(2017, 9, 13, 19, 42, 35, 0) def test_patron_activity(self, enki_test_fixture: EnkiTestFixure): db = enki_test_fixture.db diff --git a/tests/manager/api/test_opds_for_distributors.py b/tests/manager/api/test_opds_for_distributors.py index 7364401b2..eee88afa2 100644 --- a/tests/manager/api/test_opds_for_distributors.py +++ b/tests/manager/api/test_opds_for_distributors.py @@ -1,4 +1,3 @@ -import datetime import json from collections.abc import Callable from unittest.mock import MagicMock, patch @@ -479,35 +478,19 @@ def test_fulfill(self, opds_dist_api_fixture: OPDSForDistributorsAPIFixture): opds_dist_api_fixture.api.queue_response(200, content=token_response) fulfillment_time = utc_now() - fulfillment_info = opds_dist_api_fixture.api.fulfill( + fulfillment = opds_dist_api_fixture.api.fulfill( patron, "1234", pool, delivery_mechanism ) - assert opds_dist_api_fixture.collection.id == fulfillment_info.collection_id - assert data_source.name == fulfillment_info.data_source_name - assert Identifier.URI == fulfillment_info.identifier_type - assert pool.identifier.identifier == fulfillment_info.identifier - assert None == fulfillment_info.content_link - assert DeliveryMechanism.BEARER_TOKEN == fulfillment_info.content_type - assert fulfillment_info.content is not None - bearer_token_document = json.loads(fulfillment_info.content) + assert DeliveryMechanism.BEARER_TOKEN == fulfillment.content_type + assert fulfillment.content is not None + bearer_token_document = json.loads(fulfillment.content) expires_in = bearer_token_document["expires_in"] assert expires_in < 60 assert "Bearer" == bearer_token_document["token_type"] assert "token" == bearer_token_document["access_token"] assert url == bearer_token_document["location"] - # The FulfillmentInfo's content_expires is approximately the - # time you get if you add the number of seconds until the - # bearer token expires to the time at which the title was - # originally fulfilled. - expect_expiration = fulfillment_time + datetime.timedelta(seconds=expires_in) - assert fulfillment_info.content_expires is not None - assert ( - abs((fulfillment_info.content_expires - expect_expiration).total_seconds()) - < 5 - ) - class TestOPDSForDistributorsImporter: def test_import(self, opds_dist_api_fixture: OPDSForDistributorsAPIFixture): diff --git a/tests/manager/api/test_overdrive.py b/tests/manager/api/test_overdrive.py index aa836b3e4..b623ff685 100644 --- a/tests/manager/api/test_overdrive.py +++ b/tests/manager/api/test_overdrive.py @@ -16,9 +16,11 @@ from palace.manager.api.circulation import ( CirculationAPI, - FulfillmentInfo, + FetchFulfillment, + Fulfillment, HoldInfo, LoanInfo, + RedirectFulfillment, ) from palace.manager.api.circulation_exceptions import ( CannotFulfill, @@ -43,7 +45,7 @@ OverdriveCollectionReaper, OverdriveConstants, OverdriveFormatSweep, - OverdriveManifestFulfillmentInfo, + OverdriveManifestFulfillment, OverdriveRepresentationExtractor, RecentOverdriveCollectionMonitor, ) @@ -1483,23 +1485,18 @@ def test_fulfill_returns_fulfillmentinfo_if_returned_by_get_fulfillment_link( ): db = overdrive_api_fixture.db - # If get_fulfillment_link returns a FulfillmentInfo, it is returned + # If get_fulfillment_link returns a Fulfillment, it is returned # immediately and the rest of fulfill() does not run. - fulfillment = FulfillmentInfo( - overdrive_api_fixture.collection, None, None, None, None, None, None, None - ) - - class MockAPI(OverdriveAPI): - def get_fulfillment_link(*args, **kwargs): - return fulfillment - - def internal_format( - self, delivery_mechanism: LicensePoolDeliveryMechanism - ) -> str: - return "format" + fulfillment = create_autospec(Fulfillment) edition, pool = db.edition(with_license_pool=True) - api = MockAPI(db.session, overdrive_api_fixture.collection) + api = OverdriveAPI(db.session, overdrive_api_fixture.collection) + api.get_fulfillment_link = create_autospec( + api.get_fulfillment_link, return_value=fulfillment + ) + api.internal_format = create_autospec( + api.internal_format, return_value="format" + ) result = api.fulfill(MagicMock(), MagicMock(), pool, MagicMock()) assert result is fulfillment @@ -1615,7 +1612,7 @@ def test_get_fulfillment_link_returns_fulfillmentinfo_for_manifest_format( # When the format requested would result in a link to a # manifest file, the manifest link is returned as-is (wrapped - # in an OverdriveFulfillmentInfo) rather than being retrieved + # in an OverdriveManifestFulfillment) rather than being retrieved # and processed. # To keep things simple, our mock API will always return the same @@ -1650,9 +1647,9 @@ def get_fulfillment_link_from_download_link(self, *args, **kwargs): "http://download-link", overdrive_format, ) - assert isinstance(fulfillmentinfo, OverdriveManifestFulfillmentInfo) + assert isinstance(fulfillmentinfo, OverdriveManifestFulfillment) - # Before looking at the OverdriveManifestFulfillmentInfo, + # Before looking at the OverdriveManifestFulfillment, # let's see how we got there. # First, our mocked get_loan() was called. @@ -1677,10 +1674,9 @@ def get_fulfillment_link_from_download_link(self, *args, **kwargs): # Since the manifest formats cannot be retrieved by the # circulation manager, the result of get_download_link was - # wrapped in an OverdriveManifestFulfillmentInfo and returned. + # wrapped in an OverdriveManifestFulfillment and returned. # get_fulfillment_link_from_download_link was never called. assert "http://fulfillment-link/" == fulfillmentinfo.content_link - assert None == fulfillmentinfo.content_type def test_update_formats(self, overdrive_api_fixture: OverdriveAPIFixture): db = overdrive_api_fixture.db @@ -2239,10 +2235,39 @@ def test_no_drm_fulfillment(self, overdrive_api_fixture: OverdriveAPIFixture): fulfill = od_api.fulfill( patron, "pin", work.active_license_pool(), delivery_mechanism ) - - assert fulfill.content_link_redirect is True + assert isinstance(fulfill, RedirectFulfillment) + assert fulfill.content_type == Representation.EPUB_MEDIA_TYPE assert fulfill.content_link == "https://example.org/epub-redirect" + def test_drm_fulfillment(self, overdrive_api_fixture: OverdriveAPIFixture): + db = overdrive_api_fixture.db + patron = db.patron() + work = db.work(with_license_pool=True) + patron.authorization_identifier = "barcode" + + od_api = OverdriveAPI(db.session, overdrive_api_fixture.collection) + od_api._server_nickname = OverdriveConstants.TESTING_SERVERS + + # Mock get fulfillment link + od_api.get_fulfillment_link = MagicMock( + return_value=("http://example.com/acsm", "application/vnd.adobe.adept+xml") + ) + + # Mock delivery mechanism + delivery_mechanism = create_autospec(LicensePoolDeliveryMechanism) + delivery_mechanism.delivery_mechanism = create_autospec(DeliveryMechanism) + delivery_mechanism.delivery_mechanism.drm_scheme = DeliveryMechanism.ADOBE_DRM + delivery_mechanism.delivery_mechanism.content_type = ( + Representation.EPUB_MEDIA_TYPE + ) + + fulfill = od_api.fulfill( + patron, "pin", work.active_license_pool(), delivery_mechanism + ) + assert isinstance(fulfill, FetchFulfillment) + assert fulfill.content_type == "application/vnd.adobe.adept+xml" + assert fulfill.content_link == "http://example.com/acsm" + def test_no_recently_changed_books( self, overdrive_api_fixture: OverdriveAPIFixture ): @@ -2620,7 +2645,7 @@ def test_process_checkout_data(self, overdrive_api_fixture: OverdriveAPIFixture) assert DeliveryMechanism.ADOBE_DRM == delivery.drm_scheme # TODO: In the future both of these tests should return a - # LoanInfo with appropriate FulfillmentInfo. The calling code + # LoanInfo with appropriate Fulfillment. The calling code # would then decide whether or not to show the loan. @@ -2825,21 +2850,18 @@ def test_sync_patron_activity_ignores_holds_from_other_collections( assert overdrive_hold in patron.holds -class TestOverdriveManifestFulfillmentInfo: - def test_as_response(self, overdrive_api_fixture: OverdriveAPIFixture): +class TestOverdriveManifestFulfillment: + def test_response(self, overdrive_api_fixture: OverdriveAPIFixture): db = overdrive_api_fixture.db - # An OverdriveManifestFulfillmentInfo just links the client - # directly to the manifest file, bypassing normal FulfillmentInfo - # processing. - info = OverdriveManifestFulfillmentInfo( - db.default_collection(), + # An OverdriveManifestFulfillment just redirects the client + # directly to the manifest file + info = OverdriveManifestFulfillment( "http://content-link/", - "abcd-efgh", "scope string", "access token", ) - response = info.as_response + response = info.response() assert 302 == response.status_code assert "" == response.get_data(as_text=True) headers = response.headers diff --git a/tests/manager/core/test_opds2_import.py b/tests/manager/core/test_opds2_import.py index 1dee74486..92dcc48bd 100644 --- a/tests/manager/core/test_opds2_import.py +++ b/tests/manager/core/test_opds2_import.py @@ -5,12 +5,15 @@ from unittest.mock import MagicMock, patch import pytest -from pytest import LogCaptureFixture from requests import Response from webpub_manifest_parser.core.ast import Contributor as WebpubContributor from webpub_manifest_parser.opds2 import OPDS2FeedParserFactory -from palace.manager.api.circulation import CirculationAPI, FulfillmentInfo +from palace.manager.api.circulation import ( + CirculationAPI, + Fulfillment, + RedirectFulfillment, +) from palace.manager.api.circulation_exceptions import CannotFulfill from palace.manager.core.opds2_import import ( OPDS2API, @@ -728,7 +731,7 @@ def __init__(self, db: DatabaseTransactionFixture, mock_http: MagicMock): self.api = OPDS2API(db.session, self.collection) - def fulfill(self) -> FulfillmentInfo: + def fulfill(self) -> Fulfillment: return self.api.fulfill(self.patron, "", self.pool, self.mechanism) @@ -794,17 +797,16 @@ def test_opds2_with_authentication_tokens( patron, "pin", work.license_pools[0], epub_mechanism ) + assert isinstance(fulfillment, RedirectFulfillment) assert ( fulfillment.content_link == "http://example.org//getDrmFreeFile.action?documentId=1543720&mediaType=epub&authToken=plaintext-token" ) assert fulfillment.content_type == "application/epub+zip" - assert fulfillment.content is None - assert fulfillment.content_expires is None - assert fulfillment.content_link_redirect is True def test_token_fulfill(self, opds2_api_fixture: Opds2ApiFixture): - ff_info = opds2_api_fixture.fulfill() + fulfillment = opds2_api_fixture.fulfill() + assert isinstance(fulfillment, RedirectFulfillment) patron_id = opds2_api_fixture.patron.identifier_to_remote_service( opds2_api_fixture.data_source @@ -817,20 +819,20 @@ def test_token_fulfill(self, opds2_api_fixture: Opds2ApiFixture): ) assert ( - ff_info.content_link + fulfillment.content_link == "http://example.org/11234/fulfill?authToken=plaintext-auth-token" ) - assert ff_info.content_link_redirect is True def test_token_fulfill_alternate_template(self, opds2_api_fixture: Opds2ApiFixture): # Alternative templating opds2_api_fixture.mechanism.resource.representation.public_url = ( "http://example.org/11234/fulfill{?authentication_token}" ) - ff_info = opds2_api_fixture.fulfill() + fulfillment = opds2_api_fixture.fulfill() + assert isinstance(fulfillment, RedirectFulfillment) assert ( - ff_info.content_link + fulfillment.content_link == "http://example.org/11234/fulfill?authentication_token=plaintext-auth-token" ) @@ -845,28 +847,13 @@ def test_token_fulfill_no_template(self, opds2_api_fixture: Opds2ApiFixture): opds2_api_fixture.mechanism.resource.representation.public_url = ( "http://example.org/11234/fulfill" ) - ff_info = opds2_api_fixture.fulfill() - assert ff_info.content_link_redirect is False + fulfillment = opds2_api_fixture.fulfill() + assert isinstance(fulfillment, RedirectFulfillment) assert ( - ff_info.content_link + fulfillment.content_link == opds2_api_fixture.mechanism.resource.representation.public_url ) - def test_token_fulfill_no_content_link( - self, opds2_api_fixture: Opds2ApiFixture, caplog: LogCaptureFixture - ): - # No content_link on the fulfillment info coming into the function - mock = MagicMock(spec=FulfillmentInfo) - mock.content_link = None - response = opds2_api_fixture.api.fulfill_token_auth( - opds2_api_fixture.patron, opds2_api_fixture.pool, mock - ) - assert response is mock - assert ( - "No content link found in fulfillment, unable to fulfill via OPDS2 token auth" - in caplog.text - ) - def test_token_fulfill_no_endpoint_config(self, opds2_api_fixture: Opds2ApiFixture): # No token endpoint config opds2_api_fixture.api.token_auth_configuration = None diff --git a/tests/manager/core/test_opds_import.py b/tests/manager/core/test_opds_import.py index c290efd68..eae542ded 100644 --- a/tests/manager/core/test_opds_import.py +++ b/tests/manager/core/test_opds_import.py @@ -10,7 +10,7 @@ from lxml import etree from psycopg2.extras import NumericRange -from palace.manager.api.circulation import CirculationAPI, FulfillmentInfo, LoanInfo +from palace.manager.api.circulation import CirculationAPI, LoanInfo, RedirectFulfillment from palace.manager.api.circulation_exceptions import ( CurrentlyAvailable, FormatNotAvailable, @@ -2255,9 +2255,6 @@ def test_fulfill(self, opds_api_fixture: OPDSAPIFixture) -> None: opds_api_fixture.mock_licensepool, mock_lpdm, ) - assert isinstance(fulfillment, FulfillmentInfo) + assert isinstance(fulfillment, RedirectFulfillment) assert fulfillment.content_link == mock_lpdm.resource.representation.public_url assert fulfillment.content_type == mock_lpdm.resource.representation.media_type - assert fulfillment.content is None - assert fulfillment.content_expires is None - assert fulfillment.collection_id == opds_api_fixture.collection.id diff --git a/tests/manager/feed/test_library_annotator.py b/tests/manager/feed/test_library_annotator.py index 176f9e70d..2150d6985 100644 --- a/tests/manager/feed/test_library_annotator.py +++ b/tests/manager/feed/test_library_annotator.py @@ -12,7 +12,7 @@ from palace.manager.api.circulation import ( BaseCirculationAPI, CirculationAPI, - FulfillmentInfo, + RedirectFulfillment, ) from palace.manager.api.lanes import ContributorLane from palace.manager.api.metadata.novelist import NoveListAPI, NoveListApiSettings @@ -1313,23 +1313,12 @@ def test_complete_catalog_entry_with_fulfillment_info_contains_self_link( ) pool = work.license_pools[0] loan, _ = pool.loan_to(patron) - fulfillment = FulfillmentInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, - "http://link", - Representation.EPUB_MEDIA_TYPE, - None, - None, - ) annotator = LibraryLoanAndHoldAnnotator(circulation, None, circulation.library) feed_obj = OPDSAcquisitionFeed.single_entry_loans_feed( circulation, loan, annotator, - fulfillment=fulfillment, ) raw = str(feed_obj) @@ -1375,15 +1364,9 @@ def test_fulfill_feed(self, annotator_fixture: LibraryAnnotatorFixture): now = utc_now() loan, ignore = pool.loan_to(patron, start=now) - fulfillment = FulfillmentInfo( - pool.collection, - pool.data_source.name, - pool.identifier.type, - pool.identifier.identifier, + fulfillment = RedirectFulfillment( "http://streaming_link", Representation.TEXT_HTML_MEDIA_TYPE + DeliveryMechanism.STREAMING_PROFILE, - None, - None, ) annotator = LibraryLoanAndHoldAnnotator(None, None, patron.library) @@ -1695,7 +1678,7 @@ def test_acquisition_links( # non-open-access works that are available without # authentication. To get such link, you pass in a list of # LicensePoolDeliveryMechanisms as - # `direct_fufillment_delivery_mechanisms`. + # `direct_fulfillment_delivery_mechanisms`. [lp4] = work4.license_pools [lpdm4] = lp4.delivery_mechanisms lpdm4.set_rights_status(RightsStatus.IN_COPYRIGHT) diff --git a/tests/mocks/circulation.py b/tests/mocks/circulation.py index e2e0657f1..18b3d3ec8 100644 --- a/tests/mocks/circulation.py +++ b/tests/mocks/circulation.py @@ -10,7 +10,7 @@ BaseCirculationAPI, CirculationAPI, CirculationApiType, - FulfillmentInfo, + Fulfillment, HoldInfo, LoanInfo, PatronActivityCirculationAPI, @@ -92,8 +92,8 @@ def fulfill( pin: str, licensepool: LicensePool, delivery_mechanism: LicensePoolDeliveryMechanism, - ) -> FulfillmentInfo: - # Should be a FulfillmentInfo. + ) -> Fulfillment: + # Should be a Fulfillment. return self._return_or_raise("fulfill") def checkin(self, patron: Patron, pin: str, licensepool: LicensePool) -> None: @@ -110,7 +110,7 @@ def queue_checkout(self, response: LoanInfo | HoldInfo | Exception) -> None: def queue_hold(self, response: HoldInfo | Exception) -> None: self._queue("hold", response) - def queue_fulfill(self, response: FulfillmentInfo | Exception) -> None: + def queue_fulfill(self, response: Fulfillment | Exception) -> None: self._queue("fulfill", response) def queue_checkin(self, response: None | Exception = None) -> None: @@ -154,7 +154,7 @@ def queue_hold( api.queue_hold(response) def queue_fulfill( - self, licensepool: LicensePool, response: FulfillmentInfo | Exception + self, licensepool: LicensePool, response: Fulfillment | Exception ) -> None: api = self.api_for_license_pool(licensepool) api.queue_fulfill(response)