Sharpen up some of the types that we return

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,

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,
             )
 

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,16 +600,16 @@ 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(
         cls,
         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

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
     )