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

Refactor exceptions to have a common base class (PP-991) #1695

Merged
merged 9 commits into from
Feb 26, 2024
3 changes: 2 additions & 1 deletion api/authentication/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from werkzeug.datastructures import Authorization

from core.analytics import Analytics
from core.exceptions import BasePalaceException
from core.integration.base import HasLibraryIntegrationConfiguration
from core.integration.settings import BaseSettings
from core.model import CirculationEvent, Library, Patron, get_one_or_create
Expand Down Expand Up @@ -113,7 +114,7 @@ def get_credential_from_header(self, auth: Authorization) -> str | None:
]


class CannotCreateLocalPatron(Exception):
class CannotCreateLocalPatron(BasePalaceException):
"""A remote system provided information about a patron, but we could
not put it into our database schema.

Expand Down
2 changes: 1 addition & 1 deletion api/circulation_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ class CirculationException(IntegrationException, BaseProblemDetailException, ABC
def __init__(
self, message: str | None = None, debug_info: str | None = None
) -> None:
self.message = message
super().__init__(message or self.__class__.__name__, debug_info)
self.message = message

@property
def detail(self) -> str | None:
Expand Down
4 changes: 2 additions & 2 deletions api/lcp/hash.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@
from abc import ABC, abstractmethod
from enum import Enum

from core.exceptions import BaseError
from core.exceptions import BasePalaceException


class HashingAlgorithm(Enum):
SHA256 = "http://www.w3.org/2001/04/xmlenc#sha256"
SHA512 = "http://www.w3.org/2001/04/xmlenc#sha512"


class HashingError(BaseError):
class HashingError(BasePalaceException):
"""Raised in the case of errors occurred during hashing"""


Expand Down
4 changes: 2 additions & 2 deletions api/saml/configuration/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
SAMLSubjectPatronIDExtractor,
)
from api.saml.metadata.parser import SAMLMetadataParser, SAMLMetadataParsingError
from core.exceptions import BaseError
from core.exceptions import BasePalaceException
from core.integration.settings import (
ConfigurationFormItem,
ConfigurationFormItemType,
Expand All @@ -42,7 +42,7 @@
from core.util.log import LoggerMixin


class SAMLConfigurationError(BaseError):
class SAMLConfigurationError(BasePalaceException):
"""Raised in the case of any configuration errors."""


Expand Down
6 changes: 3 additions & 3 deletions api/saml/metadata/federations/loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@
)
from api.saml.metadata.federations.validator import SAMLFederatedMetadataValidator
from api.saml.metadata.parser import SAMLMetadataParser
from core.exceptions import BaseError
from core.exceptions import BasePalaceException
from core.util import first_or_default


class SAMLMetadataLoadingError(BaseError):
class SAMLMetadataLoadingError(BasePalaceException):
"""Raised in the case of any errors occurred during loading of SAML metadata from a remote source"""


Expand Down Expand Up @@ -41,7 +41,7 @@ def load_idp_metadata(self, url=None):
try:
xml_metadata = OneLogin_Saml2_IdPMetadataParser.get_metadata(url)
except Exception as exception:
raise SAMLMetadataLoadingError(inner_exception=exception)
raise SAMLMetadataLoadingError() from exception

self._logger.info(f"Finished loading IdP XML metadata from {url}")

Expand Down
10 changes: 5 additions & 5 deletions api/saml/metadata/federations/validator.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
from onelogin.saml2.xmlparser import fromstring

from api.saml.metadata.federations.model import SAMLFederation
from core.exceptions import BaseError
from core.exceptions import BasePalaceException
from core.util.datetime_helpers import from_timestamp, utc_now


class SAMLFederatedMetadataValidationError(BaseError):
class SAMLFederatedMetadataValidationError(BasePalaceException):
"""Raised in the case of any errors happened during SAML metadata validation."""


Expand Down Expand Up @@ -113,8 +113,8 @@ def validate(self, federation: SAMLFederation, metadata: str | bytes) -> None:
root = fromstring(metadata)
except Exception as exception:
raise SAMLFederatedMetadataValidationError(
"Metadata's XML is not valid", exception
)
"Metadata's XML is not valid"
) from exception

if "EntitiesDescriptor" not in root.tag:
raise SAMLFederatedMetadataValidationError(
Expand Down Expand Up @@ -182,7 +182,7 @@ def validate(self, federation, metadata):
metadata, federation.certificate, raise_exceptions=True
)
except Exception as exception:
raise SAMLFederatedMetadataValidationError(str(exception), exception)
raise SAMLFederatedMetadataValidationError(str(exception)) from exception

self._logger.info(
"Finished verifying the validity of the metadata's signature belonging to {}".format(
Expand Down
18 changes: 7 additions & 11 deletions api/saml/metadata/filter.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,18 @@
SAMLNameID,
SAMLSubject,
)
from core.exceptions import BaseError
from core.exceptions import BasePalaceException
from core.python_expression_dsl.evaluator import DSLEvaluator


class SAMLSubjectFilterError(BaseError):
class SAMLSubjectFilterError(BasePalaceException):
"""Raised in the case of any errors during execution of a filter expression."""

def __init__(self, inner_exception):
"""Initialize a new instance of SAMLSubjectFilterError class.

:param inner_exception: Inner exception
:type inner_exception: Exception
"""
def __init__(self, inner_exception: Exception) -> None:
"""Initialize a new instance of SAMLSubjectFilterError class."""
message = f"Incorrect filter expression: {str(inner_exception)}"

super().__init__(message, inner_exception)
super().__init__(message)


class SAMLSubjectFilter:
Expand Down Expand Up @@ -76,7 +72,7 @@ def execute(self, expression, subject):
],
)
except Exception as exception:
raise SAMLSubjectFilterError(exception)
raise SAMLSubjectFilterError(exception) from exception

self._logger.info(
"Finished applying expression '{}' to {}: {}".format(
Expand Down Expand Up @@ -104,4 +100,4 @@ def validate(self, expression):
try:
self._dsl_evaluator.parser.parse(expression)
except Exception as exception:
raise SAMLSubjectFilterError(exception)
raise SAMLSubjectFilterError(exception) from exception
10 changes: 5 additions & 5 deletions api/saml/metadata/parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@
SAMLSubject,
SAMLUIInfo,
)
from core.exceptions import BaseError
from core.exceptions import BasePalaceException


class SAMLMetadataParsingError(BaseError):
class SAMLMetadataParsingError(BasePalaceException):
"""Raised in the case of any errors occurred during parsing of SAML metadata"""


Expand Down Expand Up @@ -116,7 +116,7 @@ def _convert_xml_string_to_dom(
"An unhandled exception occurred during converting XML string containing SAML metadata into XML DOM"
)

raise SAMLMetadataParsingError(inner_exception=exception)
raise SAMLMetadataParsingError() from exception

self._logger.debug(
"Finished converting XML string containing SAML metadata into XML DOM"
Expand Down Expand Up @@ -149,7 +149,7 @@ def _parse_certificates(self, certificate_nodes):

certificates.append(certificate)
except XMLSyntaxError as exception:
raise SAMLMetadataParsingError(inner_exception=exception)
raise SAMLMetadataParsingError() from exception

self._logger.debug(
"Finished parsing {} certificates: {}".format(
Expand Down Expand Up @@ -605,7 +605,7 @@ def parse(self, xml_metadata):
"An unexpected error occurred during parsing an XML string containing SAML metadata"
)

raise SAMLMetadataParsingError(inner_exception=exception)
raise SAMLMetadataParsingError() from exception

self._logger.info("Finished parsing an XML string containing SAML metadata")

Expand Down
4 changes: 2 additions & 2 deletions api/selftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from sqlalchemy.orm.session import Session

from core.config import IntegrationException
from core.exceptions import BaseError
from core.exceptions import BasePalaceException
from core.model import Collection, Library, LicensePool, Patron
from core.model.integration import IntegrationConfiguration
from core.selftest import HasSelfTests, SelfTestResult
Expand All @@ -20,7 +20,7 @@ class HasPatronSelfTests(HasSelfTests, ABC):
on behalf of a specific patron.
"""

class _NoValidLibrarySelfTestPatron(BaseError):
class _NoValidLibrarySelfTestPatron(BasePalaceException):
"""Exception raised when no valid self-test patron found for library.

Attributes:
Expand Down
48 changes: 7 additions & 41 deletions core/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,50 +1,16 @@
class BaseError(Exception):
"""Base class for all errors"""
class BasePalaceException(Exception):
"""Base class for all Exceptions in the Palace manager."""

def __init__(
self, message: str | None = None, inner_exception: Exception | None = None
):
"""Initializes a new instance of BaseError class
def __init__(self, message: str | None = None):
"""Initializes a new instance of BasePalaceException class

:param message: String containing description of the error occurred
:param inner_exception: (Optional) Inner exception
Copy link
Contributor

Choose a reason for hiding this comment

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

error -> exception that

And the inner_exception has gone away yes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, inner_exception was removed. Its leftover from Python 2 days when there was no built in way to chain exceptions. Now that we are on Python 3 I replaced it with PEP3134 exception chaining in the couple places where inner_exception was previously used.

:param message: String containing description of the exception that occurred
"""
if inner_exception and not message:
message = str(inner_exception)

super().__init__(message)

self._inner_exception = str(inner_exception) if inner_exception else None

def __hash__(self):
return hash(str(self))

@property
def inner_exception(self) -> str | None:
"""Returns an inner exception

:return: Inner exception
"""
return self._inner_exception

def __eq__(self, other: object) -> bool:
"""Compares two BaseError objects

:param other: BaseError object
:return: Boolean value indicating whether two items are equal
"""
if not isinstance(other, BaseError):
return False

return str(self) == str(other)

def __repr__(self):
return "<BaseError(message={}, inner_exception={})>".format(
(self), self.inner_exception
)
self.message = message


class IntegrationException(Exception):
class IntegrationException(BasePalaceException):
"""An exception that happens when the site's connection to a
third-party service is broken.

Expand Down
3 changes: 2 additions & 1 deletion core/external_search.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
GradeLevelClassifier,
KeywordBasedClassifier,
)
from core.exceptions import BasePalaceException
from core.facets import FacetConstants
from core.lane import Pagination
from core.metadata_layer import IdentifierData
Expand Down Expand Up @@ -1334,7 +1335,7 @@ def _parse_json_join(self, query: dict) -> dict:


@define
class QueryParseException(Exception):
class QueryParseException(BasePalaceException):
detail: str = ""


Expand Down
3 changes: 2 additions & 1 deletion core/feed/opds.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from werkzeug.datastructures import MIMEAccept

from core.exceptions import BasePalaceException
from core.feed.base import FeedInterface
from core.feed.serializer.base import SerializerInterface
from core.feed.serializer.opds import OPDS1Serializer
Expand Down Expand Up @@ -95,7 +96,7 @@ def entry_as_response(
return response


class UnfulfillableWork(Exception):
class UnfulfillableWork(BasePalaceException):
"""Raise this exception when it turns out a Work currently cannot be
fulfilled through any means, *and* this is a problem sufficient to
cancel the creation of an <entry> for the Work.
Expand Down
11 changes: 6 additions & 5 deletions core/jobs/integration_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from OpenSSL.crypto import FILETYPE_PEM, X509, load_certificate

from core.crypt.aes import CryptAESCBC
from core.exceptions import BasePalaceException
from core.scripts import Script
from core.util.datetime_helpers import utc_now
from core.util.http import (
Expand Down Expand Up @@ -165,7 +166,7 @@ def do_run(self) -> None:
self._run_test(test)
except FailedIntegrationTest as ex:
self.log.error(
f"Test run failed for {test.name} {test.endpoint}: {ex.args[0]}"
f"Test run failed for {test.name} {test.endpoint}: {ex.message}"
)
if ex.exception:
self.log.error(f"Test run exception {ex.exception}")
Expand All @@ -182,7 +183,7 @@ def _run_test(self, test: IntegrationTestDetails) -> None:
)
except (RequestNetworkException, RequestTimedOut, BadResponseException) as ex:
raise FailedIntegrationTest(
f"Network Failure: {ex.url}", exception=ex.args[0]
f"Network Failure: {ex.url}", exception=ex.message
)

# Run tests on the SSL certificate
Expand Down Expand Up @@ -243,7 +244,7 @@ def _test_ssl_validity(self, test: IntegrationTestDetails):
raise FailedIntegrationTest(f"The SSL certificate expires on {expires}")


class FailedIntegrationTest(Exception):
def __init__(self, *args: object, exception=None) -> None:
class FailedIntegrationTest(BasePalaceException):
def __init__(self, message: str, exception: str | None = None) -> None:
self.exception = exception
super().__init__(*args)
super().__init__(message)
4 changes: 2 additions & 2 deletions core/lcp/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from core.exceptions import BaseError
from core.exceptions import BasePalaceException


class LCPError(BaseError):
class LCPError(BasePalaceException):
"""Base class for all LCP related exceptions"""
3 changes: 2 additions & 1 deletion core/model/collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from sqlalchemy.orm.session import Session
from sqlalchemy.sql.expression import and_, or_

from core.exceptions import BasePalaceException
from core.integration.goals import Goals
from core.model import Base, create
from core.model.constants import DataSourceConstants, EditionConstants
Expand Down Expand Up @@ -617,7 +618,7 @@ class CollectionIdentifier:
pass


class CollectionMissing(Exception):
class CollectionMissing(BasePalaceException):
"""An operation was attempted that can only happen within the context
of a Collection, but there was no Collection available.
"""
Expand Down
5 changes: 3 additions & 2 deletions core/model/devicetokens.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from sqlalchemy.exc import IntegrityError
from sqlalchemy.orm import Mapped, backref, relationship

from core.exceptions import BasePalaceException
from core.model import Base
from core.model.patron import Patron

Expand Down Expand Up @@ -82,9 +83,9 @@ def create(
return device


class InvalidTokenTypeError(Exception):
class InvalidTokenTypeError(BasePalaceException):
pass


class DuplicateDeviceTokenError(Exception):
class DuplicateDeviceTokenError(BasePalaceException):
pass
Loading