Just to clarify, there are two separate discussions here (at least):
1. What's the canonical solution for catching errors? FAB's @safe
decorator? Flask error handlers? We currently use both, and @safe is not
SIP-40 compliant (which has been approved).
2. Should exceptions have HTTP status codes, or should that mapping live
in the API layer? This discussion is orthogonal to SIP-41, so we can
have it later.
--Beto
On 9/13/23 9:12 AM, Beto Dealmeida wrote:
Hi, community!
I decided to commandeer SIP-41 "Proposal for Alignment of Backend
Error Handling" (https://github.com/apache/superset/issues/9298) from
John Bodley and bring it to discussion, since it's been open for more
than 3 years and is partially adopted in Superset. If John wants to
lead this discussion I'm happy to step back.
For context, SIP-41 calls for a standard way of raising handling
exceptions in the Superset backend. In the SIP John doesn't propose a
specific way, but expresses his preference for using Flask application
error handlers. This is also my preferred way, and how I've been
implementing new exceptions since then.
With the Flask application error handlers already in place *one can
simply raise an exception anywhere in the code*, and the backend will
return a SIP-40 (https://github.com/apache/superset/issues/9194)
compliant error payload. It greatly simplifies the API logic, since we
can call commands outside a try/except block (see
https://github.com/apache/superset/pull/13960/files#diff-f9deb9b63f5f2db38b811ff092cf90b0d4d558223f743bbbb5b203c203d2d590L607-L613).
Note that many of the APIs are still using the @safe decorator from
FAB, which prevents exceptions from bubbling up to Flask, and returns
an error payload that *is not SIP-40 compliant*
(https://github.com/apache/superset/pull/13960/files#diff-f9deb9b63f5f2db38b811ff092cf90b0d4d558223f743bbbb5b203c203d2d590L561).
These should be removed regardless of the decision on SIP-41, but
adopting Flask error handlers for SIP-41 means we can simply remove
the decorator to return SIP-40 payloads.
One controversial point is that in order to work correctly we need to
define HTTP status codes on the exceptions
(https://github.com/apache/superset/blob/8eff5a75b433592462fba3841419efbe264f9745/superset/exceptions.py#L119).
While this might violate the principle of separation of concerns, I
think this aligns pretty well with Python's principle of "practicality
beats purity", since it allows us to write a very thin API layer.
Thanks,
--Beto
