Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ekir 219 handle 502 errors when loaning #134

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
2 changes: 0 additions & 2 deletions api/controller/loan.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,6 @@ def _borrow(self, patron, credential, pool, mechanism):
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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tämä liittyy siihen syksyllä tehtyyn ongelmaan, jossa kirjaa ei ollutkaan saatavilla, kun käyttäjä yritti lainata hänelle varatun kirjan.

except CannotLoan as e:
if isinstance(e, NoAvailableCopiesWhenReserved):
result = e.as_problem_detail_document()
Expand Down
56 changes: 49 additions & 7 deletions api/odl.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
from core.service.container import Services
from core.util import base64
from core.util.datetime_helpers import to_utc, utc_now
from core.util.http import HTTP, BadResponseException
from core.util.http import HTTP, BadResponseException, RemoteIntegrationException


class ODLAPIConstants:
Expand Down Expand Up @@ -242,7 +242,9 @@ def _get_hasher(self) -> Hasher:

return self._hasher_instance

def _get(self, url: str, headers: dict[str, str] | None = None) -> Response:
def _get(
self, url: str, headers: dict[str, str] | None = None, *args: Any, **kwargs: Any
) -> Response:
"""Make a normal HTTP request, but include an authentication
header with the credentials for the collection.
"""
Expand Down Expand Up @@ -322,16 +324,34 @@ def get_license_status_document(self, loan: Loan) -> dict[str, Any]:
hint_url=self.settings.passphrase_hint_url,
)

response = self._get(url)
try:
response = self._get(url, allowed_response_codes=["2xx"])
except BadResponseException as e:
response = e.response
header_string = ", ".join(
{f"{k}: {v}" for k, v in response.headers.items()}
)
response_string = (
response.text
if len(response.text) < 100
else response.text[:100] + "..."
)
raise BadResponseException(
url,
f"Error getting License Status Document for loan ({loan.id}): Url '{url}' returned "
f"status code {response.status_code}. Expected 2XX. Response headers: {header_string}. "
f"Response content: {response_string}.",
response,
)

try:
status_doc = json.loads(response.content)
except ValueError as e:
raise BadResponseException(
raise RemoteIntegrationException(
url, "License Status Document was not valid JSON."
)
) from e
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]
Expand Down Expand Up @@ -468,7 +488,29 @@ def _checkout(
raise NoAvailableCopies()
loan, ignore = license.loan_to(patron)

doc = self.get_license_status_document(loan)
try:
doc = self.get_license_status_document(loan)
except BadResponseException as e:
_db.delete(loan)
response = e.response
# DeMarque sends "application/api-problem+json", but the ODL spec says we should
# expect "application/problem+json", so we need to check for both.
if response.headers.get("Content-Type") in [
"application/api-problem+json",
"application/problem+json",
]:
try:
json_response = response.json()
except ValueError:
json_response = {}

if (
json_response.get("type")
== "http://opds-spec.org/odl/error/checkout/unavailable"
):
raise NoAvailableCopies()
raise

status = doc.get("status")

if status not in [self.READY_STATUS, self.ACTIVE_STATUS]:
Expand Down
6 changes: 2 additions & 4 deletions api/opds_for_distributors.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import datetime
import json
from collections.abc import Generator
from collections.abc import Generator, Mapping
from typing import TYPE_CHECKING, Any

import feedparser
Expand Down Expand Up @@ -460,9 +460,7 @@ def __init__(

self.api = OPDSForDistributorsAPI(_db, collection)

def _get(
self, url: str, headers: dict[str, str]
) -> tuple[int, dict[str, str], bytes]:
def _get(self, url: str, headers: Mapping[str, str]) -> Response:
"""Make a normal HTTP request for an OPDS feed, but add in an
auth header with the credentials for the collection.
"""
Expand Down
4 changes: 2 additions & 2 deletions api/overdrive.py
Original file line number Diff line number Diff line change
Expand Up @@ -545,10 +545,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.
Expand Down
11 changes: 4 additions & 7 deletions core/opds2_import.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,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
Expand Down Expand Up @@ -1163,20 +1164,16 @@ class OPDS2ImportMonitor(OPDSImportMonitor):
PROTOCOL = ExternalIntegration.OPDS2_IMPORT
MEDIA_TYPE = OPDS2MediaTypesRegistry.OPDS_FEED.key, "application/json"

def _verify_media_type(
self, url: str, status_code: int, headers: dict[str, str], feed: bytes
) -> None:
def _verify_media_type(self, url: str, response: 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 = response.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, debug_message=feed, status_code=status_code
)
raise BadResponseException(url, message=message, response=response)

def _get_accept_header(self) -> str:
return "{}, {};q=0.9, */*;q=0.1".format(
Expand Down
29 changes: 12 additions & 17 deletions core/opds_import.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import urllib
from abc import ABC, abstractmethod
from collections import defaultdict
from collections.abc import Callable, Generator, Iterable, Sequence
from collections.abc import Callable, Generator, Iterable, Mapping, Sequence
from datetime import datetime
from io import BytesIO
from typing import TYPE_CHECKING, Any, Generic, TypeVar, cast, overload
Expand All @@ -18,6 +18,7 @@
from flask_babel import lazy_gettext as _
from lxml import etree
from pydantic import AnyHttpUrl
from requests import Response
from sqlalchemy.orm.session import Session

from api.circulation import (
Expand Down Expand Up @@ -1711,9 +1712,7 @@ def __init__(
self._feed_base_url = f"{parsed_url.scheme}://{parsed_url.hostname}{(':' + str(parsed_url.port)) if parsed_url.port else ''}/"
super().__init__(_db, collection)

def _get(
self, url: str, headers: dict[str, str]
) -> tuple[int, dict[str, str], bytes]:
def _get(self, url: str, headers: Mapping[str, str]) -> Response:
"""Make the sort of HTTP request that's normal for an OPDS feed.

Long timeout, raise error on anything but 2xx or 3xx.
Expand All @@ -1727,8 +1726,7 @@ def _get(
)
if not url.startswith("http"):
url = urljoin(self._feed_base_url, url)
response = HTTP.get_with_timeout(url, headers=headers, **kwargs)
return response.status_code, response.headers, response.content # type: ignore[return-value]
return HTTP.get_with_timeout(url, headers=headers, **kwargs)

def _get_accept_header(self) -> str:
return ",".join(
Expand All @@ -1740,7 +1738,7 @@ def _get_accept_header(self) -> str:
]
)

def _update_headers(self, headers: dict[str, str] | None) -> dict[str, str]:
def _update_headers(self, headers: Mapping[str, str] | None) -> dict[str, str]:
headers = dict(headers) if headers else {}
if self.username and self.password and not "Authorization" in headers:
headers["Authorization"] = "Basic %s" % base64.b64encode(
Expand Down Expand Up @@ -1858,22 +1856,18 @@ def identifier_needs_import(
return True
return False

def _verify_media_type(
self, url: str, status_code: int, headers: dict[str, str], feed: bytes
) -> None:
def _verify_media_type(self, url: str, response: 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 = response.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, debug_message=feed, status_code=status_code
)
raise BadResponseException(url, message=message, response=response)

def follow_one_link(
self, url: str, do_get: Callable[..., tuple[int, Any, bytes]] | None = None
self, url: str, do_get: Callable[..., Response] | None = None
) -> tuple[list[str], bytes | None]:
"""Download a representation of a URL and extract the useful
information.
Expand All @@ -1884,9 +1878,10 @@ def follow_one_link(
"""
self.log.info("Following next link: %s", url)
get = do_get or self._get
status_code, headers, feed = get(url, {})
response = get(url, {})
feed = response.content

self._verify_media_type(url, status_code, headers, feed)
self._verify_media_type(url, response)

new_data = self.feed_contains_new_data(feed)

Expand Down
Loading