You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/03/12 19:15:19 UTC
[GitHub] [incubator-superset] john-bodley opened a new issue #9298: [SIP-41]
Proposal for Alignment of Backend Error Handling
john-bodley opened a new issue #9298: [SIP-41] Proposal for Alignment of Backend Error Handling
URL: https://github.com/apache/incubator-superset/issues/9298
## [SIP-41] Proposal for Alignment of Backend Error Handling
### Motivation
[[SIP-40] Proposal for Custom Error Messages](https://github.com/apache/incubator-superset/issues/9194) 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](https://github.com/apache/incubator-superset/blob/master/superset/exceptions.py) 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](https://github.com/apache/incubator-superset/blob/52c59d689063bfac9cba73b416d2b1bdc6a1275e/superset/datasets/commands/exceptions.py) 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](https://flask.palletsprojects.com/en/1.1.x/errorhandling/#error-handlers) 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](https://github.com/apache/incubator-superset/blob/406ad8778c4b83461e534b03d165c279484b2d2c/superset/views/core.py#L2628) registered error handler in Superset and [one](https://github.com/dpgaspar/Flask-AppBuilder/blob/cfb324007ca12c6d5939e05521880ca7ba888236/examples/simpleform/app/views.py#L36) 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](https://github.com/apache/incubator-superset/blob/406ad8778c4b83461e534b03d165c279484b2d2c/superset/views/core.py#L325) is an example of returning a Flask Response with an error status. It seems this pattern is mostly used for non-API calls.
- [Here](https://github.com/apache/incubator-superset/blob/406ad8778c4b83461e534b03d165c279484b2d2c/superset/views/core.py#L315) is an example using the [`json_error_response`](https://github.com/apache/incubator-superset/blob/116200cf73c016337aa7ef1b80e80f78b14cf30e/superset/views/base.py#L69) which wraps the Flask Response ensuring that the MIME type is `application/json`. This is akin to this Flask error-handler [example](https://flask.palletsprojects.com/en/1.1.x/errorhandling/#generic-exception-handlers).
- [Here](https://github.com/apache/incubator-superset/blob/78ba7d52f64950f97e8ef0498aac594fd8b3dd42/superset/views/dashboard/api.py#L59) is an example where the `SupersetException` is re-raised as a `ValidationError`.
- [Here](https://github.com/apache/incubator-superset/blob/52c59d689063bfac9cba73b416d2b1bdc6a1275e/superset/datasets/api.py#L211) is an example where exceptions are being handled as FAB responses defined [here](https://github.com/dpgaspar/Flask-AppBuilder/blob/f412c9291f98f7d2f5450e2d9463fbc7469dcdd1/flask_appbuilder/api/__init__.py) which return a Flask `Response` where the MIME type is `application/json` (per [here](https://github.com/dpgaspar/Flask-AppBuilder/blob/f412c9291f98f7d2f5450e2d9463fbc7469dcdd1/flask_appbuilder/api/__init__.py#L663)).
- [Here](https://github.com/apache/incubator-superset/blob/607cfd1f29736590fbba397c4f8a04526be66aff/superset/views/base.py#L114) 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.,
```python
@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 approaches:
1. **Error Standardizaion**: Ensuring that we throw the relevant exceptions and using custom exceptions only when they make sense.
2. **Documentation**: Per the [CONTRIBUTING.md](https://github.com/apache/incubator-superset/blob/master/CONTRIBUTING.md#python-1) suggestion, documenting which exception a method raises.
3. **Standardization**: Ensuring that all API errors are handled using the same handling mechanism (be that Flask, FAB, decorators etc.). 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on issue #9298: [SIP-41]
Proposal for Alignment of Backend Error Handling
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on issue #9298: [SIP-41] Proposal for Alignment of Backend Error Handling
URL: https://github.com/apache/incubator-superset/issues/9298#issuecomment-598706549
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
``` json
{
"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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9298: [SIP-41] Proposal for Alignment of Backend Error Handling
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9298:
URL: https://github.com/apache/incubator-superset/issues/9298#issuecomment-661190028
Tagging @ktmud and linking to some of his thoughts on this topic from a PR comment: https://github.com/apache/incubator-superset/pull/10274#discussion_r454127147
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org