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

* Use exception chaining
* Rename BaseError to BasePalaceException
* Convert to a single exception base class.
* A bit more base exception refactoring
* Update comment
* Fix issue with CirculationException
* Clean up filter error
* Fix another SAML exception issue
* Code review feedback.

api/authentication/base.py

@@ -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
@@ -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.
 

api/circulation_exceptions.py

@@ -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:

api/lcp/hash.py

@@ -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"""
 
 

api/saml/configuration/model.py

@@ -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,
@@ -42,7 +42,7 @@
 from core.util.log import LoggerMixin
 
 
-class SAMLConfigurationError(BaseError):
+class SAMLConfigurationError(BasePalaceException):
     """Raised in the case of any configuration errors."""
 
 

api/saml/metadata/federations/loader.py

@@ -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"""
 
 
@@ -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}")
 

api/saml/metadata/federations/validator.py

@@ -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."""
 
 
@@ -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(
@@ -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(

api/saml/metadata/filter.py

@@ -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:
@@ -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(
@@ -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

api/saml/metadata/parser.py

@@ -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"""
 
 
@@ -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"
@@ -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(
@@ -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")
 

api/selftest.py

@@ -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
@@ -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:

core/exceptions.py

@@ -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
+        :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.
 

core/external_search.py

@@ -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
@@ -1334,7 +1335,7 @@ def _parse_json_join(self, query: dict) -> dict:
 
 
 @define
-class QueryParseException(Exception):
+class QueryParseException(BasePalaceException):
     detail: str = ""
 
 

core/feed/opds.py

@@ -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
@@ -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.

core/jobs/integration_test.py

@@ -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 (
@@ -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}")
@@ -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
@@ -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)

core/lcp/exceptions.py

@@ -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"""

core/model/collection.py

@@ -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
@@ -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.
     """

core/model/devicetokens.py

@@ -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
 
@@ -82,9 +83,9 @@ def create(
         return device
 
 
-class InvalidTokenTypeError(Exception):
+class InvalidTokenTypeError(BasePalaceException):
     pass
 
 
-class DuplicateDeviceTokenError(Exception):
+class DuplicateDeviceTokenError(BasePalaceException):
     pass