dpgaspar
commented
4 years ago 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
rusackas
betodealmeida
eschutho
john-bodley
mistercrunch
[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:
Superset defines a number of exceptions each deriving from the
SupersetExceptionbase 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:
json_error_responsewhich wraps the Flask Response ensuring that the MIME type isapplication/json. This is akin to this Flask error-handler example.SupersetExceptionis re-raised as aValidationError.Responsewhere the MIME type isapplication/json(per here).handle_api_exceptionmethod is a decorator, rather than serving as a Flask error-handler and i.e.,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:
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.