From 4af2faac10254d4392a45096bd954a409d2f1a8b Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Tue, 19 Nov 2024 10:47:42 -0400 Subject: [PATCH] "Could not place hold" error with multiple licenses (PP-1902) (#2173) When we have multiple licenses, it was possible for us to get out of sync with the provider, so we would try to place a hold, when a different license could have been tried instead. This updates the checkout to try all available licenses, in priority order, before resorting to a hold. --- src/palace/manager/api/odl/api.py | 155 ++++++++++-------- .../manager/sqlalchemy/model/licensing.py | 87 ++++++---- tests/manager/api/odl/test_api.py | 85 +++++++++- .../sqlalchemy/model/test_licensing.py | 56 +++++-- 4 files changed, 269 insertions(+), 114 deletions(-) diff --git a/src/palace/manager/api/odl/api.py b/src/palace/manager/api/odl/api.py index 608d19a25..c28164604 100644 --- a/src/palace/manager/api/odl/api.py +++ b/src/palace/manager/api/odl/api.py @@ -6,7 +6,6 @@ import uuid from collections.abc import Callable from functools import cached_property, partial -from typing import Any from dependency_injector.wiring import Provide, inject from flask import url_for @@ -54,11 +53,13 @@ from palace.manager.opds.base import BaseLink from palace.manager.opds.lcp.license import LicenseDocument from palace.manager.opds.lcp.status import LoanStatus +from palace.manager.service.analytics.analytics import Analytics from palace.manager.service.container import Services from palace.manager.sqlalchemy.model.collection import Collection from palace.manager.sqlalchemy.model.datasource import DataSource from palace.manager.sqlalchemy.model.licensing import ( DeliveryMechanism, + License, LicensePool, LicensePoolDeliveryMechanism, ) @@ -103,7 +104,7 @@ def __init__( self, _db: Session, collection: Collection, - analytics: Any = Provide[Services.analytics.analytics], + analytics: Analytics = Provide[Services.analytics.analytics], ) -> None: super().__init__(_db, collection) @@ -334,85 +335,56 @@ def _checkout( ) and licensepool.licenses_available < 1: raise NoAvailableCopies() - # Create a local loan so its database id can be used to - # receive notifications from the distributor. - license = licensepool.best_available_license() - if not license: - raise NoAvailableCopies() - - identifier = str(license.identifier) - checkout_id = str(uuid.uuid4()) if self.collection is None: raise PalaceValueError(f"Collection not found: {self.collection_id}") default_loan_period = self.collection.default_loan_period(patron.library) - requested_expiry = utc_now() + datetime.timedelta(days=default_loan_period) patron_id = patron.identifier_to_remote_service(licensepool.data_source) library_short_name = patron.library.short_name - hasher = self._get_hasher() unhashed_pass = self._credential_factory.get_patron_passphrase(db, patron) hashed_pass = unhashed_pass.hash(hasher) self._credential_factory.set_hashed_passphrase(db, patron, hashed_pass) encoded_pass = base64.b64encode(binascii.unhexlify(hashed_pass.hashed)) - notification_url = self._notification_url( - library_short_name, - patron_id, - identifier, - ) - - # We should never be able to get here if the license doesn't have a checkout_url, but - # we assert it anyway, to be sure we fail fast if it happens. - assert license.checkout_url is not None - url_template = URITemplate(license.checkout_url) - checkout_url = url_template.expand( - id=identifier, - checkout_id=checkout_id, - patron_id=patron_id, - expires=requested_expiry.isoformat(), - notification_url=notification_url, - passphrase=encoded_pass, - hint=self.settings.passphrase_hint, - hint_url=self.settings.passphrase_hint_url, - ) - - try: - doc = self._request_loan_status( - "POST", - checkout_url, - ignored_problem_types=[ - "http://opds-spec.org/odl/error/checkout/unavailable" - ], - ) - except OpdsWithOdlException as e: - if e.type == "http://opds-spec.org/odl/error/checkout/unavailable": - # TODO: This would be a good place to do an async availability update, since we know - # the book is unavailable, when we thought it was available. For now, we know that - # the license has no checkouts_available, so we do that update. - license.checkouts_available = 0 - if hold: - # If we have a hold, it means we thought the book was available, but it wasn't. - # So we need to update its position in the hold queue. We will put it at position - # 1, since the patron should be first in line. This may mean that there are two - # patrons in position 1 in the hold queue, but this will be resolved next time - # the hold queue is recalculated. - hold.position = 1 - hold.end = None - # Update the pool and the next holds in the queue when a license is reserved. - licensepool.update_availability_from_licenses() - raise NoAvailableCopies() - raise + licenses = licensepool.best_available_licenses() + + license_: License | None = None + loan_status: LoanStatus | None = None + for license_ in licenses: + try: + loan_status = self._checkout_license( + license_, + library_short_name, + patron_id, + requested_expiry.isoformat(), + encoded_pass, + ) + break + except NoAvailableCopies: + # This license had no available copies, so we try the next one. + ... + + if license_ is None or loan_status is None: + if hold: + # If we have a hold, it means we thought the book was available, but it wasn't. + # So we need to update its position in the hold queue. We will put it at position + # 1, since the patron should be first in line. This may mean that there are two + # patrons in position 1 in the hold queue, but this will be resolved next time + # the hold queue is recalculated. + hold.position = 1 + hold.end = None + licensepool.update_availability_from_licenses() + raise NoAvailableCopies() - if not doc.active: - # Something went wrong with this loan and we don't actually + if not loan_status.active: + # Something went wrong with this loan, and we don't actually # have the book checked out. This should never happen. - # Remove the loan we created. raise CannotLoan() # We save the link to the loan status document in the loan's external_identifier field, so # we are able to retrieve it later. - loan_status_document_link: BaseLink | None = doc.links.get( + loan_status_document_link: BaseLink | None = loan_status.links.get( rel="self", type=LoanStatus.content_type() ) @@ -424,7 +396,7 @@ def _checkout( # TODO: Raise this issue with LCP server maintainers, and try to get a fix in place. # once that is done, we should be able to remove this fallback. if not loan_status_document_link: - license_document_link = doc.links.get( + license_document_link = loan_status.links.get( rel="license", type=LicenseDocument.content_type() ) if license_document_link: @@ -441,13 +413,13 @@ def _checkout( loan = LoanInfo.from_license_pool( licensepool, - end_date=doc.potential_rights.end, + end_date=loan_status.potential_rights.end, external_identifier=loan_status_document_link.href, - license_identifier=license.identifier, + license_identifier=license_.identifier, ) # We also need to update the remaining checkouts for the license. - license.checkout() + license_.checkout() # If there was a hold CirculationAPI will take care of deleting it. So we just need to # update the license pool to reflect the loan. Since update_availability_from_licenses @@ -457,6 +429,55 @@ def _checkout( ) return loan + def _checkout_license( + self, + license_: License, + library_short_name: str | None, + patron_id: str, + expiry: str, + encoded_pass: str, + ) -> LoanStatus: + identifier = str(license_.identifier) + checkout_id = str(uuid.uuid4()) + + notification_url = self._notification_url( + library_short_name, + patron_id, + identifier, + ) + + # We should never be able to get here if the license doesn't have a checkout_url, but + # we assert it anyway, to be sure we fail fast if it happens. + assert license_.checkout_url is not None + url_template = URITemplate(license_.checkout_url) + checkout_url = url_template.expand( + id=identifier, + checkout_id=checkout_id, + patron_id=patron_id, + expires=expiry, + notification_url=notification_url, + passphrase=encoded_pass, + hint=self.settings.passphrase_hint, + hint_url=self.settings.passphrase_hint_url, + ) + + try: + return self._request_loan_status( + "POST", + checkout_url, + ignored_problem_types=[ + "http://opds-spec.org/odl/error/checkout/unavailable" + ], + ) + except OpdsWithOdlException as e: + if e.type == "http://opds-spec.org/odl/error/checkout/unavailable": + # TODO: This would be a good place to do an async availability update, since we know + # the book is unavailable, when we thought it was available. For now, we know that + # the license has no checkouts_available, so we do that update. + license_.checkouts_available = 0 + raise NoAvailableCopies() from e + raise + def fulfill( self, patron: Patron, diff --git a/src/palace/manager/sqlalchemy/model/licensing.py b/src/palace/manager/sqlalchemy/model/licensing.py index 0c634b0dd..cc04b4912 100644 --- a/src/palace/manager/sqlalchemy/model/licensing.py +++ b/src/palace/manager/sqlalchemy/model/licensing.py @@ -5,6 +5,7 @@ import datetime import logging from enum import Enum as PythonEnum +from enum import IntEnum, auto from operator import and_ from typing import TYPE_CHECKING, Literal, overload @@ -1099,44 +1100,68 @@ def on_hold_to( hold.update(start, end, position) return hold, new - def best_available_license(self) -> License | None: - """Determine the next license that should be lent out for this pool. + class _LicensePriority(IntEnum): + TIME_LIMITED = auto() + PERPETUAL = auto() + TIME_AND_LOAN_LIMITED = auto() + LOAN_LIMITED = auto() + + @staticmethod + def _time_limited_sort_key(license_: License) -> int: + if license_.expires is None: + return 0 + return int(license_.expires.timestamp()) + + @staticmethod + def _loan_limited_sort_key(license_: License) -> int: + return (license_.checkouts_left or 0) * -1 + + @classmethod + def _license_sort_func(cls, license_: License) -> tuple[_LicensePriority, int, int]: + time_limited_key = cls._time_limited_sort_key(license_) + loan_limited_key = cls._loan_limited_sort_key(license_) + + if license_.is_time_limited and license_.is_loan_limited: + return ( + cls._LicensePriority.TIME_AND_LOAN_LIMITED, + time_limited_key, + loan_limited_key, + ) + + if license_.is_time_limited: + return cls._LicensePriority.TIME_LIMITED, time_limited_key, loan_limited_key + + if license_.is_loan_limited: + return cls._LicensePriority.LOAN_LIMITED, time_limited_key, loan_limited_key + + return cls._LicensePriority.PERPETUAL, time_limited_key, loan_limited_key + + def best_available_licenses(self) -> list[License]: + """ + Determine the next license that should be lent out from this pool. + + This function returns a list of licenses that are available for lending, sorted + by priority. The highest priority license (the one that the next loan should be made from) + is the first one in the list. Time-limited licenses and perpetual licenses are the best. It doesn't matter which is used first, unless a time-limited license would expire within the loan period, in - which case it's better to loan the time-limited license so the perpetual one is still - available. We can handle this by always loaning the time-limited one first, followed - by perpetual. If there is more than one time-limited license, it's better to use the one + which case it's better to loan the time-limited license so the perpetual one remains + available. + + We handle this by always loaning the time-limited one first, followed by the perpetual + one. If there is more than one time-limited license, it's better to use the one expiring soonest. If no time-limited or perpetual licenses are available, the next best is a loan-limited - license. We should choose the license with the most remaining loans, so that we'll - maximize the number of concurrent checkouts available in the future. - - The worst option would be pay-per-use, but we don't yet support any distributors that - offer that model. + license. If a license is both time-limited and loan-limited, it's better to use it before + a license that is only loan-limited. We should choose the license with the most remaining + loans, so that we'll maximize the number of concurrent checkouts available in the future. """ - best: License | None = None - - for license in (l for l in self.licenses if l.is_available_for_borrowing): - if ( - not best - or (license.is_time_limited and not best.is_time_limited) - or ( - license.is_time_limited - and best.is_time_limited - and license.expires < best.expires # type: ignore[operator] - ) - or (license.is_perpetual and not best.is_time_limited) - or ( - license.is_loan_limited - and best.is_loan_limited - and license.checkouts_left > best.checkouts_left # type: ignore[operator] - ) - ): - best = license - - return best + return sorted( + (l for l in self.licenses if l.is_available_for_borrowing), + key=self._license_sort_func, + ) @classmethod def consolidate_works(cls, _db, batch_size=10): diff --git a/tests/manager/api/odl/test_api.py b/tests/manager/api/odl/test_api.py index c878a7531..dc30ac5e0 100644 --- a/tests/manager/api/odl/test_api.py +++ b/tests/manager/api/odl/test_api.py @@ -717,7 +717,11 @@ def test_checkout_no_available_copies_unknown_to_us( are copies available. """ # We think there are copies available. - license = opds2_with_odl_api_fixture.setup_license(concurrency=1, available=1) + pool = opds2_with_odl_api_fixture.pool + pool.licenses = [] + license_1 = db.license(pool, terms_concurrency=1, checkouts_available=1) + license_2 = db.license(pool, checkouts_available=1) + pool.update_availability_from_licenses() # But the distributor says there are no available copies. opds2_with_odl_api_fixture.mock_http.queue_response( @@ -725,13 +729,88 @@ def test_checkout_no_available_copies_unknown_to_us( response_type, content=opds2_with_odl_api_fixture.files.sample_text("unavailable.json"), ) + opds2_with_odl_api_fixture.mock_http.queue_response( + 400, + response_type, + content=opds2_with_odl_api_fixture.files.sample_text("unavailable.json"), + ) with pytest.raises(NoAvailableCopies): opds2_with_odl_api_fixture.api_checkout() assert db.session.query(Loan).count() == 0 - assert license.license_pool.licenses_available == 0 - assert license.checkouts_available == 0 + assert opds2_with_odl_api_fixture.pool.licenses_available == 0 + assert license_1.checkouts_available == 0 + assert license_2.checkouts_available == 0 + + def test_checkout_many_licenses( + self, + db: DatabaseTransactionFixture, + opds2_with_odl_api_fixture: OPDS2WithODLApiFixture, + ) -> None: + """ + The title has 5 different licenses. Several of them seem to have copies available. But + we are out of sync, so it turns out that not all of them do. + """ + # We think there are copies available. + pool = opds2_with_odl_api_fixture.pool + pool.licenses = [] + license_unavailable_1 = db.license( + pool, checkouts_available=2, expires=utc_now() + datetime.timedelta(weeks=4) + ) + license_unavailable_2 = db.license( + pool, terms_concurrency=1, checkouts_available=1 + ) + license_untouched = db.license(pool, checkouts_left=1, checkouts_available=1) + license_lent = db.license( + pool, + checkouts_left=4, + checkouts_available=4, + expires=utc_now() + datetime.timedelta(weeks=1), + ) + license_expired = db.license( + pool, + terms_concurrency=10, + checkouts_available=10, + expires=utc_now() - datetime.timedelta(weeks=1), + ) + pool.update_availability_from_licenses() + assert pool.licenses_available == 8 + + assert opds2_with_odl_api_fixture.pool.best_available_licenses() == [ + license_unavailable_1, + license_unavailable_2, + license_lent, + license_untouched, + ] + + # But the distributor says there are no available copies for license_unavailable_1 + opds2_with_odl_api_fixture.mock_http.queue_response( + 400, + "application/api-problem+json", + content=opds2_with_odl_api_fixture.files.sample_text("unavailable.json"), + ) + # And for license_unavailable_2 + opds2_with_odl_api_fixture.mock_http.queue_response( + 400, + "application/api-problem+json", + content=opds2_with_odl_api_fixture.files.sample_text("unavailable.json"), + ) + # But license_lent is still available, and we successfully check it out + opds2_with_odl_api_fixture.mock_http.queue_response( + 201, + content=opds2_with_odl_api_fixture.loan_status_document().model_dump_json(), + ) + + loan_info = opds2_with_odl_api_fixture.api_checkout() + + assert opds2_with_odl_api_fixture.pool.licenses_available == 4 + assert license_unavailable_2.checkouts_available == 0 + assert license_unavailable_1.checkouts_available == 0 + assert license_lent.checkouts_available == 3 + assert license_untouched.checkouts_available == 1 + + assert loan_info.license_identifier == license_lent.identifier def test_checkout_ready_hold_no_available_copies( self, diff --git a/tests/manager/sqlalchemy/model/test_licensing.py b/tests/manager/sqlalchemy/model/test_licensing.py index 9d016128a..60caf74da 100644 --- a/tests/manager/sqlalchemy/model/test_licensing.py +++ b/tests/manager/sqlalchemy/model/test_licensing.py @@ -27,6 +27,7 @@ ) from palace.manager.sqlalchemy.model.resource import Hyperlink, Representation from palace.manager.sqlalchemy.util import create +from palace.manager.util import first_or_default from palace.manager.util.datetime_helpers import utc_now from tests.fixtures.database import DatabaseTransactionFixture from tests.mocks.analytics_provider import MockAnalyticsProvider @@ -467,7 +468,7 @@ def test_license_checkin( assert left == l.checkouts_left assert available == l.checkouts_available - def test_best_available_license(self, licenses: LicenseTestFixture): + def test_best_available_licenses(self, licenses: LicenseTestFixture): next_week = utc_now() + datetime.timedelta(days=7) time_limited_2 = licenses.db.license( licenses.pool, @@ -479,37 +480,66 @@ def test_best_available_license(self, licenses: LicenseTestFixture): licenses.pool, expires=None, checkouts_left=2, checkouts_available=1 ) - # First, we use the time-limited license that's expiring first. - assert time_limited_2 == licenses.pool.best_available_license() + # First, make sure the overall order is correct + assert licenses.pool.best_available_licenses() == [ + time_limited_2, + licenses.time_limited, + licenses.perpetual, + licenses.time_and_loan_limited, + licenses.loan_limited, + loan_limited_2, + ] + + # We use the time-limited license that's expiring first. + assert ( + first_or_default(licenses.pool.best_available_licenses()) == time_limited_2 + ) time_limited_2.checkout() # When that's not available, we use the next time-limited license. - assert licenses.time_limited == licenses.pool.best_available_license() + assert ( + first_or_default(licenses.pool.best_available_licenses()) + == licenses.time_limited + ) licenses.time_limited.checkout() - # The time-and-loan-limited license also counts as time-limited for this. - assert licenses.time_and_loan_limited == licenses.pool.best_available_license() - licenses.time_and_loan_limited.checkout() - # Next is the perpetual license. - assert licenses.perpetual == licenses.pool.best_available_license() + assert ( + first_or_default(licenses.pool.best_available_licenses()) + == licenses.perpetual + ) licenses.perpetual.checkout() + # Next up is the time-and-loan-limited license. + assert ( + first_or_default(licenses.pool.best_available_licenses()) + == licenses.time_and_loan_limited + ) + licenses.time_and_loan_limited.checkout() + # Then the loan-limited license with the most remaining checkouts. - assert licenses.loan_limited == licenses.pool.best_available_license() + assert ( + first_or_default(licenses.pool.best_available_licenses()) + == licenses.loan_limited + ) licenses.loan_limited.checkout() # That license allows 2 concurrent checkouts, so it's still the # best license until it's checked out again. - assert licenses.loan_limited == licenses.pool.best_available_license() + assert ( + first_or_default(licenses.pool.best_available_licenses()) + == licenses.loan_limited + ) licenses.loan_limited.checkout() # There's one more loan-limited license. - assert loan_limited_2 == licenses.pool.best_available_license() + assert ( + first_or_default(licenses.pool.best_available_licenses()) == loan_limited_2 + ) loan_limited_2.checkout() # Now all licenses are either loaned out or expired. - assert None == licenses.pool.best_available_license() + assert licenses.pool.best_available_licenses() == [] class TestLicensePool: