[SIP-41] Proposal for Alignment of Backend Error Handling

[john-bodley]

[SIP-41] Proposal for Alignment of Backend Error Handling

Motivation

[SIP-40] Proposal for Custom Error Messages proposes a unified pattern for handling API errors on the frontend however currently it is not apparent what error types (surfacing from the backend) need to be supported.

Reading through the code it is not super apparent:

• What exceptions are raised.
• How errors are handled and by whom, i.e., via the Flask or FAB error-handler, re-wrapped, etc.

Superset defines a number of exceptions each deriving from the SupersetException base class which contains an HTTP response status code in the [400, 600) range. Additionally there are a number of exceptions related to dataset commands.

Raised Exceptions

It can be quite difficult to understand exactly where and when an exception will be handled. The Python community as a whole does a fairly poor job of documenting (sadly I sense a shortage of typing was not adding support for exceptions) what exception types a method can raise and thus from a development perspective tracing the exception flow can be challenging.

Error Handling

A common approach in Flask for handling errors is registering error handlers which can defined at either the application or blueprint level. This pattern is also supported by a number of the Flask RESTful extensions. Currently there only exists one registered error handler in Superset and one in FAB.

Even though Superset has a number of defined exceptions there is no consistent way in how these are propagated/handled and thus it is hard to do an audit on which errors exists. For example:

• Here is an example of returning a Flask Response with an error status. It seems this pattern is mostly used for non-API calls.
• Here is an example using the json_error_response which wraps the Flask Response ensuring that the MIME type is application/json. This is akin to this Flask error-handler example.
• Here is an example where the SupersetException is re-raised as a ValidationError.
• Here is an example where exceptions are being handled as FAB responses defined here which return a Flask Response where the MIME type is application/json (per here).
• Here seems to be where some API exceptions are handled. Note that the handle_api_exception method is a decorator, rather than serving as a Flask error-handler and i.e.,

@handle_api_exception
@expose("/v1/query/", methods=["POST"])
...

Proposed Change

In order to better understand the various types of errors and scenarios in which they can surface I propose we undertake three broad approaches:

1. Documentation: Per the CONTRIBUTING.md suggestion, documenting which exception a method raises.
2. Standardization: Ensuring that all API errors are handled using the same handling mechanism (be that Flask, FAB, decorators etc.), providing consistent status codes, error payloads, etc.
3. Simplification: Removing unnecessary redirection and obfuscation with error handling. Ideally all relevant exceptions will be handled in one place which ensures consistent responses.

To address (2) and (3) personally I prefer the Flask error-handler approach, though due to our dependency on FAB we may need to align with that model.

I don't sense there's a quick fix for any of these steps, though I think there's merit in aligning on an approach which we can then i) manually enforce in code reviews, and ii) systematically refactor the code if necessary.

New or Changed Public Interfaces

There are no new or change public interfaces. This SIP merely proposes consolidating API error handling.

New dependencies

None.

Migration Plan and Compatibility

N/A.

Rejected Alternatives

None.

[dpgaspar]

Good initiative and now is a very good time to define the path forward.

Adding a bit of more detail regarding new API implementation:

Currently we are migrating MVC and "old" API's to new REST API's. At the same time we will refactor charts and dashboards to follow SIP-35 approved proposal. So, new API's that use PUT, POST, DELETE are planned to always call a command that implements the required business logic.

A command will succeed or raise a defined custom exception, these exceptions are structured the following way:

• All command exceptions inherit from CommandException
• All command validation exceptions inherit from CommandInvalidError

A CommandInvalidError is a wrapper around a list of marshmallow ValidationError exceptions, this way we leverage mashmallow JSON error responses for field validation, aligning business validation with marshmallow schema validation. So we get expected coherent error responses like the following:

HTTP 422

{
  "message": {
    "database": [
      "Database does not exist"
    ],
    "owners": [
      "Owners are invalid"
    ]
  }
}

Generic structure:

{
    "message": {
        "<FIELD_NAME>": ["<ERROR_MSG1>", "<ERROR_MSG2>", ...],
        ...
   }
}

The main structure for errors come from FAB default {"message": ... } but we can override it or just plain raise and let Flask handle errors

[etr2460]

Tagging @ktmud and linking to some of his thoughts on this topic from a PR comment: #10274 (comment)

[rusackas]

@john-bodley let's [VOTE] if we want to move this forward :)

[betodealmeida]

@john-bodley Let's vote this?

[betodealmeida]

I called a discussion on this today.

[rusackas]

This still needs to be put up for a vote. Here's the discuss thread.

[eschutho]

Since this is an old thread, can we revisit what's being proposed? I'm not sure I'm clear on it, and I know that this topic comes up quite a bit.
A few guidelines or patterns that I've been using are:

1. Always create/raise a custom error, not a built in Python one or a base Superset Exception. This error should also be as specific as possible to where the error is raising, so if the exception is in a dashboard command, the exception class will be superset.dashboards.commands.exceptions.DashboardDeleteFailedError and we can clearly see where it raised when it is logged.
2. Only catch and re-raise if you need to augment the error or do something else before raising. Otherwise, let the error bubble up to the Flask response handler. This pattern is especially important in the api classes where we have some patterns of catching all errors and re-raising, which I think is unnecessary.
3. Don't change the status code of the error. Define the status code in the error class.
4. When using a static message in an error, it's better to define it in the exception class instead of during execution.

Is this what other people have been using as best practice recently?

[eschutho]

Pasting @betodealmeida's comments from his email which have some really useful points as well here for reference:
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 (#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 (

superset/superset/exceptions.py

Line 119 in 8eff5a7

status = 422

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

[rusackas]

@john-bodley do you intend to move this forward to a VOTE thread?

[john-bodley]

@betodealmeida and @eschutho this is somewhat of an old thread and it seems like we never really got consensus on this. I think it might be prudent to close this (for now) and re-open it if/when we have more traction/alignment.

[mistercrunch]

Pointing out to #29330 which touches some of the things mentioned here. I'd love to get more direction from this SIP and identify some clear next steps. (we don't necessarily need a super strict end-goal, but some clear next steps in the right direction would help).

My take on overall direction:

• I think we need a really solid foundation of app-level error handlers as I started to cut through in fix: refactor view error handling into a separate module #29330 . We can beef up this error_handling.py module as something that applies to the whole app (all views) as a solid safety need for error handling. This means handling both html and api (json) cases centrally (with things like if request.is_json).
• Given this assumption, views could be simplifeid and oriented to simply raise ( as in raise SupersetSpecificException(relevant_message)) and trust that error_handling.py would traffic control all this and make sure the right payload is served centrally
• About decorators (like @api_error_handler), personally I don't like them for this. It's unclear which view implements which decorator, or which ones it should implement, and makes for funky stacktraces. Maybe could be used as intermediate, reusable safety nets in some places, but I'd rather centralize in app-level error handler that ultimately catch all issues

Some requirements/improvements I care about:

• DatabaseErrors are pretty special and should be clear AF, well handled
• It would be great if DEBUG mode would more consistently carry stacktraces and bubble up to user in a "show more ..." kind of way. We inconsistently serve that. There's a config SHOW_STACKTRACES and it's rarely carried through.
• Similarly logging.error / logging.warning could more systematically carry stacktraces that we can see in our observability systems
• Could be good to centralize and review the Exception inheritance tree, making sure we have something solid there

[mistercrunch]

btw did we ever vote on this one. I'm a big yes on the ideas here, but the "done state" or direction is not clear enough. Let me re-open and invite another round of discussion.

[betodealmeida]

I think one of the biggest things to consider here is that SIP-40 was approved, but we still have many APIs that return an non-compatible error payload. Regardless of how we capture exceptions in the backend we need to make sure that the schema of the error returned in error messages follows SIP-40.

This is particularly important for OAuth2, where OAuth2 is triggered by raising a special exception (OAuth2RedirectError) deep in the stack (at the DB engine spec and database model levels) that contains the information needed to start OAuth2 (the URL where the user should go). That exception needs to bubble up all the way to the frontend — if it gets captured and wrapped in a different exception OAuth2 won't work.

[mistercrunch]

Interesting use case. I'm wondering if the contract of views generally be allowed to raise as opposed to return json_error_message(some_relevant_message) would function better. That means that the view author, would have to 1. pick the right SupersetException or Exception derivative or create one if needed, and make sure it's handled properly in superset/views/error_handling.py.

Another way to think about it is that views would be more in charge of doing their own error handling composing the right error-handling utils, and superset/views/error_handling.py becomes more of a safety net.

[mistercrunch]

re-reading the SIP, the proposal is clear about going towards a consistent and well documented approach to error handling, it is not prescriptive about which approach to take. Let me try to propose something here and maybe we can promote this into the body of this SIP.

Here's what the current SIP prescribes:

To address (2) and (3) personally I prefer the Flask error-handler approach, though due to our dependency on FAB we may need to align with that model.

Let me give this a shot ->

Preferred approach for error handling in Superset views

How exceptions are defined

• Exceptions raised in the Superset codebase should always derive SupersetException and, when relevant the corresponding exception standard library or external library relevant exception, ultimately forming a nested tree. (?) Should we allow for common exceptions form the std lib like ValueError to be raised here, I'd say yes (?)
• All exceptions can be defined in different packages (folder) where they are most relevant across the codebase but ideally should be defined in a seperated exceptions.py module (likesuperset/commands/dataset/exceptions.py, superset/commands/database/exceptions.py, ...) so that they can be imported without risking circular imports
• [?just an idea, curious what others think?] All exceptions should be federated by getting reimported in superset/exceptions.py so that we can have a full inventory of the whole tree, and programmatically asses that they are all accounted for

How exceptions are implemented/used in views

Views can simply raise the right exception while passing an error message for clarity, meaning the view shouldn't try and return an error-formatted payload (as in return error_response(msg)).

We generally would discourage the use of decorators in favor of the app-level safety net. This may need stripping decorators from existing views. [either that, or use a single @api-type decorator, point being go decorator-free or have one-or-two, extremely simple decorator]

How responses are generated

All of the handling of exception and handling of generating a response is federated and handled in superset/views/error_handling.py, using Flask error handlers. This module can insure that every exception type is properly handled, enriched, standardize, formatted, as intended. We should be able to look at that file and assume that every error that can be raised in any view goes through the logic defined in this module.

This may need some amount of taking over FAB, as FAB may already catch-and-respond in some cases. We may need to disable some things in FAB to get to our own solution in this area (?)

Steps

[TODO]

[betodealmeida]

This may need some amount of taking over FAB, as FAB may already catch-and-respond in some cases. We may need to disable some things in FAB to get to our own solution in this area (?)

I don't think FAB does anything with the errors — what's happening is that in many APIs we're using the safe decorator from FAB to format exceptions not captured by the view (in a non SIP-40 way).

[mistercrunch]

I don't think FAB does anything with the errors

Good to know, though now i wonder if @safe would take precedence over the Superset app-level handlers or vice-versa, but guessing any operator would execute first and that the app-level handler is the last-resort safety net. If that's the case maybe we should strip all decorators and move towards superset/view/error_handling.py. I guess currently there's decorators such as @safe, @api, @handle_api_error, and potentially a few others. Higher level @expose might even do some error handling (?) All this probably requires a bit of an audit.

I think we also have class-based views inheritance schemes in FAB, like ApiModelView-type things that may have their own error-handling (?)

@dpgaspar curious on your input here