From 1dd700039ba6ea947031e65d3a6796a4f5e193e1 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Wed, 4 Sep 2024 21:06:11 -0300 Subject: [PATCH] Refactor BadResponseException Refactor the exception, so it has a reference to the response that can be used to deal with the exception. --- src/palace/manager/api/odl/api.py | 12 +++--- src/palace/manager/api/overdrive.py | 4 +- src/palace/manager/core/opds2_import.py | 9 ++--- src/palace/manager/core/opds_import.py | 12 ++---- src/palace/manager/util/http.py | 50 +++++-------------------- tests/manager/api/odl/test_api.py | 4 +- tests/manager/util/test_http.py | 15 ++++---- 7 files changed, 37 insertions(+), 69 deletions(-) diff --git a/src/palace/manager/api/odl/api.py b/src/palace/manager/api/odl/api.py index e8833c37e..1e380d548 100644 --- a/src/palace/manager/api/odl/api.py +++ b/src/palace/manager/api/odl/api.py @@ -63,7 +63,7 @@ from palace.manager.sqlalchemy.util import get_one from palace.manager.util import base64 from palace.manager.util.datetime_helpers import utc_now -from palace.manager.util.http import BadResponseException +from palace.manager.util.http import BadResponseException, RemoteIntegrationException class OPDS2WithODLApi( @@ -261,16 +261,18 @@ def get_license_status_document(self, loan: Loan) -> dict[str, Any]: f"status code {response.status_code}. Expected 2XX. Response headers: {header_string}. " f"Response content: {response_string}." ) - raise BadResponseException(url, "License Status Document request failed.") + raise BadResponseException( + url, "License Status Document request failed.", response + ) try: status_doc = json.loads(response.content) except ValueError as e: raise BadResponseException( - url, "License Status Document was not valid JSON." + url, "License Status Document was not valid JSON.", response ) if status_doc.get("status") not in self.STATUS_VALUES: - raise BadResponseException( + raise RemoteIntegrationException( url, "License Status Document had an unknown status value." ) return status_doc # type: ignore[no-any-return] @@ -958,7 +960,7 @@ def update_loan(self, loan: Loan, status_doc: dict[str, Any] | None = None) -> N # We already check that the status is valid in get_license_status_document, # but if the document came from a notification it hasn't been checked yet. if status not in self.STATUS_VALUES: - raise BadResponseException( + raise RemoteIntegrationException( str(loan.license.checkout_url), "The License Status Document had an unknown status value.", ) diff --git a/src/palace/manager/api/overdrive.py b/src/palace/manager/api/overdrive.py index 60ef9a135..34e61b5a1 100644 --- a/src/palace/manager/api/overdrive.py +++ b/src/palace/manager/api/overdrive.py @@ -562,10 +562,10 @@ def get( if status_code == 401: if exception_on_401: # This is our second try. Give up. - raise BadResponseException.from_response( + raise BadResponseException( url, "Something's wrong with the Overdrive OAuth Bearer Token!", - (status_code, headers, content), + response, ) else: # Refresh the token and try again. diff --git a/src/palace/manager/core/opds2_import.py b/src/palace/manager/core/opds2_import.py index 411cfb6d8..cf13d7427 100644 --- a/src/palace/manager/core/opds2_import.py +++ b/src/palace/manager/core/opds2_import.py @@ -10,6 +10,7 @@ import webpub_manifest_parser.opds2.ast as opds2_ast from flask_babel import lazy_gettext as _ +from requests import Response from sqlalchemy.orm import Session from uritemplate import URITemplate from webpub_manifest_parser.core import ManifestParserFactory, ManifestParserResult @@ -1157,18 +1158,16 @@ class OPDS2ImportMonitor(OPDSImportMonitor): PROTOCOL = OPDS2API.label() MEDIA_TYPE = OPDS2MediaTypesRegistry.OPDS_FEED.key, "application/json" - def _verify_media_type( - self, url: str, status_code: int, headers: Mapping[str, str], feed: bytes - ) -> None: + def _verify_media_type(self, url: str, resp: Response) -> None: # Make sure we got an OPDS feed, and not an error page that was # sent with a 200 status code. - media_type = headers.get("content-type") + media_type = resp.headers.get("content-type") if not media_type or not any(x in media_type for x in self.MEDIA_TYPE): message = "Expected {} OPDS 2.0 feed, got {}".format( self.MEDIA_TYPE, media_type ) - raise BadResponseException(url, message=message, status_code=status_code) + raise BadResponseException(url, message=message, response=resp) def _get_accept_header(self) -> str: return "{}, {};q=0.9, */*;q=0.1".format( diff --git a/src/palace/manager/core/opds_import.py b/src/palace/manager/core/opds_import.py index 00d30f221..3b93bd160 100644 --- a/src/palace/manager/core/opds_import.py +++ b/src/palace/manager/core/opds_import.py @@ -1855,17 +1855,15 @@ def identifier_needs_import( return True return False - def _verify_media_type( - self, url: str, status_code: int, headers: Mapping[str, str], feed: bytes - ) -> None: + def _verify_media_type(self, url: str, resp: Response) -> None: # Make sure we got an OPDS feed, and not an error page that was # sent with a 200 status code. - media_type = headers.get("content-type") + media_type = resp.headers.get("content-type") if not media_type or not any( x in media_type for x in (OPDSFeed.ATOM_LIKE_TYPES) ): message = "Expected Atom feed, got %s" % media_type - raise BadResponseException(url, message=message, status_code=status_code) + raise BadResponseException(url, message=message, response=resp) def follow_one_link( self, url: str, do_get: Callable[..., Response] | None = None @@ -1881,10 +1879,8 @@ def follow_one_link( get = do_get or self._get resp = get(url, {}) feed = resp.content - status_code = resp.status_code - headers = resp.headers - self._verify_media_type(url, status_code, headers, feed) + self._verify_media_type(url, resp) new_data = self.feed_contains_new_data(feed) diff --git a/src/palace/manager/util/http.py b/src/palace/manager/util/http.py index d4070bced..628943ef9 100644 --- a/src/palace/manager/util/http.py +++ b/src/palace/manager/util/http.py @@ -4,7 +4,7 @@ import time from collections.abc import Callable, Mapping, Sequence from json import JSONDecodeError -from typing import TYPE_CHECKING, Any +from typing import Any from urllib.parse import urlparse import requests @@ -28,9 +28,6 @@ ProblemDetailException, ) -if TYPE_CHECKING: - from palace.manager.sqlalchemy.model.resource import HttpResponseTuple - class RemoteIntegrationException(IntegrationException, BaseProblemDetailException): """An exception that happens when we try and fail to communicate @@ -99,55 +96,28 @@ def __init__( self, url_or_service: str, message: str, + response: Response, 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 (e.g. "Overdrive"), or the specific URL that had the problem. """ + if debug_message is None: + debug_message = ( + f"Status code: {response.status_code}\nContent: {response.text}" + ) + super().__init__(url_or_service, message, debug_message) # to be set to 500, etc. - self.status_code = status_code - - @classmethod - 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, _, 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.text - - return cls( - url, - message, - status_code=status_code, - debug_message="Status code: %s\nContent: %s" - % ( - status_code, - content, - ), - ) + self.response = response @classmethod 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( + return cls( url, message, response, @@ -399,9 +369,9 @@ def _process_response( raise BadResponseException( url, error_message % code, - status_code=response.status_code, debug_message="Response content: %s" % cls._decode_response_content(response, url), + response=response, ) return response diff --git a/tests/manager/api/odl/test_api.py b/tests/manager/api/odl/test_api.py index 63b686514..f2ef0c8cb 100644 --- a/tests/manager/api/odl/test_api.py +++ b/tests/manager/api/odl/test_api.py @@ -44,7 +44,7 @@ from palace.manager.sqlalchemy.model.work import Work from palace.manager.sqlalchemy.util import create from palace.manager.util.datetime_helpers import datetime_utc, utc_now -from palace.manager.util.http import BadResponseException +from palace.manager.util.http import BadResponseException, RemoteIntegrationException from tests.fixtures.database import DatabaseTransactionFixture from tests.fixtures.odl import OPDS2WithODLApiFixture @@ -277,7 +277,7 @@ def test_get_license_status_document_errors( 200, content=json.dumps(dict(status="unknown")) ) pytest.raises( - BadResponseException, + RemoteIntegrationException, opds2_with_odl_api_fixture.api.get_license_status_document, loan, ) diff --git a/tests/manager/util/test_http.py b/tests/manager/util/test_http.py index 3fdb3aa7e..e5309268f 100644 --- a/tests/manager/util/test_http.py +++ b/tests/manager/util/test_http.py @@ -394,14 +394,14 @@ def test_with_debug_message(self): class TestBadResponseException: - def test_from_response(self): + def test__init__(self): response = MockRequestsResponse(102, content="nonsense") - exc = BadResponseException.from_response( + exc = BadResponseException( "http://url/", "Terrible response, just terrible", response ) - # the status code gets set on the exception - assert exc.status_code == 102 + # the response gets set on the exception + assert exc.response is response # Turn the exception into a problem detail document, and it's full # of useful information. @@ -418,7 +418,7 @@ def test_from_response(self): ) assert problem_detail.status_code == 502 - def test_bad_status_code(object): + def test_bad_status_code(self): response = MockRequestsResponse(500, content="Internal Server Error!") exc = BadResponseException.bad_status_code("http://url/", response) doc = exc.problem_detail @@ -434,11 +434,12 @@ def test_bad_status_code(object): ) def test_problem_detail(self): + response = MockRequestsResponse(401, content="You are not authorized!") exception = BadResponseException( "http://url/", "What even is this", debug_message="some debug info", - status_code=401, + response=response, ) document = exception.problem_detail assert 502 == document.status_code @@ -451,7 +452,7 @@ def test_problem_detail(self): "Bad response from http://url/: What even is this\n\nsome debug info" == document.debug_message ) - assert exception.status_code == 401 + assert exception.response is response class TestRequestTimedOut: