From 7f9a9a61455cf5bd347b00631732990dc6590816 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Fri, 22 Nov 2024 15:12:21 -0400 Subject: [PATCH] Sharpen up some of the types that we return --- src/palace/manager/api/controller/loan.py | 32 +++++++++-------------- src/palace/manager/api/controller/work.py | 15 ++--------- src/palace/manager/feed/acquisition.py | 17 +++++------- tests/manager/api/controller/test_loan.py | 26 ++++++++++++++++++ 4 files changed, 46 insertions(+), 44 deletions(-) diff --git a/src/palace/manager/api/controller/loan.py b/src/palace/manager/api/controller/loan.py index baa828970..80fac5106 100644 --- a/src/palace/manager/api/controller/loan.py +++ b/src/palace/manager/api/controller/loan.py @@ -3,7 +3,6 @@ import flask from flask import Response from flask_babel import lazy_gettext as _ -from lxml import etree from pydantic import TypeAdapter from werkzeug import Response as wkResponse @@ -26,7 +25,7 @@ ) from palace.manager.api.util.flask import get_request_library, get_request_patron from palace.manager.celery.tasks.patron_activity import sync_patron_activity -from palace.manager.core.problem_details import INTERNAL_SERVER_ERROR +from palace.manager.core.exceptions import PalaceValueError from palace.manager.feed.acquisition import OPDSAcquisitionFeed from palace.manager.service.redis.models.patron_activity import PatronActivity from palace.manager.sqlalchemy.model.library import Library @@ -36,7 +35,6 @@ ) from palace.manager.sqlalchemy.model.patron import Hold, Loan, Patron from palace.manager.util.flask_util import OPDSEntryResponse -from palace.manager.util.opds_writer import OPDSFeed from palace.manager.util.problem_detail import BaseProblemDetailException, ProblemDetail @@ -382,12 +380,9 @@ def fulfill( if isinstance(feed, Response): return feed else: - content = etree.tostring(feed) - return Response( - response=content, - status=200, - content_type=OPDSFeed.ACQUISITION_FEED_TYPE, - ) + raise PalaceValueError( + "Unexpected return type from OPDSAcquisitionFeed.single_entry_loans_feed" + ) try: return fulfillment.response() @@ -453,6 +448,14 @@ def revoke(self, license_pool_id: int) -> OPDSEntryResponse | ProblemDetail: ), status_code=404, ) + work = pool.work + if not work: + # Somehow we have a loan or hold for a LicensePool that has no Work. + self.log.error( + "Can't revoke loan or hold for LicensePool %r which has no Work!", + pool, + ) + return NOT_FOUND_ON_REMOTE header = self.authorization_header() credential = self.manager.auth.get_credential_from_header(header) @@ -480,19 +483,8 @@ def revoke(self, license_pool_id: int) -> OPDSEntryResponse | ProblemDetail: countdown=5, ) - work = pool.work - if not work: - # Somehow we just revoked a loan or hold for a LicensePool - # that has no Work. This shouldn't happen. - self.log.error( - "Revoked loan or hold for LicensePool %r which has no Work!", - pool, - ) - return NOT_FOUND_ON_REMOTE annotator = self.manager.annotator(None) single_entry_feed = OPDSAcquisitionFeed.single_entry(work, annotator) - if single_entry_feed is None: - return INTERNAL_SERVER_ERROR return OPDSAcquisitionFeed.entry_as_response( single_entry_feed, mime_types=flask.request.accept_mimetypes, diff --git a/src/palace/manager/api/controller/work.py b/src/palace/manager/api/controller/work.py index 3f02f4256..96d4f8bb5 100644 --- a/src/palace/manager/api/controller/work.py +++ b/src/palace/manager/api/controller/work.py @@ -21,7 +21,6 @@ from palace.manager.core.app_server import load_pagination_from_request from palace.manager.core.config import CannotLoadConfiguration from palace.manager.core.metadata_layer import ContributorData -from palace.manager.core.problem_details import INTERNAL_SERVER_ERROR from palace.manager.feed.acquisition import OPDSAcquisitionFeed from palace.manager.search.external_search import SortKeyPagination from palace.manager.sqlalchemy.model.lane import FeaturedFacets, Pagination @@ -131,23 +130,13 @@ def permalink( item = loan or hold pool = pool or pools[0] - loans_feed = OPDSAcquisitionFeed.single_entry_loans_feed( + return OPDSAcquisitionFeed.single_entry_loans_feed( self.circulation, item or pool ) - if loans_feed is None: - self.log.error( - "Could not generate loans feed for %r and %r", item, pool - ) - return INTERNAL_SERVER_ERROR - return loans_feed else: annotator = self.manager.annotator(lane=None) - single_entry = OPDSAcquisitionFeed.single_entry(work, annotator) - if single_entry is None: - # There was some problem generating the entry. - return NOT_FOUND_ON_REMOTE return OPDSAcquisitionFeed.entry_as_response( - single_entry, + OPDSAcquisitionFeed.single_entry(work, annotator), max_age=OPDSFeed.DEFAULT_MAX_AGE, ) diff --git a/src/palace/manager/feed/acquisition.py b/src/palace/manager/feed/acquisition.py index 8a958270e..df241d5bd 100644 --- a/src/palace/manager/feed/acquisition.py +++ b/src/palace/manager/feed/acquisition.py @@ -10,6 +10,7 @@ from palace.manager.api.problem_details import NOT_FOUND_ON_REMOTE from palace.manager.core.entrypoint import EntryPoint +from palace.manager.core.exceptions import PalaceValueError from palace.manager.core.facets import FacetConstants from palace.manager.core.problem_details import INVALID_INPUT from palace.manager.feed.annotator.base import Annotator @@ -536,7 +537,7 @@ def single_entry_loans_feed( annotator: LibraryAnnotator | None = None, fulfillment: UrlFulfillment | None = None, **response_kwargs: Any, - ) -> OPDSEntryResponse | ProblemDetail | None: + ) -> OPDSEntryResponse | ProblemDetail: """A single entry as a standalone feed specific to a patron""" if not item: raise ValueError("Argument 'item' must be non-empty") @@ -599,8 +600,8 @@ def single_entry_loans_feed( return cls.entry_as_response(entry, **response_kwargs) elif isinstance(entry, OPDSMessage): return cls.entry_as_response(entry, max_age=0) - - return None + else: + raise ValueError("Entry is not an instance of WorkEntry or OPDSMessage") @classmethod def single_entry( @@ -608,7 +609,7 @@ def single_entry( work: Work | Edition, annotator: Annotator, even_if_no_license_pool: bool = False, - ) -> WorkEntry | OPDSMessage | None: + ) -> WorkEntry | OPDSMessage: """Turn a work into an annotated work entry for an acquisition feed.""" identifier = None _work: Work @@ -631,8 +632,7 @@ def single_entry( # There's no reason to present a book that has no active license pool. if not identifier: - cls.logger().warning("%r HAS NO IDENTIFIER", work) - return None + raise PalaceValueError("Work has no identifier") if not active_license_pool and not even_if_no_license_pool: cls.logger().warning("NO ACTIVE LICENSE POOL FOR %r", work) @@ -665,11 +665,6 @@ def single_entry( 403, "I know about this work but can offer no way of fulfilling it.", ) - except Exception as e: - cls.logger().error( - "Exception generating OPDS entry for %r", work, exc_info=e - ) - return None @classmethod @inject diff --git a/tests/manager/api/controller/test_loan.py b/tests/manager/api/controller/test_loan.py index 4948b44bb..36537b7e4 100644 --- a/tests/manager/api/controller/test_loan.py +++ b/tests/manager/api/controller/test_loan.py @@ -1183,6 +1183,32 @@ def test_revoke_loan_exception( # Because of the error we did not queue up a sync_patron_activity_collection task sync_task.apply_async.assert_not_called() + def test_revoke_loan_licensepool_no_work( + self, + loan_fixture: LoanFixture, + ): + # Revoke loan where the license pool has no work + with ( + loan_fixture.request_context_with_library( + "/", headers=dict(Authorization=loan_fixture.valid_auth) + ), + patch( + "palace.manager.api.controller.loan.sync_patron_activity" + ) as sync_task, + ): + patron = loan_fixture.manager.loans.authenticated_patron_from_request() + loan_fixture.manager.d_circulation.queue_checkin(loan_fixture.pool) + assert isinstance(patron, Patron) + loan_fixture.pool.loan_to(patron) + loan_fixture.pool.work = None + response = loan_fixture.manager.loans.revoke(loan_fixture.pool_id) + + assert isinstance(response, ProblemDetail) + assert response == NOT_FOUND_ON_REMOTE + + # Because of the error we did not queue up a sync_patron_activity_collection task + sync_task.apply_async.assert_not_called() + @pytest.mark.parametrize( *OPDSSerializationTestHelper.PARAMETRIZED_SINGLE_ENTRY_ACCEPT_HEADERS )