Skip to content

refactor: Remove generic exception handling

stephen requested to merge generic-exception-handling into main

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 Exceptions. Additionally, while Paypal's REST API calls are wrapped in a try/except block which catch requests.HTTPErrors, Stripe SDK calls were wrapped in a try/except block which caught generic Exceptions.

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 in stripe_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 type ValueError.
  • In a few places, try/except blocks were asked to handle either ValueErrors or TypeErrors, but there was not a good reason to handle TypeError - perhaps a low-level version of the kind of bet-hedging intended by handling generic exceptions. Summarily, the TypeError 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.

Merge request reports

Loading