refactor: Remove generic exception handling
A recurring antipattern found in this codebase is the leveraging of generic Exceptions, both as something to catch and as something to throw. This method of error handling may at first blush seem like a straightforward way to be thorough about catching problems, but in reality, it obfuscates problems by causing all possible errors to get handled unilaterally, in a manner that, at best, discards the stack trace, and at worst causes an unknown show-stopper bug to be camoflaged as a known handleable issue. This all creates downstream technical debt, and can be avoided by only handling the exceptions we are prepared to handle, and allowing unexpected issues to bubble up the stack and properly crash the application.
By and large, this (anti)pattern was deployed in the Stripe and Paypal
controllers and views; errors thrown by the Paypal REST API or the
Stripe Python SDK would be caught on the controller layer, bubbled up to
the view layer via thrown generic Exception
, and handled in a
view-level try/except block which caught generic Exception
s.
Additionally, while Paypal's REST API calls are wrapped in a try/except
block which catch requests.HTTPError
s, Stripe SDK calls were wrapped
in a try/except block which caught generic Exception
s.
This commit makes the following changes to the above:
- Try/except blocks which encapsulate Stripe SDK invocations now catch
stripe.error.StripeError
exceptions exclusively. A handler for generic exceptions which resided instripe_exception_handler()
has been removed to allow those generic exceptions to bubble up the scope uncaught as per our aforementioned reasoning. - Exceptions intentionally thrown from a Stripe/Paypal controller level
are now of type
ValueError
. Similarly, try/except blocks in Stripe and Paypal views which wrap controller methods now catch exceptions of typeValueError
. - In a few places, try/except blocks were asked to handle either
ValueError
s orTypeError
s, but there was not a good reason to handleTypeError
- perhaps a low-level version of the kind of bet-hedging intended by handling generic exceptions. Summarily, theTypeError
exception handling has been removed from these blocks. - Places where the
raise
keyword was needlessly passed a parameter which it could invoke implicitly have been rewritten without the needless parameter, for the sake of brevity and clarity. - The Paypal webhook verification method made a REST API call but did
not wrap that call in the same sort of
try/except
block used elsewhere, but as that particular verification method is used to determine the legitimacy of incoming webhook POST requests, it is important that non-200 responses be caught and thrown back to the view, so that a non-200 response can be properly sent back to the issuing server. This commit makes that change.
Additionally, a few stray instances of generic exception handling within
CiviCRM-related code were addressed with this same ValueError
pattern.