From 0da3ac865c64ea28ad0d462277e16bdf28cdd1ca Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Thu, 22 Feb 2024 13:05:37 -0400 Subject: [PATCH] Refactor CirculationExceptions This refactors how we are handling CirculationExceptions, so they all inherit from BaseProblemDetailException and are able to be turned into a ProblemDetail. --- api/admin/exceptions.py | 19 +- api/admin/routes.py | 6 +- api/axis.py | 30 ++- api/bibliotheca.py | 52 ++-- api/circulation.py | 21 +- api/circulation_exceptions.py | 301 ++++++++++++++--------- api/controller/base.py | 2 +- api/controller/loan.py | 102 +------- api/enki.py | 28 ++- api/odl.py | 13 +- api/overdrive.py | 30 ++- api/problem_details.py | 12 +- api/util/patron.py | 11 +- core/app_server.py | 16 +- core/exceptions.py | 2 +- core/opds2_import.py | 4 +- core/opds_import.py | 4 +- core/user_profile.py | 17 +- core/util/http.py | 95 ++++--- core/util/problem_detail.py | 30 ++- pyproject.toml | 1 + tests/api/controller/test_loan.py | 42 +++- tests/api/metadata/test_nyt.py | 1 + tests/api/test_axis.py | 11 +- tests/api/test_bibliotheca.py | 70 ++---- tests/api/test_circulation_exceptions.py | 160 ++++++++++-- tests/api/test_circulationapi.py | 16 +- tests/api/test_enki.py | 14 +- tests/api/test_opds_for_distributors.py | 5 +- tests/api/test_overdrive.py | 13 +- tests/api/test_patron_utility.py | 6 +- tests/core/test_app_server.py | 13 +- tests/core/util/test_http.py | 184 ++++++++------ 33 files changed, 816 insertions(+), 515 deletions(-) diff --git a/api/admin/exceptions.py b/api/admin/exceptions.py index 37386cae95..410b19a227 100644 --- a/api/admin/exceptions.py +++ b/api/admin/exceptions.py @@ -1,19 +1,14 @@ -from typing import Any - from api.admin.problem_details import ADMIN_NOT_AUTHORIZED -from core.util.problem_detail import ProblemDetail - +from core.util.problem_detail import BaseProblemDetailException, ProblemDetail -class AdminNotAuthorized(Exception): - status_code = 403 - def __init__(self, *args: Any) -> None: - self.message = None - if len(args) > 0: - self.message = args[0] - super().__init__(*args) +class AdminNotAuthorized(BaseProblemDetailException): + def __init__(self, message: str | None = None) -> None: + self.message = message + super().__init__(message) - def as_problem_detail_document(self, debug=False) -> ProblemDetail: + @property + def problem_detail(self) -> ProblemDetail: return ( ADMIN_NOT_AUTHORIZED.detailed(self.message) if self.message diff --git a/api/admin/routes.py b/api/admin/routes.py index 26e43b5f55..9ed2aa1f8c 100644 --- a/api/admin/routes.py +++ b/api/admin/routes.py @@ -23,8 +23,8 @@ from api.routes import allows_library, has_library, library_route from core.app_server import ensure_pydantic_after_problem_detail, returns_problem_detail from core.util.problem_detail import ( + BaseProblemDetailException, ProblemDetail, - ProblemDetailException, ProblemDetailModel, ) @@ -95,8 +95,8 @@ def returns_json_or_response_or_problem_detail(f): def decorated(*args, **kwargs): try: v = f(*args, **kwargs) - except ProblemDetailException as ex: - # A ProblemError is the same as a ProblemDetail + except BaseProblemDetailException as ex: + # A ProblemDetailException just needs to be converted to a ProblemDetail. v = ex.problem_detail if isinstance(v, ProblemDetail): return v.response diff --git a/api/axis.py b/api/axis.py index 346923d6a0..3ef0d210ab 100644 --- a/api/axis.py +++ b/api/axis.py @@ -33,12 +33,31 @@ LoanInfo, PatronActivityCirculationAPI, ) -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + AlreadyCheckedOut, + AlreadyOnHold, + CannotFulfill, + CannotHold, + CannotLoan, + CurrentlyAvailable, + InvalidInputException, + LibraryAuthorizationFailedException, + LibraryInvalidInputException, + NoAcceptableFormat, + NoActiveLoan, + NoAvailableCopies, + NotFoundOnRemote, + NotOnHold, + PatronAuthorizationFailedException, + PatronLoanLimitReached, + RemoteInitiatedServerError, +) from api.selftest import HasCollectionSelfTests, SelfTestResult from api.web_publication_manifest import FindawayManifest, SpineItem from core.analytics import Analytics from core.config import CannotLoadConfiguration from core.coverage import BibliographicCoverageProvider, CoverageFailure +from core.exceptions import IntegrationException from core.integration.settings import ( ConfigurationFormItem, ConfigurationFormItemType, @@ -74,6 +93,7 @@ Subject, ) from core.monitor import CollectionMonitor, IdentifierSweepMonitor, TimelineMonitor +from core.problem_details import INTEGRATION_ERROR from core.service.container import Services from core.util.datetime_helpers import datetime_utc, strptime_utc, utc_now from core.util.flask_util import Response @@ -382,8 +402,8 @@ def checkin(self, patron: Patron, pin: str, licensepool: LicensePool) -> None: CheckinResponseParser(licensepool.collection).process_first( response.content ) - except etree.XMLSyntaxError as e: - raise RemoteInitiatedServerError(response.content, self.label()) + except etree.XMLSyntaxError: + raise RemoteInitiatedServerError(response.text, self.label()) def _checkin(self, title_id: str | None, patron_id: str | None) -> RequestsResponse: """Make a request to the EarlyCheckInTitle endpoint.""" @@ -424,8 +444,8 @@ def checkout( if loan_info is None: raise CannotLoan() return loan_info - except etree.XMLSyntaxError as e: - raise RemoteInitiatedServerError(response.content, self.label()) + except etree.XMLSyntaxError: + raise RemoteInitiatedServerError(response.text, self.label()) def _checkout( self, title_id: str | None, patron_id: str | None, internal_format: str diff --git a/api/bibliotheca.py b/api/bibliotheca.py index fc6289b335..c0f629e58f 100644 --- a/api/bibliotheca.py +++ b/api/bibliotheca.py @@ -30,7 +30,21 @@ LoanInfo, PatronActivityCirculationAPI, ) -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + AlreadyCheckedOut, + AlreadyOnHold, + CannotHold, + CannotLoan, + CannotReleaseHold, + CurrentlyAvailable, + NoAvailableCopies, + NoLicenses, + NotCheckedOut, + NotOnHold, + PatronHoldLimitReached, + PatronLoanLimitReached, + RemoteInitiatedServerError, +) from api.selftest import HasCollectionSelfTests, SelfTestResult from api.web_publication_manifest import FindawayManifest, SpineItem from core.analytics import Analytics @@ -79,6 +93,7 @@ from core.util import base64 from core.util.datetime_helpers import datetime_utc, strptime_utc, to_utc, utc_now from core.util.http import HTTP +from core.util.problem_detail import BaseProblemDetailException from core.util.xmlparser import XMLParser, XMLProcessor @@ -894,23 +909,7 @@ def date_from_subtag(self, tag, key, required=True): return self.parse_date(value) -class BibliothecaException(Exception): - pass - - -class WorkflowException(BibliothecaException): - def __init__(self, actual_status, statuses_that_would_work): - self.actual_status = actual_status - self.statuses_that_would_work = statuses_that_would_work - - def __str__(self): - return "Book status is {}, must be: {}".format( - self.actual_status, - ", ".join(self.statuses_that_would_work), - ) - - -class ErrorParser(BibliothecaParser[Exception]): +class ErrorParser(BibliothecaParser[BaseProblemDetailException]): """Turn an error document from the Bibliotheca web service into a CheckoutException""" wrong_status = re.compile( @@ -930,13 +929,20 @@ class ErrorParser(BibliothecaParser[Exception]): def xpath_expression(self) -> str: return "//Error" - def process_first(self, string: str | bytes) -> Exception: + def process_first(self, string: str | bytes) -> BaseProblemDetailException: try: return_val = super().process_first(string) except Exception as e: # The server sent us an error with an incorrect or # nonstandard syntax. - return RemoteInitiatedServerError(string, BibliothecaAPI.SERVICE_NAME) + if isinstance(string, bytes): + try: + debug = string.decode("utf-8") + except UnicodeDecodeError: + debug = "Unreadable error message (Unicode decode error)." + else: + debug = string + return RemoteInitiatedServerError(debug, BibliothecaAPI.SERVICE_NAME) if return_val is None: # We were not able to interpret the result as an error. @@ -950,7 +956,7 @@ def process_first(self, string: str | bytes) -> Exception: def process_one( self, error_tag: _Element, namespaces: dict[str, str] | None - ) -> Exception: + ) -> BaseProblemDetailException: message = self.text_of_optional_subtag(error_tag, "Message") if not message: return RemoteInitiatedServerError( @@ -982,7 +988,7 @@ def process_one( m = self.wrong_status.search(message) if not m: - return BibliothecaException(message) + return RemoteInitiatedServerError(message, BibliothecaAPI.SERVICE_NAME) actual, expected = m.groups() expected = expected.split(",") @@ -1010,7 +1016,7 @@ def process_one( if "CAN_LOAN" in expected: return CannotLoan(message) - return BibliothecaException(message) + return RemoteInitiatedServerError(message, BibliothecaAPI.SERVICE_NAME) class PatronCirculationParser(XMLParser): diff --git a/api/circulation.py b/api/circulation.py index 7a3195142c..74dedb1a32 100644 --- a/api/circulation.py +++ b/api/circulation.py @@ -16,7 +16,25 @@ from pydantic import PositiveInt from sqlalchemy.orm import Query -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + AlreadyCheckedOut, + AlreadyOnHold, + CannotFulfill, + CannotRenew, + CannotReturn, + CurrentlyAvailable, + DeliveryMechanismConflict, + DeliveryMechanismError, + DeliveryMechanismMissing, + NoAcceptableFormat, + NoActiveLoan, + NoAvailableCopies, + NoLicenses, + NotCheckedOut, + NotOnHold, + PatronHoldLimitReached, + PatronLoanLimitReached, +) from api.util.patron import PatronUtility from core.analytics import Analytics from core.integration.base import HasLibraryIntegrationConfiguration @@ -45,6 +63,7 @@ from core.model.integration import IntegrationConfiguration from core.util.datetime_helpers import utc_now from core.util.log import LoggerMixin +from core.util.problem_detail import ProblemDetail class CirculationInfo: diff --git a/api/circulation_exceptions.py b/api/circulation_exceptions.py index 094c7b613c..67dcfd1dee 100644 --- a/api/circulation_exceptions.py +++ b/api/circulation_exceptions.py @@ -1,96 +1,126 @@ +from abc import ABC, abstractmethod + from flask_babel import lazy_gettext as _ from api.problem_details import ( + BAD_DELIVERY_MECHANISM, BLOCKED_CREDENTIALS, + CANNOT_FULFILL, + CANNOT_RELEASE_HOLD, + CHECKOUT_FAILED, + COULD_NOT_MIRROR_TO_REMOTE, + DELIVERY_CONFLICT, EXPIRED_CREDENTIALS, + HOLD_FAILED, HOLD_LIMIT_REACHED, + INVALID_CREDENTIALS, LOAN_LIMIT_REACHED, + NO_ACCEPTABLE_FORMAT, + NO_ACTIVE_LOAN, NO_LICENSES, + NOT_FOUND_ON_REMOTE, + OUTSTANDING_FINES, + RENEW_FAILED, SPECIFIC_HOLD_LIMIT_MESSAGE, SPECIFIC_LOAN_LIMIT_MESSAGE, ) from core.config import IntegrationException -from core.problem_details import INTEGRATION_ERROR, INTERNAL_SERVER_ERROR -from core.util.problem_detail import ProblemDetail - - -class CirculationException(IntegrationException): - """An exception occured when carrying out a circulation operation. +from core.problem_details import INTEGRATION_ERROR, INTERNAL_SERVER_ERROR, INVALID_INPUT +from core.util import MoneyUtility +from core.util.problem_detail import BaseProblemDetailException, ProblemDetail - `status_code` is the status code that should be returned to the patron. - """ - status_code = 400 +class CirculationException(IntegrationException, BaseProblemDetailException, ABC): + """An exception occurred when carrying out a circulation operation.""" - def __init__(self, message=None, debug_info=None): - message = message or self.__class__.__name__ - super().__init__(message, debug_info) + def __init__( + self, message: str | None = None, debug_info: str | None = None + ) -> None: + self.message = message + super().__init__(message or self.__class__.__name__, debug_info) + @property + def detail(self) -> str | None: + return self.message -class InternalServerError(IntegrationException): - status_code = 500 + @property + @abstractmethod + def base(self) -> ProblemDetail: + """A ProblemDetail, used as the basis for conversion of this exception into a + problem detail document.""" - def as_problem_detail_document(self, debug=False): + @property + def problem_detail(self) -> ProblemDetail: """Return a suitable problem detail document.""" + if self.detail is not None: + return self.base.detailed( + detail=self.detail, debug_message=self.debug_message + ) + elif self.debug_message is not None: + return self.base.with_debug(self.debug_message) + else: + return self.base + + +class InternalServerError(IntegrationException, BaseProblemDetailException): + @property + def problem_detail(self) -> ProblemDetail: return INTERNAL_SERVER_ERROR class RemoteInitiatedServerError(InternalServerError): """One of the servers we communicate with had an internal error.""" - status_code = 502 - - def __init__(self, message, service_name): - super().__init__(message) + def __init__(self, debug_info: str, service_name: str): + super().__init__(debug_info) self.service_name = service_name - def as_problem_detail_document(self, debug=False): - """Return a suitable problem detail document.""" + @property + def problem_detail(self) -> ProblemDetail: msg = _( "Integration error communicating with %(service_name)s", service_name=self.service_name, ) - return INTEGRATION_ERROR.detailed(msg) - - -class NoOpenAccessDownload(CirculationException): - """We expected a book to have an open-access download, but it didn't.""" - - status_code = 500 - - -class AuthorizationFailedException(CirculationException): - status_code = 401 + return INTEGRATION_ERROR.detailed(msg, debug_message=str(self)) -class PatronAuthorizationFailedException(AuthorizationFailedException): - status_code = 400 - - -class RemotePatronCreationFailedException(CirculationException): - status_code = 500 +class PatronAuthorizationFailedException(CirculationException): + @property + def base(self) -> ProblemDetail: + return INVALID_CREDENTIALS class LibraryAuthorizationFailedException(CirculationException): - status_code = 500 + @property + def base(self) -> ProblemDetail: + return INTEGRATION_ERROR class InvalidInputException(CirculationException): """The patron gave invalid input to the library.""" - status_code = 400 + @property + def base(self) -> ProblemDetail: + return INVALID_INPUT.detailed("The patron gave invalid input to the library.") class LibraryInvalidInputException(InvalidInputException): """The library gave invalid input to the book provider.""" - status_code = 500 + @property + def base(self) -> ProblemDetail: + return INVALID_INPUT.detailed( + "The library gave invalid input to the book provider." + ) class DeliveryMechanismError(InvalidInputException): - status_code = 400 """The patron broke the rules about delivery mechanisms.""" + @property + def base(self) -> ProblemDetail: + return BAD_DELIVERY_MECHANISM + class DeliveryMechanismMissing(DeliveryMechanismError): """The patron needed to specify a delivery mechanism and didn't.""" @@ -101,25 +131,57 @@ class DeliveryMechanismConflict(DeliveryMechanismError): one already set in stone. """ + @property + def base(self) -> ProblemDetail: + return DELIVERY_CONFLICT + class CannotLoan(CirculationException): - status_code = 500 + @property + def base(self) -> ProblemDetail: + return CHECKOUT_FAILED class OutstandingFines(CannotLoan): """The patron has outstanding fines above the limit in the library's policy.""" - status_code = 403 + def __init__( + self, + message: str | None = None, + debug_info: str | None = None, + fines: str | None = None, + ) -> None: + parsed_fines = None + if fines: + try: + parsed_fines = MoneyUtility.parse(fines).amount + except ValueError: + # If the fines are not in a valid format, we'll just leave them as None. + ... + + self.fines = parsed_fines + super().__init__(message, debug_info) + + @property + def detail(self) -> str | None: + if self.fines: + return _( # type: ignore[no-any-return] + "You must pay your $%(fine_amount).2f outstanding fines before you can borrow more books.", + fine_amount=self.fines, + ) + return super().detail + + @property + def base(self) -> ProblemDetail: + return OUTSTANDING_FINES class AuthorizationExpired(CannotLoan): """The patron's authorization has expired.""" - status_code = 403 - - def as_problem_detail_document(self, debug=False): - """Return a suitable problem detail document.""" + @property + def base(self) -> ProblemDetail: return EXPIRED_CREDENTIALS @@ -130,89 +192,106 @@ class AuthorizationBlocked(CannotLoan): For instance, the patron has been banned from the library. """ - status_code = 403 - - def as_problem_detail_document(self, debug=False): - """Return a suitable problem detail document.""" + @property + def base(self) -> ProblemDetail: return BLOCKED_CREDENTIALS -class LimitReached(CirculationException): +class LimitReached(CirculationException, ABC): """The patron cannot carry out an operation because it would push them above some limit set by library policy. - - This exception cannot be used on its own. It must be subclassed and the following constants defined: - * `BASE_DOC`: A ProblemDetail, used as the basis for conversion of this exception into a - problem detail document. - * `MESSAGE_WITH_LIMIT` A string containing the interpolation value "%(limit)s", which - offers a more specific explanation of the limit exceeded. """ - status_code = 403 - BASE_DOC: ProblemDetail | None = None - MESSAGE_WITH_LIMIT = None - - def __init__(self, message=None, debug_info=None, limit=None): - super().__init__(message=message, debug_info=debug_info) - self.limit = limit if limit else None - - def as_problem_detail_document(self, debug=False): - """Return a suitable problem detail document.""" - doc = self.BASE_DOC - if not self.limit: - return doc - detail = self.MESSAGE_WITH_LIMIT % dict(limit=self.limit) - return doc.detailed(detail=detail) + def __init__( + self, + message: str | None = None, + debug_info: str | None = None, + limit: int | None = None, + ): + super().__init__(message, debug_info=debug_info) + self.limit = limit + + @property + @abstractmethod + def message_with_limit(self) -> str: + """A string containing the interpolation value "%(limit)s", which + offers a more specific explanation of the limit exceeded.""" + + @property + def detail(self) -> str | None: + if self.limit: + return self.message_with_limit % dict(limit=self.limit) + elif self.message: + return self.message + return None class PatronLoanLimitReached(CannotLoan, LimitReached): - BASE_DOC = LOAN_LIMIT_REACHED - MESSAGE_WITH_LIMIT = SPECIFIC_LOAN_LIMIT_MESSAGE + @property + def base(self) -> ProblemDetail: + return LOAN_LIMIT_REACHED + + @property + def message_with_limit(self) -> str: + return SPECIFIC_LOAN_LIMIT_MESSAGE # type: ignore[no-any-return] class CannotReturn(CirculationException): - status_code = 500 + @property + def base(self) -> ProblemDetail: + return COULD_NOT_MIRROR_TO_REMOTE class CannotHold(CirculationException): - status_code = 500 + @property + def base(self) -> ProblemDetail: + return HOLD_FAILED class PatronHoldLimitReached(CannotHold, LimitReached): - BASE_DOC = HOLD_LIMIT_REACHED - MESSAGE_WITH_LIMIT = SPECIFIC_HOLD_LIMIT_MESSAGE + @property + def base(self) -> ProblemDetail: + return HOLD_LIMIT_REACHED + + @property + def message_with_limit(self) -> str: + return SPECIFIC_HOLD_LIMIT_MESSAGE # type: ignore[no-any-return] class CannotReleaseHold(CirculationException): - status_code = 500 + @property + def base(self) -> ProblemDetail: + return CANNOT_RELEASE_HOLD class CannotFulfill(CirculationException): - status_code = 500 - - -class CannotPartiallyFulfill(CannotFulfill): - status_code = 400 + @property + def base(self) -> ProblemDetail: + return CANNOT_FULFILL class FormatNotAvailable(CannotFulfill): """Our format information for this book was outdated, and it's no longer available in the requested format.""" - status_code = 502 + @property + def base(self) -> ProblemDetail: + return NO_ACCEPTABLE_FORMAT class NotFoundOnRemote(CirculationException): """We know about this book but the remote site doesn't seem to.""" - status_code = 404 + @property + def base(self) -> ProblemDetail: + return NOT_FOUND_ON_REMOTE class NoLicenses(NotFoundOnRemote): """The library no longer has licenses for this book.""" - def as_problem_detail_document(self, debug=False): - """Return a suitable problem detail document.""" + @property + def base(self) -> ProblemDetail: return NO_LICENSES @@ -222,7 +301,9 @@ class CannotRenew(CirculationException): Probably because it's not available for renewal. """ - status_code = 400 + @property + def base(self) -> ProblemDetail: + return RENEW_FAILED class NoAvailableCopies(CannotLoan): @@ -230,59 +311,43 @@ class NoAvailableCopies(CannotLoan): copies are already checked out. """ - status_code = 400 - class AlreadyCheckedOut(CannotLoan): """The patron can't put check this book out because they already have it checked out. """ - status_code = 400 - class AlreadyOnHold(CannotHold): """The patron can't put this book on hold because they already have it on hold. """ - status_code = 400 - class NotCheckedOut(CannotReturn): """The patron can't return this book because they don't have it checked out in the first place. """ - status_code = 400 - - -class RemoteRefusedReturn(CannotReturn): - """The remote refused to count this book as returned.""" - - status_code = 500 - class NotOnHold(CannotReleaseHold): """The patron can't release a hold for this book because they don't have it on hold in the first place. """ - status_code = 400 - class CurrentlyAvailable(CannotHold): """The patron can't put this book on hold because it's available now.""" - status_code = 400 - class NoAcceptableFormat(CannotFulfill): """We can't fulfill the patron's loan because the book is not available in an acceptable format. """ - status_code = 400 + @property + def base(self) -> ProblemDetail: + return super().base.detailed("No acceptable format", status_code=400) class FulfilledOnIncompatiblePlatform(CannotFulfill): @@ -291,7 +356,11 @@ class FulfilledOnIncompatiblePlatform(CannotFulfill): exclusive to that platform. """ - status_code = 451 + @property + def base(self) -> ProblemDetail: + return super().base.detailed( + "Fulfilled on an incompatible platform", status_code=451 + ) class NoActiveLoan(CannotFulfill): @@ -299,8 +368,8 @@ class NoActiveLoan(CannotFulfill): active loan. """ - status_code = 400 - - -class PatronNotFoundOnRemote(NotFoundOnRemote): - status_code = 404 + @property + def base(self) -> ProblemDetail: + return NO_ACTIVE_LOAN.detailed( + "Can't fulfill loan because you have no active loan for this book.", + ) diff --git a/api/controller/base.py b/api/controller/base.py index 2eed5dcd70..c28b4bd359 100644 --- a/api/controller/base.py +++ b/api/controller/base.py @@ -3,7 +3,7 @@ from flask_babel import lazy_gettext as _ from werkzeug.datastructures import Authorization -from api.circulation_exceptions import * +from api.circulation_exceptions import RemoteInitiatedServerError from api.problem_details import ( INVALID_CREDENTIALS, LIBRARY_NOT_FOUND, diff --git a/api/controller/loan.py b/api/controller/loan.py index 27ad91d00a..ca2ab32e8d 100644 --- a/api/controller/loan.py +++ b/api/controller/loan.py @@ -8,45 +8,15 @@ from lxml import etree from werkzeug import Response as wkResponse -from api.circulation_exceptions import ( - AuthorizationBlocked, - AuthorizationExpired, - CannotFulfill, - CannotHold, - CannotLoan, - CannotReleaseHold, - CannotRenew, - CannotReturn, - CirculationException, - DeliveryMechanismConflict, - DeliveryMechanismError, - FormatNotAvailable, - NoActiveLoan, - NoOpenAccessDownload, - NotFoundOnRemote, - OutstandingFines, - PatronAuthorizationFailedException, - PatronHoldLimitReached, - PatronLoanLimitReached, - RemoteRefusedReturn, -) +from api.circulation_exceptions import CirculationException from api.controller.circulation_manager import CirculationManagerController from api.problem_details import ( BAD_DELIVERY_MECHANISM, - CANNOT_FULFILL, CANNOT_RELEASE_HOLD, - CHECKOUT_FAILED, - COULD_NOT_MIRROR_TO_REMOTE, - DELIVERY_CONFLICT, HOLD_FAILED, - INVALID_CREDENTIALS, - NO_ACCEPTABLE_FORMAT, NO_ACTIVE_LOAN, NO_ACTIVE_LOAN_OR_HOLD, NO_LICENSES, - NOT_FOUND_ON_REMOTE, - OUTSTANDING_FINES, - RENEW_FAILED, ) from core.feed.acquisition import OPDSAcquisitionFeed from core.model import DataSource, DeliveryMechanism, Loan, Patron, Representation @@ -170,48 +140,14 @@ def _borrow(self, patron, credential, pool, mechanism): created but a Hold could be), or a ProblemDetail (if the entire operation failed). """ - result = None is_new = False try: loan, hold, is_new = self.circulation.borrow( patron, credential, pool, mechanism ) result = loan or hold - except NoOpenAccessDownload as e: - result = NO_LICENSES.detailed( - _("Couldn't find an open-access download link for this book."), - status_code=404, - ) - except PatronAuthorizationFailedException as e: - result = INVALID_CREDENTIALS - except (PatronLoanLimitReached, PatronHoldLimitReached) as e: - result = e.as_problem_detail_document().with_debug(str(e)) - except DeliveryMechanismError as e: - result = BAD_DELIVERY_MECHANISM.with_debug( - str(e), status_code=e.status_code - ) - except OutstandingFines as e: - result = OUTSTANDING_FINES.detailed( - _( - "You must pay your $%(fine_amount).2f outstanding fines before you can borrow more books.", - fine_amount=patron.fines, - ) - ) - except AuthorizationExpired as e: - result = e.as_problem_detail_document(debug=False) - except AuthorizationBlocked as e: - result = e.as_problem_detail_document(debug=False) - except CannotLoan as e: - result = CHECKOUT_FAILED.with_debug(str(e)) - except CannotHold as e: - result = HOLD_FAILED.with_debug(str(e)) - except CannotRenew as e: - result = RENEW_FAILED.with_debug(str(e)) - except NotFoundOnRemote as e: - result = NOT_FOUND_ON_REMOTE except CirculationException as e: - # Generic circulation error. - result = CHECKOUT_FAILED.with_debug(str(e)) + result = e.problem_detail if result is None: # This shouldn't happen, but if it does, it means no exception @@ -386,19 +322,8 @@ def fulfill( requested_license_pool, mechanism, ) - except DeliveryMechanismConflict as e: - return DELIVERY_CONFLICT.detailed(str(e)) - except NoActiveLoan as e: - return NO_ACTIVE_LOAN.detailed( - _("Can't fulfill loan because you have no active loan for this book."), - status_code=e.status_code, - ) - except FormatNotAvailable as e: - return NO_ACCEPTABLE_FORMAT.with_debug(str(e), status_code=e.status_code) - except CannotFulfill as e: - return CANNOT_FULFILL.with_debug(str(e), status_code=e.status_code) - except DeliveryMechanismError as e: - return BAD_DELIVERY_MECHANISM.with_debug(str(e), status_code=e.status_code) + except CirculationException as e: + return e.problem_detail # A subclass of FulfillmentInfo may want to bypass the whole # response creation process. @@ -454,7 +379,7 @@ def fulfill( ) headers = dict(resp_headers) except RemoteIntegrationException as e: - return e.as_problem_detail_document(debug=False) + return e.problem_detail else: status_code = 200 if fulfillment.content_type: @@ -521,25 +446,16 @@ def revoke(self, license_pool_id): if loan: try: self.circulation.revoke_loan(patron, credential, pool) - except RemoteRefusedReturn as e: - title = _( - "Loan deleted locally but remote refused. Loan is likely to show up again on next sync." - ) - return COULD_NOT_MIRROR_TO_REMOTE.detailed(title, status_code=503) - except CannotReturn as e: - title = _("Loan deleted locally but remote failed.") - return COULD_NOT_MIRROR_TO_REMOTE.detailed(title, 503).with_debug( - str(e) - ) + except CirculationException as e: + return e.problem_detail elif hold: if not self.circulation.can_revoke_hold(pool, hold): title = _("Cannot release a hold once it enters reserved state.") return CANNOT_RELEASE_HOLD.detailed(title, 400) try: self.circulation.release_hold(patron, credential, pool) - except CannotReleaseHold as e: - title = _("Hold released locally but remote failed.") - return CANNOT_RELEASE_HOLD.detailed(title, 503).with_debug(str(e)) + except CirculationException as e: + return e.problem_detail work = pool.work annotator = self.manager.annotator(None) diff --git a/api/enki.py b/api/enki.py index 278cb81fd3..080115f1dd 100644 --- a/api/enki.py +++ b/api/enki.py @@ -21,7 +21,13 @@ LoanInfo, PatronActivityCirculationAPI, ) -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + CannotFulfill, + CannotLoan, + NoAvailableCopies, + PatronAuthorizationFailedException, + RemoteInitiatedServerError, +) from api.selftest import HasCollectionSelfTests, SelfTestResult from core.analytics import Analytics from core.config import ConfigurationAttributeValue @@ -404,7 +410,9 @@ def checkout( patron.authorization_identifier, pin, enki_id, enki_library_id ) if response.status_code != 200: - raise CannotLoan(response.status_code) + raise CannotLoan( + debug_info=f"Unexpected HTTP status: {response.status_code}" + ) result = json.loads(response.content)["result"] if not result["success"]: message = result["message"] @@ -416,7 +424,7 @@ def checkout( "User validation against Enki server with %s / %s was unsuccessful." % (patron.authorization_identifier, pin) ) - raise AuthorizationFailedException() + raise PatronAuthorizationFailedException() due_date = result["checkedOutItems"][0]["duedate"] expires = self._epoch_to_struct(due_date) @@ -471,7 +479,9 @@ def fulfill( patron.authorization_identifier, pin, book_id, enki_library_id ) if response.status_code != 200: - raise CannotFulfill(response.status_code) + raise CannotFulfill( + debug_info=f"Unexpected HTTP status: {response.status_code}" + ) result = json.loads(response.content)["result"] if not result["success"]: message = result["message"] @@ -483,7 +493,7 @@ def fulfill( "User validation against Enki server with %s / %s was unsuccessful." % (patron.authorization_identifier, pin) ) - raise AuthorizationFailedException() + raise PatronAuthorizationFailedException() url, item_type, expires = self.parse_fulfill_result(result) # We don't know for sure which DRM scheme is in use, (that is, @@ -526,17 +536,19 @@ def patron_activity( patron.authorization_identifier, pin, enki_library_id ) if response.status_code != 200: - raise PatronNotFoundOnRemote(response.status_code) + raise PatronAuthorizationFailedException( + debug_info=f"Unexpected HTTP status: {response.status_code}" + ) result = json.loads(response.content).get("result", {}) if not result.get("success"): message = result.get("message", "") if "Login unsuccessful" in message: - raise AuthorizationFailedException() + raise PatronAuthorizationFailedException() else: self.log.error( "Unexpected error in patron_activity: %r", response.content ) - raise CirculationException(response.content) + raise RemoteInitiatedServerError(response.text, self.label()) for loan in result["checkedOutItems"]: yield self.parse_patron_loans(loan) for type, holds in list(result["holds"].items()): diff --git a/api/odl.py b/api/odl.py index dc4595405d..4fb7f53485 100644 --- a/api/odl.py +++ b/api/odl.py @@ -26,7 +26,18 @@ LoanInfo, PatronActivityCirculationAPI, ) -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + AlreadyCheckedOut, + AlreadyOnHold, + CannotFulfill, + CannotLoan, + CurrentlyAvailable, + FormatNotAvailable, + NoAvailableCopies, + NoLicenses, + NotCheckedOut, + NotOnHold, +) from api.lcp.hash import Hasher, HasherFactory, HashingAlgorithm from core import util from core.integration.settings import ( diff --git a/api/overdrive.py b/api/overdrive.py index c2e24b2f3f..3bfc054adb 100644 --- a/api/overdrive.py +++ b/api/overdrive.py @@ -34,13 +34,27 @@ LoanInfo, PatronActivityCirculationAPI, ) -from api.circulation_exceptions import * -from api.circulation_exceptions import CannotFulfill +from api.circulation_exceptions import ( + CannotFulfill, + CannotHold, + CannotLoan, + CannotReleaseHold, + CannotRenew, + FormatNotAvailable, + FulfilledOnIncompatiblePlatform, + NoAcceptableFormat, + NoActiveLoan, + NoAvailableCopies, + PatronAuthorizationFailedException, + PatronHoldLimitReached, + PatronLoanLimitReached, +) from api.selftest import HasCollectionSelfTests, SelfTestResult from core.analytics import Analytics from core.config import CannotLoadConfiguration, Configuration from core.connection_config import ConnectionSetting from core.coverage import BibliographicCoverageProvider +from core.exceptions import IntegrationException from core.integration.base import ( HasChildIntegrationConfiguration, integration_settings_update, @@ -533,9 +547,9 @@ def get( response: Response = self._do_get( url, request_headers, allowed_response_codes=["2xx", "3xx", "401", "404"] ) - status_code: int = response.status_code - headers: CaseInsensitiveDict = response.headers - content: bytes = response.content + status_code = response.status_code + headers = response.headers + content = response.content if status_code == 401: if exception_on_401: @@ -569,7 +583,7 @@ def fulfillment_authorization_header(self) -> str: testing=is_test_mode ) except CannotLoadConfiguration as e: - raise CannotFulfill(*e.args) + raise CannotFulfill() from e s = b"%s:%s" % ( client_credentials["key"].encode(), @@ -963,7 +977,6 @@ def refresh_patron_access_token( error = response.get("error_description") if error: message += "/" + error - diagnostic = None debug = message if error == "Requested record not found": debug = "The patron failed Overdrive's cross-check against the library's ILS." @@ -1684,7 +1697,7 @@ def release_hold(self, patron, pin, licensepool): if data["errorCode"] == "PatronDoesntHaveTitleOnHold": # There was never a hold to begin with, so we're fine. return True - raise CannotReleaseHold(response.content) + raise CannotReleaseHold(debug_info=response.text) def circulation_lookup(self, book): if isinstance(book, str): @@ -1854,7 +1867,6 @@ def get_download_link(self, checkout_response, format_type, error_url): else: # We don't know what happened -- most likely our # format data is bad. - format_list = ", ".join(available_formats) msg = "Could not find specified format %s. Available formats: %s" raise NoAcceptableFormat( msg % (use_format_type, ", ".join(available_formats)) diff --git a/api/problem_details.py b/api/problem_details.py index aab43dba57..429608e3d5 100644 --- a/api/problem_details.py +++ b/api/problem_details.py @@ -57,7 +57,7 @@ NO_ACCEPTABLE_FORMAT = pd( "http://librarysimplified.org/terms/problem/no-acceptable-format", - 400, + 502, _("No acceptable format."), _("Could not deliver this book in an acceptable format."), ) @@ -167,8 +167,8 @@ COULD_NOT_MIRROR_TO_REMOTE = pd( "http://librarysimplified.org/terms/problem/cannot-mirror-to-remote", - 502, - _("Could not mirror local state to remote."), + 503, + _("Loan deleted locally but remote failed."), _( "Could not convince a third party to accept the change you made. It's likely to show up again soon." ), @@ -216,7 +216,7 @@ CANNOT_FULFILL = pd( "http://librarysimplified.org/terms/problem/cannot-fulfill-loan", - 400, + 500, _("Could not fulfill loan."), _("Could not fulfill loan."), ) @@ -237,8 +237,8 @@ CANNOT_RELEASE_HOLD = pd( "http://librarysimplified.org/terms/problem/cannot-release-hold", - 400, - _("Could not release hold."), + 503, + _("Hold released locally but remote failed."), _("Could not release hold."), ) diff --git a/api/util/patron.py b/api/util/patron.py index f2b68b2d04..a8084102ae 100644 --- a/api/util/patron.py +++ b/api/util/patron.py @@ -3,7 +3,12 @@ import dateutil from money import Money -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + AuthorizationBlocked, + AuthorizationExpired, + CannotLoan, + OutstandingFines, +) from api.config import Configuration from core.model.patron import Patron from core.util import MoneyUtility @@ -69,7 +74,7 @@ def assert_borrowing_privileges(cls, patron): raise AuthorizationExpired() if cls.has_excess_fines(patron): - raise OutstandingFines() + raise OutstandingFines(fines=patron.fines) from api.authentication.base import PatronData @@ -83,7 +88,7 @@ def assert_borrowing_privileges(cls, patron): # The authentication mechanism itself may know that # the patron has outstanding fines, even if the circulation # manager is not configured to make that deduction. - raise OutstandingFines() + raise OutstandingFines(fines=patron.fines) raise AuthorizationBlocked() diff --git a/core/app_server.py b/core/app_server.py index c17b4aa937..25ea1009c5 100644 --- a/core/app_server.py +++ b/core/app_server.py @@ -20,7 +20,7 @@ from core.problem_details import INVALID_URN from core.util.log import LoggerMixin from core.util.opds_writer import OPDSMessage -from core.util.problem_detail import ProblemDetail, ProblemDetailException +from core.util.problem_detail import BaseProblemDetailException, ProblemDetail if TYPE_CHECKING: from api.util.flask import PalaceFlask @@ -189,16 +189,10 @@ def handle(self, exception: Exception) -> Response | HTTPException: # By default, the error will be logged at log level ERROR. log_method = self.log.error - document = None - response = None # If we can, we will turn the exception into a problem detail - if hasattr(exception, "as_problem_detail_document"): - document = exception.as_problem_detail_document(debug=False) - elif isinstance(exception, ProblemDetailException): + if isinstance(exception, BaseProblemDetailException): document = exception.problem_detail - - if document: document.debug_message = None if document.status_code == 502: # This is an error in integrating with some upstream @@ -207,16 +201,14 @@ def handle(self, exception: Exception) -> Response | HTTPException: # WARN. log_method = self.log.warning response = make_response(document.response) - - if isinstance(exception, OperationalError): + elif isinstance(exception, OperationalError): # This is an error, but it is probably unavoidable. Likely it was caused by # the database dropping our connection which can happen then the database is # restarted for maintenance. We'll log it at log level WARN. log_method = self.log.warning body = "Service temporarily unavailable. Please try again later." response = make_response(body, 503, {"Content-Type": "text/plain"}) - - if response is None: + else: # There's no way to turn this exception into a problem # document. This is probably indicative of a bug in our # software. diff --git a/core/exceptions.py b/core/exceptions.py index 340fe42cb1..24028fc845 100644 --- a/core/exceptions.py +++ b/core/exceptions.py @@ -53,7 +53,7 @@ class IntegrationException(Exception): missing or obviously wrong (CannotLoadConfiguration). """ - def __init__(self, message, debug_message=None): + def __init__(self, message: str | None, debug_message: str | None = None) -> None: """Constructor. :param message: The normal message passed to any Exception diff --git a/core/opds2_import.py b/core/opds2_import.py index 357f302948..2ab56d4c3a 100644 --- a/core/opds2_import.py +++ b/core/opds2_import.py @@ -1151,9 +1151,7 @@ def _verify_media_type( self.MEDIA_TYPE, media_type ) - raise BadResponseException( - url, message=message, debug_message=feed, status_code=status_code - ) + raise BadResponseException(url, message=message, status_code=status_code) def _get_accept_header(self) -> str: return "{}, {};q=0.9, */*;q=0.1".format( diff --git a/core/opds_import.py b/core/opds_import.py index eb765a1208..7d8dad0aea 100644 --- a/core/opds_import.py +++ b/core/opds_import.py @@ -1871,9 +1871,7 @@ def _verify_media_type( x in media_type for x in (OPDSFeed.ATOM_LIKE_TYPES) ): message = "Expected Atom feed, got %s" % media_type - raise BadResponseException( - url, message=message, debug_message=feed, status_code=status_code - ) + raise BadResponseException(url, message=message, status_code=status_code) def follow_one_link( self, url: str, do_get: Callable[..., HttpResponseTuple] | None = None diff --git a/core/user_profile.py b/core/user_profile.py index 36b0d51abc..749f31c0ba 100644 --- a/core/user_profile.py +++ b/core/user_profile.py @@ -7,6 +7,7 @@ INVALID_INPUT, UNSUPPORTED_MEDIA_TYPE, ) +from core.util.problem_detail import BaseProblemDetailException class ProfileController: @@ -32,14 +33,13 @@ def get(self): :param return: A ProblemDetail if there is a problem; otherwise, a 3-tuple (entity-body, response code, headers) """ - profile_document = None try: profile_document = self.storage.profile_document + except BaseProblemDetailException as e: + return e.problem_detail except Exception as e: - if hasattr(e, "as_problem_detail_document"): - return e.as_problem_detail_document() - else: - return INTERNAL_SERVER_ERROR.with_debug(str(e)) + return INTERNAL_SERVER_ERROR.with_debug(str(e)) + if not isinstance(profile_document, dict): return INTERNAL_SERVER_ERROR.with_debug( _("Profile document is not a JSON object: %r.") % (profile_document) @@ -89,12 +89,11 @@ def put(self, headers, body): try: # Update the profile storage with the new settings. self.storage.update(new_settings, profile_document) + except BaseProblemDetailException as e: + return e.problem_detail except Exception as e: # There was a problem updating the profile storage. - if hasattr(e, "as_problem_detail_document"): - return e.as_problem_detail_document() - else: - return INTERNAL_SERVER_ERROR.with_debug(str(e)) + return INTERNAL_SERVER_ERROR.with_debug(str(e)) return body, 200, {"Content-Type": "text/plain"} diff --git a/core/util/http.py b/core/util/http.py index 2a92fb2c09..3e1ebc8fa3 100644 --- a/core/util/http.py +++ b/core/util/http.py @@ -1,14 +1,17 @@ +from __future__ import annotations + import logging import time from collections.abc import Callable from json import JSONDecodeError -from typing import Any +from typing import TYPE_CHECKING, Any from urllib.parse import urlparse import requests from flask_babel import lazy_gettext as _ from requests import sessions from requests.adapters import HTTPAdapter, Response +from typing_extensions import Self from urllib3 import Retry import core @@ -16,10 +19,17 @@ from core.problem_details import INTEGRATION_ERROR from core.util.log import LoggerMixin from core.util.problem_detail import JSON_MEDIA_TYPE as PROBLEM_DETAIL_JSON_MEDIA_TYPE -from core.util.problem_detail import ProblemDetailException +from core.util.problem_detail import ( + BaseProblemDetailException, + ProblemDetail, + ProblemDetailException, +) + +if TYPE_CHECKING: + from core.model.resource import HttpResponseTuple -class RemoteIntegrationException(IntegrationException): +class RemoteIntegrationException(IntegrationException, BaseProblemDetailException): """An exception that happens when we try and fail to communicate with a third-party service over HTTP. """ @@ -30,7 +40,9 @@ class RemoteIntegrationException(IntegrationException): ) internal_message = "Error accessing %s: %s" - def __init__(self, url_or_service, message, debug_message=None): + def __init__( + self, url_or_service: str, message: str, debug_message: str | None = None + ) -> None: """Indicate that a remote integration has failed. `param url_or_service` The name of the service that failed @@ -43,31 +55,29 @@ def __init__(self, url_or_service, message, debug_message=None): self.service = urlparse(url_or_service).netloc else: self.url = self.service = url_or_service - if not debug_message: - debug_message = self.internal_message % (self.url, message) + super().__init__(message, debug_message) - def __str__(self): + def __str__(self) -> str: message = super().__str__() + if self.debug_message: + message += "\n\n" + self.debug_message return self.internal_message % (self.url, message) - def document_detail(self, debug=True): - if debug: - return _(str(self.detail), service=self.url) - return _(str(self.detail), service=self.service) - - def document_debug_message(self, debug=True): - if debug: - return _(str(self.detail), service=self.url) - return None - - def as_problem_detail_document(self, debug): + @property + def problem_detail(self) -> ProblemDetail: return INTEGRATION_ERROR.detailed( - detail=self.document_detail(debug), + detail=self.document_detail(), title=self.title, - debug_message=self.document_debug_message(debug), + debug_message=self.document_debug_message(), ) + def document_detail(self) -> str: + return _(str(self.detail), service=self.service) + + def document_debug_message(self) -> str: + return str(self) + class BadResponseException(RemoteIntegrationException): """The request seemingly went okay, but we got a bad response.""" @@ -82,7 +92,13 @@ class BadResponseException(RemoteIntegrationException): "Got status code %s from external server, cannot continue." ) - def __init__(self, url_or_service, message, debug_message=None, status_code=None): + def __init__( + self, + url_or_service: str, + message: str, + debug_message: str | None = None, + status_code: int | None = None, + ): """Indicate that a remote integration has failed. `param url_or_service` The name of the service that failed @@ -92,31 +108,28 @@ def __init__(self, url_or_service, message, debug_message=None, status_code=None # to be set to 500, etc. self.status_code = status_code - def document_debug_message(self, debug=True): - if debug: - msg = str(self) - if self.debug_message: - msg += "\n\n" + self.debug_message - return msg - return None - @classmethod - def from_response(cls, url, message, response): + def from_response( + cls, url: str, message: str, response: HttpResponseTuple | Response + ) -> Self: """Helper method to turn a `requests` Response object into a BadResponseException. """ if isinstance(response, tuple): # The response has been unrolled into a (status_code, # headers, body) 3-tuple. - status_code, headers, content = response + status_code, _, content_bytes = response + # The HTTP content response is a bytestring that we want to + # convert to unicode for the debug message. + if content_bytes: + content = content_bytes.decode("utf-8") + else: + content = "" else: status_code = response.status_code - content = response.content - # The HTTP content response is a bytestring that we want to - # convert to unicode for the debug message. - if content and isinstance(content, bytes): - content = content.decode("utf-8") - return BadResponseException( + content = response.text + + return cls( url, message, status_code=status_code, @@ -128,7 +141,7 @@ def from_response(cls, url, message, response): ) @classmethod - def bad_status_code(cls, url, response): + def bad_status_code(cls, url: str, response: Response) -> Self: """The response is bad because the status code is wrong.""" message = cls.BAD_STATUS_CODE_MESSAGE % response.status_code return cls.from_response( @@ -303,11 +316,11 @@ def _request_with_timeout( except requests.exceptions.Timeout as e: # Wrap the requests-specific Timeout exception # in a generic RequestTimedOut exception. - raise RequestTimedOut(url, e) + raise RequestTimedOut(url, str(e)) from e except requests.exceptions.RequestException as e: # Wrap all other requests-specific exceptions in # a generic RequestNetworkException. - raise RequestNetworkException(url, e) + raise RequestNetworkException(url, str(e)) from e return process_response_with( url, @@ -391,7 +404,7 @@ def _decode_response_content(cls, expected_encoding, response, url) -> str: try: response_content = response_content.decode(expected_encoding) except Exception as e: - raise RequestNetworkException(url, e) + raise RequestNetworkException(url, str(e)) from e return response_content @classmethod diff --git a/core/util/problem_detail.py b/core/util/problem_detail.py index 00844582f6..ac574f3250 100644 --- a/core/util/problem_detail.py +++ b/core/util/problem_detail.py @@ -6,6 +6,7 @@ import json as j import logging +from abc import ABC, abstractmethod from flask_babel import LazyString from pydantic import BaseModel @@ -131,8 +132,35 @@ def __repr__(self) -> str: self.debug_message, ) + def __eq__(self, other: object) -> bool: + """Compares two ProblemDetail objects. -class ProblemDetailException(BaseError): + :param other: ProblemDetail object + :return: Boolean value indicating whether two items are equal + """ + if not isinstance(other, ProblemDetail): + return False + + return ( + self.uri == other.uri + and self.title == other.title + and self.status_code == other.status_code + and self.detail == other.detail + and self.debug_message == other.debug_message + ) + + +class BaseProblemDetailException(Exception, ABC): + """Mixin for exceptions that can be converted into a ProblemDetail.""" + + @property + @abstractmethod + def problem_detail(self) -> ProblemDetail: + """Convert this object into a ProblemDetail.""" + ... + + +class ProblemDetailException(BaseError, BaseProblemDetailException): """Exception class allowing to raise and catch ProblemDetail objects.""" def __init__(self, problem_detail: ProblemDetail) -> None: diff --git a/pyproject.toml b/pyproject.toml index e9ca38eb2d..86ddefefee 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -81,6 +81,7 @@ module = [ "api.adobe_vendor_id", "api.axis", "api.circulation", + "api.circulation_exceptions", "api.controller.marc", "api.discovery.*", "api.enki", diff --git a/tests/api/controller/test_loan.py b/tests/api/controller/test_loan.py index 9024e77611..540b9d8371 100644 --- a/tests/api/controller/test_loan.py +++ b/tests/api/controller/test_loan.py @@ -21,6 +21,8 @@ ) from api.circulation_exceptions import ( AlreadyOnHold, + CannotReleaseHold, + CannotReturn, NoAvailableCopies, NoLicenses, NotFoundOnRemote, @@ -29,8 +31,10 @@ from api.problem_details import ( BAD_DELIVERY_MECHANISM, CANNOT_RELEASE_HOLD, + COULD_NOT_MIRROR_TO_REMOTE, HOLD_LIMIT_REACHED, NO_ACTIVE_LOAN, + NO_LICENSES, NOT_FOUND_ON_REMOTE, OUTSTANDING_FINES, ) @@ -679,7 +683,7 @@ def test_borrow_nolicenses(self, loan_fixture: LoanFixture): pool.identifier.type, pool.identifier.identifier ) assert 404 == response.status_code - assert NOT_FOUND_ON_REMOTE == response + assert NO_LICENSES == response def test_borrow_creates_local_hold_if_remote_hold_exists( self, loan_fixture: LoanFixture @@ -1092,6 +1096,24 @@ def test_revoke_loan( assert 200 == response.status_code _ = serialization_helper.verify_and_get_single_entry_feed_links(response) + def test_revoke_loan_exception( + self, + loan_fixture: LoanFixture, + ): + # Revoke loan where an exception is raised from the circulation api + with loan_fixture.request_context_with_library( + "/", headers=dict(Authorization=loan_fixture.valid_auth) + ): + loan_fixture.manager.d_circulation.revoke_loan = MagicMock( + side_effect=CannotReturn() + ) + patron = loan_fixture.manager.loans.authenticated_patron_from_request() + loan_fixture.pool.loan_to(patron) + response = loan_fixture.manager.loans.revoke(loan_fixture.pool.id) + + assert isinstance(response, ProblemDetail) + assert response == COULD_NOT_MIRROR_TO_REMOTE + @pytest.mark.parametrize( *OPDSSerializationTestHelper.PARAMETRIZED_SINGLE_ENTRY_ACCEPT_HEADERS ) @@ -1121,6 +1143,24 @@ def test_revoke_hold( assert 200 == response.status_code _ = serialization_helper.verify_and_get_single_entry_feed_links(response) + def test_revoke_hold_exception( + self, + loan_fixture: LoanFixture, + ): + # Revoke hold where an exception is raised from the circulation api + with loan_fixture.request_context_with_library( + "/", headers=dict(Authorization=loan_fixture.valid_auth) + ): + loan_fixture.manager.d_circulation.release_hold = MagicMock( + side_effect=CannotReleaseHold() + ) + patron = loan_fixture.manager.loans.authenticated_patron_from_request() + loan_fixture.pool.on_hold_to(patron, position=0) + response = loan_fixture.manager.loans.revoke(loan_fixture.pool.id) + + assert isinstance(response, ProblemDetail) + assert response == CANNOT_RELEASE_HOLD + def test_revoke_hold_nonexistent_licensepool(self, loan_fixture: LoanFixture): with loan_fixture.request_context_with_library( "/", headers=dict(Authorization=loan_fixture.valid_auth) diff --git a/tests/api/metadata/test_nyt.py b/tests/api/metadata/test_nyt.py index 5191780d75..c2b8c09ff7 100644 --- a/tests/api/metadata/test_nyt.py +++ b/tests/api/metadata/test_nyt.py @@ -136,6 +136,7 @@ def result_500(*args, **kwargs): raise Exception("Expected an IntegrationException!") except IntegrationException as e: assert "Unknown API error (status 500)" == str(e) + assert e.debug_message is not None assert e.debug_message.startswith("Response from") assert e.debug_message.endswith("was: 'bad value'") diff --git a/tests/api/test_axis.py b/tests/api/test_axis.py index 1bb8d64e34..dd25b36d2e 100644 --- a/tests/api/test_axis.py +++ b/tests/api/test_axis.py @@ -32,7 +32,16 @@ JSONResponseParser, ) from api.circulation import FulfillmentInfo, HoldInfo, LoanInfo -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + AlreadyCheckedOut, + AlreadyOnHold, + CannotFulfill, + NoActiveLoan, + NotFoundOnRemote, + NotOnHold, + PatronAuthorizationFailedException, + RemoteInitiatedServerError, +) from api.web_publication_manifest import FindawayManifest, SpineItem from core.analytics import Analytics from core.coverage import CoverageFailure diff --git a/tests/api/test_bibliotheca.py b/tests/api/test_bibliotheca.py index 8dc3122a2e..2c41d0112e 100644 --- a/tests/api/test_bibliotheca.py +++ b/tests/api/test_bibliotheca.py @@ -4,7 +4,7 @@ import random from datetime import datetime, timedelta from io import BytesIO, StringIO -from typing import TYPE_CHECKING, ClassVar, Protocol, cast, runtime_checkable +from typing import TYPE_CHECKING, cast from unittest import mock from unittest.mock import MagicMock, create_autospec @@ -64,7 +64,6 @@ from core.scripts import RunCollectionCoverageProviderScript from core.util.datetime_helpers import datetime_utc, utc_now from core.util.http import BadResponseException -from core.util.problem_detail import ProblemDetail from core.util.web_publication_manifest import AudiobookManifest from tests.api.mockapi.bibliotheca import MockBibliothecaAPI @@ -856,86 +855,49 @@ class TestErrorParser: "" ) - @runtime_checkable - class CirculationExceptionWithProblemDetail(Protocol): - status_code: ClassVar[int] - - def as_problem_detail_document(self, debug=False) -> ProblemDetail: - ... - @pytest.mark.parametrize( - "incoming_message, error_class, error_code, problem_detail_title, problem_detail_code", + "incoming_message, error_class", [ ( "Patron cannot loan more than 12 documents", PatronLoanLimitReached, - 500, - "Loan limit reached.", - 403, ), ( "Patron cannot have more than 15 holds", PatronHoldLimitReached, - 500, - "Limit reached.", - 403, ), ( "the patron document status was CAN_WISH and not one of CAN_LOAN,RESERVATION", NoLicenses, - 404, - "No licenses.", - 404, ), ( "the patron document status was CAN_HOLD and not one of CAN_LOAN,RESERVATION", NoAvailableCopies, - 400, - None, - None, ), ( "the patron document status was LOAN and not one of CAN_LOAN,RESERVATION", AlreadyCheckedOut, - 400, - None, - None, ), ( "The patron has no eBooks checked out", NotCheckedOut, - 400, - None, - None, ), ( "the patron document status was CAN_LOAN and not one of CAN_HOLD", CurrentlyAvailable, - 400, - None, - None, ), ( "the patron document status was HOLD and not one of CAN_HOLD", AlreadyOnHold, - 400, - None, - None, ), ( "The patron does not have the book on hold", NotOnHold, - 400, - None, - None, ), # This is such a weird case we don't have a special exception for it. ( "the patron document status was LOAN and not one of CAN_HOLD", CannotHold, - 500, - None, - None, ), ], ) @@ -943,22 +905,13 @@ def test_exception( self, incoming_message: str, error_class: type[CirculationException], - error_code: int, - problem_detail_title: str, - problem_detail_code: int, ): document = self.BIBLIOTHECA_ERROR_RESPONSE_BODY_TEMPLATE.format( message=incoming_message ) error = ErrorParser().process_first(document) - assert isinstance(error, error_class) + assert error.__class__ is error_class assert incoming_message == str(error) - assert error_code == error.status_code - - if isinstance(error, self.CirculationExceptionWithProblemDetail): - problem = error.as_problem_detail_document() - assert problem_detail_code == problem.status_code - assert problem_detail_title == problem.title @pytest.mark.parametrize( "incoming_message, incoming_message_from_file, error_string", @@ -969,6 +922,18 @@ def test_exception( None, "The server has encountered an error", ), + ( + # Simulate an unexpected response, which is not a unicode string. + b"Beep boop bytes", + None, + "Beep boop bytes", + ), + ( + # Simulate an unexpected response, which cannot be decoded as a string. + b"\xDE\xAD\xBE\xEF", + None, + "Unreadable error message (Unicode decode error).", + ), ( # Simulate the message we get when the server gives a vague error. None, @@ -998,7 +963,7 @@ def test_exception( ) def test_remote_initiated_server_error( self, - incoming_message: str | None, + incoming_message: str | bytes | None, incoming_message_from_file: str | None, error_string: str, api_bibliotheca_files_fixture: BibliothecaFilesFixture, @@ -1012,10 +977,9 @@ def test_remote_initiated_server_error( assert isinstance(error, RemoteInitiatedServerError) assert BibliothecaAPI.SERVICE_NAME == error.service_name - assert 502 == error.status_code assert error_string == str(error) - problem = error.as_problem_detail_document() + problem = error.problem_detail assert 502 == problem.status_code assert "Integration error communicating with Bibliotheca" == problem.detail assert "Third-party service failed." == problem.title diff --git a/tests/api/test_circulation_exceptions.py b/tests/api/test_circulation_exceptions.py index c562c70001..42b7cd0084 100644 --- a/tests/api/test_circulation_exceptions.py +++ b/tests/api/test_circulation_exceptions.py @@ -1,54 +1,172 @@ +import pytest from flask_babel import lazy_gettext as _ -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + AlreadyCheckedOut, + AlreadyOnHold, + AuthorizationBlocked, + AuthorizationExpired, + CannotFulfill, + CannotHold, + CannotLoan, + CannotReleaseHold, + CannotRenew, + CannotReturn, + CirculationException, + CurrentlyAvailable, + DeliveryMechanismConflict, + DeliveryMechanismError, + DeliveryMechanismMissing, + FormatNotAvailable, + FulfilledOnIncompatiblePlatform, + InternalServerError, + InvalidInputException, + LibraryAuthorizationFailedException, + LibraryInvalidInputException, + LimitReached, + NoAcceptableFormat, + NoActiveLoan, + NoAvailableCopies, + NoLicenses, + NotCheckedOut, + NotFoundOnRemote, + NotOnHold, + OutstandingFines, + PatronAuthorizationFailedException, + PatronHoldLimitReached, + PatronLoanLimitReached, + RemoteInitiatedServerError, +) +from api.problem_details import ( + HOLD_LIMIT_REACHED, + LOAN_LIMIT_REACHED, + OUTSTANDING_FINES, +) +from core.problem_details import INTEGRATION_ERROR, INTERNAL_SERVER_ERROR from core.util.problem_detail import ProblemDetail -from tests.fixtures.database import DatabaseTransactionFixture class TestCirculationExceptions: - def test_as_problem_detail_document(self): + @pytest.mark.parametrize( + "exception", + [ + PatronAuthorizationFailedException, + LibraryAuthorizationFailedException, + InvalidInputException, + LibraryInvalidInputException, + DeliveryMechanismError, + DeliveryMechanismMissing, + DeliveryMechanismConflict, + CannotLoan, + AuthorizationExpired, + AuthorizationBlocked, + CannotReturn, + CannotHold, + CannotReleaseHold, + CannotFulfill, + FormatNotAvailable, + NotFoundOnRemote, + NoLicenses, + CannotRenew, + NoAvailableCopies, + AlreadyCheckedOut, + AlreadyOnHold, + NotCheckedOut, + NotOnHold, + CurrentlyAvailable, + NoAcceptableFormat, + FulfilledOnIncompatiblePlatform, + NoActiveLoan, + OutstandingFines, + PatronLoanLimitReached, + PatronHoldLimitReached, + ], + ) + def test_problem_detail(self, exception: type[CirculationException]) -> None: """Verify that circulation exceptions can be turned into ProblemDetail documents. """ + e = exception() + expected_pd = e.base - e = RemoteInitiatedServerError("message", "some service") - doc = e.as_problem_detail_document() - assert "Integration error communicating with some service" == doc.detail + assert e.problem_detail == expected_pd - e = AuthorizationExpired() - assert EXPIRED_CREDENTIALS == e.as_problem_detail_document() + e_with_detail = exception("A message") + assert e_with_detail.problem_detail == expected_pd.detailed("A message") - e = AuthorizationBlocked() - assert BLOCKED_CREDENTIALS == e.as_problem_detail_document() + e_with_debug = exception(debug_info="A debug message") + assert e_with_debug.problem_detail == expected_pd.with_debug("A debug message") - e = PatronHoldLimitReached() - assert HOLD_LIMIT_REACHED == e.as_problem_detail_document() + e_with_detail_and_debug = exception("A message", "A debug message") + assert e_with_detail_and_debug.problem_detail == expected_pd.detailed( + "A message" + ).with_debug("A debug message") - e = NoLicenses() - assert NO_LICENSES == e.as_problem_detail_document() + def test_outstanding_fines(self) -> None: + e = OutstandingFines() + assert e.problem_detail == OUTSTANDING_FINES + e = OutstandingFines(fines="$5,000,000.001") + assert e.problem_detail == OUTSTANDING_FINES.detailed( + "You must pay your $5000000.00 outstanding fines before you can borrow more books." + ) -class TestLimitReached: - """Test LimitReached, which may send different messages depending on the limit.""" + e = OutstandingFines(fines="invalid amount") + assert e.problem_detail == OUTSTANDING_FINES - def test_as_problem_detail_document(self, db: DatabaseTransactionFixture): + def test_limit_reached(self) -> None: generic_message = _( "You exceeded the limit, but I don't know what the limit was." ) pd = ProblemDetail("http://uri/", 403, _("Limit exceeded."), generic_message) class Mock(LimitReached): - BASE_DOC = pd - MESSAGE_WITH_LIMIT = _("The limit was %(limit)d.") + @property + def message_with_limit(self) -> str: + return _("The limit was %(limit)d.") + + @property + def base(self) -> ProblemDetail: + return pd # No limit -> generic message. ex = Mock() - pd = ex.as_problem_detail_document() + pd = ex.problem_detail assert ex.limit is None assert generic_message == pd.detail # Limit -> specific message. ex = Mock(limit=14) assert 14 == ex.limit - pd = ex.as_problem_detail_document() + pd = ex.problem_detail assert "The limit was 14." == pd.detail + + @pytest.mark.parametrize( + "exception,pd,limit_type", + [ + (PatronHoldLimitReached, HOLD_LIMIT_REACHED, "hold"), + (PatronLoanLimitReached, LOAN_LIMIT_REACHED, "loan"), + ], + ) + def test_patron_limit_reached( + self, exception: type[LimitReached], pd: ProblemDetail, limit_type: str + ) -> None: + e = exception() + assert e.problem_detail == pd + + limit = 10 + e = exception(limit=limit) + assert e.problem_detail.detail is not None + assert e.problem_detail.detail.startswith( + f"You have reached your {limit_type} limit of {limit}." + ) + + def test_internal_server_error(self) -> None: + e = InternalServerError("message", "debug") + assert e.problem_detail == INTERNAL_SERVER_ERROR + + def test_remote_initiated_server_error(self) -> None: + e = RemoteInitiatedServerError("debug message", "some service") + assert e.problem_detail == INTEGRATION_ERROR.detailed( + "Integration error communicating with some service" + ).with_debug("debug message") diff --git a/tests/api/test_circulationapi.py b/tests/api/test_circulationapi.py index 30c585a9cc..68aa54607d 100644 --- a/tests/api/test_circulationapi.py +++ b/tests/api/test_circulationapi.py @@ -18,7 +18,21 @@ HoldInfo, LoanInfo, ) -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + AlreadyCheckedOut, + AlreadyOnHold, + AuthorizationBlocked, + AuthorizationExpired, + CannotRenew, + CurrentlyAvailable, + FormatNotAvailable, + NoActiveLoan, + NoAvailableCopies, + NoLicenses, + OutstandingFines, + PatronHoldLimitReached, + PatronLoanLimitReached, +) from core.analytics import Analytics from core.mock_analytics_provider import MockAnalyticsProvider from core.model import ( diff --git a/tests/api/test_enki.py b/tests/api/test_enki.py index 6c43bb1656..f356a5ac0a 100644 --- a/tests/api/test_enki.py +++ b/tests/api/test_enki.py @@ -8,7 +8,11 @@ import pytest from api.circulation import FulfillmentInfo, LoanInfo -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + NoAvailableCopies, + PatronAuthorizationFailedException, + RemoteInitiatedServerError, +) from api.enki import BibliographicParser, EnkiAPI, EnkiCollectionReaper, EnkiImport from core.analytics import Analytics from core.metadata_layer import CirculationData, Metadata, TimestampData @@ -442,7 +446,7 @@ def test_checkout_success(self, enki_test_fixture: EnkiTestFixure): def test_checkout_bad_authorization(self, enki_test_fixture: EnkiTestFixure): """Test that the correct exception is thrown upon an unsuccessful login.""" db = enki_test_fixture.db - with pytest.raises(AuthorizationFailedException): + with pytest.raises(PatronAuthorizationFailedException): data = enki_test_fixture.files.sample_data("login_unsuccessful.json") enki_test_fixture.api.queue_response(200, content=data) result = json.loads(data) @@ -580,15 +584,15 @@ def test_patron_activity_failure(self, enki_test_fixture: EnkiTestFixure): patron = db.patron() enki_test_fixture.api.queue_response(404, "No such patron") collect = lambda: list(enki_test_fixture.api.patron_activity(patron, "pin")) - pytest.raises(PatronNotFoundOnRemote, collect) + pytest.raises(PatronAuthorizationFailedException, collect) msg = dict(result=dict(message="Login unsuccessful.")) enki_test_fixture.api.queue_response(200, content=json.dumps(msg)) - pytest.raises(AuthorizationFailedException, collect) + pytest.raises(PatronAuthorizationFailedException, collect) msg = dict(result=dict(message="Some other error.")) enki_test_fixture.api.queue_response(200, content=json.dumps(msg)) - pytest.raises(CirculationException, collect) + pytest.raises(RemoteInitiatedServerError, collect) class TestBibliographicParser: diff --git a/tests/api/test_opds_for_distributors.py b/tests/api/test_opds_for_distributors.py index 52239b5d98..9499c06a77 100644 --- a/tests/api/test_opds_for_distributors.py +++ b/tests/api/test_opds_for_distributors.py @@ -5,7 +5,10 @@ import pytest -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + CannotFulfill, + LibraryAuthorizationFailedException, +) from api.opds_for_distributors import ( OPDSForDistributorsAPI, OPDSForDistributorsImporter, diff --git a/tests/api/test_overdrive.py b/tests/api/test_overdrive.py index 27d7ccabe7..0ad906e01d 100644 --- a/tests/api/test_overdrive.py +++ b/tests/api/test_overdrive.py @@ -15,7 +15,18 @@ from sqlalchemy.orm.exc import StaleDataError from api.circulation import CirculationAPI, FulfillmentInfo, HoldInfo, LoanInfo -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + CannotFulfill, + CannotHold, + CannotLoan, + CannotRenew, + FormatNotAvailable, + FulfilledOnIncompatiblePlatform, + NoAcceptableFormat, + NoAvailableCopies, + PatronHoldLimitReached, + PatronLoanLimitReached, +) from api.config import Configuration from api.overdrive import ( GenerateOverdriveAdvantageAccountList, diff --git a/tests/api/test_patron_utility.py b/tests/api/test_patron_utility.py index d2241aaf9b..aec7da5014 100644 --- a/tests/api/test_patron_utility.py +++ b/tests/api/test_patron_utility.py @@ -5,7 +5,11 @@ import pytest from api.authentication.base import PatronData -from api.circulation_exceptions import * +from api.circulation_exceptions import ( + AuthorizationBlocked, + AuthorizationExpired, + OutstandingFines, +) from api.util.patron import Patron, PatronUtility from core.util import MoneyUtility from core.util.datetime_helpers import utc_now diff --git a/tests/core/test_app_server.py b/tests/core/test_app_server.py index 5dc384a675..7200885335 100644 --- a/tests/core/test_app_server.py +++ b/tests/core/test_app_server.py @@ -31,7 +31,11 @@ from core.problem_details import INTEGRATION_ERROR, INVALID_INPUT, INVALID_URN from core.service.logging.configuration import LogLevel from core.util.opds_writer import OPDSFeed, OPDSMessage -from core.util.problem_detail import ProblemDetailException +from core.util.problem_detail import ( + BaseProblemDetailException, + ProblemDetail, + ProblemDetailException, +) from tests.fixtures.database import DatabaseTransactionFixture from tests.fixtures.library import LibraryFixture @@ -304,7 +308,7 @@ def process_urns(self, urns, **kwargs): controller = Mock(session) with data.app.test_request_context("/?urn=foobar"): response = controller.work_lookup(annotator=object()) - assert INVALID_INPUT == response + assert response is INVALID_INPUT def test_permalink(self, urn_lookup_controller_fixture: URNLookupControllerFixture): data, session = ( @@ -473,12 +477,13 @@ def from_request(cls, get_arg, default_size, **kwargs): # pagination classes. -class CanBeProblemDetailDocument(Exception): +class CanBeProblemDetailDocument(BaseProblemDetailException): """A fake exception that can be represented as a problem detail document. """ - def as_problem_detail_document(self, debug): + @property + def problem_detail(self) -> ProblemDetail: return INVALID_URN.detailed( _("detail info"), debug_message="A debug_message which should only appear in debug mode.", diff --git a/tests/core/util/test_http.py b/tests/core/util/test_http.py index 236b9e0f75..9f33094671 100644 --- a/tests/core/util/test_http.py +++ b/tests/core/util/test_http.py @@ -1,4 +1,3 @@ -import json from unittest import mock import pytest @@ -189,30 +188,22 @@ def fake_200_response(*args, **kwargs): exc = e assert exc is not None - debug_doc = exc.as_problem_detail_document(debug=True) + problem_detail = exc.problem_detail # 502 is the status code to be returned if this integration error # interrupts the processing of an incoming HTTP request, not the # status code that caused the problem. # - assert 502 == debug_doc.status_code - assert "Bad response" == debug_doc.title + assert 502 == problem_detail.status_code + assert "Bad response" == problem_detail.title assert ( - "The server made a request to http://url/, and got an unexpected or invalid response." - == debug_doc.detail + "The server made a request to url, and got an unexpected or invalid response." + == problem_detail.detail ) assert ( "Bad response from http://url/: Got status code 200 from external server, cannot continue.\n\nResponse content: Hurray" - == debug_doc.debug_message - ) - - no_debug_doc = exc.as_problem_detail_document(debug=False) - assert "Bad response" == no_debug_doc.title - assert ( - "The server made a request to url, and got an unexpected or invalid response." - == no_debug_doc.detail + == problem_detail.debug_message ) - assert None == no_debug_doc.debug_message def test_unicode_converted_to_utf8(self): """Any Unicode that sneaks into the URL, headers or body is @@ -316,115 +307,158 @@ def test_with_service_name(self): "Unreliable Service", "I just can't handle your request right now." ) - # Since only the service name is provided, there are no details to - # elide in the non-debug version of a problem detail document. - debug_detail = exc.document_detail(debug=True) - other_detail = exc.document_detail(debug=False) - assert debug_detail == other_detail - + details = exc.document_detail() assert ( "The server tried to access Unreliable Service but the third-party service experienced an error." - == debug_detail + == details + ) + + debug_details = exc.document_debug_message() + assert ( + "Error accessing Unreliable Service: I just can't handle your request right now." + == debug_details + ) + + assert str(exc) == debug_details + + assert exc.problem_detail.title == "Failure contacting external service" + assert exc.problem_detail.detail == details + assert exc.problem_detail.debug_message == debug_details + + def test_with_service_url(self): + # If you do provide a URL, it's included in the error message. + exc = RemoteIntegrationException( + "http://unreliable-service/", + "I just can't handle your request right now.", + ) + + # The url isn't included in the main details + details = exc.document_detail() + assert ( + "The server tried to access unreliable-service but the third-party service experienced an error." + == details + ) + + # But it is included in the debug details. + debug_details = exc.document_debug_message() + assert ( + "Error accessing http://unreliable-service/: I just can't handle your request right now." + == debug_details + ) + + assert str(exc) == debug_details + + assert exc.problem_detail.title == "Failure contacting external service" + assert exc.problem_detail.detail == details + assert exc.problem_detail.debug_message == debug_details + + def test_with_debug_message(self): + # If you provide a debug message, it's included in the debug details. + exc = RemoteIntegrationException( + "http://unreliable-service/", + "I just can't handle your request right now.", + "technical details", + ) + details = exc.document_detail() + assert ( + "The server tried to access unreliable-service but the third-party service experienced an error." + == details + ) + + debug_details = exc.document_debug_message() + assert ( + "Error accessing http://unreliable-service/: I just can't handle your request right now.\n\ntechnical details" + == debug_details ) class TestBadResponseException: - def test_helper_constructor(self): + def test_from_response(self): response = MockRequestsResponse(102, content="nonsense") exc = BadResponseException.from_response( "http://url/", "Terrible response, just terrible", response ) + # the status code gets set on the exception + assert exc.status_code == 102 + # Turn the exception into a problem detail document, and it's full # of useful information. - doc, status_code, headers = exc.as_problem_detail_document(debug=True).response - doc = json.loads(doc) + problem_detail = exc.problem_detail - assert "Bad response" == doc["title"] + assert problem_detail.title == "Bad response" assert ( - "The server made a request to http://url/, and got an unexpected or invalid response." - == doc["detail"] + problem_detail.detail + == "The server made a request to url, and got an unexpected or invalid response." ) assert ( - "Bad response from http://url/: Terrible response, just terrible\n\nStatus code: 102\nContent: nonsense" - == doc["debug_message"] + problem_detail.debug_message + == "Bad response from http://url/: Terrible response, just terrible\n\nStatus code: 102\nContent: nonsense" ) + assert problem_detail.status_code == 502 - # Unless debug is turned off, in which case none of that - # information is present. - doc, status_code, headers = exc.as_problem_detail_document(debug=False).response - assert "debug_message" not in json.loads(doc) - - def test_bad_status_code_helper(object): + def test_bad_status_code(object): response = MockRequestsResponse(500, content="Internal Server Error!") exc = BadResponseException.bad_status_code("http://url/", response) - doc, status_code, headers = exc.as_problem_detail_document(debug=True).response - doc = json.loads(doc) + doc = exc.problem_detail - assert doc["debug_message"].startswith( - "Bad response from http://url/: Got status code 500 from external server, cannot continue." + assert doc.title == "Bad response" + assert ( + doc.detail + == "The server made a request to url, and got an unexpected or invalid response." + ) + assert ( + doc.debug_message + == "Bad response from http://url/: Got status code 500 from external server, cannot continue.\n\nStatus code: 500\nContent: Internal Server Error!" ) - def test_as_problem_detail_document(self): + def test_problem_detail(self): exception = BadResponseException( - "http://url/", "What even is this", debug_message="some debug info" + "http://url/", + "What even is this", + debug_message="some debug info", + status_code=401, ) - document = exception.as_problem_detail_document(debug=True) + document = exception.problem_detail assert 502 == document.status_code assert "Bad response" == document.title assert ( - "The server made a request to http://url/, and got an unexpected or invalid response." + "The server made a request to url, and got an unexpected or invalid response." == document.detail ) assert ( "Bad response from http://url/: What even is this\n\nsome debug info" == document.debug_message ) + assert exception.status_code == 401 class TestRequestTimedOut: - def test_as_problem_detail_document(self): + def test_problem_detail(self): exception = RequestTimedOut("http://url/", "I give up") - debug_detail = exception.as_problem_detail_document(debug=True) - assert "Timeout" == debug_detail.title - assert ( - "The server made a request to http://url/, and that request timed out." - == debug_detail.detail - ) - - # If we're not in debug mode, we hide the URL we accessed and just - # show the hostname. - standard_detail = exception.as_problem_detail_document(debug=False) + detail = exception.problem_detail + assert "Timeout" == detail.title assert ( "The server made a request to url, and that request timed out." - == standard_detail.detail + == detail.detail ) - - # The status code corresponding to an upstream timeout is 502. - document, status_code, headers = standard_detail.response - assert 502 == status_code + assert detail.status_code == 502 + assert detail.debug_message == "Timeout accessing http://url/: I give up" class TestRequestNetworkException: - def test_as_problem_detail_document(self): + def test_problem_detail(self): exception = RequestNetworkException("http://url/", "Colossal failure") - debug_detail = exception.as_problem_detail_document(debug=True) - assert "Network failure contacting third-party service" == debug_detail.title + detail = exception.problem_detail + assert "Network failure contacting third-party service" == detail.title assert ( - "The server experienced a network error while contacting http://url/." - == debug_detail.detail + "The server experienced a network error while contacting url." + == detail.detail ) - - # If we're not in debug mode, we hide the URL we accessed and just - # show the hostname. - standard_detail = exception.as_problem_detail_document(debug=False) + assert detail.status_code == 502 assert ( - "The server experienced a network error while contacting url." - == standard_detail.detail + detail.debug_message + == "Network error contacting http://url/: Colossal failure" ) - - # The status code corresponding to an upstream timeout is 502. - document, status_code, headers = standard_detail.response - assert 502 == status_code